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

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 <[email protected]> =E4=BA=8E2026=E5=B9=B48=E6=
=9C=882=E6=97=A5=E5=91=A8=E6=97=A5 17:46=E5=86=99=E9=81=93=EF=BC=9A
>
> On Sun, Jul 26, 2026 at 08:35:09PM +0900, HE WEI (=E3=82=AE=E3=82=AB=E3=
=82=AF) 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 =3D usb_control_msg(usbio->udev, pipe, 0, request | USB_DIR_I=
N, 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 =3D min(spec.field_width, 64);
> >
> >       for (i =3D 0; i < len; ++i) {
> >               if (buf < end)
> >                       *buf =3D 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 =3D 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 =3D 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, i=
t
> > 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 lef=
t
> > 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 th=
e
> > 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 o=
n
> > hardware or on dummy_hcd.
> >
> > Fixes: 121a0f839dbb ("usb: misc: Add Intel USBIO bridge driver")
> > Cc: [email protected]
> > Assisted-by: Claude:claude-opus-5 asan
> > Signed-off-by: HE WEI (=E3=82=AE=E3=82=AB=E3=82=AF) <[email protected]=
>
> > ---
> > --- 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 =3D sizeof(*cpkt) + ibuf_len;
> >       ret =3D usb_control_msg(usbio->udev, pipe, 0, request | USB_DIR_I=
N, 0, 0,
> >                             cpkt, cpkt_len, USBIO_CTRLXFER_TIMEOUT);
> > +     /*
> > +      * cpkt->len has just been written by the device and is not valid=
ated
> > +      * until below, while %*phN dereferences up to 64 bytes of whatev=
er
> > +      * field width it is handed.  Bound the dump by what was received=
.
> > +      */
> > +     dbg_len =3D (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