[net,v4,0/7] usbnet: ipheth: prevent OoB reads of NDP16

Message

Foster Snowhill Jan. 5, 2025, 1:01 a.m. UTC

iOS devices support two types of tethering over USB: regular, where the
internet connetion is shared from the phone to the attached computer,
and reverse, where the internet connection is shared from the attached
computer to the phone.

The `ipheth` driver is responsible for regular tethering only. With this
tethering type, iOS devices support two encapsulation modes on RX:
legacy and NCM.

In "NCM mode", the iOS device encapsulates RX (phone->computer) traffic
in NCM Transfer Blocks (similarly to CDC NCM). However, unlike reverse
tethering, regular tethering is not compliant with the CDC NCM spec:

* Does not have the required CDC NCM descriptors
* TX (computer->phone) is not NCM-encapsulated at all

Thus `ipheth` implements a very limited subset of the spec with the sole
purpose of parsing RX URBs. This driver does not aim to be
a CDC NCM-compliant implementation and, in fact, can't be one because of
the points above.

For a complete spec-compliant CDC NCM implementation, there is already
the `cdc_ncm` driver. This driver is used for reverse tethering on iOS
devices. This patch series does not in any way change `cdc_ncm`.

In the first iteration of the NCM mode implementation in `ipheth`,
there were a few potential out of bounds reads when processing malformed
URBs received from a connected device:

* Only the start of NDP16 (wNdpIndex) was checked to fit in the URB
  buffer.
* Datagram length check as part of DPEs could overflow.
* DPEs could be read past the end of NDP16 and even end of URB buffer
  if a trailer DPE wasn't encountered.

The above is not expected to happen in normal device operation.

To address the above issues for iOS devices in NCM mode, rely on
and check for a specific fixed format of incoming URBs expected from
an iOS device:

* 12-byte NTH16
* 96-byte NDP16, allowing up to 22 DPEs (up to 21 datagrams + trailer)

On iOS, NDP16 directly follows NTH16, and its length is constant
regardless of the DPE count.

As the regular tethering implementation of iOS devices isn't compliant
with CDC NCM, it's not possible to use the `cdc_ncm` driver to handle
this functionality. Furthermore, while the logic required to properly
parse URBs with NCM-encapsulated frames is already part of said driver,
I haven't found a nice way to reuse the existing code without messing
with the `cdc_ncm` driver itself.

I didn't want to reimplement more of the spec than I absolutely had to,
because that work had already been done in `cdc_ncm`. Instead, to limit
the scope, I chose to rely on the specific URB format of iOS devices
that hasn't changed since the NCM mode was introduced there.

I tested each individual patch in the series with iPhone 15 Pro Max,
iOS 18.2: compiled cleanly, ran iperf3 between phone and computer,
observed no errors in either kernel log or interface statistics.


Foster Snowhill (7):
  usbnet: ipheth: break up NCM header size computation
  usbnet: ipheth: fix possible overflow in DPE length check
  usbnet: ipheth: check that DPE points past NCM header
  usbnet: ipheth: use static NDP16 location in URB
  usbnet: ipheth: refactor NCM datagram loop
  usbnet: ipheth: fix DPE OoB read
  usbnet: ipheth: document scope of NCM implementation

 drivers/net/usb/ipheth.c | 69 ++++++++++++++++++++++++++--------------
 1 file changed, 45 insertions(+), 24 deletions(-)

Comments

Jakub Kicinski Jan. 8, 2025, 1:31 a.m. UTC | #1

On Sun,  5 Jan 2025 02:01:14 +0100 Foster Snowhill wrote:
> iOS devices support two types of tethering over USB: regular, where the
> internet connetion is shared from the phone to the attached computer,
> and reverse, where the internet connection is shared from the attached
> computer to the phone.

Reviewed-by: Jakub Kicinski <[e-mail]>

Please add that to each patch, address Greg's comment, and repost.

Foster Snowhill Jan. 13, 2025, 1:48 a.m. UTC | #2

Hello Jakub,

> Reviewed-by: Jakub Kicinski <[e-mail]>
> 
> 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/[e-mail]/
[p5]: https://lore.kernel.org/netdev/[e-mail]/
[p6]: https://lore.kernel.org/netdev/[e-mail]/
[p7]: https://lore.kernel.org/netdev/[e-mail]/

Jakub Kicinski Jan. 13, 2025, 8:52 p.m. UTC | #3

On Mon, 13 Jan 2025 02:48:58 +0100 Foster Snowhill wrote:
> 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.

The opinions on the exact rules differ but you can definitely add my tag
on the patches which won't change.

> 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?

Yes, it's fine as part of the 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?

Maybe it's because I don't work with USB networking much but to me
the comment was useful.