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

[RESEND 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

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

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

> 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