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

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

[Foster Snowhill]

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(-)

-- 
2.45.1

[PATCH net v4 1/7] usbnet: ipheth: break up NCM header size computation

[Foster Snowhill]

Originally, the total NCM header size was computed as the sum of two
vaguely labelled constants. While accurate, it wasn't particularly clear
where they were coming from.

Use sizes of existing NCM structs where available. Define the total
NDP16 size based on the maximum amount of DPEs that can fit into the
iOS-specific fixed-size header.

This change does not fix any particular issue. Rather, it introduces
intermediate constants that will simplify subsequent commits.
It should also make it clearer for the reader where the constant values
come from.

Signed-off-by: Foster Snowhill <forst@pen.gy>
---
v4:
    No code changes. Remove "Fixes" from the commit message, and clarify
    the purpose/scope of the commit.
v3: https://lore.kernel.org/netdev/20241123235432.821220-1-forst@pen.gy/
    * NDP16 header size is computed from max DPE count constant,
    not the other way around.
    * Split out from a monolithic patch in v2.
v2: https://lore.kernel.org/netdev/20240912211817.1707844-1-forst@pen.gy/
    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/
---
 drivers/net/usb/ipheth.c | 13 ++++++++++++-
 1 file changed, 12 insertions(+), 1 deletion(-)

diff --git a/drivers/net/usb/ipheth.c b/drivers/net/usb/ipheth.c
index 46afb95ffabe..2084b940b4ea 100644
--- a/drivers/net/usb/ipheth.c
+++ b/drivers/net/usb/ipheth.c
@@ -61,7 +61,18 @@
 #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 regardless of DPE count:
+ * - NTH16 (NCMH): 12 bytes, as per CDC NCM 1.0 spec
+ * - NDP16 (NCM0): 96 bytes, of which
+ *    - NDP16 fixed header: 8 bytes
+ *    - maximum of 22 DPEs (21 datagrams + trailer), 4 bytes each
+ */
+#define IPHETH_NDP16_MAX_DPE	22
+#define IPHETH_NDP16_HEADER_SIZE (sizeof(struct usb_cdc_ncm_ndp16) + \
+				  IPHETH_NDP16_MAX_DPE * \
+				  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
-- 
2.45.1

[PATCH net v4 2/7] usbnet: ipheth: fix possible overflow in DPE length check

[Foster Snowhill]

Originally, it was possible for the DPE length check to overflow if
wDatagramIndex + wDatagramLength > U16_MAX. This could lead to an OoB
read.

Move the wDatagramIndex term to the other side of the inequality.

An existing condition ensures that wDatagramIndex < urb->actual_length.

Fixes: a2d274c62e44 ("usbnet: ipheth: add CDC NCM support")
Signed-off-by: Foster Snowhill <forst@pen.gy>
---
v4:
    No change since v3.
v3: https://lore.kernel.org/netdev/20241123235432.821220-2-forst@pen.gy/
    Split out from a monolithic patch in v2 as an atomic change.
v2: https://lore.kernel.org/netdev/20240912211817.1707844-1-forst@pen.gy/
    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/
---
 drivers/net/usb/ipheth.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/net/usb/ipheth.c b/drivers/net/usb/ipheth.c
index 2084b940b4ea..4b590a5269fd 100644
--- a/drivers/net/usb/ipheth.c
+++ b/drivers/net/usb/ipheth.c
@@ -254,8 +254,8 @@ static int ipheth_rcvbulk_callback_ncm(struct urb *urb)
 	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) {
+		    le16_to_cpu(dpe->wDatagramLength) > urb->actual_length -
+		    le16_to_cpu(dpe->wDatagramIndex)) {
 			dev->net->stats.rx_length_errors++;
 			return retval;
 		}
-- 
2.45.1

[PATCH net v4 3/7] usbnet: ipheth: check that DPE points past NCM header

[Foster Snowhill]

By definition, a DPE points at the start of a network frame/datagram.
Thus it makes no sense for it to point at anything that's part of the
NCM header. It is not a security issue, but merely an indication of
a malformed DPE.

Enforce that all DPEs point at the data portion of the URB, past the
NCM header.

