Re: [PATCH net-next] usbnet: ipheth: prevent OoB reads of NDP16

[Foster Snowhill <forst@pen.gy>]

Hi,

I think you already got the idea, but just in case, a more concise
explanation for Apple's tethering implementation would be "they just
happened to use NCM encapsulation for RX, everything else about it
has nothing to do with CDC NCM".

On 2024-09-09 13:04, Oliver Neukum wrote:
> May I suggest a reformulation of the
> commit message. It reads like this patch is intended for generic CDC-NCM.

No problem, does the commit message below read better? Suggestions are
absolutely welcome.

For one, I added a paragraph closer to the beginning that's explicit
about the intentions of this driver: it doesn't aim to be and can't be
a generic spec-compliant implementation. I can't avoid naming "CDC NCM"
completely, but I only use it in the first paragraph to clarify the
difference. There was one subsequent mention of it, and I replaced it
with a more generic "NCM mode".

If this is good, I'll give v1 a day or two more for any more feedback,
and then resubmit v2 with the updated commit message.

Cheers,
Foster

---

usbnet: ipheth: prevent OoB reads of NDP16

In "NCM mode", the iOS device encapsulates RX (phone->computer) traffic
in NCM Transfer Blocks (similarly to CDC NCM). However, unlike reverse
tethering (handled by the `cdc_ncm` driver), regular tethering is not
compliant with the CDC NCM spec, as the device is missing the necessary
descriptors, and TX (computer->phone) traffic is not encapsulated
at all. Thus `ipheth` implements a very limited subset of the spec with
the sole purpose of parsing RX URBs.

In the first iteration of the NCM mode implementation, 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.

The format above was observed on all iOS versions starting with iOS 16,
where NCM mode was introduced, up until the latest stable iOS release,
which is iOS 17.6.1 at the moment of writing.

Adapt the driver to use the fixed URB format. Set an upper bound for
the DPE count based on the expected header size. Always expect a null
trailer DPE.

The minimal URB length of 108 bytes (IPHETH_NCM_HEADER_SIZE) in NCM mode
is already enforced in ipheth since introduction of NCM mode support.