[PATCH v2 1/3] usb: misc: usbio: reject endpoints smaller than the packet header

[PATCH v2 1/3] usb: misc: usbio: reject endpoints smaller than the packet header

[HE WEI (ギカク)]

usbio_ctrl_msg() and usbio_bulk_msg() bound the caller's transfer sizes
against the endpoint packet size minus the fixed protocol header:

	if ((obuf_len > (usbio->txbuf_len - sizeof(*bpkt))) ||
	    (ibuf_len > (usbio->txbuf_len - sizeof(*bpkt))))
		return -EMSGSIZE;

usbio->txbuf_len is a u16 and sizeof(*bpkt) is a size_t, so the
subtraction is done in size_t.  struct usbio_bulk_packet is 5 bytes and
struct usbio_ctrl_packet is 4 bytes, so any endpoint smaller than that
makes the expression wrap to a value close to ULONG_MAX, both
comparisons become false and the check is disabled.

txbuf_len and rxbuf_len come from the bulk endpoint wMaxPacketSize.
usb_parse_endpoint() only clamps wMaxPacketSize downwards, it never
enforces a lower bound:

	if (maxp > j) {
		dev_notice(ddev, "... has invalid maxpacket %d, setting to %d\n",
		    ..., maxp, j);
		maxp = j;
		endpoint->desc.wMaxPacketSize = cpu_to_le16(i | maxp);
	}

A wMaxPacketSize of 0 is merely logged with dev_notice(), and a bulk
endpoint declaring 1 is accepted verbatim.  usb_submit_urb() rejects
maxpacket 0, but nothing rejects 1.

A device claiming one of the ids in usbio_table[] and advertising a bulk
out endpoint with wMaxPacketSize 1 therefore ends up with a one byte
usbio->txbuf, and the very first I2C transfer reaches usbio_bulk_msg()
via usbio_i2c_init() with obuf_len = 7.  The wrapped check passes and the
packet header stores overflow the slab object before memcpy() is even
reached:

	bpkt = usbio->txbuf;
	bpkt->header.type = type;	/* txbuf[0] */
	bpkt->header.cmd = cmd;		/* txbuf[1], out of bounds */
	bpkt->header.flags = ...;	/* txbuf[2], out of bounds */
	bpkt->len = cpu_to_le16(obuf_len);	/* txbuf[3..4] */
	memcpy(bpkt->data, obuf, obuf_len);	/* txbuf[5..] */

Note that this is all complete before usb_bulk_msg() is called, so it
does not depend on the host controller being willing to run a transfer
on such an endpoint.  With KASAN it is a slab-out-of-bounds write.
Through usbio_i2c_write() obuf_len becomes sizeof(struct usbio_i2c_rw) +
msg->len, so the length and the contents of the overflow are controlled
by whoever can issue I2C transfers, up to the 4096 byte adapter limit.

wMaxPacketSize 0 is worse in a different way: devm_kzalloc(dev, 0)
returns ZERO_SIZE_PTR rather than NULL, so the existing

	if (!usbio->txbuf)
		return -ENOMEM;

does not catch it and the same header stores dereference ZERO_SIZE_PTR.
usbio_bulk_recv() has the same problem on the receive side: it reads
bpkt->header.flags at offset 2 of usbio->rxbuf before any length
validation.

The control side is different.  For low, full and high speed hub.c
forces ep0 wMaxPacketSize to 8, 16, 32 or 64, all larger than the
control header, so usbio_ctrl_msg()'s check cannot wrap there.  For
SuperSpeed it accepts any bMaxPacketSize0 that encodes a non-zero value:

	i = maxp0;
	if (udev->speed >= USB_SPEED_SUPER) {
		if (maxp0 <= 16)
			i = 1 << maxp0;
		else
			i = 0;		/* Invalid */
	}

combined with "(udev->speed >= USB_SPEED_SUPER && i > 0)" below, so
bMaxPacketSize0 of 0 or 1 gives a ctrlbuf of 1 or 2 bytes and the same
wrap, during the five usbio_ctrl_msg() calls in usbio_probe().  Whether
a given host controller will operate such an ep0 has not been
established; the control length is checked here regardless so that the
invariant is stated once for all three buffers.

Validate the three lengths in usbio_probe(), which is the only place
that assigns them, instead of hardening each arithmetic site.  A bridge
whose endpoints cannot even carry the protocol header is unusable, so
refusing to probe is the correct outcome.  Rejecting the equal case as
well is deliberate: it does not wrap, but it leaves no room for a
payload, and excluding it makes every "<buf>_len - sizeof(*pkt)" in the
driver a valid size of at least one.

No supported bridge is affected.  The low, full and high speed devices
this driver binds to have an ep0 packet size of at least 8, and they use
bulk endpoints of 64, or 63 via USBIO_QUIRK_BULK_MAXP_63.

Found by code review, doing variant analysis on the code around
8c6314489550.  The overflow was reproduced under AddressSanitizer with a
userspace model of usbio_probe() and usbio_bulk_msg() that uses this
driver's struct definitions, checks and stores verbatim; it has not been
exercised on hardware or on dummy_hcd.

Fixes: 121a0f839dbb ("usb: misc: Add Intel USBIO bridge driver")
Cc: stable@vger.kernel.org
Assisted-by: Claude:claude-opus-5 asan
Signed-off-by: HE WEI (ギカク) <skyexpoc@gmail.com>
---
--- a/drivers/usb/misc/usbio.c
+++ b/drivers/usb/misc/usbio.c
@@ -577,6 +577,24 @@
 
 	usbio->ctrl_pipe = usb_endpoint_num(&udev->ep0.desc);
 	usbio->ctrlbuf_len = usb_maxpacket(udev, usbio->ctrl_pipe);