Fixes: a2d274c62e44 ("usbnet: ipheth: add CDC NCM support")
Signed-off-by: Foster Snowhill <forst@pen.gy>
---
v4:
    No change since v3.
v3: https://lore.kernel.org/netdev/20241123235432.821220-3-forst@pen.gy/
    Split out from a monolithic patch in v2 as an atomic change.
v2: https://lore.kernel.org/netdev/20240912211817.1707844-1-forst@pen.gy/
    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/
---
 drivers/net/usb/ipheth.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/net/usb/ipheth.c b/drivers/net/usb/ipheth.c
index 4b590a5269fd..48c79e69bb7b 100644
--- a/drivers/net/usb/ipheth.c
+++ b/drivers/net/usb/ipheth.c
@@ -253,7 +253,8 @@ static int ipheth_rcvbulk_callback_ncm(struct urb *urb)
 	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 ||
+		if (le16_to_cpu(dpe->wDatagramIndex) < IPHETH_NCM_HEADER_SIZE ||
+		    le16_to_cpu(dpe->wDatagramIndex) >= urb->actual_length ||
 		    le16_to_cpu(dpe->wDatagramLength) > urb->actual_length -
 		    le16_to_cpu(dpe->wDatagramIndex)) {
 			dev->net->stats.rx_length_errors++;
-- 
2.45.1

[PATCH net v4 4/7] usbnet: ipheth: use static NDP16 location in URB

[Foster Snowhill]

Original code allowed for the start of NDP16 to be anywhere within the
URB based on the `wNdpIndex` value in NTH16. Only the start position of
NDP16 was checked, so it was possible for even the fixed-length part
of NDP16 to extend past the end of URB, leading to an out-of-bounds
read.

On iOS devices, the NDP16 header always directly follows NTH16. Rely on
and check for this specific format.

This, along with NCM-specific minimal URB length check that already
exists, will ensure that the fixed-length part of NDP16 plus a set
amount of DPEs fit within the URB.

Note that this commit alone does not fully address the OoB read.
The limit on the amount of DPEs needs to be enforced separately.

Fixes: a2d274c62e44 ("usbnet: ipheth: add CDC NCM support")
Signed-off-by: Foster Snowhill <forst@pen.gy>
---
v4:
    Additionally check that ncmh->wNdpIndex points immediately after
    NTH16. This covers an unexpected on real devices, and highly
    unlikely otherwise, case where the bytes after NTH16 are set to
    `USB_CDC_NCM_NDP16_NOCRC_SIGN`, yet aren't the beginning of the
    NDP16 header.
v3: https://lore.kernel.org/netdev/20241123235432.821220-4-forst@pen.gy/
    Split out from a monolithic patch in v2 as an atomic change.
v2: https://lore.kernel.org/netdev/20240912211817.1707844-1-forst@pen.gy/
    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/
---
 drivers/net/usb/ipheth.c | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/drivers/net/usb/ipheth.c b/drivers/net/usb/ipheth.c
index 48c79e69bb7b..35f507db2c4a 100644
--- a/drivers/net/usb/ipheth.c
+++ b/drivers/net/usb/ipheth.c
@@ -237,15 +237,14 @@ 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) {
+	    /* On iOS, NDP16 directly follows NTH16 */
+	    ncmh->wNdpIndex != cpu_to_le16(sizeof(struct usb_cdc_ncm_nth16))) {
 		dev->net->stats.rx_errors++;
 		return retval;
 	}
 
