[PATCH net 0/2] net: usb: pegasus: fix receive length handling

[PATCH net 0/2] net: usb: pegasus: fix receive length handling

[Wei Jie Law]

Two fixes for read_bulk_callback(), both reachable by a USB device that
identifies itself as a Pegasus II adapter.

Patch 1 fixes an out-of-bounds write: on the 0x8513, every frame that
is dropped because of an oversized length field advances the receive
buffer by two bytes, until the host controller writes past the end of
the skb data area.

Patch 2 drops frames whose claimed length exceeds the data actually
received, instead of passing the uninitialized remainder of the receive
buffer up the stack.

Both bugs need a malicious or emulated USB device, which
Documentation/process/threat-model.rst treats as a regular bug rather
than a vulnerability, so they are posted publicly.

Testing: the series builds without warnings (W=1) on net and passes
checkpatch --strict. It applies unchanged to v6.12.105, where it was
tested on a KASAN kernel with a device emulated by raw-gadget +
dummy_hcd:

- 3000 frames with pkt_len 0xfff, then 20 valid frames. Before: KASAN
  slab-out-of-bounds writes and a general protection fault in
  skb_release_data(). After: no KASAN reports, and all 20 valid frames
  are received.

- Five each of: 1536-byte transfers with pkt_len 1534, 1535 and 1536;
  101-byte transfers claiming 1500 and 97 bytes; normal frames.
  Before: all 30 received. After: the 15 that fit are received, and
  rx_bytes matches; the rest are dropped.

Both bugs were found, and the patches written, with LLM assistance,
hence the Assisted-by tags.

Wei Jie Law (2):
  net: usb: pegasus: do not advance rx_skb->data on dropped frames
  net: usb: pegasus: check rx pkt_len against the received length

 drivers/net/usb/pegasus.c | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)


base-commit: af32da41b0327b9c6a37856ba82b6760d6c8d10e
-- 
2.43.0

[PATCH net 1/2] net: usb: pegasus: do not advance rx_skb->data on dropped frames

[Wei Jie Law]

For the 0x8513 chip, read_bulk_callback() skips the 2-byte receive
header by advancing pegasus->rx_skb->data in place. This is done before
the pkt_len > PEGASUS_MTU check, and when that check drops the frame
the goon path resubmits the same skb with the advanced rx_skb->data as
the transfer buffer, still PEGASUS_MTU bytes long.