+
+	/*
+	 * Every transfer starts with a fixed size packet header and the length
+	 * checks in usbio_ctrl_msg() and usbio_bulk_msg() are computed as
+	 * "<buf>_len - sizeof(*pkt)".  Those lengths are u16 while sizeof() is
+	 * size_t, so an endpoint smaller than its header makes the subtraction
+	 * wrap to a huge value, disabling the checks and overflowing the
+	 * buffers.  An endpoint exactly the size of the header does not wrap
+	 * but leaves no room for a payload, so reject that too and every
+	 * "<buf>_len - sizeof(*pkt)" below is a valid size of at least one.
+	 * usbio_probe() is the only place assigning these lengths, so
+	 * validating them once here covers every user.
+	 */
+	if (usbio->ctrlbuf_len <= sizeof(struct usbio_ctrl_packet))
+		return dev_err_probe(dev, -EINVAL,
+				     "Control endpoint maxpacket too small: %u\n",
+				     usbio->ctrlbuf_len);
+
 	usbio->ctrlbuf = devm_kzalloc(dev, usbio->ctrlbuf_len, GFP_KERNEL);
 	if (!usbio->ctrlbuf)
 		return -ENOMEM;
@@ -596,6 +614,11 @@
 	else
 		usbio->txbuf_len = usb_endpoint_maxp(ep_out);
 
+	if (usbio->txbuf_len <= sizeof(struct usbio_bulk_packet))
+		return dev_err_probe(dev, -EINVAL,
+				     "Bulk out endpoint maxpacket too small: %u\n",
+				     usbio->txbuf_len);
+
 	usbio->txbuf = devm_kzalloc(dev, usbio->txbuf_len, GFP_KERNEL);
 	if (!usbio->txbuf)
 		return -ENOMEM;
@@ -607,6 +630,11 @@
 	else
 		usbio->rxbuf_len = usb_endpoint_maxp(ep_in);
 
+	if (usbio->rxbuf_len <= sizeof(struct usbio_bulk_packet))
+		return dev_err_probe(dev, -EINVAL,
+				     "Bulk in endpoint maxpacket too small: %u\n",
+				     usbio->rxbuf_len);
+
 	usbio->rxbuf = devm_kzalloc(dev, usbio->rxbuf_len, GFP_KERNEL);
 	if (!usbio->rxbuf)
 		return -ENOMEM;
-- 
2.51.0

Re: [PATCH v2 1/3] usb: misc: usbio: reject endpoints smaller than the packet header

[Greg Kroah-Hartman]

On Sun, Jul 26, 2026 at 08:35:07PM +0900, HE WEI (ギカク) wrote:
> usbio_ctrl_msg() and usbio_bulk_msg() bound the caller's transfer sizes
> against the endpoint packet size minus the fixed protocol header:
> 
> 	if ((obuf_len > (usbio->txbuf_len - sizeof(*bpkt))) ||
> 	    (ibuf_len > (usbio->txbuf_len - sizeof(*bpkt))))
> 		return -EMSGSIZE;
> 
> usbio->txbuf_len is a u16 and sizeof(*bpkt) is a size_t, so the
> subtraction is done in size_t.  struct usbio_bulk_packet is 5 bytes and
> struct usbio_ctrl_packet is 4 bytes, so any endpoint smaller than that
> makes the expression wrap to a value close to ULONG_MAX, both
> comparisons become false and the check is disabled.
> 
> txbuf_len and rxbuf_len come from the bulk endpoint wMaxPacketSize.
> usb_parse_endpoint() only clamps wMaxPacketSize downwards, it never
> enforces a lower bound:
> 
> 	if (maxp > j) {
> 		dev_notice(ddev, "... has invalid maxpacket %d, setting to %d\n",
> 		    ..., maxp, j);
> 		maxp = j;
> 		endpoint->desc.wMaxPacketSize = cpu_to_le16(i | maxp);
> 	}
> 
> A wMaxPacketSize of 0 is merely logged with dev_notice(), and a bulk
> endpoint declaring 1 is accepted verbatim.  usb_submit_urb() rejects
> maxpacket 0, but nothing rejects 1.
> 
> A device claiming one of the ids in usbio_table[] and advertising a bulk
> out endpoint with wMaxPacketSize 1 therefore ends up with a one byte
> usbio->txbuf, and the very first I2C transfer reaches usbio_bulk_msg()
> via usbio_i2c_init() with obuf_len = 7.  The wrapped check passes and the
> packet header stores overflow the slab object before memcpy() is even
> reached:
> 
> 	bpkt = usbio->txbuf;
> 	bpkt->header.type = type;	/* txbuf[0] */
> 	bpkt->header.cmd = cmd;		/* txbuf[1], out of bounds */
> 	bpkt->header.flags = ...;	/* txbuf[2], out of bounds */
> 	bpkt->len = cpu_to_le16(obuf_len);	/* txbuf[3..4] */
> 	memcpy(bpkt->data, obuf, obuf_len);	/* txbuf[5..] */
> 
> Note that this is all complete before usb_bulk_msg() is called, so it
> does not depend on the host controller being willing to run a transfer
> on such an endpoint.  With KASAN it is a slab-out-of-bounds write.
> Through usbio_i2c_write() obuf_len becomes sizeof(struct usbio_i2c_rw) +
> msg->len, so the length and the contents of the overflow are controlled
> by whoever can issue I2C transfers, up to the 4096 byte adapter limit.
> 
> wMaxPacketSize 0 is worse in a different way: devm_kzalloc(dev, 0)
> returns ZERO_SIZE_PTR rather than NULL, so the existing
> 
> 	if (!usbio->txbuf)
> 		return -ENOMEM;
> 
> does not catch it and the same header stores dereference ZERO_SIZE_PTR.
> usbio_bulk_recv() has the same problem on the receive side: it reads
> bpkt->header.flags at offset 2 of usbio->rxbuf before any length
> validation.
> 
> The control side is different.  For low, full and high speed hub.c
> forces ep0 wMaxPacketSize to 8, 16, 32 or 64, all larger than the
> control header, so usbio_ctrl_msg()'s check cannot wrap there.  For
> SuperSpeed it accepts any bMaxPacketSize0 that encodes a non-zero value:
> 
> 	i = maxp0;
> 	if (udev->speed >= USB_SPEED_SUPER) {
> 		if (maxp0 <= 16)
> 			i = 1 << maxp0;
> 		else
> 			i = 0;		/* Invalid */
> 	}
> 
> combined with "(udev->speed >= USB_SPEED_SUPER && i > 0)" below, so
> bMaxPacketSize0 of 0 or 1 gives a ctrlbuf of 1 or 2 bytes and the same
> wrap, during the five usbio_ctrl_msg() calls in usbio_probe().  Whether
> a given host controller will operate such an ep0 has not been
> established; the control length is checked here regardless so that the
> invariant is stated once for all three buffers.
> 
> Validate the three lengths in usbio_probe(), which is the only place
> that assigns them, instead of hardening each arithmetic site.  A bridge
> whose endpoints cannot even carry the protocol header is unusable, so
> refusing to probe is the correct outcome.  Rejecting the equal case as
> well is deliberate: it does not wrap, but it leaves no room for a
> payload, and excluding it makes every "<buf>_len - sizeof(*pkt)" in the
> driver a valid size of at least one.
> 
> No supported bridge is affected.  The low, full and high speed devices
> this driver binds to have an ep0 packet size of at least 8, and they use
> bulk endpoints of 64, or 63 via USBIO_QUIRK_BULK_MAXP_63.
> 
> Found by code review, doing variant analysis on the code around
> 8c6314489550.  The overflow was reproduced under AddressSanitizer with a
> userspace model of usbio_probe() and usbio_bulk_msg() that uses this
> driver's struct definitions, checks and stores verbatim; it has not been
> exercised on hardware or on dummy_hcd.
> 
> Fixes: 121a0f839dbb ("usb: misc: Add Intel USBIO bridge driver")
> Cc: stable@vger.kernel.org
> Assisted-by: Claude:claude-opus-5 asan
> Signed-off-by: HE WEI (ギカク) <skyexpoc@gmail.com>
> ---
> --- a/drivers/usb/misc/usbio.c
> +++ b/drivers/usb/misc/usbio.c
> @@ -577,6 +577,24 @@
>  
>  	usbio->ctrl_pipe = usb_endpoint_num(&udev->ep0.desc);
>  	usbio->ctrlbuf_len = usb_maxpacket(udev, usbio->ctrl_pipe);
> +
> +	/*
> +	 * Every transfer starts with a fixed size packet header and the length
> +	 * checks in usbio_ctrl_msg() and usbio_bulk_msg() are computed as
> +	 * "<buf>_len - sizeof(*pkt)".  Those lengths are u16 while sizeof() is
> +	 * size_t, so an endpoint smaller than its header makes the subtraction
> +	 * wrap to a huge value, disabling the checks and overflowing the
> +	 * buffers.  An endpoint exactly the size of the header does not wrap
> +	 * but leaves no room for a payload, so reject that too and every
> +	 * "<buf>_len - sizeof(*pkt)" below is a valid size of at least one.
> +	 * usbio_probe() is the only place assigning these lengths, so
> +	 * validating them once here covers every user.
> +	 */
> +	if (usbio->ctrlbuf_len <= sizeof(struct usbio_ctrl_packet))
> +		return dev_err_probe(dev, -EINVAL,
> +				     "Control endpoint maxpacket too small: %u\n",
> +				     usbio->ctrlbuf_len);

