Re: [PATCH net-next v2] 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 1 potential
issue(s) to consider:

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

- [High] The fix is incomplete: the sibling four-byte Data-header read in
  the same function is still allowed to read one byte past skb->len,…

Pre-existing issues:
- [Low] Missing lower bound on the device-supplied frame length in
  asix_rx_fixup_internal() (drivers/net/usb/asix_common.c:190-214).

--

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

This adds a bounds check in asix_rx_fixup_internal() before the four-byte
Data header read in the "start a new frame" branch, so a three-byte tail
no longer pushes offset past skb->len and wraps the copy_length clamp.
On a short header the parser state is reset and the URB skb is dropped.

> 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) {
> +					netdev_err(dev->net, "asix_rx_fixup() Short Data header, offset %d, len %d\n",
> +						   offset, skb->len);
> +					reset_asix_rx_fixup_info(rx);
> +					return 0;
> +				}
> +

[Severity: High]
This guard only covers the header read inside the loop.  Does the other
four-byte header read in the same function, the resynchronisation read
before the loop, still go one byte past skb->len under the same
device-supplied odd skb->len?

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;
	...
}

The predicate uses the unrounded rx->remaining, but the read happens at
the even-rounded offset.  For odd rx->remaining that offset is
rx->remaining + 1, so the read consumes bytes
[rx->remaining + 1 .. rx->remaining + 4].

With rx->remaining == 5 and a following URB of skb->len == 9 the test
passes (5 + 4 <= 9), offset becomes 6, and bytes 6..9 are read while the
last valid index is 8.

Is odd rx->remaining reachable?  rx->remaining is set to
size - copy_length with copy_length = skb->len - offset, and offset only
ever advances by sizeof(u16), sizeof(u32) or (copy_length + 1) & 0xfffe,
so an odd-sized frame spanning a URB boundary leaves an odd remainder.

The byte that gets read past the data then feeds the size versus
complement verdict that decides whether the in-flight rx->ax_skb is kept
or thrown away, so a good partial frame can be discarded or a real
desynchronisation accepted.

Would matching the two sites be enough here, e.g.

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

so that the pre-loop predicate describes the offset the read actually
uses?

>  				rx->header = get_unaligned_le32(skb->data +
>  								offset);
>  				offset += sizeof(u32);

[Severity: Low]
This is a pre-existing issue and not something this patch introduces, but
while looking at the header validation just below this read: is there a
lower bound on the device-supplied frame length?

drivers/net/usb/asix_common.c:asix_rx_fixup_internal() {
	size = (u16)(rx->header & 0x7ff);
	if (size != ((~rx->header >> 16) & 0x7ff)) {
	...
	if (size > dev->net->mtu + ETH_HLEN + VLAN_HLEN) {
	...
	rx->ax_skb = netdev_alloc_skb_ip_align(dev->net, size);
	rx->remaining = size;
}

A header of 0x07FF0000 gives size == 0 and (~header >> 16) & 0x7ff == 0,
so the complement test passes and only the upper bound is checked.  Any
size in 1..13 passes too.

The resulting runt ax_skb goes straight to usbnet_skb_return(), and since
asix sets FLAG_MULTI_PACKET the skb->len < ETH_HLEN filter in rx_process()
is not applied, so eth_type_trans() -> eth_skb_pull_mac() dereferences
(struct ethhdr *)skb->data and takes h_dest and h_proto from bytes that
were never written.

For the record, this does not hang the parser: offset already advances by
sizeof(u32) (or sizeof(u16) in the split-head path) in the same iteration
that reads the header, so a size == 0 stream still terminates the loop.

Would a size < ETH_HLEN rejection alongside the existing upper-bound check
be appropriate?

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