-	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) {
+	ncm0 = urb->transfer_buffer + sizeof(struct usb_cdc_ncm_nth16);
+	if (ncm0->dwSignature != cpu_to_le32(USB_CDC_NCM_NDP16_NOCRC_SIGN)) {
 		dev->net->stats.rx_errors++;
 		return retval;
 	}
-- 
2.45.1

[PATCH net v4 5/7] usbnet: ipheth: refactor NCM datagram loop

[Foster Snowhill]

Introduce an rx_error label to reduce repetitions in the header
signature checks.

Store wDatagramIndex and wDatagramLength after endianness conversion to
avoid repeated le16_to_cpu() calls.

Rewrite the loop to return on a null trailing DPE, which is required
by the CDC NCM spec. In case it is missing, fall through to rx_error.

This change does not fix any particular issue. Its purpose is to
simplify a subsequent commit that fixes a potential OoB read by limiting
the maximum amount of processed DPEs.

Fix an out-of-bounds DPE read, limit the number of processed DPEs to
the amount that fits into the fixed-size NDP16 header.

Signed-off-by: Foster Snowhill <forst@pen.gy>
---
v4:
    Split from "usbnet: ipheth: refactor NCM datagram loop, fix DPE OoB
    read" in v3. This commit is responsible for the code refactor, while
    keeping the behaviour the same.
v3: https://lore.kernel.org/netdev/20241123235432.821220-5-forst@pen.gy/
    Split out from a monolithic patch in v2 as an atomic change.
v2: https://lore.kernel.org/netdev/20240912211817.1707844-1-forst@pen.gy/
    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/
---
 drivers/net/usb/ipheth.c | 42 ++++++++++++++++++++++------------------
 1 file changed, 23 insertions(+), 19 deletions(-)

diff --git a/drivers/net/usb/ipheth.c b/drivers/net/usb/ipheth.c
index 35f507db2c4a..03249208612e 100644
--- a/drivers/net/usb/ipheth.c
+++ b/drivers/net/usb/ipheth.c
@@ -224,9 +224,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;
 
@@ -238,39 +238,43 @@ static int ipheth_rcvbulk_callback_ncm(struct urb *urb)
 	ncmh = urb->transfer_buffer;
 	if (ncmh->dwSignature != cpu_to_le32(USB_CDC_NCM_NTH16_SIGN) ||
 	    /* On iOS, NDP16 directly follows NTH16 */
-	    ncmh->wNdpIndex != cpu_to_le16(sizeof(struct usb_cdc_ncm_nth16))) {
-		dev->net->stats.rx_errors++;
-		return retval;
-	}
+	    ncmh->wNdpIndex != cpu_to_le16(sizeof(struct usb_cdc_ncm_nth16)))
+		goto rx_error;
 
 	ncm0 = urb->transfer_buffer + sizeof(struct usb_cdc_ncm_nth16);
-	if (ncm0->dwSignature != cpu_to_le32(USB_CDC_NCM_NDP16_NOCRC_SIGN)) {
-		dev->net->stats.rx_errors++;
-		return retval;
-	}
+	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) < IPHETH_NCM_HEADER_SIZE ||
-		    le16_to_cpu(dpe->wDatagramIndex) >= urb->actual_length ||
-		    le16_to_cpu(dpe->wDatagramLength) > urb->actual_length -
-		    le16_to_cpu(dpe->wDatagramIndex)) {
+	while (true) {
+		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) {
 			dev->net->stats.rx_length_errors++;
 			return retval;
 		}
 
-		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)
-- 
2.45.1

[PATCH net v4 6/7] usbnet: ipheth: fix DPE OoB read

[Foster Snowhill]

Fix an out-of-bounds DPE read, limit the number of processed DPEs to
the amount that fits into the fixed-size NDP16 header.

Fixes: a2d274c62e44 ("usbnet: ipheth: add CDC NCM support")
Signed-off-by: Foster Snowhill <forst@pen.gy>
---
v4:
    Split from "usbnet: ipheth: refactor NCM datagram loop, fix DPE OoB
    read" in v3. This commit is responsible for addressing the potential
    OoB read.
v3: https://lore.kernel.org/netdev/20241123235432.821220-5-forst@pen.gy/
    Split out from a monolithic patch in v2 as an atomic change.
v2: https://lore.kernel.org/netdev/20240912211817.1707844-1-forst@pen.gy/
    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/
---
 drivers/net/usb/ipheth.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/net/usb/ipheth.c b/drivers/net/usb/ipheth.c
index 03249208612e..5347cd7e295b 100644
--- a/drivers/net/usb/ipheth.c
+++ b/drivers/net/usb/ipheth.c
@@ -246,7 +246,7 @@ static int ipheth_rcvbulk_callback_ncm(struct urb *urb)
 		goto rx_error;
 
 	dpe = ncm0->dpe16;
-	while (true) {
+	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);
 
