Re: [RESEND PATCH v2 2/2] HID: usbhid: free unsent raw output reports in usbhid_stop()
From: sashiko-bot@kernel.org
Date: 2026-09-07 10:19:28
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider: - [High] Emptying the output queue in `usbhid_stop()` causes the URB completion handler `hid_irq_out()` to advance `outtail` past `outhead`, leading to a NULL pointer dereference in `hid_submit_out()`. -- commit fa290a16b56c6527830ed98c05dc263369208959 Author: Dmitry Antipov [off-list ref] HID: usbhid: free unsent raw output reports in usbhid_stop() 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. Closes: https://syzkaller.appspot.com/bug?extid=e2c057ea576d2644e2be
quoted hunk ↗ jump to hunk
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]
Does emptying the output queue here cause a NULL pointer dereference
when usb_kill_urb() subsequently cancels the in-flight URB?
When usb_kill_urb(usbhid->urbout) invokes the hid_irq_out() completion
handler with -ENOENT, unplug is 0. Unlike hid_ctrl(), hid_irq_out()
unconditionally advances outtail without checking if the queue is empty:
drivers/hid/usbhid/hid-core.c:hid_irq_out() {
...
if (unplug) {
usbhid->outtail = usbhid->outhead;
} else {
usbhid->outtail = (usbhid->outtail + 1) & (HID_OUTPUT_FIFO_SIZE - 1);
if (usbhid->outhead != usbhid->outtail &&
hid_submit_out(hid) == 0) {
...
}
Because this loop in usbhid_stop() already made outtail equal to outhead,
advancing it moves outtail into an uninitialized slot and makes the
outhead != outtail condition true.
This then triggers hid_submit_out(), which dereferences the NULL report
pointer from the uninitialized slot:
drivers/hid/usbhid/hid-core.c:hid_submit_out() {
...
report = usbhid->out[usbhid->outtail].report;
...
usbhid->urbout->transfer_buffer_length = hid_report_len(report);
...
}
Should hid_irq_out() be updated to check if outhead != outtail before
advancing the queue, similar to what hid_ctrl() does?
spin_unlock_irq(&usbhid->lock); usb_kill_urb(usbhid->urbin);
-- Sashiko AI review · https://sashiko.dev/#/patchset/20260907100645.86990-1-dmantipov@yandex.ru?part=2