diff mbox series

[net-next,v2] usbnet: ipheth: prevent OoB reads of NDP16

Message ID 20240912211817.1707844-1-forst@pen.gy
State New
Headers show
Series [net-next,v2] usbnet: ipheth: prevent OoB reads of NDP16 | expand

Commit Message

Foster Snowhill Sept. 12, 2024, 9:18 p.m. UTC 
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.

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.

Signed-off-by: Foster Snowhill <forst@pen.gy>
Tested-by: Georgi Valkov <gvalkov@gmail.com>
---
v2: No code changes. Update commit message to further clarify that
    `ipheth` is not and does not aim to be a complete or spec-compliant
    CDC NCM implementation.
v1: https://lore.kernel.org/netdev/20240907230108.978355-1-forst@pen.gy/

This should perhaps go into "net" rather than "net-next"? I submitted
the previous patch series to "net-next", but it got merged into "net"
[1]. However it's quite late in the 6.11-rc cycle, so not sure.

[1]: https://lore.kernel.org/netdev/172320844026.3782387.2037318141249570355.git-patchwork-notify@kernel.org/
---
 drivers/net/usb/ipheth.c | 64 ++++++++++++++++++++++------------------
 1 file changed, 36 insertions(+), 28 deletions(-)

Comments

Paolo Abeni Sept. 19, 2024, 8:05 a.m. UTC | #1 
On 9/12/24 23:18, Foster Snowhill wrote:
> 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.
> 
> 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.
> 
> Signed-off-by: Foster Snowhill <forst@pen.gy>
> Tested-by: Georgi Valkov <gvalkov@gmail.com>
> ---
> v2: No code changes. Update commit message to further clarify that
>      `ipheth` is not and does not aim to be a complete or spec-compliant
>      CDC NCM implementation.
> v1: https://lore.kernel.org/netdev/20240907230108.978355-1-forst@pen.gy/
> 
> This should perhaps go into "net" rather than "net-next"? I submitted
> the previous patch series to "net-next", but it got merged into "net"
> [1]. However it's quite late in the 6.11-rc cycle, so not sure.

This indeed looks like a fix. I suggest to post it for the net tree 
including a suitable fixes tag.

Additionally since it looks like the patch addressed several issues, it 
would be probably better to split it in a small series, each patch 
addressing a single issue - and each patch with it's own fixed tag.

Thanks,

Paolo
diff mbox series

Patch

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)