LLMs are so chatty it's not even funny.

Just check the value and return an error, it's obvious what you are
doing, no need to write a 10 line comment about the thing, or hundred+
lines of changelog text, right?

Please look at how we normally write kernel changelog texts, the
documentation for how to do so should be very complete and the way your
LLM generated this is obviously not in line with that.


> +
>  	usbio->ctrlbuf = devm_kzalloc(dev, usbio->ctrlbuf_len, GFP_KERNEL);
>  	if (!usbio->ctrlbuf)
>  		return -ENOMEM;
> @@ -596,6 +614,11 @@

How did git generate this diff, no function name?


>  	else
>  		usbio->txbuf_len = usb_endpoint_maxp(ep_out);
>  
> +	if (usbio->txbuf_len <= sizeof(struct usbio_bulk_packet))

Why not use the proper function to verify endpoint sizes instead of
manually checking them like this?


> +		return dev_err_probe(dev, -EINVAL,
> +				     "Bulk out endpoint maxpacket too small: %u\n",
> +				     usbio->txbuf_len);
> +
>  	usbio->txbuf = devm_kzalloc(dev, usbio->txbuf_len, GFP_KERNEL);
>  	if (!usbio->txbuf)
>  		return -ENOMEM;
> @@ -607,6 +630,11 @@
>  	else
>  		usbio->rxbuf_len = usb_endpoint_maxp(ep_in);
>  
> +	if (usbio->rxbuf_len <= sizeof(struct usbio_bulk_packet))

and this, they both can be checked at the same time, with the same
function call.

thanks,

greg k-h

Re: [PATCH v2 1/3] usb: misc: usbio: reject endpoints smaller than the packet header

[Andi Shyti]

Hi He Wei,

On Sun, Jul 26, 2026 at 08:35:07PM +0900, HE WEI (ギカク) wrote:
> usbio_ctrl_msg() and usbio_bulk_msg() bound the caller's transfer sizes
> against the endpoint packet size minus the fixed protocol header:
> 
> 	if ((obuf_len > (usbio->txbuf_len - sizeof(*bpkt))) ||
> 	    (ibuf_len > (usbio->txbuf_len - sizeof(*bpkt))))
> 		return -EMSGSIZE;
> 
> usbio->txbuf_len is a u16 and sizeof(*bpkt) is a size_t, so the
> subtraction is done in size_t.  struct usbio_bulk_packet is 5 bytes and

... blah blah blah ...

