[PATCH] net: usb: cdc_ncm: fix out-of-bounds read of chained NDP32 offset

[PATCH] net: usb: cdc_ncm: fix out-of-bounds read of chained NDP32 offset

[Ryan Prinz]

[-- Attachment #1.1: Type: text/plain, Size: 2629 bytes --]

The NDP offset read from dwNextNdpIndex on the 32-bit NTB (NDP32)
path is not checked for a negative value before use. A
device-supplied dwNextNdpIndex in the 0xFFFFFFF0-0xFFFFFFFF range
sign-converts to a small negative int, bypassing the bounds check in
cdc_ncm_rx_verify_ndp32() via size_t wraparound, causing an
out-of-bounds read of up to 16 bytes before the skb's data buffer.

Fixes: 0fa81b304a79 ("cdc_ncm: Implement the 32-bit version of NCM Transfer Block")
Signed-off-by: Ryan Prinz <rc4d5a@protonmail.com>
---
Reproduced by calling cdc_ncm_rx_fixup() directly from a small
out-of-tree test module with a hand-crafted NTB. With memory-sanitizer
instrumentation enabled to pinpoint the vulnerability:

    BUG: KASAN: slab-out-of-bounds in cdc_ncm_rx_verify_ndp32+0x11e/0x130 [cdc_ncm]
    Read of size 2 at addr ffff88800fcd2a34 by task insmod/367
     cdc_ncm_rx_verify_ndp32+0x11e/0x130 [cdc_ncm]
     cdc_ncm_rx_fixup+0x7a3/0xed0 [cdc_ncm]

Scope/impact:
- Triggerin
g the 32-bit NTB (NDP32) path requires the host driver to
  set CDC_NCM_FLAG_PREFER_NTB32 for the device. The only driver that
  does so unconditionally is huawei_cdc_ncm.c, for Huawei mobile
  broadband hardware. Local/physical-USB-attachment or
  compromised-device-firmware attack surface, not remote.
- Reachable offset range is -1 to -16, bounded by
  sizeof(struct usb_cdc_ncm_ndp32); the subsequent dpe32 array read
  always lands back inside the legitimate buffer, so this doesn't
  escalate into a larger OOB excursion.
- No out-of-bounds write found, and the leaked bytes only feed a
  signature-match comparison. There is no path found for them to
  reach a delivered packet or the device/attacker.

Given the local-only trigger and lack of a confirmed write primitive
or crash, my read is this doesn't need security@kernel.org handling,
but happy to redirect there if you think otherwise.

AI Disclosure: An AI assistant (Claude) was used to assist with the
analysis and
 testing. I have verified the vulnerability, the fix,
and the test results myself and take responsibility for this
contribution, per Documentation/process/coding-assistants.rst.

 drivers/net/usb/cdc_ncm.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/net/usb/cdc_ncm.c b/drivers/net/usb/cdc_ncm.c
index 19db5a668..f5c8a6070 100644
--- a/drivers/net/usb/cdc_ncm.c
+++ b/drivers/net/usb/cdc_ncm.c
@@ -1838,6 +1838,9 @@ int cdc_ncm_rx_fixup(struct usbnet *dev, struct sk_buff *skb_in)
 	else
 		ndpoffset = le32_to_cpu(ndp.ndp32->dwNextNdpIndex);
 
+	if (ndpoffset < 0)
+		goto error;
+
 	if (ndpoffset && loopcount--)
 		goto next_ndp;
 
-- 
2.51.2

[-- Attachment #1.2: publickey - rc4d5a@protonmail.com - 0xE775BF83.asc --]
[-- Type: application/pgp-keys, Size: 897 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 322 bytes --]

Re: [PATCH] net: usb: cdc_ncm: fix out-of-bounds read of chained NDP32 offset

[Jakub Kicinski]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
---
net: usb: cdc_ncm: fix out-of-bounds read of chained NDP32 offset

This patch adds a negative check on the dwNextNdpIndex value that
cdc_ncm_rx_fixup() reads from a chained NDP32, before it jumps back to
next_ndp. A value from the device in the 0xFFFFFFF0-0xFFFFFFFF range
becomes a small negative int. That value would otherwise get past the
size_t bounds check in cdc_ncm_rx_verify_ndp32().

> net: usb: cdc_ncm: fix out-of-bounds read of chained NDP32 offset
>
> This is an OpenPGP/MIME signed message (RFC 4880 and 3156)
> --------0069204657d9df8cf4194503c90bb3fa2463289ee31cd304c80a0972fb84c9ce
> Content-Type: multipart/mixed;
>  boundary=2a1cb552390cbfe7fd100b88c3ace6355fd4256b41310b91c8759db253cb
> From: Ryan Prinz <rc4d5a@protonmail.com>

[ ... ]

> Subject: [PATCH] net: usb: cdc_ncm: fix out-of-bounds read of chained NDP32 offset

[ ... ]

> MIME-Version: 1.0
>
> --2a1cb552390cbfe7fd100b88c3ace6355fd4256b41310b91c8759db253cb
> Content-Type: text/plain; charset=UTF-8
> Content-Transfer-Encoding: 8bit

[Severity: Low]
This isn't a bug, but the mail client's PGP/MIME envelope ended up in
the changelog. The boundary strings, a second copy of the mail headers
and the Content-Type lines all sit before the real description. The log
would have to be edited by hand before the patch could be applied.

The notes below the --- line also look damaged in transit. Some words
are split across lines ("Triggerin" / "g the 32-bit ...", "analysis
and" / " testing").

Could this be resent as plain text without PGP/MIME? It also carries a
Fixes: tag, so netdev would expect the subject prefix "[PATCH net]"
instead of "[PATCH]".

> The NDP offset read from dwNextNdpIndex on the 32-bit NTB (NDP32)
> path is not checked for a negative value before use. A
> device-supplied dwNextNdpIndex in the 0xFFFFFFF0-0xFFFFFFFF range
> sign-converts to a small negative int, bypassing the bounds check in
> cdc_ncm_rx_verify_ndp32() via size_t wraparound, causing an
> out-of-bounds read of up to 16 bytes before the skb's data buffer.

[Severity: Low]
This isn't a bug, but could the changelog say where those 16 bytes come
from on real hardware?

usbnet allocates every rx skb in rx_submit():

drivers/net/usb/usbnet.c:rx_submit() {
    ...
	if (test_bit(EVENT_NO_IP_ALIGN, &dev->flags))
		skb = __netdev_alloc_skb(dev->net, size, flags);
	else
		skb = __netdev_alloc_skb_ip_align(dev->net, size, flags);
    ...
}

Both allocators reserve NET_SKB_PAD of headroom, which is at least 32
bytes. When ndpoffset is between -16 and -1, the read at
skb_in->data + ndpoffset stays inside the same allocation. It picks up
stale headroom bytes, and those bytes only feed the wLength,
dwSignature and dwNextNdpIndex checks.

"Before the skb's data buffer" is accurate relative to skb->data. The
notes also say the KASAN slab-out-of-bounds report came from a
hand-crafted test module. Adding one sentence that the bytes come from
the skb headroom might still make the impact clearer to people reading
the log.

> diff --git a/drivers/net/usb/cdc_ncm.c b/drivers/net/usb/cdc_ncm.c
> index 19db5a668dcbc..f5c8a60704233 100644
> --- a/drivers/net/usb/cdc_ncm.c
> +++ b/drivers/net/usb/cdc_ncm.c
> @@ -1838,6 +1838,9 @@ int cdc_ncm_rx_fixup(struct usbnet *dev, struct sk_buff *skb_in)
>  	else
>  		ndpoffset = le32_to_cpu(ndp.ndp32->dwNextNdpIndex);
>  
> +	if (ndpoffset < 0)
> +		goto error;
> +

[Severity: Low]
This isn't a bug, but would it be more robust to reject a negative
offset inside cdc_ncm_rx_verify_ndp32() itself?

The helper is exported and takes a signed int. Its bounds checks can
still wrap through size_t in the same way when ndpoffset is between -16
and -1:

drivers/net/usb/cdc_ncm.c:cdc_ncm_rx_verify_ndp32() {
    ...
	if ((ndpoffset + sizeof(struct usb_cdc_ncm_ndp32)) > skb_in->len) {
		netif_dbg(dev, rx_err, dev->net, "invalid NDP offset  <%u>\n",
			  ndpoffset);
		goto error;
	}
	ndp32 = (struct usb_cdc_ncm_ndp32 *)(skb_in->data + ndpoffset);
    ...
	if (ndpoffset + ndp_len > skb_in->len) {
    ...
}

In the current tree, cdc_ncm_rx_fixup() is the only caller. Both paths
into next_ndp now reject a negative ndpoffset: the existing check after
cdc_ncm_rx_verify_nth32() and the new one here. So the in-tree path is
closed, and the new check follows the pattern already used for the
cdc_ncm_rx_verify_nth32() return value.

A check inside the helper would also protect any future caller. The
netif_dbg() in the helper prints the signed ndpoffset with %u, and that
could be fixed in the same change.

>  	if (ndpoffset && loopcount--)
>  		goto next_ndp;
>
-- 
pw-bot: cr