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

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

[Dmitry Antipov]

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 <sashiko-bot@kernel.org>
Closes: https://syzkaller.appspot.com/bug?extid=e2c057ea576d2644e2be
Fixes: 4916b3a57fc9 ("[PATCH] Generic HID layer - USB API")
Signed-off-by: Dmitry Antipov <dmantipov@yandex.ru>
---
v2: initial version to join the series
---
 drivers/hid/usbhid/usbhid.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/hid/usbhid/usbhid.h b/drivers/hid/usbhid/usbhid.h
index 75fe85d3d27a..c408e694a7b2 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 */
 	unsigned char outhead, outtail;                                 /* Output pipe fifo head & tail */
 	char *outbuf;                                                   /* Output buffer */
 	dma_addr_t outbuf_dma;                                          /* Output buffer dma */
-- 
2.55.0

[PATCH v2 2/2] HID: usbhid: free unsent raw output reports in usbhid_stop()

[Dmitry Antipov]

When HID device is stalled or disconnected, there might be an
unsent (i.e. never passed via 'hid_submit_out()') output reports.
So free their raw buffers in 'usbhid_stop()' to avoid memory leaks.

This is a companion of commit f7744fa16b96 ("HID: usbhid: free
raw_report buffers in usbhid_stop") and inspired by that one.

Reported-by: syzbot+e2c057ea576d2644e2be@syzkaller.appspotmail.com
Closes: https://syzkaller.appspot.com/bug?extid=e2c057ea576d2644e2be
Fixes: c500c9714011 ("HID: hid, make parsing event driven")
Assisted-by: Sashiko:gemini/gemini-3.1-pro-preview
Signed-off-by: Dmitry Antipov <dmantipov@yandex.ru>
---
v2: adjust loop to match actual FIFO size and tweak commit message
---
 drivers/hid/usbhid/hid-core.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/drivers/hid/usbhid/hid-core.c b/drivers/hid/usbhid/hid-core.c
index 96b0181cf819..60662616fd2a 100644
--- a/drivers/hid/usbhid/hid-core.c
+++ b/drivers/hid/usbhid/hid-core.c
@@ -1253,6 +1253,14 @@ static void usbhid_stop(struct hid_device *hid)
 		usbhid->ctrltail = (usbhid->ctrltail + 1) &
 			(HID_CONTROL_FIFO_SIZE - 1);
 	}
+
+	while (usbhid->outtail != usbhid->outhead) {
+		kfree(usbhid->out[usbhid->outtail].raw_report);
+		usbhid->out[usbhid->outtail].raw_report = NULL;
+
+		usbhid->outtail = (usbhid->outtail + 1) &
+			(HID_OUTPUT_FIFO_SIZE - 1);
+	}
 	spin_unlock_irq(&usbhid->lock);
 
 	usb_kill_urb(usbhid->urbin);
-- 
2.55.0

Re: [PATCH v2 2/2] HID: usbhid: free unsent raw output reports in usbhid_stop()

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Emptying the `out` queue in `usbhid_stop` without updating the corresponding `hid_irq_out` completion handler causes an out-of-bounds queue access, leading to a NULL pointer dereference and kernel panic.
--

--- Patch [2]: [PATCH v2 2/2] HID: usbhid: free unsent raw output reports in usbhid_stop() ---
commit 026e2d1c35a0ae9588e9e7e4b956512ff4689607
Author: Dmitry Antipov <dmantipov@yandex.ru>

HID: usbhid: free unsent raw output reports in usbhid_stop()

When a HID device is stalled or disconnected, there might be unsent
output reports. This patch frees their raw buffers in usbhid_stop()
to avoid memory leaks.

Closes: https://syzkaller.appspot.com/bug?extid=e2c057ea576d2644e2be

> diff --git a/drivers/hid/usbhid/hid-core.c b/drivers/hid/usbhid/hid-core.c
> index 96b0181cf8194..60662616fd2a0 100644
> --- a/drivers/hid/usbhid/hid-core.c
> +++ b/drivers/hid/usbhid/hid-core.c
> @@ -1253,6 +1253,14 @@ static void usbhid_stop(struct hid_device *hid)
>  		usbhid->ctrltail = (usbhid->ctrltail + 1) &
>  			(HID_CONTROL_FIFO_SIZE - 1);
>  	}
> +
> +	while (usbhid->outtail != usbhid->outhead) {
> +		kfree(usbhid->out[usbhid->outtail].raw_report);
> +		usbhid->out[usbhid->outtail].raw_report = NULL;
> +
> +		usbhid->outtail = (usbhid->outtail + 1) &
> +			(HID_OUTPUT_FIFO_SIZE - 1);
> +	}

[Severity: High]
Could emptying the out queue here cause a NULL pointer dereference if an
output URB is currently in-flight?

If an in-flight URB is canceled by the subsequent usb_kill_urb() call, it
will synchronously trigger the completion handler hid_irq_out() with status
-ENOENT.

Unlike hid_ctrl(), hid_irq_out() unconditionally increments outtail:

drivers/hid/usbhid/hid-core.c:hid_irq_out() {
    ...
    } else {
        usbhid->outtail = (usbhid->outtail + 1) & (HID_OUTPUT_FIFO_SIZE - 1);

        if (usbhid->outhead != usbhid->outtail &&
                hid_submit_out(hid) == 0) {
    ...
}

If usbhid_stop() has already made outtail equal to outhead, this increment
pushes outtail out of bounds. The outhead != outtail check will then pass,
and it will call hid_submit_out() which will dereference a NULL report:

drivers/hid/usbhid/hid-core.c:hid_submit_out() {
    ...
    report = usbhid->out[usbhid->outtail].report;
    usbhid->urbout->transfer_buffer_length = hid_report_len(report);
    ...
}

>  	spin_unlock_irq(&usbhid->lock);
>  
>  	usb_kill_urb(usbhid->urbin);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260819111203.2883239-1-dmantipov@yandex.ru?part=2

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

[sashiko-bot]

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 <dmantipov@yandex.ru>

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 <sashiko-bot@kernel.org>
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/20260819111203.2883239-1-dmantipov@yandex.ru?part=1