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

[Jakub Kicinski <kuba@kernel.org>]

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