> via usbio_i2c_init() with obuf_len = 7.  The wrapped check passes and the
> packet header stores overflow the slab object before memcpy() is even
> reached:
> 
> 	bpkt = usbio->txbuf;
> 	bpkt->header.type = type;	/* txbuf[0] */
> 	bpkt->header.cmd = cmd;		/* txbuf[1], out of bounds */
> 	bpkt->header.flags = ...;	/* txbuf[2], out of bounds */
> 	bpkt->len = cpu_to_le16(obuf_len);	/* txbuf[3..4] */
> 	memcpy(bpkt->data, obuf, obuf_len);	/* txbuf[5..] */
> 
> Note that this is all complete before usb_bulk_msg() is called, so it
> does not depend on the host controller being willing to run a transfer
> on such an endpoint.  With KASAN it is a slab-out-of-bounds write.

... blah blah blah ...

> combined with "(udev->speed >= USB_SPEED_SUPER && i > 0)" below, so
> bMaxPacketSize0 of 0 or 1 gives a ctrlbuf of 1 or 2 bytes and the same
> wrap, during the five usbio_ctrl_msg() calls in usbio_probe().  Whether
> a given host controller will operate such an ep0 has not been

... blah blah blah ...

> Found by code review, doing variant analysis on the code around
> 8c6314489550.  The overflow was reproduced under AddressSanitizer with a
> userspace model of usbio_probe() and usbio_bulk_msg() that uses this
> driver's struct definitions, checks and stores verbatim; it has not been
> exercised on hardware or on dummy_hcd.
> 
> Fixes: 121a0f839dbb ("usb: misc: Add Intel USBIO bridge driver")
> Cc: stable@vger.kernel.org
> Assisted-by: Claude:claude-opus-5 asan
> Signed-off-by: HE WEI (ギカク) <skyexpoc@gmail.com>

Please use Latin characters, with the first letter of each name
capitalized: "He Wei", not "HE WEI".

Also, your commit logs are far too long. If I had to spend this
much time reading AI generated commit logs, I'd end up spending
all my time chatting with AI instead of real people.

Please check what you are posting before sending it. Write the
commit logs yourself and use AI only to help with the grammar.

Thanks,
Andi

Re: [PATCH v2 1/3] usb: misc: usbio: reject endpoints smaller than the packet header

[Greg Kroah-Hartman]

On Sun, Jul 26, 2026 at 08:35:07PM +0900, HE WEI (ギカク) wrote:
> usbio_ctrl_msg() and usbio_bulk_msg() bound the caller's transfer sizes
> against the endpoint packet size minus the fixed protocol header:
> 
> 	if ((obuf_len > (usbio->txbuf_len - sizeof(*bpkt))) ||
> 	    (ibuf_len > (usbio->txbuf_len - sizeof(*bpkt))))
> 		return -EMSGSIZE;
> 
> usbio->txbuf_len is a u16 and sizeof(*bpkt) is a size_t, so the
> subtraction is done in size_t.  struct usbio_bulk_packet is 5 bytes and
> struct usbio_ctrl_packet is 4 bytes, so any endpoint smaller than that
> makes the expression wrap to a value close to ULONG_MAX, both
> comparisons become false and the check is disabled.
> 
> txbuf_len and rxbuf_len come from the bulk endpoint wMaxPacketSize.
> usb_parse_endpoint() only clamps wMaxPacketSize downwards, it never
> enforces a lower bound:
> 
> 	if (maxp > j) {
> 		dev_notice(ddev, "... has invalid maxpacket %d, setting to %d\n",
> 		    ..., maxp, j);
> 		maxp = j;
> 		endpoint->desc.wMaxPacketSize = cpu_to_le16(i | maxp);
> 	}
> 
> A wMaxPacketSize of 0 is merely logged with dev_notice(), and a bulk
> endpoint declaring 1 is accepted verbatim.  usb_submit_urb() rejects
> maxpacket 0, but nothing rejects 1.
> 
> A device claiming one of the ids in usbio_table[] and advertising a bulk
> out endpoint with wMaxPacketSize 1 therefore ends up with a one byte
> usbio->txbuf, and the very first I2C transfer reaches usbio_bulk_msg()
> via usbio_i2c_init() with obuf_len = 7.  The wrapped check passes and the
> packet header stores overflow the slab object before memcpy() is even
> reached:
> 
> 	bpkt = usbio->txbuf;
> 	bpkt->header.type = type;	/* txbuf[0] */
> 	bpkt->header.cmd = cmd;		/* txbuf[1], out of bounds */
> 	bpkt->header.flags = ...;	/* txbuf[2], out of bounds */
> 	bpkt->len = cpu_to_le16(obuf_len);	/* txbuf[3..4] */
> 	memcpy(bpkt->data, obuf, obuf_len);	/* txbuf[5..] */
> 
> Note that this is all complete before usb_bulk_msg() is called, so it
> does not depend on the host controller being willing to run a transfer
> on such an endpoint.  With KASAN it is a slab-out-of-bounds write.
> Through usbio_i2c_write() obuf_len becomes sizeof(struct usbio_i2c_rw) +
> msg->len, so the length and the contents of the overflow are controlled
> by whoever can issue I2C transfers, up to the 4096 byte adapter limit.
> 
> wMaxPacketSize 0 is worse in a different way: devm_kzalloc(dev, 0)
> returns ZERO_SIZE_PTR rather than NULL, so the existing
> 
> 	if (!usbio->txbuf)
> 		return -ENOMEM;
> 
> does not catch it and the same header stores dereference ZERO_SIZE_PTR.
> usbio_bulk_recv() has the same problem on the receive side: it reads
> bpkt->header.flags at offset 2 of usbio->rxbuf before any length
> validation.
> 
> The control side is different.  For low, full and high speed hub.c
> forces ep0 wMaxPacketSize to 8, 16, 32 or 64, all larger than the
> control header, so usbio_ctrl_msg()'s check cannot wrap there.  For
> SuperSpeed it accepts any bMaxPacketSize0 that encodes a non-zero value:
> 
> 	i = maxp0;
> 	if (udev->speed >= USB_SPEED_SUPER) {
> 		if (maxp0 <= 16)
> 			i = 1 << maxp0;
> 		else
> 			i = 0;		/* Invalid */
> 	}
> 
> combined with "(udev->speed >= USB_SPEED_SUPER && i > 0)" below, so
> bMaxPacketSize0 of 0 or 1 gives a ctrlbuf of 1 or 2 bytes and the same
> wrap, during the five usbio_ctrl_msg() calls in usbio_probe().  Whether
> a given host controller will operate such an ep0 has not been
> established; the control length is checked here regardless so that the
> invariant is stated once for all three buffers.
> 
> Validate the three lengths in usbio_probe(), which is the only place
> that assigns them, instead of hardening each arithmetic site.  A bridge
> whose endpoints cannot even carry the protocol header is unusable, so
> refusing to probe is the correct outcome.  Rejecting the equal case as
> well is deliberate: it does not wrap, but it leaves no room for a
> payload, and excluding it makes every "<buf>_len - sizeof(*pkt)" in the
> driver a valid size of at least one.
> 
> No supported bridge is affected.  The low, full and high speed devices
> this driver binds to have an ep0 packet size of at least 8, and they use
> bulk endpoints of 64, or 63 via USBIO_QUIRK_BULK_MAXP_63.
> 
> Found by code review, doing variant analysis on the code around
> 8c6314489550.  The overflow was reproduced under AddressSanitizer with a
> userspace model of usbio_probe() and usbio_bulk_msg() that uses this
> driver's struct definitions, checks and stores verbatim; it has not been
> exercised on hardware or on dummy_hcd.
> 
> Fixes: 121a0f839dbb ("usb: misc: Add Intel USBIO bridge driver")
> Cc: stable@vger.kernel.org
> Assisted-by: Claude:claude-opus-5 asan
> Signed-off-by: HE WEI (ギカク) <skyexpoc@gmail.com>
> ---
> --- a/drivers/usb/misc/usbio.c
> +++ b/drivers/usb/misc/usbio.c
> @@ -577,6 +577,24 @@

