Re: [PATCH net-next v2] net: usb: asix: reject a truncated Data header in rx_fixup

Re: [PATCH net-next v2] net: usb: asix: reject a truncated Data header in rx_fixup

[netdev-bot+sashiko]

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

Re: [PATCH net-next v2] net: usb: asix: reject a truncated Data header in rx_fixup

[Aamir Ahmed]

On Tue, Sep 15, 2026 at 10:35:59AM +0300, Andy Shevchenko wrote:
> On Mon, Sep 14, 2026 at 10:23:45PM +0100, Aamir Ahmed wrote:
> > Only a device emitting an odd skb->len can get there - every asix
> > rx_urb_size is even and offset always advances by an even number of
> > bytes - so this is net-next material rather than a stable backport.
> >
> > Fixes: 8b5b6f5413e9 ("net: asix: handle packets crossing URB boundaries")
>
> If it's a fix, why net-next and not net in the Subject?

Yes, on review this should be on net ideally. I'll raise a v3 later
with this correction. Thank you.

pw-bot: changes-requested

Kind Regards
Aamir A.

Re: [PATCH net-next v2] net: usb: asix: reject a truncated Data header in rx_fixup

[Andy Shevchenko]

On Mon, Sep 14, 2026 at 10:23:45PM +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 - every asix
> rx_urb_size is even and offset always advances by an even number of
> bytes - so this is net-next material rather than a stable backport.
> 
> Fixes: 8b5b6f5413e9 ("net: asix: handle packets crossing URB boundaries")

If it's a fix, why net-next and not net in the Subject?

-- 
With Best Regards,
Andy Shevchenko

[PATCH net-next v2] 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 - every asix
rx_urb_size is even and offset always advances by an even number of
bytes - so this is net-next material rather than a stable backport.

Fixes: 8b5b6f5413e9 ("net: asix: handle packets crossing URB boundaries")
Assisted-by: LLM
Signed-off-by: Aamir Ahmed <elb12345@hotmail.co.uk>
---
v2:
  - guard the header read inside the loop, not the pre-loop predicate
  - rewrite the changelog around the unsigned underflow
  - target net-next, drop Cc: stable, repoint Fixes at 8b5b6f5413e9
  - add Assisted-by: LLM
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 4f03f4e57655..8c5f863efe7d 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);

base-commit: c297ed90fbba72d32b7759aae362b36d15b2db1f
-- 
2.43.0