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

[Greg Kroah-Hartman <gregkh@linuxfoundation.org>]

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