[PATCH 0/3] usbio: fix two out-of-bounds accesses and a hang

[PATCH 0/3] usbio: fix two out-of-bounds accesses and a hang

[HE WEI (ギカク)]

This is variant analysis around 8c6314489550 ("usb: misc: usbio: bound
bulk IN response length to the received transfer"), which fixed a slab
out-of-bounds read in usbio_bulk_msg().  Re-reading that function,
usbio_ctrl_msg() and the I2C client built on top of them turned up three
further issues.

Patch 1 fixes the length checks in usbio_ctrl_msg() and
usbio_bulk_msg().  They are computed as "<buf>_len - sizeof(*pkt)", u16
minus size_t, which wraps for any endpoint smaller than the protocol
header.  usb_parse_endpoint() only clamps wMaxPacketSize downwards, so
a device can advertise 1 and turn the check off entirely; the first I2C
transfer then overflows a one byte slab object.  The overflow completes
before usb_bulk_msg() is reached, so it does not depend on the host
controller accepting the transfer.  Fixed by validating the three
lengths once in usbio_probe(), which is the only place that assigns
them.

Patch 2 is the equivalent arithmetic in i2c-usbio.c, where the overhead
is 10 rather than 5.  The interesting value there is exactly 10: the
chunk size is then 0 and the split loop in usbio_i2c_read() never
advances, so a device that keeps answering keeps the loop alive
uninterruptibly while holding usbio->bulk_mutex.  That is a hang rather
than memory corruption, and patch 1 does not address it.  This is also
the one patch in the series with a behaviour change -- a bridge whose
bulk wMaxPacketSize is below 11 no longer gets an I2C adapter -- and the
commit message spells out why such an adapter could never have
completed a transfer in the first place.

Patch 3 is a different bug from the two above, and the one that affects
existing hardware.  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.  hex_string() caps the field width at 64, not at
the buffer size, so the dump reads up to byte 67 of ctrlbuf and byte 68
of rxbuf: out of bounds for every low, full and high speed ep0 packet
size and for the bulk sizes these bridges actually use.  It needs no
malformed descriptor at all, only the dev_dbg() calls enabled.

The new probe checks do not reject any supported bridge: 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.

What was verified, and what was not:

 - All three build cleanly with x86_64 gcc 14.2.0 at W=1, with
   CONFIG_DYNAMIC_DEBUG both disabled and enabled.

 - The out-of-bounds accesses in patches 1 and 3, and the
   non-terminating loop in patch 2, reproduce against a userspace model
   that copies the struct definitions from usbio.h and the checks and
   stores from usbio.c and i2c-usbio.c verbatim, with only
   devm_kzalloc(), usb_bulk_msg() and usb_control_msg() replaced; the
   memory errors are reported by AddressSanitizer.  The same source
   built with the patches applied is clean, and both a normal device
   (ep0 64, bulk 64/64) and the USBIO_QUIRK_BULK_MAXP_63 path still
   work.

 - The decision space was swept exhaustively, endpoint sizes 0..2048
   against caller lengths 0..4101: the mainline bulk check can be
   bypassed for exactly 0..4 and the control check for exactly 0..3.
   After patch 1 no accepted endpoint size admits an overflow, and no
   size that could carry a payload byte is rejected.

 - I have not run any of this on hardware or on dummy_hcd.  The
   reachability arguments are read from usbcore and from
   lib/vsprintf.c, not observed at runtime.  I am happy to build a
   raw-gadget reproducer if that would help review.

Patches 1 and 3 touch drivers/usb/misc/usbio.c and patch 2 touches
drivers/i2c/busses/i2c-usbio.c; MAINTAINERS lists all three files under
the same INTEL USBIO entry, so they are sent as one series.  They apply
to mainline 3dab139d4795, to usb-next and to usb-testing.

HE WEI (ギカク) (3):
  usb: misc: usbio: reject endpoints smaller than the packet header
  i2c: usbio: reject bridges with undersized transfer buffers
  usb: misc: usbio: bound the debug hex dumps by the received length

 drivers/i2c/busses/i2c-usbio.c | 14 ++++++++++++
 drivers/usb/misc/usbio.c       | 50 ++++++++++++++++++++++++++++++++++++++----
 2 files changed, 60 insertions(+), 4 deletions(-)

-- 
2.51.0

[PATCH 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
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

[PATCH 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
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 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
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 0/3] usbio: fix two out-of-bounds accesses and a hang

[Greg Kroah-Hartman]

On Sun, Jul 26, 2026 at 04:59:10PM +0900, HE WEI (ギカク) wrote:
> This is variant analysis around 8c6314489550 ("usb: misc: usbio: bound
> bulk IN response length to the received transfer"), which fixed a slab
> out-of-bounds read in usbio_bulk_msg().  Re-reading that function,
> usbio_ctrl_msg() and the I2C client built on top of them turned up three
> further issues.
> 
> Patch 1 fixes the length checks in usbio_ctrl_msg() and
> usbio_bulk_msg().  They are computed as "<buf>_len - sizeof(*pkt)", u16
> minus size_t, which wraps for any endpoint smaller than the protocol
> header.  usb_parse_endpoint() only clamps wMaxPacketSize downwards, so
> a device can advertise 1 and turn the check off entirely; the first I2C
> transfer then overflows a one byte slab object.  The overflow completes
> before usb_bulk_msg() is reached, so it does not depend on the host
> controller accepting the transfer.  Fixed by validating the three
> lengths once in usbio_probe(), which is the only place that assigns
> them.
> 
> Patch 2 is the equivalent arithmetic in i2c-usbio.c, where the overhead
> is 10 rather than 5.  The interesting value there is exactly 10: the
> chunk size is then 0 and the split loop in usbio_i2c_read() never
> advances, so a device that keeps answering keeps the loop alive
> uninterruptibly while holding usbio->bulk_mutex.  That is a hang rather
> than memory corruption, and patch 1 does not address it.  This is also
> the one patch in the series with a behaviour change -- a bridge whose
> bulk wMaxPacketSize is below 11 no longer gets an I2C adapter -- and the
> commit message spells out why such an adapter could never have
> completed a transfer in the first place.
> 
> Patch 3 is a different bug from the two above, and the one that affects
> existing hardware.  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.  hex_string() caps the field width at 64, not at
> the buffer size, so the dump reads up to byte 67 of ctrlbuf and byte 68
> of rxbuf: out of bounds for every low, full and high speed ep0 packet
> size and for the bulk sizes these bridges actually use.  It needs no
> malformed descriptor at all, only the dev_dbg() calls enabled.
> 
> The new probe checks do not reject any supported bridge: 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.
> 
> What was verified, and what was not:
> 
>  - All three build cleanly with x86_64 gcc 14.2.0 at W=1, with
>    CONFIG_DYNAMIC_DEBUG both disabled and enabled.
> 
>  - The out-of-bounds accesses in patches 1 and 3, and the
>    non-terminating loop in patch 2, reproduce against a userspace model
>    that copies the struct definitions from usbio.h and the checks and
>    stores from usbio.c and i2c-usbio.c verbatim, with only
>    devm_kzalloc(), usb_bulk_msg() and usb_control_msg() replaced; the
>    memory errors are reported by AddressSanitizer.  The same source
>    built with the patches applied is clean, and both a normal device
>    (ep0 64, bulk 64/64) and the USBIO_QUIRK_BULK_MAXP_63 path still
>    work.
> 
>  - The decision space was swept exhaustively, endpoint sizes 0..2048
>    against caller lengths 0..4101: the mainline bulk check can be
>    bypassed for exactly 0..4 and the control check for exactly 0..3.
>    After patch 1 no accepted endpoint size admits an overflow, and no
>    size that could carry a payload byte is rejected.
> 
>  - I have not run any of this on hardware or on dummy_hcd.  The
>    reachability arguments are read from usbcore and from
>    lib/vsprintf.c, not observed at runtime.  I am happy to build a
>    raw-gadget reproducer if that would help review.
> 
> Patches 1 and 3 touch drivers/usb/misc/usbio.c and patch 2 touches
> drivers/i2c/busses/i2c-usbio.c; MAINTAINERS lists all three files under
> the same INTEL USBIO entry, so they are sent as one series.  They apply
> to mainline 3dab139d4795, to usb-next and to usb-testing.
> 
> HE WEI (ギカク) (3):
>   usb: misc: usbio: reject endpoints smaller than the packet header
>   i2c: usbio: reject bridges with undersized transfer buffers
>   usb: misc: usbio: bound the debug hex dumps by the received length
> 
>  drivers/i2c/busses/i2c-usbio.c | 14 ++++++++++++
>  drivers/usb/misc/usbio.c       | 50 ++++++++++++++++++++++++++++++++++++++----
>  2 files changed, 60 insertions(+), 4 deletions(-)

As you obviously used a LLM for these, why did it not properly tag the
changes with an "Assisted-by:" tag?  That is required.

thanks,

greg k-h

Re: [PATCH 0/3] usbio: fix two out-of-bounds accesses and a hang

[skyexpoc]

[-- Attachment #1.1: Type: text/plain, Size: 5745 bytes --]

I’m sorry. I should have included an attribution.
That was my oversight.

I did use an LLM this time to assist with the audit and translation. It performed variant analysis on the code around 8c6314489550, identified these three issues, and wrote the patches and changelog. I ran the build, reviewed the results, decided what was worth submitting, and sent everything out. The Signed-off-by line is mine, and I take responsibility for this work.

I have now read ⁠coding-assistants.rst and ⁠generated-content.rst. The following has been added to all three patches in v2:

Assisted-by: Claude:claude-opus-5 asan

The cover letter also explains what the tool did and what I did. There are no code changes compared with v1.

Thank you,

HE WEI

> On Sunday, Jul 26, 2026 at 8:11 PM, Greg Kroah-Hartman <gregkh@linuxfoundation.org (mailto:gregkh@linuxfoundation.org)> wrote:
> On Sun, Jul 26, 2026 at 04:59:10PM +0900, HE WEI (ギカク) wrote:
> > This is variant analysis around 8c6314489550 ("usb: misc: usbio: bound
> > bulk IN response length to the received transfer"), which fixed a slab
> > out-of-bounds read in usbio_bulk_msg(). Re-reading that function,
> > usbio_ctrl_msg() and the I2C client built on top of them turned up three
> > further issues.
> >
> > Patch 1 fixes the length checks in usbio_ctrl_msg() and
> > usbio_bulk_msg(). They are computed as "<buf>_len - sizeof(*pkt)", u16
> > minus size_t, which wraps for any endpoint smaller than the protocol
> > header. usb_parse_endpoint() only clamps wMaxPacketSize downwards, so
> > a device can advertise 1 and turn the check off entirely; the first I2C
> > transfer then overflows a one byte slab object. The overflow completes
> > before usb_bulk_msg() is reached, so it does not depend on the host
> > controller accepting the transfer. Fixed by validating the three
> > lengths once in usbio_probe(), which is the only place that assigns
> > them.
> >
> > Patch 2 is the equivalent arithmetic in i2c-usbio.c, where the overhead
> > is 10 rather than 5. The interesting value there is exactly 10: the
> > chunk size is then 0 and the split loop in usbio_i2c_read() never
> > advances, so a device that keeps answering keeps the loop alive
> > uninterruptibly while holding usbio->bulk_mutex. That is a hang rather
> > than memory corruption, and patch 1 does not address it. This is also
> > the one patch in the series with a behaviour change -- a bridge whose
> > bulk wMaxPacketSize is below 11 no longer gets an I2C adapter -- and the
> > commit message spells out why such an adapter could never have
> > completed a transfer in the first place.
> >
> > Patch 3 is a different bug from the two above, and the one that affects
> > existing hardware. 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. hex_string() caps the field width at 64, not at
> > the buffer size, so the dump reads up to byte 67 of ctrlbuf and byte 68
> > of rxbuf: out of bounds for every low, full and high speed ep0 packet
> > size and for the bulk sizes these bridges actually use. It needs no
> > malformed descriptor at all, only the dev_dbg() calls enabled.
> >
> > The new probe checks do not reject any supported bridge: 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.
> >
> > What was verified, and what was not:
> >
> > - All three build cleanly with x86_64 gcc 14.2.0 at W=1, with
> > CONFIG_DYNAMIC_DEBUG both disabled and enabled.
> >
> > - The out-of-bounds accesses in patches 1 and 3, and the
> > non-terminating loop in patch 2, reproduce against a userspace model
> > that copies the struct definitions from usbio.h and the checks and
> > stores from usbio.c and i2c-usbio.c verbatim, with only
> > devm_kzalloc(), usb_bulk_msg() and usb_control_msg() replaced; the
> > memory errors are reported by AddressSanitizer. The same source
> > built with the patches applied is clean, and both a normal device
> > (ep0 64, bulk 64/64) and the USBIO_QUIRK_BULK_MAXP_63 path still
> > work.
> >
> > - The decision space was swept exhaustively, endpoint sizes 0..2048
> > against caller lengths 0..4101: the mainline bulk check can be
> > bypassed for exactly 0..4 and the control check for exactly 0..3.
> > After patch 1 no accepted endpoint size admits an overflow, and no
> > size that could carry a payload byte is rejected.
> >
> > - I have not run any of this on hardware or on dummy_hcd. The
> > reachability arguments are read from usbcore and from
> > lib/vsprintf.c, not observed at runtime. I am happy to build a
> > raw-gadget reproducer if that would help review.
> >
> > Patches 1 and 3 touch drivers/usb/misc/usbio.c and patch 2 touches
> > drivers/i2c/busses/i2c-usbio.c; MAINTAINERS lists all three files under
> > the same INTEL USBIO entry, so they are sent as one series. They apply
> > to mainline 3dab139d4795, to usb-next and to usb-testing.
> >
> > HE WEI (ギカク) (3):
> > usb: misc: usbio: reject endpoints smaller than the packet header
> > i2c: usbio: reject bridges with undersized transfer buffers
> > usb: misc: usbio: bound the debug hex dumps by the received length
> >
> > drivers/i2c/busses/i2c-usbio.c | 14 ++++++++++++
> > drivers/usb/misc/usbio.c | 50 ++++++++++++++++++++++++++++++++++++++----
> > 2 files changed, 60 insertions(+), 4 deletions(-)
>
> As you obviously used a LLM for these, why did it not properly tag the
> changes with an "Assisted-by:" tag? That is required.
>
> thanks,
>
> greg k-h

[-- Attachment #1.2: Type: text/html, Size: 6593 bytes --]

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 885 bytes --]

Re: [PATCH 0/3] usbio: fix two out-of-bounds accesses and a hang

[Greg Kroah-Hartman]

On Sun, Jul 26, 2026 at 08:32:25PM +0900, skyexpoc wrote:
> I’m sorry. I should have included an attribution.
> That was my oversight.
> 
> I did use an LLM this time to assist with the audit and translation. It performed variant analysis on the code around 8c6314489550, identified these three issues, and wrote the patches and changelog. I ran the build, reviewed the results, decided what was worth submitting, and sent everything out. The Signed-off-by line is mine, and I take responsibility for this work.

Yes, but the LLM did _all_ the work here, that's not good, you can't
rely on it being correct (hint, you also sent an html email here which
the mailing list rejected...)

> I have now read ⁠coding-assistants.rst and ⁠generated-content.rst. The following has been added to all three patches in v2:
> 
> Assisted-by: Claude:claude-opus-5 asan
> 
> The cover letter also explains what the tool did and what I did. There are no code changes compared with v1.

And the LLM got this all wrong, again, please don't assume that what
they spit out is correct.  I see on average, only 2/3 of the patches
they create are proper.  Given my initial review of your first patch,
those odds are still in play here...

thanks,

greg k-h

Re: [PATCH 0/3] usbio: fix two out-of-bounds accesses and a hang

[HE WEI（ギカク）]

Thank you for your suggestion. I apologize for my oversight, which
added to your workload.

The html was my mail client, I have switched back to plain text.

I won't resubmit all three patches.
I'll submit only the one for the debug hex dump,
which I'll organize and document myself, and generate using git format-patch.

Thanks again,
HE WEI


Greg Kroah-Hartman <gregkh@linuxfoundation.org> Written on Sunday,
July 26, 2026 at 03:44.：
>
> On Sun, Jul 26, 2026 at 03:32:25PM +0900, skyexpoc wrote:
> > I’m sorry. I should have included an attribution.
> > That was my oversight.
> >
> > I did use an LLM this time to assist with the audit and translation. It performed variant analysis on the code around 8c6314489550, identified these three issues, and wrote the patches and changelog. I ran the build, reviewed the results, decided what was worth submitting, and sent everything out. The Signed-off-by line is mine, and I take responsibility for this work.
>
> Yes, but the LLM did _all_ the work here, that's not good, you can't
> rely on it being correct (hint, you also sent an html email here which
> the mailing list rejected...)
>
> > I have now read ⁠coding-assistants.rst and ⁠generated-content.rst. The following has been added to all three patches in v2:
> >
> > Assisted-by: Claude:claude-opus-5 asan
> >
> > The cover letter also explains what the tool did and what I did. There are no code changes compared with v1.
>
> And the LLM got this all wrong, again, please don't assume that what
> they spit out is correct.  I see on average, only 2/3 of the patches
> they create are proper.  Given my initial review of your first patch,
> those odds are still in play here...
>
> thanks,
>
> greg k-h

Re: [PATCH 0/3] usbio: fix two out-of-bounds accesses and a hang

[Greg Kroah-Hartman]

On Sun, Jul 26, 2026 at 08:59:58PM +0900, HE WEI（ギカク） wrote:
> Thank you for your suggestion. I apologize for my oversight, which
> added to your workload.
> 
> The html was my mail client, I have switched back to plain text.
> 
> I won't resubmit all three patches.
> I'll submit only the one for the debug hex dump,
> which I'll organize and document myself, and generate using git format-patch.

Why not fix up the first two to be sane instead?

thanks,

greg k-h