Re: [PATCH] net: usb: net1080: fix out-of-bounds access in rx_fixup()

Re: [PATCH] net: usb: net1080: fix out-of-bounds access in rx_fixup()

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 1 potential
issue(s) to consider:

Critical: 0 · High: 0 · Medium: 1 · Low: 0

- [Medium] The commit message for net1080_rx_fixup()
  (drivers/net/usb/net1080.c) misdescribes the bug it claims to fix.

--

net: usb: net1080: fix out-of-bounds access in rx_fixup()

This adds a length check in net1080_rx_fixup() so that frames whose
device-supplied hdr_len does not leave room for the two byte nc_trailer
are rejected before skb_pull() is called. The change is described as a
fix for an out-of-bounds read reachable from a malicious USB device, and
carries a Fixes: tag and Cc: stable.

> diff --git a/drivers/net/usb/net1080.c b/drivers/net/usb/net1080.c
> index 19f6e1222d931..64477a4caa07b 100644
> --- a/drivers/net/usb/net1080.c
> +++ b/drivers/net/usb/net1080.c
> @@ -374,6 +374,13 @@ static int net1080_rx_fixup(struct usbnet *dev, struct sk_buff *skb)
>  		nc_ensure_sync(dev);
>  		// switch (vendor/product ids) { ... }
>  	}
> +	if (hdr_len + sizeof(*trailer) > skb->len) {
> +		dev->net->stats.rx_frame_errors++;
> +		netdev_dbg(dev->net, "header too long for frame, %d\n",
> +			   hdr_len);
> +		nc_ensure_sync(dev);
> +		return 0;
> +	}
>  	skb_pull(skb, hdr_len);
>  
>  	trailer = (struct nc_trailer *)

[Severity: Medium]
Does the out-of-bounds read described in the changelog actually exist?
The commit message says:

  "the pointer arithmetic underflows, pointing before skb->data. The
   subsequent read of trailer->packet_id accesses memory outside the skb
   buffer."

skb_pull() subtracts from skb->len exactly what it adds to skb->data:

include/linux/skbuff.h:__skb_pull() {
	skb->len -= len;
	...
	skb->data += len;
}

So the sum skb->data + skb->len is unchanged by the pull, and in
net1080_rx_fixup():

	trailer = (struct nc_trailer *)
		(skb->data + skb->len - sizeof *trailer);

always resolves to the last two bytes of the frame as it was received,
regardless of hdr_len. The pointer can land inside the just-stripped
header bytes, which is before the post-pull skb->data, but not before
skb->head.

The entry guard also bounds the minimum frame size:

	/* This check is no longer done by usbnet */
	if (skb->len < dev->net->hard_header_len)
		return 0;

with net1080_bind() setting

	unsigned	extra = sizeof (struct nc_header)
				+ 1
				+ sizeof (struct nc_trailer);

	dev->net->hard_header_len += extra;

which gives hard_header_len = ETH_HLEN + 6 + 1 + 2 = 23, so skb->len is
at least 23 whenever the trailer arithmetic runs and data + len - 2 stays
well inside the linear area.

For the hdr_len > skb->len case, skb_pull() returns NULL and leaves
skb->data and skb->len untouched, so the trailer computation is again in
bounds. And the following

	skb_trim(skb, skb->len - sizeof *trailer);

does wrap the unsigned argument when the post-pull skb->len is 0 or 1,
but skb_trim() only shrinks:

net/core/skbuff.c:skb_trim() {
	if (skb->len > len)
		__skb_trim(skb, len);
}

so that is a no-op rather than an overread.

What the new check does change is that frames whose declared hdr_len
leaves less than sizeof(nc_trailer) of payload are now rejected, which
also covers the previously ignored NULL return from skb_pull() where the
nc_header stayed un-stripped and a malformed frame could be passed up if
packet_len and packet_id happened to match. That looks like a reasonable
hardening, and real devices report hdr_len == MIN_HEADER == 6.

Given that, could the subject and changelog be reworded to describe the
robustness fix instead of an out-of-bounds access, and could the
Fixes: tag and Cc: stable be re-evaluated on that basis?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/AS8P251MB0001E8B611B22B19AA882542C8B22%40AS8P251MB0001.EURP251.PROD.OUTLOOK.COM

[PATCH] net: usb: net1080: fix out-of-bounds access in rx_fixup()

[Aamir Ahmed]

net1080_rx_fixup() reads hdr_len from the USB device and uses it to
strip the header with skb_pull(). It then calculates the trailer
pointer as:

  trailer = (struct nc_trailer *)(skb->data + skb->len - sizeof *trailer);

If hdr_len is close to skb->len (e.g. skb->len - 1), the pull leaves
fewer bytes than sizeof(nc_trailer) = 2, and the pointer arithmetic
underflows, pointing before skb->data. The subsequent read of
trailer->packet_id accesses memory outside the skb buffer.

A malicious USB device can craft a frame with hdr_len >= MIN_HEADER (6)
but large enough to leave insufficient room for the trailer after the
header is stripped.

Add a check that the frame is long enough to contain both the header
and trailer before pulling.

Fixes: 904813cd8a0b3 ("[PATCH] USB: usbnet (4/9) module for net1080 cables")
Cc: stable@vger.kernel.org
Signed-off-by: Aamir Ahmed <elb12345@hotmail.co.uk>
Assisted-by: Claude (Anthropic)
---
 drivers/net/usb/net1080.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/drivers/net/usb/net1080.c b/drivers/net/usb/net1080.c
index 19f6e1222d931..64477a4caa07b 100644
--- a/drivers/net/usb/net1080.c
+++ b/drivers/net/usb/net1080.c
@@ -374,6 +374,13 @@ static int net1080_rx_fixup(struct usbnet *dev, struct sk_buff *skb)
 		nc_ensure_sync(dev);
 		// switch (vendor/product ids) { ... }
 	}
+	if (hdr_len + sizeof(*trailer) > skb->len) {
+		dev->net->stats.rx_frame_errors++;
+		netdev_dbg(dev->net, "header too long for frame, %d\n",
+			   hdr_len);
+		nc_ensure_sync(dev);
+		return 0;
+	}
 	skb_pull(skb, hdr_len);
 
 	trailer = (struct nc_trailer *)
-- 
2.43.0

Re: [PATCH] net: usb: net1080: fix out-of-bounds access in rx_fixup()

[Aamir Ahmed]

Please drop this patch.

I rechecked the code earlier and there is no out-of-bounds access to
fix here, so the commit message is wrong.

skb_pull() returns NULL without modifying the skb when hdr_len is
larger than skb->len, and when it does pull it leaves skb->data +
skb->len unchanged. Either way

  trailer = (struct nc_trailer *)(skb->data + skb->len - sizeof *trailer);

resolves to the last two bytes of the frame as received, which is in
bounds because of the earlier skb->len < hard_header_len check. The
read of skb->data[packet_len] is likewise already guarded by the
packet_len >= skb->len test above it.

Thank you.

pw-bot: changes-requested