@@ -268,8 +268,6 @@ static int ipheth_rcvbulk_callback_ncm(struct urb *urb)
 		retval = ipheth_consume_skb(buf, dg_len, dev);
 		if (retval != 0)
 			return retval;
-
-		dpe++;
 	}
 
 rx_error:
-- 
2.45.1

[PATCH net v4 7/7] usbnet: ipheth: document scope of NCM implementation

[Foster Snowhill]

Clarify that the "NCM" implementation in `ipheth` is very limited, as
iOS devices aren't compatible with the CDC NCM specification in regular
tethering mode.

For a standards-compliant implementation, one shall turn to
the `cdc_ncm` module.

Signed-off-by: Foster Snowhill <forst@pen.gy>
---
v4:
    No changes.
v3: https://lore.kernel.org/netdev/20241123235432.821220-6-forst@pen.gy/
    This comment was part of the commit message for v2. With v3, given
    how the patches are split up, it makes more sense to add the comment
    directly in code.
v2: https://lore.kernel.org/netdev/20240912211817.1707844-1-forst@pen.gy/
    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: n/a
---
 drivers/net/usb/ipheth.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/drivers/net/usb/ipheth.c b/drivers/net/usb/ipheth.c
index 5347cd7e295b..a19789b57190 100644
--- a/drivers/net/usb/ipheth.c
+++ b/drivers/net/usb/ipheth.c
@@ -218,6 +218,14 @@ static int ipheth_rcvbulk_callback_legacy(struct urb *urb)
 	return ipheth_consume_skb(buf, len, dev);
 }
 
