Re: [PATCH net v3] net: usb: asix: reject a truncated Data header in rx_fixup

[netdev-bot+sashiko@kernel.org]

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

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

- [High] asix_rx_fixup_internal() (drivers/net/usb/asix_common.c) still
  reads a 4-byte Data header that does not fit inside skb->len at the…
- [Low] The netdev_err() added by this patch in asix_rx_fixup_internal()
  (drivers/net/usb/asix_common.c:183-184) formats skb->len with %d, but…

Pre-existing issues:
- [Medium] In asix_rx_fixup_internal() the device-declared frame length
  is only validated against an upper bound (`if (size > dev->net->mtu +…

--

net: usb: asix: reject a truncated Data header in rx_fixup

This adds a bound check in asix_rx_fixup_internal() before the four-byte
Data header read that starts a new frame, so a three-byte tail no longer
pushes offset past skb->len and wraps the copy_length clamp. On a short
header it logs, calls reset_asix_rx_fixup_info() and returns 0.

> diff --git a/drivers/net/usb/asix_common.c b/drivers/net/usb/asix_common.c
> index 4f03f4e57655d..8c5f863efe7d6 100644
> --- a/drivers/net/usb/asix_common.c
> +++ b/drivers/net/usb/asix_common.c
> @@ -179,6 +179,13 @@ int asix_rx_fixup_internal(struct usbnet *dev, struct sk_buff *skb,
>  				rx->split_head = false;
>  				offset += sizeof(u16);
>  			} else {
> +				if (offset + sizeof(u32) > skb->len) {

[Severity: High]

Should the resynchronisation read at the top of the same function get the
same bound?  It reads a four-byte Data header too, but its guard is
expressed on the un-rounded rx->remaining while the read uses the
even-rounded offset:

drivers/net/usb/asix_common.c:asix_rx_fixup_internal() {
	if (rx->remaining && (rx->remaining + sizeof(u32) <= skb->len)) {
		offset = ((rx->remaining + 1) & 0xfffe);
		rx->header = get_unaligned_le32(skb->data + offset);
		offset = 0;
	...
}

For odd rx->remaining the rounding adds one, so with rx->remaining == 61
and skb->len == 65 the guard passes (61 + 4 <= 65) but the read is done at
offset 62 and consumes bytes 62..65, while the last received byte is at
index 64.

rx->remaining is odd whenever the device declares an odd frame size
(size = rx->header & 0x7ff) that spans a URB boundary, and the extra
condition skb->len == rx->remaining + 4 makes skb->len odd, which is the
same odd-skb->len precondition the commit message calls out.

The stale byte then feeds the bits the synchronisation test looks at:

		size = (u16)(rx->header & 0x7ff);
		if (size != ((~rx->header >> 16) & 0x7ff)) {
			netdev_err(dev->net, "asix_rx_fixup() Data Header synchronisation was lost, remaining %d\n",
				   rx->remaining);
			reset_asix_rx_fixup_info(rx);
		}

so the verdict can be made on a byte the device never delivered, either
spuriously reporting lost synchronisation and dropping the in-flight
rx->ax_skb via reset_asix_rx_fixup_info()->kfree_skb(), or spuriously
accepting a desynchronised stream.

Would writing the guard on the rounded offset cover both sites
consistently, for example:

	if (rx->remaining &&
	    (((rx->remaining + 1) & 0xfffe) + sizeof(u32) <= skb->len)) {

> +					netdev_err(dev->net, "asix_rx_fixup() Short Data header, offset %d, len %d\n",
> +						   offset, skb->len);

[Severity: Low]

This isn't a bug, but skb->len is declared unsigned int in
include/linux/skbuff.h:

	unsigned int		len,
				data_len;

so %u would be the matching conversion here.  offset is int, so its %d is
fine.  The neighbouring "Bad SKB Length %d, %d" message has the same
mismatch, so this new line inherits it.

> +					reset_asix_rx_fixup_info(rx);
> +					return 0;
> +				}
> +
>  				rx->header = get_unaligned_le32(skb->data +
>  								offset);
>  				offset += sizeof(u32);

[Severity: Medium]

This isn't a bug introduced by this patch, but while looking at the
device-supplied length validation a few lines below the new check, only the
upper bound is tested:

drivers/net/usb/asix_common.c:asix_rx_fixup_internal() {
	...
			if (size > dev->net->mtu + ETH_HLEN + VLAN_HLEN) {
				netdev_dbg(dev->net, "asix_rx_fixup() Bad RX Length %d\n",
					   size);
				reset_asix_rx_fixup_info(rx);
				return 0;
			}
	...
			rx->ax_skb = netdev_alloc_skb_ip_align(dev->net, size);

			rx->remaining = size;
	...
}

Is there anything rejecting size below ETH_HLEN?  A header of 0x07ff0000
passes the complement test with size == 0, so netdev_alloc_skb_ip_align()
is called with 0, rx->remaining is 0, copy_length is 0 and the empty skb
goes straight out:

		if (rx->ax_skb) {
			skb_put_data(rx->ax_skb, skb->data + offset,
				     copy_length);
			if (!rx->remaining) {
				usbnet_skb_return(dev, rx->ax_skb);
				rx->ax_skb = NULL;
			}
		}

usbnet_skb_return() calls eth_type_trans() because skb->protocol is 0, and
eth_type_trans() dereferences a full struct ethhdr regardless of skb->len,
since skb_pull_inline() only returns NULL without stopping the caller:

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

	skb_pull_inline(skb, ETH_HLEN);
	return eth;
}

Can skb->protocol and skb->pkt_type then come from uninitialised bytes of
the freshly allocated skb, with a 0 to 13 byte runt handed to netif_rx()?
Any size in 1..ETH_HLEN-1 looks the same, just with a partially
uninitialised header.

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