Re: [PATCH] usb: gadget: fix f_printer ep0 overflow/list race and f_hid/f_tcm/f_eem bounds

[ivy lopez <skunkolee@gmail.com>]

On Fri, Sep 19, 2026 at 10:35 PM UTC, Hui Peng wrote:
> Fix multiple memory corruption bugs in USB gadget function drivers:
>
> 1. In printer_func_setup() and printer_reset_interface()
>    (drivers/usb/gadget/function/f_printer.c), bound GET_DEVICE_ID copies
>    to USB_COMP_EP0_BUFSIZ (1024 bytes) under lock, and dequeue from
>    dev->rx_reqs_active instead of dev->rx_buffers in
>    printer_reset_interface().
> 2. In drivers/usb/gadget/function/f_hid.c, f_tcm.c, and f_eem.c,
>    validate setup wLength, command lengths, and skb bounds.
>
> Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
> Assisted-by: LLM
> Signed-off-by: Hui Peng <benquike@gmail.com>

This touches four unrelated drivers (f_printer, f_hid, f_tcm, f_eem) 
under one Fixes tag, and 1da177e4c3f4 isn't a real Fixes tag for any
of it, it's what you get when nobody runs git blame per hunk.
Please split this into one patch per driver, each with its own
actual introducing commit.

> -		while (likely(!(list_empty(&dev->rx_reqs_active)))) {
> -			req = container_of(dev->rx_buffers.next, struct usb_request,
> +			req = container_of(dev->rx_reqs_active.next, struct usb_request,

This is genuinely bad. After the loop above it drains rx_buffers,
rx_buffers.next points back to rx_buffers itself, so this second loop
computes a fake usb_request via container_of on a list_head embedded
in printer_dev, then writes through it via list_del_init/list_add.
It also never drains rx_reqs_active since it's dequeuing from the
wrong list, so this spins corrupting memory each iteration.

git blame puts the actual introducing commit at b185f01a9ab7a ("usb: gadget:
Restructure printer gadget", 2015-03-03), not 1da177e4c3f4. Please
use that as the Fixes tag when you split this out.

> -			value = strlen(*dev->pnp_string);
> -			buf[0] = (value >> 8) & 0xFF;
> -			buf[1] = value & 0xFF;
> +			value = min_t(size_t, strlen(*dev->pnp_string),
> +				      USB_COMP_EP0_BUFSIZ - 2);
> +			buf[0] = ((value + 2) >> 8) & 0xFF;
> +			buf[1] = (value + 2) & 0xFF;

Nothing bounds strlen(*dev->pnp_string) against buf's
capacity before the memcpy below it, and pnp_string is configfs
settable with no length cap in f_printer_opts_pnp_string_store either,
so this is a genuine overflow path.

Separately from the overflow, the length prefix per IEEE 1284.3 is supposed to
include the two length bytes themselves, which the original strlen()
value doesn't. Probably worth its own patch too.

Separately, in f_hid.c:

> +	if (!hidg->func.config || !hidg->func.config->cdev)
> +		return -ENODEV;
>
>  	if (hidg->use_out_ep)
>  		return f_hidg_intout_read(file, buffer, count, ptr);

This hunk (and the matching ones in f_hidg_write() and
f_hidg_get_report()) doesn't close the race it's aimed at.
hidg_unbind() does:

> +	usb_free_all_descriptors(f);
> +	hidg->func.config = NULL;

with no lock shared with the checks above, so this is an
unsynchronized check followed by an unsynchronized use, racing an
unsynchronized write. It shrinks the window, it doesn't close it. If
this is worth fixing, it needs whatever synchronization already
coordinates unbind against the fops paths elsewhere in the driver, not
a bare pointer check with no lock behind it.

And:

> 	if (ptr) {
> 		/* Report already exists in list - update it */
> -		if (copy_from_user(&ptr->report_data, buffer,
> -				sizeof(struct usb_hidg_report))) {
> -			spin_unlock_irqrestore(&hidg->get_report_spinlock, flags);
> -			ERROR(cdev, "copy_from_user error\n");
> -			kfree(entry);
> -			return -EINVAL;
> -		}
> +		ptr->report_data = entry->report_data;

This one looks good to me and worth keeping. The existing code does
two copy_from_user() calls against the same user buffer for no reason
(once into entry->report_data unconditionally at the top of the
function, again into ptr->report_data if an entry already existed),
and this removes the redundant, racy second read. That one's real,
keep it.

Also please skip the func.config hunk unless you're going to actually
synchronize it against unbind.

ivy