Every frame with an oversized length field therefore moves the receive
buffer two more bytes towards the end of the skb data area. A device
that keeps sending such frames makes the host controller write received
data over skb_shared_info and past the end of the allocation, and the
corrupted skb_shared_info is used when the skb is eventually freed.
With an emulated device sending frames filled with 0x5a on a KASAN
kernel:

  BUG: KASAN: slab-out-of-bounds in dummy_timer+0xade/0x3f00 [dummy_hcd]
  Write of size 1536 at addr ffff888105821202 by task swapper/1/0
  ...
  Oops: general protection fault, probably for non-canonical address 0xeb4b474b4b4b4b4c: 0000 [#1] PREEMPT SMP KASAN NOPTI
  KASAN: maybe wild-memory-access in range [0x5a5a5a5a5a5a5a60-0x5a5a5a5a5a5a5a67]
  RIP: 0010:skb_release_data+0x36f/0x800

Only skip the header with skb_reserve() once the frame is going to be
passed up, so the receive buffer always starts at the beginning of the
skb data area. Unlike the old pointer adjustment, skb_reserve() also
moves skb->tail, so the two header bytes now count against the buffer
size: lower the length limit for the 0x8513 accordingly, otherwise a
pkt_len of PEGASUS_MTU - 1 or PEGASUS_MTU would trip skb_over_panic().

Found by code review with LLM assistance and reproduced with an
emulated USB device (raw-gadget + dummy_hcd).

Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
Cc: stable@vger.kernel.org
Assisted-by: LLM
Signed-off-by: Wei Jie Law <law.wei.jie@gt.tech.gov.sg>
---
 drivers/net/usb/pegasus.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/net/usb/pegasus.c b/drivers/net/usb/pegasus.c
index aba1a640fc26..7c7d6b69998f 100644
--- a/drivers/net/usb/pegasus.c
+++ b/drivers/net/usb/pegasus.c
@@ -461,6 +461,7 @@ static void read_bulk_callback(struct urb *urb)
 	u8 *buf = urb->transfer_buffer;
 	int rx_status, count = urb->actual_length;
 	int status = urb->status;
+	int rx_offset = 0;
 	__u16 pkt_len;
 
 	if (!pegasus)
@@ -510,7 +511,7 @@ static void read_bulk_callback(struct urb *urb)
 	if (pegasus->chip == 0x8513) {
 		pkt_len = le32_to_cpu(*(__le32 *)urb->transfer_buffer);
 		pkt_len &= 0x0fff;
-		pegasus->rx_skb->data += 2;
+		rx_offset = 2;
 	} else {
 		pkt_len = buf[count - 3] << 8;
 		pkt_len += buf[count - 4];
@@ -522,13 +523,14 @@ static void read_bulk_callback(struct urb *urb)
 	 * If the packet is unreasonably long, quietly drop it rather than
 	 * kernel panicing by calling skb_put.
 	 */
-	if (pkt_len > PEGASUS_MTU)
+	if (pkt_len > PEGASUS_MTU - rx_offset)
 		goto goon;
 
 	/*
 	 * at this point we are sure pegasus->rx_skb != NULL
 	 * so we go ahead and pass up the packet.
 	 */
+	skb_reserve(pegasus->rx_skb, rx_offset);
 	skb_put(pegasus->rx_skb, pkt_len);
 	pegasus->rx_skb->protocol = eth_type_trans(pegasus->rx_skb, net);
 	netif_rx(pegasus->rx_skb);
-- 
2.43.0

[PATCH net 2/2] net: usb: pegasus: check rx pkt_len against the received length

[Wei Jie Law]

read_bulk_callback() takes the frame length from the device, from the
receive header on the 0x8513 and from the trailing status bytes on the
other chips, and only checks it against PEGASUS_MTU before passing that
many bytes up the stack. A device that sends a short transfer but
claims a long frame gets the uninitialized remainder of the receive
skb delivered as packet data.

Drop frames whose claimed length, plus the 0x8513 header, does not fit
in the bytes actually received. urb->actual_length never exceeds the
PEGASUS_MTU sized transfer buffer, so this also covers the existing
PEGASUS_MTU check.

Found by code review with LLM assistance. With an emulated USB device,
a 101-byte transfer claiming a 1500-byte frame was passed up as a
1500-byte packet before this change and is dropped after it. The extra
bytes happened to be zero in that test, so disclosure of actual kernel
data was not demonstrated.

Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
Cc: stable@vger.kernel.org
Assisted-by: LLM
Signed-off-by: Wei Jie Law <law.wei.jie@gt.tech.gov.sg>
---
 drivers/net/usb/pegasus.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/drivers/net/usb/pegasus.c b/drivers/net/usb/pegasus.c
index 7c7d6b69998f..7bc9312eaa9e 100644
--- a/drivers/net/usb/pegasus.c
+++ b/drivers/net/usb/pegasus.c
@@ -520,10 +520,11 @@ static void read_bulk_callback(struct urb *urb)
 	}
 
 	/*
-	 * If the packet is unreasonably long, quietly drop it rather than
-	 * kernel panicing by calling skb_put.
+	 * If the packet is longer than what was actually received, quietly
+	 * drop it rather than kernel panicing by calling skb_put or passing
+	 * up stale buffer contents. count is at most PEGASUS_MTU.
 	 */
-	if (pkt_len > PEGASUS_MTU - rx_offset)
+	if (pkt_len + rx_offset > count)
 		goto goon;
 
 	/*
-- 
2.43.0

Re: [PATCH net 2/2] net: usb: pegasus: check rx pkt_len against the received length

[netdev-bot+sashiko]

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

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

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [High] In read_bulk_callback() (drivers/net/usb/pegasus.c), the new
  check `if (pkt_len + rx_offset > count) goto goon;` only sets an upper…
- [Medium] net: usb: pegasus: uninitialized memory read in eth_type_trans
  on short packets

--

net: usb: pegasus: check rx pkt_len against the received length

This patch changes the rx length check in read_bulk_callback(). Frames
whose device-reported length, plus the 0x8513 header, does not fit in
urb->actual_length are now dropped. Before, the length was only checked
against PEGASUS_MTU.

> diff --git a/drivers/net/usb/pegasus.c b/drivers/net/usb/pegasus.c
> index 7c7d6b69998f2..7bc9312eaa9ec 100644
> --- a/drivers/net/usb/pegasus.c
> +++ b/drivers/net/usb/pegasus.c
> @@ -520,10 +520,11 @@ static void read_bulk_callback(struct urb *urb)
>  	}
>  
>  	/*
> -	 * If the packet is unreasonably long, quietly drop it rather than
> -	 * kernel panicing by calling skb_put.
> +	 * If the packet is longer than what was actually received, quietly
> +	 * drop it rather than kernel panicing by calling skb_put or passing
> +	 * up stale buffer contents. count is at most PEGASUS_MTU.
>  	 */
> -	if (pkt_len > PEGASUS_MTU - rx_offset)
> +	if (pkt_len + rx_offset > count)
>  		goto goon;

[Severity: Medium]
This check bounds pkt_len from above, but is anything stopping a
pkt_len smaller than ETH_HLEN from reaching eth_type_trans()?

On a 0x8513 chip, a 4-byte transfer with buf[0..2] = 0 decodes to
pkt_len = 0 with rx_offset = 2. rx_status is buf[2], so the 0x1c
error bits are clear. 0 + 2 > 4 is false, so the frame is accepted.
The non-0x8513 path gets the same result from a trailer that decodes
to 4, since pkt_len -= 4 brings it to 0.

read_bulk_callback() then does skb_reserve(pegasus->rx_skb, 2) and
skb_put(pegasus->rx_skb, 0), and calls eth_type_trans().

eth_type_trans() reads the full 14-byte Ethernet header regardless of
skb->len. eth_skb_pkt_type() looks at h_dest, and h_proto is used to
pick skb->protocol. Only 2 of those 14 bytes came from the device.

The other 12 lie past urb->actual_length. They are either left over
from an earlier transfer, when the skb was reused on the goon path, or
uninitialized memory from __netdev_alloc_skb_ip_align().

KMSAN would report this as an uninit read. skb->pkt_type and
skb->protocol are then derived from whatever was in the buffer before
netif_rx() hands the skb to the stack.

The old PEGASUS_MTU check had the same hole. However, the new comment
says this check prevents passing up stale buffer contents. Would it
make sense to also drop frames with pkt_len < ETH_HLEN here, perhaps
counting them in rx_length_errors?

[Severity: High]
This sets an upper limit on pkt_len. Does it also need a lower limit?

Nothing checks that pkt_len is at least ETH_HLEN, so a short transfer that
claims a runt frame still gets through. For example, on a non-0x8513 chip a
4-byte transfer of 04 00 00 00 gives:

	pkt_len = buf[count - 3] << 8;    /* 0 */
	pkt_len += buf[count - 4];        /* 4 */
	pkt_len &= 0xfff;
	pkt_len -= 4;                     /* 0 */

rx_status is buf[2] = 0, and 0 + 0 > 4 is false, so the frame is accepted.

On 0x8513, a 4-byte transfer with buf[0..1] = 0 gives pkt_len = 0 and
rx_offset = 2, which also passes. Any pkt_len below 14 that fits in count
gets through the same way.

After skb_put(pegasus->rx_skb, pkt_len), eth_type_trans() calls
eth_skb_pull_mac():

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

	skb_pull_inline(skb, ETH_HLEN);
	return eth;
}

skb_pull_inline() returns NULL because ETH_HLEN > skb->len, but nothing
checks the return value. eth_type_trans() then reads h_dest and h_proto from
the header anyway.

Those bytes lie past urb->actual_length. They are either left over from an
earlier transfer, when the skb is reused on the goon path, or uninitialized
memory from __netdev_alloc_skb_ip_align().

Those bytes set skb->pkt_type and skb->protocol, and netif_rx() then
delivers the skb. For an ETH_P_ALL packet socket the chain continues:

packet_rcv()
  dev_parse_header()
    eth_header_parse()
      memcpy(haddr, eth->h_source, ETH_ALEN);

packet_recvmsg() then copies sll_addr back to userspace. Can that expose up
to 6 bytes of stale rx buffer contents through sockaddr_ll?

The new comment says this check stops "passing up stale buffer contents".
Should this check also drop frames with pkt_len < ETH_HLEN?

>  
>  	/*

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261014025307.25233-1-law.wei.jie%40gt.tech.gov.sg