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

Message

Foster Snowhill Jan. 25, 2025, 11:54 p.m. UTC

iOS devices support two types of tethering over USB: regular, where the
internet connection 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 v5 series with iPhone 15 Pro Max,
iOS 18.2.1: compiled cleanly, ran iperf3 between phone and computer,
observed no errors in either kernel log or interface statistics.

v4 was Reviewed-by Jakub Kicinski <[e-mail]>. Compared to v4,
v5 has no code changes. The two differences are:

* Patch "usbnet: ipheth: break up NCM header size computation"
  moved later in the series, closer to a subsequent commit that makes
  use of the change.
* In patch "usbnet: ipheth: refactor NCM datagram loop", removed
  a stray paragraph in commit msg.

Above items are also noted in the changelogs of respective patches.


Foster Snowhill (7):
  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: break up NCM header size computation
  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

Bjørn Mork Jan. 26, 2025, 11:19 a.m. UTC | #1

Foster Snowhill <[e-mail]> writes:

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

Sorry for taking this up at such a late stage, and it might already been
discussed, but I'm not convinced that you shouldn't "mess" with the
`cdc_ncm` driver.  We did a lot of that when prepping it for cdc_mbim
reuse. Some of it, like supporting multiple NDPs, is completely useless
for NCM.  We still added code to cdc_ncm just to be able to share it
with cdc_mbim. I think the generalized solutions still adds value to
cdc_ncm by making the shared code targeted by more developers and users.

And the huawei_cdc_ncm driver demonstrates that cdc_ncm can be reused
for non-compliant vendor-specific protocols, so that's not a real
problem.

I don't really see the assymmetric TX/RX protocols as a big problem
either.  Reusing only the RX code from cdc_ncm should be fine.

What I can understand is that you don't want to build on the current
cdc_ncm code because you don't like the implementation and want to
improve it.  But this is also the main reason why I think code sharing
would be good.  The cdc_ncm code could certainly do with more developers
looking at it and improving stuff.  Like any other drivers, really.

Yes, I know I'm saying this much too late for code that's ready for
merging.  And, of course, I don't expect you to start all over at this
point. I just wanted to comment on that "messing with" because it's so
wrong, and I remember feeling that way too.  Messing with existing code
is never a problem in Linux!  Existing code and (internal) interfaces
can always be changed to accommodate whatever needs you have.  This is
one of may ways the Linux development model wins over everything else.


Bjørn

Foster Snowhill Jan. 26, 2025, 1:44 p.m. UTC | #2

Hello Bjørn,

Thank you very much for the feedback! In my opinion, it's never late.

On 2025-01-26 12:19, Bjørn Mork wrote:
> We did a lot of that when prepping it for cdc_mbim
> reuse. Some of it, like supporting multiple NDPs, is completely useless
> for NCM.  We still added code to cdc_ncm just to be able to share it
> with cdc_mbim. I think the generalized solutions still adds value to
> cdc_ncm by making the shared code targeted by more developers and users.
> 
> And the huawei_cdc_ncm driver demonstrates that cdc_ncm can be reused
> for non-compliant vendor-specific protocols, so that's not a real
> problem.

Very much appreciate the pointers to look at how `cdc_mbim` and
`huawei_cdc_ncm` share code with `cdc_ncm`, I'm definitely interested
and will have a look.

> Sorry for taking this up at such a late stage, and it might already been
> discussed, but I'm not convinced that you shouldn't "mess" with the
> `cdc_ncm` driver.
> 
> ...
> 
> What I can understand is that you don't want to build on the current
> cdc_ncm code because you don't like the implementation and want to
> improve it.  But this is also the main reason why I think code sharing
> would be good.  The cdc_ncm code could certainly do with more developers
> looking at it and improving stuff.  Like any other drivers, really.
> 
> Yes, I know I'm saying this much too late for code that's ready for
> merging.  And, of course, I don't expect you to start all over at this
> point. I just wanted to comment on that "messing with" because it's so
> wrong, and I remember feeling that way too.  Messing with existing code
> is never a problem in Linux!  Existing code and (internal) interfaces
> can always be changed to accommodate whatever needs you have.  This is
> one of may ways the Linux development model wins over everything else.

I call it "mess with it" not because I see something wrong with the
existing CDC drivers (I don't), but rather because of my own skill
level. This patch series is fixing mistakes I originally made when
implementing NCM mode in `ipheth`, so you can tell I still have a lot
of learning to do. Especially before touching better maintained and
afaik more widely used drivers like `cdc_ncm` and co.

I should've mentioned in the cover letter perhaps that I see this
series as more of an interim set of fixes to the issues I found
in my existing code. For longer term, I already have some ideas and
questions on further improving the `ipheth` driver, in particular:

1. Can I make `ipheth` use the `usbnet` framework, rather than it
   re-implementing lower-level things like USB interface handling?
2. How can I reuse parts of `cdc_ncm` to parse incoming URBs?
   Does it require complying with the `usbnet` framework?
3. How can I make use of multiple parallel USB transfers to improve
   throughput? Does `usbnet` support this already, or would it have
   to be implemented?
   Last time I checked, the official driver on macOS does 16 parallel
   transfers. This results in a significant useful throughput increase
   on RX on newer devices over 10 Gbps USB-C: from 650-850 Mbps on
   Linux with `ipheth` to ~3.1 Gbps on macOS.
4. How far back do I keep driver compatibility?
   So far all the changes have been non-breaking, I've been asking
   Georgi Valkov <[e-mail]> to test on devices as old as
   iPhone 3G with iOS 4.2.1, which pretty much covers all iOS devices
   and versions capable of internet sharing.
5. Even longer term, would be interesting to investigate the unknown
   control transfers that the official driver does. I avoid doing any
   reverse engineering myself ("clean room" principle), so thinking
   of maybe somehow emulating an iOS device and trying to return
   different values and observing how macOS behaves. Alternatively
   just asking someone else to do the reserve-engineering part, but
   not sure there's any interest in that.

To be clear, these questions are not for you specifically, Bjørn, just
my own notes on what I'd like to investigate next. But if you have any
insight or opinion on any the above, it's absolutely welcome.

These haven't been discussed before, so you didn't miss anything.
I didn't mention it in the cover letter because I'm not sure when I'll
be able to work on these items, didn't want to make promises.
Excuses-excuses, I know. :)