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

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 <[email protected]>

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/[email protected]?part=2