* [PATCH v2 0/3] usbio: fix two out-of-bounds accesses and a hang
@ 2026-07-26 11:35 HE WEI (ギカク)
2026-07-26 11:35 ` [PATCH v2 1/3] usb: misc: usbio: reject endpoints smaller than the packet header HE WEI (ギカク)
` (2 more replies)
0 siblings, 3 replies; 5+ messages in thread
From: HE WEI (ギカク) @ 2026-07-26 11:35 UTC (permalink / raw)
To: Hans de Goede, Greg Kroah-Hartman, Andi Shyti
Cc: Sakari Ailus, linux-usb, linux-i2c, linux-kernel, 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. It is also the
only patch here that changes behaviour. A bridge whose bulk
wMaxPacketSize is below 11 no longer gets an I2C adapter. The commit
message explains why such an adapter could never have completed a
transfer anyway.
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.
Documentation/process/generated-content.rst asks where the content came
from. I used Claude (claude-opus-5). I gave it this driver,
8c6314489550, and asked for variant analysis of the code around it. It
found the three issues and wrote the patches, the changelogs, this cover
letter, the userspace model used for the AddressSanitizer runs, and the
sweep program described below. It also ran the mechanical checks and
re-read its own code quotes back against the tree.
I ran the builds, read the result, decided what was worth sending, and
sent it. The Signed-off-by is mine. I am answerable for the content
and will answer review comments on it.
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.
Changes in v2:
- Add the Assisted-by tag and the provenance note above, per
Documentation/process/coding-assistants.rst and
generated-content.rst. No code changes: the
three patches are byte-identical to v1.
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
^ permalink raw reply [flat|nested] 5+ messages in thread
* [PATCH v2 1/3] usb: misc: usbio: reject endpoints smaller than the packet header
2026-07-26 11:35 [PATCH v2 0/3] usbio: fix two out-of-bounds accesses and a hang HE WEI (ギカク)
@ 2026-07-26 11:35 ` HE WEI (ギカク)
2026-07-26 11:41 ` Greg Kroah-Hartman
2026-07-26 11:35 ` [PATCH v2 2/3] i2c: usbio: reject bridges with undersized transfer buffers HE WEI (ギカク)
2026-07-26 11:35 ` [PATCH v2 3/3] usb: misc: usbio: bound the debug hex dumps by the received length HE WEI (ギカク)
2 siblings, 1 reply; 5+ messages in thread
From: HE WEI (ギカク) @ 2026-07-26 11:35 UTC (permalink / raw)
To: Hans de Goede, Greg Kroah-Hartman, Andi Shyti
Cc: Sakari Ailus, linux-usb, linux-i2c, linux-kernel, HE WEI, stable
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
^ permalink raw reply [flat|nested] 5+ messages in thread
* [PATCH v2 2/3] i2c: usbio: reject bridges with undersized transfer buffers
2026-07-26 11:35 [PATCH v2 0/3] usbio: fix two out-of-bounds accesses and a hang HE WEI (ギカク)
2026-07-26 11:35 ` [PATCH v2 1/3] usb: misc: usbio: reject endpoints smaller than the packet header HE WEI (ギカク)
@ 2026-07-26 11:35 ` HE WEI (ギカク)
2026-07-26 11:35 ` [PATCH v2 3/3] usb: misc: usbio: bound the debug hex dumps by the received length HE WEI (ギカク)
2 siblings, 0 replies; 5+ messages in thread
From: HE WEI (ギカク) @ 2026-07-26 11:35 UTC (permalink / raw)
To: Hans de Goede, Greg Kroah-Hartman, Andi Shyti
Cc: Sakari Ailus, linux-usb, linux-i2c, linux-kernel, HE WEI, stable
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
^ permalink raw reply [flat|nested] 5+ messages in thread
* [PATCH v2 3/3] usb: misc: usbio: bound the debug hex dumps by the received length
2026-07-26 11:35 [PATCH v2 0/3] usbio: fix two out-of-bounds accesses and a hang HE WEI (ギカク)
2026-07-26 11:35 ` [PATCH v2 1/3] usb: misc: usbio: reject endpoints smaller than the packet header HE WEI (ギカク)
2026-07-26 11:35 ` [PATCH v2 2/3] i2c: usbio: reject bridges with undersized transfer buffers HE WEI (ギカク)
@ 2026-07-26 11:35 ` HE WEI (ギカク)
2 siblings, 0 replies; 5+ messages in thread
From: HE WEI (ギカク) @ 2026-07-26 11:35 UTC (permalink / raw)
To: Hans de Goede, Greg Kroah-Hartman, Andi Shyti
Cc: Sakari Ailus, linux-usb, linux-i2c, linux-kernel, HE WEI, stable
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
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH v2 1/3] usb: misc: usbio: reject endpoints smaller than the packet header
2026-07-26 11:35 ` [PATCH v2 1/3] usb: misc: usbio: reject endpoints smaller than the packet header HE WEI (ギカク)
@ 2026-07-26 11:41 ` Greg Kroah-Hartman
0 siblings, 0 replies; 5+ messages in thread
From: Greg Kroah-Hartman @ 2026-07-26 11:41 UTC (permalink / raw)
To: HE WEI (ギカク)
Cc: Hans de Goede, Andi Shyti, Sakari Ailus, linux-usb, linux-i2c,
linux-kernel, stable
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
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2026-07-26 11:42 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2026-07-26 11:35 [PATCH v2 0/3] usbio: fix two out-of-bounds accesses and a hang HE WEI (ギカク)
2026-07-26 11:35 ` [PATCH v2 1/3] usb: misc: usbio: reject endpoints smaller than the packet header HE WEI (ギカク)
2026-07-26 11:41 ` Greg Kroah-Hartman
2026-07-26 11:35 ` [PATCH v2 2/3] i2c: usbio: reject bridges with undersized transfer buffers HE WEI (ギカク)
2026-07-26 11:35 ` [PATCH v2 3/3] usb: misc: usbio: bound the debug hex dumps by the received length HE WEI (ギカク)
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.