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

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

[netdev-bot+sashiko]

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

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

[Andy Shevchenko]

On Wed, Sep 16, 2026 at 09:19:13AM +0100, Aamir Ahmed wrote:
> On Wed, Sep 16, 2026 at 09:50:21AM +0300, Andy Shevchenko wrote:

...

> > I don't know the rules about __func__ in the error messages in net, but above
> > may be simplified as
> >
> >     netdev_err(dev->net, "%s(): Short Data header, offset %d, len %d\n",
> >                __func__, offset, skb->len);
> 
> I've based the error messages on others I found in this function, here
> are some examples:
> 
>   netdev_err(dev->net, "asix_rx_fixup() Data Header synchronisation was lost, remaining %d\n", ...);
>   netdev_err(dev->net, "asix_rx_fixup() Bad Header Length 0x%x, offset %d\n", ...);
>   netdev_dbg(dev->net, "asix_rx_fixup() Bad RX Length %d\n", ...);
>   netdev_err(dev->net, "asix_rx_fixup() Bad SKB Length %d, %d\n", ...);
> 
> __func__ here expands to asix_rx_fixup_internal, the internal helper
> rather than the asix_rx_fixup() the others name.

Yes, and current state of affairs is confusing. If this is _internal() helper,
it should either take a name as a parameter, or use __func__, or ideally drop
it altogether and make error messages to be unique enough that one may easily
guess where the problem came from.

But okay, since it's a common style this discussion and possible amendment is
for another patch.

-- 
With Best Regards,
Andy Shevchenko

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

[Aamir Ahmed]

On Wed, Sep 16, 2026 at 09:50:21AM +0300, Andy Shevchenko wrote:
> I don't know the rules about __func__ in the error messages in net, but above
> may be simplified as
>
>     netdev_err(dev->net, "%s(): Short Data header, offset %d, len %d\n",
>                __func__, offset, skb->len);

I've based the error messages on others I found in this function, here
are some examples:

  netdev_err(dev->net, "asix_rx_fixup() Data Header synchronisation was lost, remaining %d\n", ...);
  netdev_err(dev->net, "asix_rx_fixup() Bad Header Length 0x%x, offset %d\n", ...);
  netdev_dbg(dev->net, "asix_rx_fixup() Bad RX Length %d\n", ...);
  netdev_err(dev->net, "asix_rx_fixup() Bad SKB Length %d, %d\n", ...);

__func__ here expands to asix_rx_fixup_internal, the internal helper
rather than the asix_rx_fixup() the others name.

Kind Regards
Aamir A.

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

[Andy Shevchenko]

On Tue, Sep 15, 2026 at 11:57:24PM +0100, Aamir Ahmed wrote:
> asix_rx_fixup_internal() runs its parsing loop while two bytes remain,
> but the branch that starts a new frame reads a four-byte Data header.
> Only a two-byte tail is special-cased, via split_head, so a three-byte
> tail reaches that read and leaves offset at skb->len + 1. The clamp
> below it then takes the unsigned difference skb->len - offset, which
> wraps, so copy_length becomes the full length the device asked for:
> skb_put_data() copies from one byte past the received data and
> usbnet_skb_return() passes the frame to the stack, before the trailing
> skb->len != offset check can report it.
> 
> Reject a Data header that does not fit and reset the parser state, as
> the other malformed-header paths do.
> 
> Only a device emitting an odd skb->len can get there, since offset
> always advances by an even number of bytes.

...

> int asix_rx_fixup_internal(struct usbnet *dev, struct sk_buff *skb,

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

I don't know the rules about __func__ in the error messages in net, but above
may be simplified as

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

-- 
With Best Regards,
Andy Shevchenko

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

[Aamir Ahmed]

asix_rx_fixup_internal() runs its parsing loop while two bytes remain,
but the branch that starts a new frame reads a four-byte Data header.
Only a two-byte tail is special-cased, via split_head, so a three-byte
tail reaches that read and leaves offset at skb->len + 1. The clamp
below it then takes the unsigned difference skb->len - offset, which
wraps, so copy_length becomes the full length the device asked for:
skb_put_data() copies from one byte past the received data and
usbnet_skb_return() passes the frame to the stack, before the trailing
skb->len != offset check can report it.

Reject a Data header that does not fit and reset the parser state, as
the other malformed-header paths do.

Only a device emitting an odd skb->len can get there, since offset
always advances by an even number of bytes.

Fixes: 8b5b6f5413e9 ("net: asix: handle packets crossing URB boundaries")
Assisted-by: LLM
Signed-off-by: Aamir Ahmed <elb12345@hotmail.co.uk>
---

Notes:
    v3:
      - target net rather than net-next (Andy Shevchenko)
      - trim the last paragraph to the offset argument
      No code change from v2.
    v2: https://lore.kernel.org/netdev/AS8P251MB000193C798F98A11C73C3E9DC8BB2@AS8P251MB0001.EURP251.PROD.OUTLOOK.COM/
    v1: https://lore.kernel.org/netdev/AS8P251MB00014BB4591CE6011152DB4FC8B22@AS8P251MB0001.EURP251.PROD.OUTLOOK.COM/
    
    Built with W=1 (asix_common.o) on x86_64; no warnings. I have no asix
    hardware, so this is not runtime-tested.

 drivers/net/usb/asix_common.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/drivers/net/usb/asix_common.c b/drivers/net/usb/asix_common.c
index 4f03f4e..8c5f863 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;
+				}
+
 				rx->header = get_unaligned_le32(skb->data +
 								offset);
 				offset += sizeof(u32);
-- 
2.53.0.windows.1