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

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: [email protected]
> Assisted-by: Claude:claude-opus-5 asan
> Signed-off-by: HE WEI (ギカク) <[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 = 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