| Message ID | 20240907230108.978355-1-forst@pen.gy (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net-next] usbnet: ipheth: prevent OoB reads of NDP16 | expand |
Hi, On 08.09.24 01:01, Foster Snowhill wrote: > To address the above issues without reimplementing more of CDC NCM, > 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) I am afraid this is an approach we must not take. We cannot rely on a specific device's behavior in a class driver. This is a NACK. Regards Oliver
Hello Oliver, Thank you for the feedback. >> To address the above issues without reimplementing more of CDC NCM, >> 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) > > I am afraid this is an approach we must not take. We cannot rely on > a specific device's behavior in a class driver. > > This is a NACK. The `ipheth` driver, that the patch is for, is designed specifically for interacting with iPhones. iPhones' "NCM" implementation for regular tethering (sharing mobile/cellular internet with an attached Linux system) 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 the `ipheth` 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 (sharing computer's internet connection with an attached phone) on iPhones. This patch does not in any way change `cdc_ncm`. With all of the above, does your NACK still stand? Thanks!
On 09.09.24 11:04, Foster Snowhill wrote: > Thus the `ipheth` 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 (sharing > computer's internet connection with an attached phone) on iPhones. This > patch does not in any way change `cdc_ncm`. > > With all of the above, does your NACK still stand? Thanks! Hi, I am sorry for the confusion. May I suggest a reformulation of the commit message. It reads like this patch is intended for generic CDC-NCM. I withdraw my objections. They were based on a confusion. Regards Oliver
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.
On 09.09.24 15:39, Foster Snowhill wrote: > > 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. Splendid. Perfect. Regards Oliver
diff --git a/drivers/net/usb/ipheth.c b/drivers/net/usb/ipheth.c index 46afb95ffabe..8c62501f47a9 100644 --- a/drivers/net/usb/ipheth.c +++ b/drivers/net/usb/ipheth.c @@ -61,7 +61,16 @@ #define IPHETH_USBINTF_PROTO 1 #define IPHETH_IP_ALIGN 2 /* padding at front of URB */ -#define IPHETH_NCM_HEADER_SIZE (12 + 96) /* NCMH + NCM0 */ +/* On iOS devices, NCM headers in RX have a fixed size: + * - NTH16 (NCMH): 12 bytes, as per CDC NCM 1.0 spec + * - NDP16 (NCM0): 96 bytes + */ +#define IPHETH_NDP16_HEADER_SIZE 96 +#define IPHETH_NDP16_MAX_DPE ((IPHETH_NDP16_HEADER_SIZE - \ + sizeof(struct usb_cdc_ncm_ndp16)) / \ + sizeof(struct usb_cdc_ncm_dpe16)) +#define IPHETH_NCM_HEADER_SIZE (sizeof(struct usb_cdc_ncm_nth16) + \ + IPHETH_NDP16_HEADER_SIZE) #define IPHETH_TX_BUF_SIZE ETH_FRAME_LEN #define IPHETH_RX_BUF_SIZE_LEGACY (IPHETH_IP_ALIGN + ETH_FRAME_LEN) #define IPHETH_RX_BUF_SIZE_NCM 65536 @@ -213,9 +222,9 @@ static int ipheth_rcvbulk_callback_ncm(struct urb *urb) struct usb_cdc_ncm_ndp16 *ncm0; struct usb_cdc_ncm_dpe16 *dpe; struct ipheth_device *dev; + u16 dg_idx, dg_len; int retval = -EINVAL; char *buf; - int len; dev = urb->context; @@ -225,41 +234,40 @@ static int ipheth_rcvbulk_callback_ncm(struct urb *urb) } ncmh = urb->transfer_buffer; - if (ncmh->dwSignature != cpu_to_le32(USB_CDC_NCM_NTH16_SIGN) || - le16_to_cpu(ncmh->wNdpIndex) >= urb->actual_length) { - dev->net->stats.rx_errors++; - return retval; - } + if (ncmh->dwSignature != cpu_to_le32(USB_CDC_NCM_NTH16_SIGN)) + goto rx_error; - ncm0 = urb->transfer_buffer + le16_to_cpu(ncmh->wNdpIndex); - if (ncm0->dwSignature != cpu_to_le32(USB_CDC_NCM_NDP16_NOCRC_SIGN) || - le16_to_cpu(ncmh->wHeaderLength) + le16_to_cpu(ncm0->wLength) >= - urb->actual_length) { - dev->net->stats.rx_errors++; - return retval; - } + /* On iOS, NDP16 directly follows NTH16 */ + ncm0 = urb->transfer_buffer + sizeof(struct usb_cdc_ncm_nth16); + if (ncm0->dwSignature != cpu_to_le32(USB_CDC_NCM_NDP16_NOCRC_SIGN)) + goto rx_error; dpe = ncm0->dpe16; - while (le16_to_cpu(dpe->wDatagramIndex) != 0 && - le16_to_cpu(dpe->wDatagramLength) != 0) { - if (le16_to_cpu(dpe->wDatagramIndex) >= urb->actual_length || - le16_to_cpu(dpe->wDatagramIndex) + - le16_to_cpu(dpe->wDatagramLength) > urb->actual_length) { - dev->net->stats.rx_length_errors++; - return retval; - } + for (int dpe_i = 0; dpe_i < IPHETH_NDP16_MAX_DPE; ++dpe_i, ++dpe) { + dg_idx = le16_to_cpu(dpe->wDatagramIndex); + dg_len = le16_to_cpu(dpe->wDatagramLength); + + /* Null DPE must be present after last datagram pointer entry + * (3.3.1 USB CDC NCM spec v1.0) + */ + if (dg_idx == 0 && dg_len == 0) + return 0; + + if (dg_idx < IPHETH_NCM_HEADER_SIZE || + dg_idx >= urb->actual_length || + dg_len > urb->actual_length - dg_idx) + goto rx_error; - buf = urb->transfer_buffer + le16_to_cpu(dpe->wDatagramIndex); - len = le16_to_cpu(dpe->wDatagramLength); + buf = urb->transfer_buffer + dg_idx; - retval = ipheth_consume_skb(buf, len, dev); + retval = ipheth_consume_skb(buf, dg_len, dev); if (retval != 0) return retval; - - dpe++; } - return 0; +rx_error: + dev->net->stats.rx_errors++; + return retval; } static void ipheth_rcvbulk_callback(struct urb *urb)