+/* 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.
+ */
 static int ipheth_rcvbulk_callback_ncm(struct urb *urb)
 {
 	struct usb_cdc_ncm_nth16 *ncmh;
-- 
2.45.1

Re: [PATCH net v4 6/7] usbnet: ipheth: fix DPE OoB read

[Greg KH]

On Sun, Jan 05, 2025 at 02:01:20AM +0100, Foster Snowhill wrote:
> Fix an out-of-bounds DPE read, limit the number of processed DPEs to
> the amount that fits into the fixed-size NDP16 header.
> 
> Fixes: a2d274c62e44 ("usbnet: ipheth: add CDC NCM support")
> Signed-off-by: Foster Snowhill <forst@pen.gy>
> ---
> v4:
>     Split from "usbnet: ipheth: refactor NCM datagram loop, fix DPE OoB
>     read" in v3. This commit is responsible for addressing the potential
>     OoB read.
> v3: https://lore.kernel.org/netdev/20241123235432.821220-5-forst@pen.gy/
>     Split out from a monolithic patch in v2 as an atomic change.
> v2: https://lore.kernel.org/netdev/20240912211817.1707844-1-forst@pen.gy/
>     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/
> ---
>  drivers/net/usb/ipheth.c | 4 +---
>  1 file changed, 1 insertion(+), 3 deletions(-)

Hi,

This is the friendly patch-bot of Greg Kroah-Hartman.  You have sent him
a patch that has triggered this response.  He used to manually respond
to these common problems, but in order to save his sanity (he kept
writing the same thing over and over, yet to different people), I was
created.  Hopefully you will not take offence and will fix the problem
in your patch and resubmit it so that it can be accepted into the Linux
kernel tree.

You are receiving this message because of the following common error(s)
as indicated below:

- You have marked a patch with a "Fixes:" tag for a commit that is in an
  older released kernel, yet you do not have a cc: stable line in the
  signed-off-by area at all, which means that the patch will not be
  applied to any older kernel releases.  To properly fix this, please
  follow the documented rules in the
  Documentation/process/stable-kernel-rules.rst file for how to resolve
  this.

If you wish to discuss this problem further, or you have questions about
how to resolve this issue, please feel free to respond to this email and
Greg will reply once he has dug out from the pending patches received
from other developers.

thanks,

greg k-h's patch email bot

Re: [PATCH net v4 4/7] usbnet: ipheth: use static NDP16 location in URB

[Greg KH]

On Sun, Jan 05, 2025 at 02:01:18AM +0100, Foster Snowhill wrote:
> Original code allowed for the start of NDP16 to be anywhere within the
> URB based on the `wNdpIndex` value in NTH16. Only the start position of
> NDP16 was checked, so it was possible for even the fixed-length part
> of NDP16 to extend past the end of URB, leading to an out-of-bounds
> read.
> 
> On iOS devices, the NDP16 header always directly follows NTH16. Rely on
> and check for this specific format.
> 
> This, along with NCM-specific minimal URB length check that already
> exists, will ensure that the fixed-length part of NDP16 plus a set
> amount of DPEs fit within the URB.
> 
> Note that this commit alone does not fully address the OoB read.
> The limit on the amount of DPEs needs to be enforced separately.
> 
> Fixes: a2d274c62e44 ("usbnet: ipheth: add CDC NCM support")
> Signed-off-by: Foster Snowhill <forst@pen.gy>
> ---
> v4:
>     Additionally check that ncmh->wNdpIndex points immediately after
>     NTH16. This covers an unexpected on real devices, and highly
>     unlikely otherwise, case where the bytes after NTH16 are set to
>     `USB_CDC_NCM_NDP16_NOCRC_SIGN`, yet aren't the beginning of the
>     NDP16 header.
> v3: https://lore.kernel.org/netdev/20241123235432.821220-4-forst@pen.gy/
>     Split out from a monolithic patch in v2 as an atomic change.
> v2: https://lore.kernel.org/netdev/20240912211817.1707844-1-forst@pen.gy/
>     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/
> ---
>  drivers/net/usb/ipheth.c | 9 ++++-----
>  1 file changed, 4 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/net/usb/ipheth.c b/drivers/net/usb/ipheth.c
> index 48c79e69bb7b..35f507db2c4a 100644
> --- a/drivers/net/usb/ipheth.c
> +++ b/drivers/net/usb/ipheth.c
> @@ -237,15 +237,14 @@ 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) {
> +	    /* On iOS, NDP16 directly follows NTH16 */
> +	    ncmh->wNdpIndex != cpu_to_le16(sizeof(struct usb_cdc_ncm_nth16))) {
>  		dev->net->stats.rx_errors++;
>  		return retval;
>  	}
>  
> -	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) {
> +	ncm0 = urb->transfer_buffer + sizeof(struct usb_cdc_ncm_nth16);
> +	if (ncm0->dwSignature != cpu_to_le32(USB_CDC_NCM_NDP16_NOCRC_SIGN)) {
>  		dev->net->stats.rx_errors++;
>  		return retval;
>  	}
> -- 
> 2.45.1
> 
> 

Hi,

This is the friendly patch-bot of Greg Kroah-Hartman.  You have sent him
a patch that has triggered this response.  He used to manually respond
to these common problems, but in order to save his sanity (he kept
writing the same thing over and over, yet to different people), I was
created.  Hopefully you will not take offence and will fix the problem
in your patch and resubmit it so that it can be accepted into the Linux
kernel tree.

You are receiving this message because of the following common error(s)
as indicated below:

- You have marked a patch with a "Fixes:" tag for a commit that is in an
  older released kernel, yet you do not have a cc: stable line in the
  signed-off-by area at all, which means that the patch will not be
  applied to any older kernel releases.  To properly fix this, please
  follow the documented rules in the
  Documentation/process/stable-kernel-rules.rst file for how to resolve
  this.

If you wish to discuss this problem further, or you have questions about
how to resolve this issue, please feel free to respond to this email and
Greg will reply once he has dug out from the pending patches received
from other developers.

thanks,

greg k-h's patch email bot

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

[Jakub Kicinski]

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 <kuba@kernel.org>

Please add that to each patch, address Greg's comment, and repost.
-- 
pw-bot: cr

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

[Foster Snowhill]

Hello Jakub,

> Reviewed-by: Jakub Kicinski <kuba@kernel.org>
> 
> 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/20250105010121.12546-2-forst@pen.gy/
[p5]: https://lore.kernel.org/netdev/20250105010121.12546-6-forst@pen.gy/
[p6]: https://lore.kernel.org/netdev/20250105010121.12546-7-forst@pen.gy/
[p7]: https://lore.kernel.org/netdev/20250105010121.12546-8-forst@pen.gy/

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

[Jakub Kicinski]

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.