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

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

[Dmitry Antipov]

When HID device is stalled or unexpectedly removed, 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.

Reported-by: syzbot+e2c057ea576d2644e2be@syzkaller.appspotmail.com
Closes: https://syzkaller.appspot.com/bug?extid=e2c057ea576d2644e2be
Signed-off-by: Dmitry Antipov <dmantipov@yandex.ru>
---
This is an (obvious?) companion patch for f7744fa16b9 ("HID: usbhid:
free raw_report buffers in usbhid_stop").
---
 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..599a7566b825 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_CONTROL_FIFO_SIZE - 1);
+	}
 	spin_unlock_irq(&usbhid->lock);
 
 	usb_kill_urb(usbhid->urbin);
-- 
2.55.0

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

[sashiko-bot]

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

New issues:
- [Critical] Advancing `outtail` to `outhead` in `usbhid_stop()` before killing the active URB causes a queue underflow in `hid_irq_out()`, leading to a NULL pointer dereference.
- [Low] The output queue cleanup loop incorrectly uses `HID_CONTROL_FIFO_SIZE` instead of `HID_OUTPUT_FIFO_SIZE` to wrap the `outtail` index.

Pre-existing issues:
- [High] The patch fails to resolve the `raw_report` memory leak when a device is unexpectedly removed, contradicting the commit message's primary claim.
--

--- Patch [1]: [PATCH] HID: usbhid: free unsent raw output reports in usbhid_stop() ---
commit 2636e8bec5c393ff996cba8abd150f864da2447f
Author: Dmitry Antipov <dmantipov@yandex.ru>
HID: usbhid: free unsent raw output reports in usbhid_stop()

This commit attempts to fix memory leaks of unsent raw output reports
when a HID device is stalled or unexpectedly removed. It adds a loop
in usbhid_stop() to free the raw_report buffers and advance the outtail
index.

Link: 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..599a7566b825a 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) {

[Severity: High]
This isn't a bug introduced by this patch, but does this actually fix the leak
when the device is unexpectedly removed?

If the device is unplugged while an output report is in flight, the URB
completion handler hid_irq_out() runs with status -ESHUTDOWN and sets
unplug = 1. It then does this:

drivers/hid/usbhid/hid-core.c:hid_irq_out() {
    ...
    if (unplug) {
        usbhid->outtail = usbhid->outhead;
    }
    ...
}

This discards the pending queue entries before they are freed. Later, when
usbhid_stop() runs, usbhid->outtail already equals usbhid->outhead, so this
new while loop will be skipped entirely.

> +		kfree(usbhid->out[usbhid->outtail].raw_report);
> +		usbhid->out[usbhid->outtail].raw_report = NULL;
> +
> +		usbhid->outtail = (usbhid->outtail + 1) &
> +			(HID_CONTROL_FIFO_SIZE - 1);

[Severity: Low]
Does this use the wrong bitmask size?

The output queue size is defined by HID_OUTPUT_FIFO_SIZE, but this wraps
using HID_CONTROL_FIFO_SIZE - 1.

While the array in usbhid_device is sized using HID_CONTROL_FIFO_SIZE and
won't cause memory corruption, it forces outtail to traverse up to 192 unused
queue slots before wrapping.

> +	}
>  	spin_unlock_irq(&usbhid->lock);
>  
>  	usb_kill_urb(usbhid->urbin);
>  	usb_kill_urb(usbhid->urbout);

[Severity: Critical]
Can clearing the queue here lead to a NULL pointer dereference in the URB
completion handler?

If an output URB is active during unbind, the new loop above advances outtail
until it equals outhead. Then the lock is dropped and usb_kill_urb() cancels
the in-flight URB.

The cancellation synchronously invokes hid_irq_out() with status -ENOENT:

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) {
    ...
}

Because outtail already equals outhead from the loop in usbhid_stop(),
incrementing outtail here pushes it past outhead. The check
(usbhid->outhead != usbhid->outtail) then evaluates to true, tricking the
driver into calling hid_submit_out() on an empty slot. This accesses a
zero-initialized slot and dereferences a NULL report pointer.

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