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

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

[Foster Snowhill]

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

-- 
2.45.1

[PATCH net v5 1/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")
Cc: stable@vger.kernel.org
Signed-off-by: Foster Snowhill <forst@pen.gy>
Reviewed-by: Jakub Kicinski <kuba@kernel.org>
---
v5:
    No code changes. Added Cc to stable and Reviewed-by Jakub tags.
v4: https://lore.kernel.org/netdev/20250105010121.12546-3-forst@pen.gy/
    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 46afb95ffabe..45daae234cb8 100644
--- a/drivers/net/usb/ipheth.c
+++ b/drivers/net/usb/ipheth.c
@@ -243,8 +243,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 v5 2/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")
Cc: stable@vger.kernel.org
Signed-off-by: Foster Snowhill <forst@pen.gy>
Reviewed-by: Jakub Kicinski <kuba@kernel.org>
---
v5:
    No code changes. Added Cc to stable and Reviewed-by Jakub tags.
v4: https://lore.kernel.org/netdev/20250105010121.12546-4-forst@pen.gy/
    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 45daae234cb8..1ff5f7076ad5 100644
--- a/drivers/net/usb/ipheth.c
+++ b/drivers/net/usb/ipheth.c
@@ -242,7 +242,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 v5 3/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")
Cc: stable@vger.kernel.org
Signed-off-by: Foster Snowhill <forst@pen.gy>
Reviewed-by: Jakub Kicinski <kuba@kernel.org>
---
v5:
    No code changes. Added Cc to stable and Reviewed-by Jakub tags.
v4: https://lore.kernel.org/netdev/20250105010121.12546-5-forst@pen.gy/
    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 1ff5f7076ad5..c385623596d2 100644
--- a/drivers/net/usb/ipheth.c
+++ b/drivers/net/usb/ipheth.c
@@ -226,15 +226,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 v5 4/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.

Cc: stable@vger.kernel.org # 6.5.x
Signed-off-by: Foster Snowhill <forst@pen.gy>
Reviewed-by: Jakub Kicinski <kuba@kernel.org>
---
v5:
    No code changes. Removed stray "Fix an out-of-bounds DPE read..."
    paragraph from commit msg. Added Cc to stable and Reviewed-by Jakub
    tags.
v4: https://lore.kernel.org/netdev/20250105010121.12546-6-forst@pen.gy/
    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 c385623596d2..069979e2bb6e 100644
--- a/drivers/net/usb/ipheth.c
+++ b/drivers/net/usb/ipheth.c
@@ -213,9 +213,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;
 
@@ -227,39 +227,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 v5 5/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.

Cc: stable@vger.kernel.org # 6.5.x
Signed-off-by: Foster Snowhill <forst@pen.gy>
Reviewed-by: Jakub Kicinski <kuba@kernel.org>
---
v5:
    No code changes. Moved patch closer in series to subsequent patch
    "usbnet: ipheth: fix DPE OoB read", as that is when one of the
    introduced preprocessor constants is first used. Added Cc to stable
    and Reviewed-by Jakub tags.
v4: https://lore.kernel.org/netdev/20250105010121.12546-2-forst@pen.gy/
    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 069979e2bb6e..03249208612e 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 v5 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")
Cc: stable@vger.kernel.org
Signed-off-by: Foster Snowhill <forst@pen.gy>
Reviewed-by: Jakub Kicinski <kuba@kernel.org>
---
v5:
    No code changes. Added Cc to stable and Reviewed-by Jakub tags.
v4: https://lore.kernel.org/netdev/20250105010121.12546-7-forst@pen.gy/
    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 v5 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.

Cc: stable@vger.kernel.org # 6.5.x
Signed-off-by: Foster Snowhill <forst@pen.gy>
Reviewed-by: Jakub Kicinski <kuba@kernel.org>
---
v5:
    No code changes. Added Cc to stable and Reviewed-by Jakub tags.
v4: https://lore.kernel.org/netdev/20250105010121.12546-8-forst@pen.gy/
    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 v5 0/7] usbnet: ipheth: prevent OoB reads of NDP16

[Bjørn Mork]

Foster Snowhill <forst@pen.gy> 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

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

[Foster Snowhill]

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 <gvalkov@gmail.com> 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. :)

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

[patchwork-bot+netdevbpf]

Hello:

This series was applied to netdev/net.git (main)
by Paolo Abeni <pabeni@redhat.com>:

On Sun, 26 Jan 2025 00:54:02 +0100 you wrote:
> 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.
> 
> [...]

Here is the summary with links:
  - [net,v5,1/7] usbnet: ipheth: fix possible overflow in DPE length check
    https://git.kernel.org/netdev/net/c/c219427ed296
  - [net,v5,2/7] usbnet: ipheth: check that DPE points past NCM header
    https://git.kernel.org/netdev/net/c/429fa68b58ce
  - [net,v5,3/7] usbnet: ipheth: use static NDP16 location in URB
    https://git.kernel.org/netdev/net/c/86586dcb75cb
  - [net,v5,4/7] usbnet: ipheth: refactor NCM datagram loop
    https://git.kernel.org/netdev/net/c/2a9a196429e9
  - [net,v5,5/7] usbnet: ipheth: break up NCM header size computation
    https://git.kernel.org/netdev/net/c/efcbc678a14b
  - [net,v5,6/7] usbnet: ipheth: fix DPE OoB read
    https://git.kernel.org/netdev/net/c/ee591f2b2817
  - [net,v5,7/7] usbnet: ipheth: document scope of NCM implementation
    https://git.kernel.org/netdev/net/c/be154b598fa5

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html