This is a very odd diff, what tool created it?  Did "Claude" manually
write a diff?  Please use the correct git diff output so that we can
correctly validate it is proper.

thanks,

greg k-h

[PATCH v2 2/3] i2c: usbio: reject bridges with undersized transfer buffers

[HE WEI (ギカク)]

usbio_i2c_read() and usbio_i2c_write() derive their per-transfer chunk
size from the bridge buffer sizes:

	u16 rxchunk = i2c->rxbuf_len - I2C_RW_OVERHEAD;
	u16 txchunk = i2c->txbuf_len - I2C_RW_OVERHEAD;

I2C_RW_OVERHEAD is sizeof(struct usbio_bulk_packet) +
sizeof(struct usbio_i2c_rw), i.e. a size_t of value 10, while the two
lengths are u16.  The subtraction is therefore done in size_t and, for a
bridge reporting less than 10, wraps before being truncated back into a
u16.  rxbuf_len = 8, a legal full speed bulk wMaxPacketSize, gives
rxchunk = 65534.

The chunk size decides whether a transfer has to be split:

	if (msg->len > rxchunk) {
		/* Need to split the input buffer */

The adapter quirks cap msg->len at 4096, so a wrapped chunk size makes
that condition permanently false, the splitting path becomes dead code,
and the single-shot path asks usbio_bulk_msg() for more than the bridge
buffer can hold.

The boundary value is worse.  rxbuf_len of exactly 10 gives rxchunk 0,
and I2C_AQ_NO_ZERO_LEN guarantees msg->len is at least 1, so the split
loop is entered and never advances:

		do {
			if (msg->len - len < rxchunk)
				rxchunk = msg->len - len;
			ret = usbio_bulk_msg(...);
			if (ret < 0)
				return ret;
			memcpy(&msg->buf[len], rbuf->data, rxchunk);
			len += rxchunk;
		} while (msg->len > len);

"msg->len - len < rxchunk" is 1 < 0, so rxchunk stays 0 and len never
grows.  The only exit is an error return, so a device that keeps
answering keeps the loop running indefinitely and uninterruptibly, while
holding both usbio->bulk_mutex and the client mutex, which blocks every
other user of the bridge.  txbuf_len is independent and can be left at a
normal value, so usbio_i2c_init() succeeds and the read is reached.

Check both lengths once at probe time.

This is a behaviour change: a bridge reporting a bulk wMaxPacketSize
below 11 no longer gets an I2C adapter at all.  Such an adapter could
never have completed a transfer.  usbio_i2c_xfer() runs usbio_i2c_init()
first and bails out on failure, and that needs obuf_len = 7 to fit in
txbuf_len - sizeof(struct usbio_bulk_packet), so anything below 12
already returned -EMSGSIZE for every single transfer.  Failing to probe
is better than registering an adapter that cannot work.  No supported
bridge is affected: they use 64, or 63 via USBIO_QUIRK_BULK_MAXP_63.

This additionally covers the case where usbio_get_txrxbuf_len() returns
without writing its output parameters because the bridge is already
gone; the lengths then stay 0 and the bridge is rejected instead of
being used.

Found by code review.  The non-terminating loop was reproduced with a
userspace model of usbio_i2c_read()'s split path; it has not been
exercised on hardware or on dummy_hcd.

Fixes: 121a0f839dbb ("usb: misc: Add Intel USBIO bridge driver")
Cc: stable@vger.kernel.org
Assisted-by: Claude:claude-opus-5 asan
Signed-off-by: HE WEI (ギカク) <skyexpoc@gmail.com>
---
--- a/drivers/i2c/busses/i2c-usbio.c
+++ b/drivers/i2c/busses/i2c-usbio.c
@@ -245,6 +245,20 @@
 
 	usbio_acpi_bind(i2c->adev, usbio_i2c_acpi_hids);
 	usbio_get_txrxbuf_len(i2c->adev, &i2c->txbuf_len, &i2c->rxbuf_len);
+
+	/*
+	 * usbio_i2c_read() and usbio_i2c_write() compute their chunk size as
+	 * "<buf>_len - I2C_RW_OVERHEAD".  Those lengths are u16 and the
+	 * overhead is a size_t, so a bridge reporting less than the overhead
+	 * makes the subtraction wrap and the truncated u16 chunk size then
+	 * defeats the splitting logic, while reporting exactly the overhead
+	 * makes the chunk 0 and the split loop in usbio_i2c_read() never
+	 * advance.  Refuse to attach to such a bridge.
+	 */
+	if (i2c->txbuf_len <= I2C_RW_OVERHEAD || i2c->rxbuf_len <= I2C_RW_OVERHEAD)
+		return dev_err_probe(dev, -EINVAL,
+				     "Bridge buffers too small: tx %u rx %u\n",
+				     i2c->txbuf_len, i2c->rxbuf_len);
 
 	i2c->rwbuf = devm_kzalloc(dev, max(i2c->txbuf_len, i2c->rxbuf_len), GFP_KERNEL);
 	if (!i2c->rwbuf)
-- 
2.51.0

[PATCH v2 3/3] usb: misc: usbio: bound the debug hex dumps by the received length

[HE WEI (ギカク)]

usbio_ctrl_msg() and usbio_bulk_msg() hex dump the reply with "%*phN",
using a length the device supplied and that has not been validated yet:

	ret = usb_control_msg(usbio->udev, pipe, 0, request | USB_DIR_IN, 0, 0,
			      cpkt, cpkt_len, USBIO_CTRLXFER_TIMEOUT);
	dev_dbg(usbio->dev, "control in %d hdr %*phN data %*phN\n", ret,
		(int)sizeof(*cpkt), cpkt, (int)cpkt->len, cpkt->data);

cpkt->len is a u8 read back out of usbio->ctrlbuf after the IN transfer,
and bpkt_len in usbio_bulk_msg() is le16_to_cpu(bpkt->len) read out of
usbio->rxbuf.  Both are handed to "%*phN" as the field width.
hex_string() in lib/vsprintf.c caps that at 64, and dereferences every
byte up to that cap regardless of how much room the output buffer has:

	if (spec.field_width > 0)
		len = min(spec.field_width, 64);

	for (i = 0; i < len; ++i) {
		if (buf < end)
			*buf = hex_asc_hi(addr[i]);

So the dump reads up to byte 67 of ctrlbuf and byte 68 of rxbuf.  Both
buffers are sized from the endpoint packet sizes, so this is out of
bounds whenever ep0 wMaxPacketSize is below 68 and whenever the bulk in
endpoint is below 69.  That covers every low, full and high speed ep0
(8, 16, 32 or 64) and the bulk sizes the supported bridges actually use
(64, or 63 for the Synaptics Sabre via USBIO_QUIRK_BULK_MAXP_63).

A device answering one of the five usbio_ctrl_msg() calls in
usbio_probe() with cpkt->len = 255 therefore leaks up to 60 bytes of
adjacent slab memory into the kernel log during enumeration, before any
user space is involved.  On the bulk path a reply claiming
bpkt->len = 0xffff reads 5 bytes past a 64 byte rxbuf.

Unlike the endpoint size issues this needs no malformed descriptor at
all; it only needs the dev_dbg() calls to be enabled.  When they are, it
is a slab-out-of-bounds read under KASAN and the bytes reach dmesg.

Bound both dumps by the number of bytes actually received.  That is in
bounds by construction and also stops the dump printing stale bytes left
over from an earlier transfer.  The matching dumps on the two OUT paths
are left alone: they use lengths the driver itself just wrote, which the
size checks above them already bound.

Found by code review.  The out-of-bounds read was reproduced under
AddressSanitizer with a userspace model of the two dev_dbg() call sites
and of hex_string()'s field-width handling; it has not been exercised on
hardware or on dummy_hcd.

Fixes: 121a0f839dbb ("usb: misc: Add Intel USBIO bridge driver")
Cc: stable@vger.kernel.org
Assisted-by: Claude:claude-opus-5 asan
Signed-off-by: HE WEI (ギカク) <skyexpoc@gmail.com>
---
--- a/drivers/usb/misc/usbio.c
+++ b/drivers/usb/misc/usbio.c
@@ -14,6 +14,7 @@
 #include <linux/dev_printk.h>
 #include <linux/device.h>
 #include <linux/lockdep.h>
+#include <linux/minmax.h>
 #include <linux/mutex.h>
 #include <linux/string.h>
 #include <linux/types.h>
@@ -141,7 +142,7 @@
 	struct usbio_ctrl_packet *cpkt;
 	unsigned int pipe;
 	u16 cpkt_len;
-	int ret;
+	int dbg_len, ret;
 
 	lockdep_assert_held(&usbio->ctrl_mutex);
 
@@ -181,8 +182,15 @@
 	cpkt_len = sizeof(*cpkt) + ibuf_len;
 	ret = usb_control_msg(usbio->udev, pipe, 0, request | USB_DIR_IN, 0, 0,
 			      cpkt, cpkt_len, USBIO_CTRLXFER_TIMEOUT);
+	/*
+	 * cpkt->len has just been written by the device and is not validated
+	 * until below, while %*phN dereferences up to 64 bytes of whatever
+	 * field width it is handed.  Bound the dump by what was received.
+	 */
+	dbg_len = (ret > (int)sizeof(*cpkt)) ?
+		  min_t(int, cpkt->len, ret - (int)sizeof(*cpkt)) : 0;
 	dev_dbg(usbio->dev, "control in %d hdr %*phN data %*phN\n", ret,
-		(int)sizeof(*cpkt), cpkt, (int)cpkt->len, cpkt->data);
+		(int)sizeof(*cpkt), cpkt, dbg_len, cpkt->data);
 
 	if (ret < sizeof(*cpkt)) {
 		dev_err(usbio->dev, "USB control in failed: %d\n", ret);
@@ -258,7 +266,7 @@
 	struct usbio_client *client = adev_to_client(adev);
 	struct usbio_device *usbio = client->bridge;
 	struct usbio_bulk_packet *bpkt;
-	int ret, act = 0;
+	int ret, act = 0, dbg_len;
 	u16 bpkt_len;
 
 	lockdep_assert_held(&client->mutex);
@@ -314,8 +322,14 @@
 	act = usbio->rxdat_len;
 	bpkt = usbio->rxbuf;
 	bpkt_len = le16_to_cpu(bpkt->len);
+	/*
+	 * Same as in usbio_ctrl_msg(): bpkt_len is device supplied and is only
+	 * validated below, so bound the dump by what was received.
+	 */
+	dbg_len = (act > (int)sizeof(*bpkt)) ?
+		  min_t(int, bpkt_len, act - (int)sizeof(*bpkt)) : 0;
 	dev_dbg(usbio->dev, "bulk in %d hdr %*phN data %*phN\n", act,
-		(int)sizeof(*bpkt), bpkt, bpkt_len, bpkt->data);
+		(int)sizeof(*bpkt), bpkt, dbg_len, bpkt->data);
 
 	/*
 	 * Unsupported bulk commands get only an usbio_packet_header with
-- 
2.51.0

Re: [PATCH v2 3/3] usb: misc: usbio: bound the debug hex dumps by the received length

[Greg Kroah-Hartman]

On Sun, Jul 26, 2026 at 08:35:09PM +0900, HE WEI (ギカク) wrote:
> usbio_ctrl_msg() and usbio_bulk_msg() hex dump the reply with "%*phN",
> using a length the device supplied and that has not been validated yet:
> 
> 	ret = usb_control_msg(usbio->udev, pipe, 0, request | USB_DIR_IN, 0, 0,
> 			      cpkt, cpkt_len, USBIO_CTRLXFER_TIMEOUT);
> 	dev_dbg(usbio->dev, "control in %d hdr %*phN data %*phN\n", ret,
> 		(int)sizeof(*cpkt), cpkt, (int)cpkt->len, cpkt->data);
> 
> cpkt->len is a u8 read back out of usbio->ctrlbuf after the IN transfer,
> and bpkt_len in usbio_bulk_msg() is le16_to_cpu(bpkt->len) read out of
> usbio->rxbuf.  Both are handed to "%*phN" as the field width.
> hex_string() in lib/vsprintf.c caps that at 64, and dereferences every
> byte up to that cap regardless of how much room the output buffer has:
> 
> 	if (spec.field_width > 0)
> 		len = min(spec.field_width, 64);
> 
> 	for (i = 0; i < len; ++i) {
> 		if (buf < end)
> 			*buf = hex_asc_hi(addr[i]);
> 
> So the dump reads up to byte 67 of ctrlbuf and byte 68 of rxbuf.  Both
> buffers are sized from the endpoint packet sizes, so this is out of
> bounds whenever ep0 wMaxPacketSize is below 68 and whenever the bulk in
> endpoint is below 69.  That covers every low, full and high speed ep0
> (8, 16, 32 or 64) and the bulk sizes the supported bridges actually use
> (64, or 63 for the Synaptics Sabre via USBIO_QUIRK_BULK_MAXP_63).
> 
> A device answering one of the five usbio_ctrl_msg() calls in
> usbio_probe() with cpkt->len = 255 therefore leaks up to 60 bytes of
> adjacent slab memory into the kernel log during enumeration, before any
> user space is involved.  On the bulk path a reply claiming
> bpkt->len = 0xffff reads 5 bytes past a 64 byte rxbuf.
> 
> Unlike the endpoint size issues this needs no malformed descriptor at
> all; it only needs the dev_dbg() calls to be enabled.  When they are, it
> is a slab-out-of-bounds read under KASAN and the bytes reach dmesg.
> 
> Bound both dumps by the number of bytes actually received.  That is in
> bounds by construction and also stops the dump printing stale bytes left
> over from an earlier transfer.  The matching dumps on the two OUT paths
> are left alone: they use lengths the driver itself just wrote, which the
> size checks above them already bound.
> 
> Found by code review.  The out-of-bounds read was reproduced under
> AddressSanitizer with a userspace model of the two dev_dbg() call sites
> and of hex_string()'s field-width handling; it has not been exercised on
> hardware or on dummy_hcd.
> 
> Fixes: 121a0f839dbb ("usb: misc: Add Intel USBIO bridge driver")
> Cc: stable@vger.kernel.org
> Assisted-by: Claude:claude-opus-5 asan
> Signed-off-by: HE WEI (ギカク) <skyexpoc@gmail.com>
> ---
> --- a/drivers/usb/misc/usbio.c
> +++ b/drivers/usb/misc/usbio.c
> @@ -14,6 +14,7 @@
>  #include <linux/dev_printk.h>
>  #include <linux/device.h>
>  #include <linux/lockdep.h>
> +#include <linux/minmax.h>
>  #include <linux/mutex.h>
>  #include <linux/string.h>
>  #include <linux/types.h>
> @@ -141,7 +142,7 @@
>  	struct usbio_ctrl_packet *cpkt;
>  	unsigned int pipe;
>  	u16 cpkt_len;
> -	int ret;
> +	int dbg_len, ret;
>  
>  	lockdep_assert_held(&usbio->ctrl_mutex);
>  
> @@ -181,8 +182,15 @@
>  	cpkt_len = sizeof(*cpkt) + ibuf_len;
>  	ret = usb_control_msg(usbio->udev, pipe, 0, request | USB_DIR_IN, 0, 0,
>  			      cpkt, cpkt_len, USBIO_CTRLXFER_TIMEOUT);
> +	/*
> +	 * cpkt->len has just been written by the device and is not validated
> +	 * until below, while %*phN dereferences up to 64 bytes of whatever
> +	 * field width it is handed.  Bound the dump by what was received.
> +	 */
> +	dbg_len = (ret > (int)sizeof(*cpkt)) ?
> +		  min_t(int, cpkt->len, ret - (int)sizeof(*cpkt)) : 0;

LLMs love to write comments, please think about what it is attempting to
do here and if a comment is even needed.

And that line is crazy, please write code for people first, compilers
second.  Make that readable, because as-is, it's not.

And finally, it's obviously LLM generated as they do not know how to
properly deal with min_t() lines due to it being trained on "old" kernel
code.  That's wrong, please do it properly, you should almost never be
using min_t() anymore.

thanks,

greg k-h

Re: [PATCH v2 3/3] usb: misc: usbio: bound the debug hex dumps by the received length

[HE WEI（ギカク）]

Answering your question on 1/3: the diff was generated by hand with
diff -u, not git format-patch. That is why the hunk headers have no
function names.

Your comments on 3/3 are correct as well, including min_t.

I am not resubmitting this series, so please do not spend more time
reviewing it. Sorry for the wasted effort.

Thanks again.

Greg Kroah-Hartman <gregkh@linuxfoundation.org> 于2026年8月2日周日 17:46写道：
>
> On Sun, Jul 26, 2026 at 08:35:09PM +0900, HE WEI (ギカク) wrote:
> > usbio_ctrl_msg() and usbio_bulk_msg() hex dump the reply with "%*phN",
> > using a length the device supplied and that has not been validated yet:
> >
> >       ret = usb_control_msg(usbio->udev, pipe, 0, request | USB_DIR_IN, 0, 0,
> >                             cpkt, cpkt_len, USBIO_CTRLXFER_TIMEOUT);
> >       dev_dbg(usbio->dev, "control in %d hdr %*phN data %*phN\n", ret,
> >               (int)sizeof(*cpkt), cpkt, (int)cpkt->len, cpkt->data);
> >
> > cpkt->len is a u8 read back out of usbio->ctrlbuf after the IN transfer,
> > and bpkt_len in usbio_bulk_msg() is le16_to_cpu(bpkt->len) read out of
> > usbio->rxbuf.  Both are handed to "%*phN" as the field width.
> > hex_string() in lib/vsprintf.c caps that at 64, and dereferences every
> > byte up to that cap regardless of how much room the output buffer has:
> >
> >       if (spec.field_width > 0)
> >               len = min(spec.field_width, 64);
> >
> >       for (i = 0; i < len; ++i) {
> >               if (buf < end)
> >                       *buf = hex_asc_hi(addr[i]);
> >
> > So the dump reads up to byte 67 of ctrlbuf and byte 68 of rxbuf.  Both
> > buffers are sized from the endpoint packet sizes, so this is out of
> > bounds whenever ep0 wMaxPacketSize is below 68 and whenever the bulk in
> > endpoint is below 69.  That covers every low, full and high speed ep0
> > (8, 16, 32 or 64) and the bulk sizes the supported bridges actually use
> > (64, or 63 for the Synaptics Sabre via USBIO_QUIRK_BULK_MAXP_63).
> >
> > A device answering one of the five usbio_ctrl_msg() calls in
> > usbio_probe() with cpkt->len = 255 therefore leaks up to 60 bytes of
> > adjacent slab memory into the kernel log during enumeration, before any
> > user space is involved.  On the bulk path a reply claiming
> > bpkt->len = 0xffff reads 5 bytes past a 64 byte rxbuf.
> >
> > Unlike the endpoint size issues this needs no malformed descriptor at
> > all; it only needs the dev_dbg() calls to be enabled.  When they are, it
> > is a slab-out-of-bounds read under KASAN and the bytes reach dmesg.
> >
> > Bound both dumps by the number of bytes actually received.  That is in
> > bounds by construction and also stops the dump printing stale bytes left
> > over from an earlier transfer.  The matching dumps on the two OUT paths
> > are left alone: they use lengths the driver itself just wrote, which the
> > size checks above them already bound.
> >
> > Found by code review.  The out-of-bounds read was reproduced under
> > AddressSanitizer with a userspace model of the two dev_dbg() call sites
> > and of hex_string()'s field-width handling; it has not been exercised on
> > hardware or on dummy_hcd.
> >
> > Fixes: 121a0f839dbb ("usb: misc: Add Intel USBIO bridge driver")
> > Cc: stable@vger.kernel.org
> > Assisted-by: Claude:claude-opus-5 asan
> > Signed-off-by: HE WEI (ギカク) <skyexpoc@gmail.com>
> > ---
> > --- a/drivers/usb/misc/usbio.c
> > +++ b/drivers/usb/misc/usbio.c
> > @@ -14,6 +14,7 @@
> >  #include <linux/dev_printk.h>
> >  #include <linux/device.h>
> >  #include <linux/lockdep.h>
> > +#include <linux/minmax.h>
> >  #include <linux/mutex.h>
> >  #include <linux/string.h>
> >  #include <linux/types.h>
> > @@ -141,7 +142,7 @@
> >       struct usbio_ctrl_packet *cpkt;
> >       unsigned int pipe;
> >       u16 cpkt_len;
> > -     int ret;
> > +     int dbg_len, ret;
> >
> >       lockdep_assert_held(&usbio->ctrl_mutex);
> >
> > @@ -181,8 +182,15 @@
> >       cpkt_len = sizeof(*cpkt) + ibuf_len;
> >       ret = usb_control_msg(usbio->udev, pipe, 0, request | USB_DIR_IN, 0, 0,
> >                             cpkt, cpkt_len, USBIO_CTRLXFER_TIMEOUT);
> > +     /*
> > +      * cpkt->len has just been written by the device and is not validated
> > +      * until below, while %*phN dereferences up to 64 bytes of whatever
> > +      * field width it is handed.  Bound the dump by what was received.
> > +      */
> > +     dbg_len = (ret > (int)sizeof(*cpkt)) ?
> > +               min_t(int, cpkt->len, ret - (int)sizeof(*cpkt)) : 0;
>
> LLMs love to write comments, please think about what it is attempting to
> do here and if a comment is even needed.
>
> And that line is crazy, please write code for people first, compilers
> second.  Make that readable, because as-is, it's not.
>
> And finally, it's obviously LLM generated as they do not know how to
> properly deal with min_t() lines due to it being trained on "old" kernel
> code.  That's wrong, please do it properly, you should almost never be
> using min_t() anymore.
>
> thanks,
>
> greg k-h