Skip to content

Commit 3763dba

Browse files
httpstormhttpstorm
committed
ipheth: fix DPE OoB read
integrate upstream patches to fix DPE OoB read 1. fix possible overflow in DPE length check 2. check that DPE points past NCM header 3. use static NDP16 location in URB 4. refactor NCM datagram loop 5. break up NCM header size computation 6. fix DPE OoB read 7. document scope of NCM implementation Signed-off-by: Georgi Valkov <[email protected]>
1 parent 02f0aa7 commit 3763dba

7 files changed

+344
-0
lines changed

‎target/linux/generic/pending-6.6/781-usbnet-ipheth-fix-possible-overflow-in-DPE-length-ch.patch‎

Lines changed: 35 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,35 @@
1+
From c219427ed296f94bb4b91d08626776dc7719ee27 Mon Sep 17 00:00:00 2001
2+
From: Foster Snowhill <[email protected]>
3+
Date: Sun, 26 Jan 2025 00:54:03 +0100
4+
Subject: [PATCH 1/7] usbnet: ipheth: fix possible overflow in DPE length check
5+
6+
Originally, it was possible for the DPE length check to overflow if
7+
wDatagramIndex + wDatagramLength > U16_MAX. This could lead to an OoB
8+
read.
9+
10+
Move the wDatagramIndex term to the other side of the inequality.
11+
12+
An existing condition ensures that wDatagramIndex < urb->actual_length.
13+
14+
Fixes: a2d274c62e44 ("usbnet: ipheth: add CDC NCM support")
15+
Cc: [email protected]
16+
Signed-off-by: Foster Snowhill <[email protected]>
17+
Reviewed-by: Jakub Kicinski <[email protected]>
18+
Signed-off-by: Paolo Abeni <[email protected]>
19+
---
20+
drivers/net/usb/ipheth.c | 4 ++--
21+
1 file changed, 2 insertions(+), 2 deletions(-)
22+
23+
--- a/drivers/net/usb/ipheth.c
24+
+++ b/drivers/net/usb/ipheth.c
25+
@@ -243,8 +243,8 @@ static int ipheth_rcvbulk_callback_ncm(s
26+
while (le16_to_cpu(dpe->wDatagramIndex) != 0 &&
27+
le16_to_cpu(dpe->wDatagramLength) != 0) {
28+
if (le16_to_cpu(dpe->wDatagramIndex) >= urb->actual_length ||
29+
- le16_to_cpu(dpe->wDatagramIndex) +
30+
- le16_to_cpu(dpe->wDatagramLength) > urb->actual_length) {
31+
+ le16_to_cpu(dpe->wDatagramLength) > urb->actual_length -
32+
+ le16_to_cpu(dpe->wDatagramIndex)) {
33+
dev->net->stats.rx_length_errors++;
34+
return retval;
35+
}

‎target/linux/generic/pending-6.6/782-usbnet-ipheth-check-that-DPE-points-past-NCM-header.patch‎

Lines changed: 34 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,34 @@
1+
From 429fa68b58cefb9aa9de27e4089637298b46b757 Mon Sep 17 00:00:00 2001
2+
From: Foster Snowhill <[email protected]>
3+
Date: Sun, 26 Jan 2025 00:54:04 +0100
4+
Subject: [PATCH 2/7] usbnet: ipheth: check that DPE points past NCM header
5+
6+
By definition, a DPE points at the start of a network frame/datagram.
7+
Thus it makes no sense for it to point at anything that's part of the
8+
NCM header. It is not a security issue, but merely an indication of
9+
a malformed DPE.
10+
11+
Enforce that all DPEs point at the data portion of the URB, past the
12+
NCM header.
13+
14+
Fixes: a2d274c62e44 ("usbnet: ipheth: add CDC NCM support")
15+
Cc: [email protected]
16+
Signed-off-by: Foster Snowhill <[email protected]>
17+
Reviewed-by: Jakub Kicinski <[email protected]>
18+
Signed-off-by: Paolo Abeni <[email protected]>
19+
---
20+
drivers/net/usb/ipheth.c | 3 ++-
21+
1 file changed, 2 insertions(+), 1 deletion(-)
22+
23+
--- a/drivers/net/usb/ipheth.c
24+
+++ b/drivers/net/usb/ipheth.c
25+
@@ -242,7 +242,8 @@ static int ipheth_rcvbulk_callback_ncm(s
26+
dpe = ncm0->dpe16;
27+
while (le16_to_cpu(dpe->wDatagramIndex) != 0 &&
28+
le16_to_cpu(dpe->wDatagramLength) != 0) {
29+
- if (le16_to_cpu(dpe->wDatagramIndex) >= urb->actual_length ||
30+
+ if (le16_to_cpu(dpe->wDatagramIndex) < IPHETH_NCM_HEADER_SIZE ||
31+
+ le16_to_cpu(dpe->wDatagramIndex) >= urb->actual_length ||
32+
le16_to_cpu(dpe->wDatagramLength) > urb->actual_length -
33+
le16_to_cpu(dpe->wDatagramIndex)) {
34+
dev->net->stats.rx_length_errors++;

‎target/linux/generic/pending-6.6/783-usbnet-ipheth-use-static-NDP16-location-in-URB.patch‎

Lines changed: 52 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,52 @@
1+
From 86586dcb75cb8fd062a518aca8ee667938b91efb Mon Sep 17 00:00:00 2001
2+
From: Foster Snowhill <[email protected]>
3+
Date: Sun, 26 Jan 2025 00:54:05 +0100
4+
Subject: [PATCH 3/7] usbnet: ipheth: use static NDP16 location in URB
5+
6+
Original code allowed for the start of NDP16 to be anywhere within the
7+
URB based on the `wNdpIndex` value in NTH16. Only the start position of
8+
NDP16 was checked, so it was possible for even the fixed-length part
9+
of NDP16 to extend past the end of URB, leading to an out-of-bounds
10+
read.
11+
12+
On iOS devices, the NDP16 header always directly follows NTH16. Rely on
13+
and check for this specific format.
14+
15+
This, along with NCM-specific minimal URB length check that already
16+
exists, will ensure that the fixed-length part of NDP16 plus a set
17+
amount of DPEs fit within the URB.
18+
19+
Note that this commit alone does not fully address the OoB read.
20+
The limit on the amount of DPEs needs to be enforced separately.
21+
22+
Fixes: a2d274c62e44 ("usbnet: ipheth: add CDC NCM support")
23+
Cc: [email protected]
24+
Signed-off-by: Foster Snowhill <[email protected]>
25+
Reviewed-by: Jakub Kicinski <[email protected]>
26+
Signed-off-by: Paolo Abeni <[email protected]>
27+
---
28+
drivers/net/usb/ipheth.c | 9 ++++-----
29+
1 file changed, 4 insertions(+), 5 deletions(-)
30+
31+
--- a/drivers/net/usb/ipheth.c
32+
+++ b/drivers/net/usb/ipheth.c
33+
@@ -226,15 +226,14 @@ static int ipheth_rcvbulk_callback_ncm(s
34+
35+
ncmh = urb->transfer_buffer;
36+
if (ncmh->dwSignature != cpu_to_le32(USB_CDC_NCM_NTH16_SIGN) ||
37+
- le16_to_cpu(ncmh->wNdpIndex) >= urb->actual_length) {
38+
+ /* On iOS, NDP16 directly follows NTH16 */
39+
+ ncmh->wNdpIndex != cpu_to_le16(sizeof(struct usb_cdc_ncm_nth16))) {
40+
dev->net->stats.rx_errors++;
41+
return retval;
42+
}
43+
44+
- ncm0 = urb->transfer_buffer + le16_to_cpu(ncmh->wNdpIndex);
45+
- if (ncm0->dwSignature != cpu_to_le32(USB_CDC_NCM_NDP16_NOCRC_SIGN) ||
46+
- le16_to_cpu(ncmh->wHeaderLength) + le16_to_cpu(ncm0->wLength) >=
47+
- urb->actual_length) {
48+
+ ncm0 = urb->transfer_buffer + sizeof(struct usb_cdc_ncm_nth16);
49+
+ if (ncm0->dwSignature != cpu_to_le32(USB_CDC_NCM_NDP16_NOCRC_SIGN)) {
50+
dev->net->stats.rx_errors++;
51+
return retval;
52+
}

‎target/linux/generic/pending-6.6/784-usbnet-ipheth-refactor-NCM-datagram-loop.patch‎

Lines changed: 101 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,101 @@
1+
From 2a9a196429e98fcc64078366c2679bc40aba5466 Mon Sep 17 00:00:00 2001
2+
From: Foster Snowhill <[email protected]>
3+
Date: Sun, 26 Jan 2025 00:54:06 +0100
4+
Subject: [PATCH 4/7] usbnet: ipheth: refactor NCM datagram loop
5+
6+
Introduce an rx_error label to reduce repetitions in the header
7+
signature checks.
8+
9+
Store wDatagramIndex and wDatagramLength after endianness conversion to
10+
avoid repeated le16_to_cpu() calls.
11+
12+
Rewrite the loop to return on a null trailing DPE, which is required
13+
by the CDC NCM spec. In case it is missing, fall through to rx_error.
14+
15+
This change does not fix any particular issue. Its purpose is to
16+
simplify a subsequent commit that fixes a potential OoB read by limiting
17+
the maximum amount of processed DPEs.
18+
19+
Cc: [email protected] # 6.5.x
20+
Signed-off-by: Foster Snowhill <[email protected]>
21+
Reviewed-by: Jakub Kicinski <[email protected]>
22+
Signed-off-by: Paolo Abeni <[email protected]>
23+
---
24+
drivers/net/usb/ipheth.c | 42 ++++++++++++++++++++++------------------
25+
1 file changed, 23 insertions(+), 19 deletions(-)
26+
27+
--- a/drivers/net/usb/ipheth.c
28+
+++ b/drivers/net/usb/ipheth.c
29+
@@ -213,9 +213,9 @@ static int ipheth_rcvbulk_callback_ncm(s
30+
struct usb_cdc_ncm_ndp16 *ncm0;
31+
struct usb_cdc_ncm_dpe16 *dpe;
32+
struct ipheth_device *dev;
33+
+ u16 dg_idx, dg_len;
34+
int retval = -EINVAL;
35+
char *buf;
36+
- int len;
37+
38+
dev = urb->context;
39+
40+
@@ -227,39 +227,43 @@ static int ipheth_rcvbulk_callback_ncm(s
41+
ncmh = urb->transfer_buffer;
42+
if (ncmh->dwSignature != cpu_to_le32(USB_CDC_NCM_NTH16_SIGN) ||
43+
/* On iOS, NDP16 directly follows NTH16 */
44+
- ncmh->wNdpIndex != cpu_to_le16(sizeof(struct usb_cdc_ncm_nth16))) {
45+
- dev->net->stats.rx_errors++;
46+
- return retval;
47+
- }
48+
+ ncmh->wNdpIndex != cpu_to_le16(sizeof(struct usb_cdc_ncm_nth16)))
49+
+ goto rx_error;
50+
51+
ncm0 = urb->transfer_buffer + sizeof(struct usb_cdc_ncm_nth16);
52+
- if (ncm0->dwSignature != cpu_to_le32(USB_CDC_NCM_NDP16_NOCRC_SIGN)) {
53+
- dev->net->stats.rx_errors++;
54+
- return retval;
55+
- }
56+
+ if (ncm0->dwSignature != cpu_to_le32(USB_CDC_NCM_NDP16_NOCRC_SIGN))
57+
+ goto rx_error;
58+
59+
dpe = ncm0->dpe16;
60+
- while (le16_to_cpu(dpe->wDatagramIndex) != 0 &&
61+
- le16_to_cpu(dpe->wDatagramLength) != 0) {
62+
- if (le16_to_cpu(dpe->wDatagramIndex) < IPHETH_NCM_HEADER_SIZE ||
63+
- le16_to_cpu(dpe->wDatagramIndex) >= urb->actual_length ||
64+
- le16_to_cpu(dpe->wDatagramLength) > urb->actual_length -
65+
- le16_to_cpu(dpe->wDatagramIndex)) {
66+
+ while (true) {
67+
+ dg_idx = le16_to_cpu(dpe->wDatagramIndex);
68+
+ dg_len = le16_to_cpu(dpe->wDatagramLength);
69+
+
70+
+ /* Null DPE must be present after last datagram pointer entry
71+
+ * (3.3.1 USB CDC NCM spec v1.0)
72+
+ */
73+
+ if (dg_idx == 0 && dg_len == 0)
74+
+ return 0;
75+
+
76+
+ if (dg_idx < IPHETH_NCM_HEADER_SIZE ||
77+
+ dg_idx >= urb->actual_length ||
78+
+ dg_len > urb->actual_length - dg_idx) {
79+
dev->net->stats.rx_length_errors++;
80+
return retval;
81+
}
82+
83+
- buf = urb->transfer_buffer + le16_to_cpu(dpe->wDatagramIndex);
84+
- len = le16_to_cpu(dpe->wDatagramLength);
85+
+ buf = urb->transfer_buffer + dg_idx;
86+
87+
- retval = ipheth_consume_skb(buf, len, dev);
88+
+ retval = ipheth_consume_skb(buf, dg_len, dev);
89+
if (retval != 0)
90+
return retval;
91+
92+
dpe++;
93+
}
94+
95+
- return 0;
96+
+rx_error:
97+
+ dev->net->stats.rx_errors++;
98+
+ return retval;
99+
}
100+
101+
static void ipheth_rcvbulk_callback(struct urb *urb)

‎target/linux/generic/pending-6.6/785-usbnet-ipheth-break-up-NCM-header-size-computation.patch‎

Lines changed: 48 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,48 @@
1+
From efcbc678a14be268040ffc1fa33c98faf2d55141 Mon Sep 17 00:00:00 2001
2+
From: Foster Snowhill <[email protected]>
3+
Date: Sun, 26 Jan 2025 00:54:07 +0100
4+
Subject: [PATCH 5/7] usbnet: ipheth: break up NCM header size computation
5+
6+
Originally, the total NCM header size was computed as the sum of two
7+
vaguely labelled constants. While accurate, it wasn't particularly clear
8+
where they were coming from.
9+
10+
Use sizes of existing NCM structs where available. Define the total
11+
NDP16 size based on the maximum amount of DPEs that can fit into the
12+
iOS-specific fixed-size header.
13+
14+
This change does not fix any particular issue. Rather, it introduces
15+
intermediate constants that will simplify subsequent commits.
16+
It should also make it clearer for the reader where the constant values
17+
come from.
18+
19+
Cc: [email protected] # 6.5.x
20+
Signed-off-by: Foster Snowhill <[email protected]>
21+
Reviewed-by: Jakub Kicinski <[email protected]>
22+
Signed-off-by: Paolo Abeni <[email protected]>
23+
---
24+
drivers/net/usb/ipheth.c | 13 ++++++++++++-
25+
1 file changed, 12 insertions(+), 1 deletion(-)
26+
27+
--- a/drivers/net/usb/ipheth.c
28+
+++ b/drivers/net/usb/ipheth.c
29+
@@ -61,7 +61,18 @@
30+
#define IPHETH_USBINTF_PROTO 1
31+
32+
#define IPHETH_IP_ALIGN 2 /* padding at front of URB */
33+
-#define IPHETH_NCM_HEADER_SIZE (12 + 96) /* NCMH + NCM0 */
34+
+/* On iOS devices, NCM headers in RX have a fixed size regardless of DPE count:
35+
+ * - NTH16 (NCMH): 12 bytes, as per CDC NCM 1.0 spec
36+
+ * - NDP16 (NCM0): 96 bytes, of which
37+
+ * - NDP16 fixed header: 8 bytes
38+
+ * - maximum of 22 DPEs (21 datagrams + trailer), 4 bytes each
39+
+ */
40+
+#define IPHETH_NDP16_MAX_DPE 22
41+
+#define IPHETH_NDP16_HEADER_SIZE (sizeof(struct usb_cdc_ncm_ndp16) + \
42+
+ IPHETH_NDP16_MAX_DPE * \
43+
+ sizeof(struct usb_cdc_ncm_dpe16))
44+
+#define IPHETH_NCM_HEADER_SIZE (sizeof(struct usb_cdc_ncm_nth16) + \
45+
+ IPHETH_NDP16_HEADER_SIZE)
46+
#define IPHETH_TX_BUF_SIZE ETH_FRAME_LEN
47+
#define IPHETH_RX_BUF_SIZE_LEGACY (IPHETH_IP_ALIGN + ETH_FRAME_LEN)
48+
#define IPHETH_RX_BUF_SIZE_NCM 65536

‎target/linux/generic/pending-6.6/786-usbnet-ipheth-fix-DPE-OoB-read.patch‎

Lines changed: 37 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,37 @@
1+
From ee591f2b281721171896117f9946fced31441418 Mon Sep 17 00:00:00 2001
2+
From: Foster Snowhill <[email protected]>
3+
Date: Sun, 26 Jan 2025 00:54:08 +0100
4+
Subject: [PATCH 6/7] usbnet: ipheth: fix DPE OoB read
5+
6+
Fix an out-of-bounds DPE read, limit the number of processed DPEs to
7+
the amount that fits into the fixed-size NDP16 header.
8+
9+
Fixes: a2d274c62e44 ("usbnet: ipheth: add CDC NCM support")
10+
Cc: [email protected]
11+
Signed-off-by: Foster Snowhill <[email protected]>
12+
Reviewed-by: Jakub Kicinski <[email protected]>
13+
Signed-off-by: Paolo Abeni <[email protected]>
14+
---
15+
drivers/net/usb/ipheth.c | 4 +---
16+
1 file changed, 1 insertion(+), 3 deletions(-)
17+
18+
--- a/drivers/net/usb/ipheth.c
19+
+++ b/drivers/net/usb/ipheth.c
20+
@@ -246,7 +246,7 @@ static int ipheth_rcvbulk_callback_ncm(s
21+
goto rx_error;
22+
23+
dpe = ncm0->dpe16;
24+
- while (true) {
25+
+ for (int dpe_i = 0; dpe_i < IPHETH_NDP16_MAX_DPE; ++dpe_i, ++dpe) {
26+
dg_idx = le16_to_cpu(dpe->wDatagramIndex);
27+
dg_len = le16_to_cpu(dpe->wDatagramLength);
28+
29+
@@ -268,8 +268,6 @@ static int ipheth_rcvbulk_callback_ncm(s
30+
retval = ipheth_consume_skb(buf, dg_len, dev);
31+
if (retval != 0)
32+
return retval;
33+
-
34+
- dpe++;
35+
}
36+
37+
rx_error:

‎target/linux/generic/pending-6.6/787-usbnet-ipheth-document-scope-of-NCM-implementation.patch‎

Lines changed: 37 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,37 @@
1+
From be154b598fa54136e2be17d6dd13c8a8bc0078ce Mon Sep 17 00:00:00 2001
2+
From: Foster Snowhill <[email protected]>
3+
Date: Sun, 26 Jan 2025 00:54:09 +0100
4+
Subject: [PATCH 7/7] usbnet: ipheth: document scope of NCM implementation
5+
6+
Clarify that the "NCM" implementation in `ipheth` is very limited, as
7+
iOS devices aren't compatible with the CDC NCM specification in regular
8+
tethering mode.
9+
10+
For a standards-compliant implementation, one shall turn to
11+
the `cdc_ncm` module.
12+
13+
Cc: [email protected] # 6.5.x
14+
Signed-off-by: Foster Snowhill <[email protected]>
15+
Reviewed-by: Jakub Kicinski <[email protected]>
16+
Signed-off-by: Paolo Abeni <[email protected]>
17+
---
18+
drivers/net/usb/ipheth.c | 8 ++++++++
19+
1 file changed, 8 insertions(+)
20+
21+
--- a/drivers/net/usb/ipheth.c
22+
+++ b/drivers/net/usb/ipheth.c
23+
@@ -218,6 +218,14 @@ static int ipheth_rcvbulk_callback_legac
24+
return ipheth_consume_skb(buf, len, dev);
25+
}
26+
27+
+/* In "NCM mode", the iOS device encapsulates RX (phone->computer) traffic
28+
+ * in NCM Transfer Blocks (similarly to CDC NCM). However, unlike reverse
29+
+ * tethering (handled by the `cdc_ncm` driver), regular tethering is not
30+
+ * compliant with the CDC NCM spec, as the device is missing the necessary
31+
+ * descriptors, and TX (computer->phone) traffic is not encapsulated
32+
+ * at all. Thus `ipheth` implements a very limited subset of the spec with
33+
+ * the sole purpose of parsing RX URBs.
34+
+ */
35+
static int ipheth_rcvbulk_callback_ncm(struct urb *urb)
36+
{
37+
struct usb_cdc_ncm_nth16 *ncmh;

0 commit comments

Comments
 (0)