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@xxxxxxxxxxxxxxxxxxx> 于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@xxxxxxxxxxxxxxx
> > Assisted-by: Claude:claude-opus-5 asan
> > Signed-off-by: HE WEI (ギカク) <skyexpoc@xxxxxxxxx>
> > ---
> > --- 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