Re: [PATCH] net: usb: catc: fix out-of-bounds read in catc_rx_done()

Re: [PATCH] net: usb: catc: fix out-of-bounds read in catc_rx_done()

[netdev-bot+sashiko]

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

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

- [Medium] catc_rx_done() (drivers/net/usb/catc.c) still accepts a
  device-supplied pkt_len that is smaller than an Ethernet header.

Pre-existing issues:
- [High] For F5U011 (Belkin) devices catc_rx_done() can return with the
  RX_RUNNING flag set and no bulk-IN URB in flight, which permanently…
- [Low] The new bounds validation in catc_rx_done() re-reads
  urb->actual_length (line 237-238: `remaining = urb->actual_length -…

--

net: usb: catc: fix out-of-bounds read in catc_rx_done()

The patch reworks the length validation in catc_rx_done() so the 2-byte
per-packet header offset is accounted for and so each packet in a
multi-packet URB is checked against the bytes remaining from the current
pkt_start rather than against urb->actual_length. A few questions about
the new check and the surrounding code follow.

> diff --git a/drivers/net/usb/catc.c b/drivers/net/usb/catc.c
> index 96e82f94edcf8..e4f8b9a0caf19 100644
> --- a/drivers/net/usb/catc.c
> +++ b/drivers/net/usb/catc.c
> @@ -234,8 +234,16 @@ static void catc_rx_done(struct urb *urb)
>  
>  	do {
>  		if(!catc->is_f5u011) {
> +			int remaining = urb->actual_length -
> +					(pkt_start - (u8 *)urb->transfer_buffer);
> +
> +			if (remaining < pkt_offset) {
> +				catc->netdev->stats.rx_length_errors++;
> +				catc->netdev->stats.rx_errors++;
> +				break;
> +			}
>  			pkt_len = le16_to_cpup((__le16*)pkt_start);
> -			if (pkt_len > urb->actual_length) {
> +			if (pkt_len + pkt_offset > remaining) {
>  				catc->netdev->stats.rx_length_errors++;
>  				catc->netdev->stats.rx_errors++;
>  				break;

[Severity: Medium]
Since this block is being rewritten to sanitise the device-supplied
length, should it also reject a pkt_len smaller than an Ethernet header?

As written, any value in [0, remaining - 2] is accepted, including 0, and
the rest of the loop then does:

drivers/net/usb/catc.c:catc_rx_done() {
	...
	if (!(skb = dev_alloc_skb(pkt_len)))
		return;

	skb_copy_to_linear_data(skb, pkt_start + pkt_offset, pkt_len);
	skb_put(skb, pkt_len);

	skb->protocol = eth_type_trans(skb, catc->netdev);
	netif_rx(skb);
	...
}

eth_type_trans() dereferences 14 bytes unconditionally:

net/ethernet/eth.c:eth_type_trans() {
	...
	eth = eth_skb_pull_mac(skb);
	eth_skb_pkt_type(skb, dev);
	...
	if (likely(eth_proto_is_802_3(eth->h_proto)))
		return eth->h_proto;
	...
}

include/linux/etherdevice.h:eth_skb_pull_mac() {
	struct ethhdr *eth = (struct ethhdr *)skb->data;

	skb_pull_inline(skb, ETH_HLEN);
	return eth;
}

With pkt_len between 0 and 13, h_dest and h_proto are read from bytes
that were never copied into the skb, so skb->pkt_type and skb->protocol
are derived from uninitialised tailroom. skb_pull_inline() also declines
the ETH_HLEN pull when ETH_HLEN > skb->len, so the runt skb reaches
netif_rx() with its MAC header still unpulled.

Note this does not look like memory corruption: skb->len is not
underflowed, since skb_pull_inline() returns NULL without touching
skb->len or skb->data when len > skb->len.

The is_f5u011 branch of the same loop takes the length straight from the
transfer:

		} else {
			pkt_len = urb->actual_length;
		}

so a zero-length transfer reaches the same path there. Would a shared
pkt_len < ETH_HLEN test cover both branches? A sanity maximum against
PKT_SZ or dev->mtu might be worth adding at the same time, as the new
check only bounds pkt_len by what fits in the URB.

[Severity: High]
This is a pre-existing issue and is not introduced by this patch, but
while this loop is under discussion: can reception stop permanently on
F5U011 devices?

RX_RUNNING is only cleared early for the non-F5U011 case:

drivers/net/usb/catc.c:catc_rx_done() {
	if (!catc->is_f5u011) {
		clear_bit(RX_RUNNING, &catc->flags);
		pkt_offset = 2;
	}
	...
}

so on the F5U011 path the flag is only cleared (or the URB resubmitted)
by the tail block:

	if (catc->is_f5u011) {
		if (atomic_read(&catc->recq_sz)) {
			int state;
			atomic_dec(&catc->recq_sz);
			netdev_dbg(catc->netdev, "getting extra packet\n");
			urb->dev = catc->usbdev;
			if ((state = usb_submit_urb(urb, GFP_ATOMIC)) < 0) {
				netdev_dbg(catc->netdev,
					   "submit(rx_urb) status %d\n", state);
			}
		} else {
			clear_bit(RX_RUNNING, &catc->flags);
		}
	}

Two paths appear to skip re-arming the endpoint:

  - the "if (!(skb = dev_alloc_skb(pkt_len))) return;" early return in
    the loop leaves the function before the tail block, so RX_RUNNING
    stays set and recq_sz is not decremented;

  - a usb_submit_urb() failure in the tail block is only logged, after
    recq_sz has already been decremented, so no URB is in flight and
    RX_RUNNING remains set.

catc_irq_done() is the only other place that starts the bulk transfer,
and it is gated purely on the flag:

drivers/net/usb/catc.c:catc_irq_done() {
	if (hasdata) {
		if (test_and_set_bit(RX_RUNNING, &catc->flags)) {
			if (catc->is_f5u011)
				atomic_inc(&catc->recq_sz);
		} else {
			catc->rx_urb->dev = catc->usbdev;
			if ((res = usb_submit_urb(catc->rx_urb, GFP_ATOMIC)) < 0) {
	...
}

With the bit stuck set, every later data notification only increments
recq_sz and never resubmits catc->rx_urb, so RX would stay dead until
the interface is brought down and up. The submit-failure path also loses
one unit of the pending-frame credit, leaving recq_sz permanently out of
step with what the adapter holds.

[Severity: Low]
This is also pre-existing rather than something the patch introduces, but
the new computation re-reads urb->actual_length on every iteration:

			int remaining = urb->actual_length -
					(pkt_start - (u8 *)urb->transfer_buffer);

For non-F5U011 devices RX_RUNNING is cleared before the parsing loop
starts, and catc_irq_done() only checks that bit before resubmitting the
same URB over catc->rx_buf. If the two ever overlap, usb_submit_urb()
resets urb->actual_length to 0 and the controller refills catc->rx_buf,
so the value validated here and the two header bytes read as pkt_len can
change before skb_copy_to_linear_data() runs.

The accesses stay inside catc->rx_buf, since rx_urb->transfer_buffer_length
equals sizeof(catc->rx_buf), so the visible effect would be garbled
frames handed to netif_rx() plus spurious rx_length_errors rather than an
out-of-bounds access. This one is speculative: URB giveback for a single
host controller is normally serialised, so it is not clear that the bulk
and interrupt completion handlers of this device can actually run at the
same time. Is the bit flag intended to provide exclusion here?

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

[PATCH] net: usb: catc: fix out-of-bounds read in catc_rx_done()

[Aamir Ahmed]

catc_rx_done() parses multi-packet USB transfers by iterating over the
received data. For each packet, it reads a 2-byte little-endian length
from the buffer, then copies that many bytes as the packet payload.

The bounds check "pkt_len > urb->actual_length" is insufficient:

1. It does not account for the 2-byte header offset, so the memcpy can
   read 2 bytes beyond the received data.

2. For subsequent packets in the same URB, pkt_start advances through
   the buffer but the check still compares against the total
   urb->actual_length rather than the remaining bytes, allowing reads
   well past the end.

Fix this by calculating the remaining bytes from the current pkt_start
position and checking both that enough bytes exist to read the header
and that the packet length plus header fits within the remaining data.

A malicious USB device can craft transfers that trigger the out-of-bounds
heap read.

Fixes: 1da177e4c3f41 ("Linux-2.6.12-rc2")
Cc: stable@vger.kernel.org
Signed-off-by: Aamir Ahmed <elb12345@hotmail.co.uk>
Assisted-by: Claude (Anthropic)
---
 drivers/net/usb/catc.c | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/drivers/net/usb/catc.c b/drivers/net/usb/catc.c
index 96e82f94edcf8..e4f8b9a0caf19 100644
--- a/drivers/net/usb/catc.c
+++ b/drivers/net/usb/catc.c
@@ -234,8 +234,16 @@ static void catc_rx_done(struct urb *urb)
 
 	do {
 		if(!catc->is_f5u011) {
+			int remaining = urb->actual_length -
+					(pkt_start - (u8 *)urb->transfer_buffer);
+
+			if (remaining < pkt_offset) {
+				catc->netdev->stats.rx_length_errors++;
+				catc->netdev->stats.rx_errors++;
+				break;
+			}
 			pkt_len = le16_to_cpup((__le16*)pkt_start);
-			if (pkt_len > urb->actual_length) {
+			if (pkt_len + pkt_offset > remaining) {
 				catc->netdev->stats.rx_length_errors++;
 				catc->netdev->stats.rx_errors++;
 				break;
-- 
2.43.0