Re: [PATCH v2 1/2] HID: usbhid: adjust output pipe FIFO size

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [High] Memory leak of dynamically allocated `raw_report` buffers in the output and control queues upon device unplug and driver unbind.
--

--- Patch [1]: [PATCH v2 1/2] HID: usbhid: adjust output pipe FIFO size ---
commit bf134e3969fdce1c368463a2ad06d7c96d56d312
Author: Dmitry Antipov <[email protected]>

HID: usbhid: adjust output pipe FIFO size

For an USB-specific HID device, size of output FIFO should
be HID_OUTPUT_FIFO_SIZE rather than HID_CONTROL_FIFO_SIZE.

Reported-by: Sashiko <[email protected]>
Closes: https://syzkaller.appspot.com/bug?extid=e2c057ea576d2644e2be
Fixes: 4916b3a57fc9 ("[PATCH] Generic HID layer - USB API")

> diff --git a/drivers/hid/usbhid/usbhid.h b/drivers/hid/usbhid/usbhid.h
> index 75fe85d3d27a0..c408e694a7b27 100644
> --- a/drivers/hid/usbhid/usbhid.h
> +++ b/drivers/hid/usbhid/usbhid.h
> @@ -74,7 +74,7 @@ struct usbhid_device {
>  	unsigned long last_ctrl;						/* record of last output for timeouts */
>  
>  	struct urb *urbout;                                             /* Output URB */
> -	struct hid_output_fifo out[HID_CONTROL_FIFO_SIZE];              /* Output pipe fifo */
> +	struct hid_output_fifo out[HID_OUTPUT_FIFO_SIZE];               /* Output pipe fifo */

[Severity: High]
This isn't a bug introduced by this patch, but while adjusting the output
FIFO size, I noticed a pre-existing memory leak related to how these output
and control queues are cleaned up.

In drivers/hid/usbhid/hid-core.c, when a device is physically unplugged,
the URB completion callbacks receive an error and handle it by clearing
the FIFOs.

For example, in hid_irq_out():

    if (unplug) {
        usbhid->outtail = usbhid->outhead;
    } else {

And similarly in hid_ctrl():

    if (unplug) {
        usbhid->ctrltail = usbhid->ctrlhead;
    } else if (usbhid->ctrlhead != usbhid->ctrltail) {

Does this drop the queued items without calling kfree() on their
dynamically allocated raw_report buffers?

Additionally, during driver unbind, usbhid_stop() handles draining the
control queue and correctly freeing raw_report:

    while (usbhid->ctrltail != usbhid->ctrlhead) {
        if (usbhid->ctrl[usbhid->ctrltail].dir == USB_DIR_OUT) {
            kfree(usbhid->ctrl[usbhid->ctrltail].raw_report);
            usbhid->ctrl[usbhid->ctrltail].raw_report = NULL;
        }
        ...
    }

However, it completely omits an equivalent cleanup loop for the output
queue (usbhid->out).

Could this repeatedly leak memory if userspace or the input subsystem
queues output reports while device unbinds or disconnects occur?

>  	unsigned char outhead, outtail;                                 /* Output pipe fifo head & tail */
>  	char *outbuf;                                                   /* Output buffer */
>  	dma_addr_t outbuf_dma;                                          /* Output buffer dma */

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1