netdev - Re: [PATCH net v4 0/7] usbnet: ipheth: prevent OoB reads of NDP16

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <e587e1c7-a2ff-4e28-9e25-b57f68545134@pen.gy>
Date: Mon, 13 Jan 2025 02:48:58 +0100
From: Foster Snowhill <forst@....gy>
To: Jakub Kicinski <kuba@...nel.org>
Cc: "David S. Miller" <davem@...emloft.net>,
 Eric Dumazet <edumazet@...gle.com>, Paolo Abeni <pabeni@...hat.com>,
 Georgi Valkov <gvalkov@...il.com>, Simon Horman <horms@...nel.org>,
 Oliver Neukum <oneukum@...e.com>, netdev@...r.kernel.org,
 linux-usb@...r.kernel.org
Subject: Re: [PATCH net v4 0/7] usbnet: ipheth: prevent OoB reads of NDP16

Hello Jakub,

> Reviewed-by: Jakub Kicinski <kuba@...nel.org>
> 
> Please add that to each patch, address Greg's comment, and repost.

Thank you very much for the review!

I went through the series again, noticed a couple minor things I think
I should fix:

* Patch 1/7 ("usbnet: ipheth: break up NCM header size computation")
  [p1] introduces two new preprocessor constants. Only one of them is
  used (the other one is intermediate, for clarity), and the usage is
  all the way in patch 6/7 ("usbnet: ipheth: fix DPE OoB read") [p6].
  I'd like to move the constant introduction patch right before the
  patch that uses one of them. There's no good reason they're spread
  out like they are in v4.
* Commit message in patch 5/7 ("usbnet: ipheth: refactor NCM datagram
  loop") [p5] has a stray paragraph starting with "Fix an out-of-bounds
  DPE read...". This needs to be removed.

I'd like to get this right. I'll make the changes above, add Cc stable,
re-test all patches in sequence, and submit v5 soon. As this will be
a different revision, I figure I can't formally apply your "Reviewed-by"
anymore, the series may need another look once I post v5.

Also I have some doubts about patch 7/7 [p7] with regards to its
applicability to backporting to older stable releases. This only adds a
documentation comment, without fixing any particular issue. Doesn't
sound like something that should go into stable. But maybe fine if it's
part of a series? I can also add that text in a commit message rather
than the source code of the driver itself, or even just keep it in the
cover letter. Do you have any opinion on this?

Thank you!

[p1]: https://lore.kernel.org/netdev/20250105010121.12546-2-forst@pen.gy/
[p5]: https://lore.kernel.org/netdev/20250105010121.12546-6-forst@pen.gy/
[p6]: https://lore.kernel.org/netdev/20250105010121.12546-7-forst@pen.gy/
[p7]: https://lore.kernel.org/netdev/20250105010121.12546-8-forst@pen.gy/