Re: [PATCH v2 3/3] HID: rapoo: fix missing hid_is_usb() check

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Missing hid_hw_stop() cleanup on error paths in rapoo_probe() causes hardware resource leaks and a Use-After-Free.
--

commit ed7992bab1a88e74142aa29aa65b4061257a86cb
Author: Jann Horn <jannh@google.com>

HID: rapoo: fix missing hid_is_usb() check

This commit adds a hid_is_usb() check before calling to_usb_interface() to
handle uhid devices reporting BUS_USB without an actual USB parent. It also
adds a USB_HID dependency for hid_is_usb().

[ ... ]

> diff --git a/drivers/hid/hid-rapoo.c b/drivers/hid/hid-rapoo.c
> index 4c81f3086de43..5c9c396fabf77 100644
> --- a/drivers/hid/hid-rapoo.c
> +++ b/drivers/hid/hid-rapoo.c
> @@ -36,7 +36,7 @@ static int rapoo_probe(struct hid_device *hdev, const struct hid_device_id *id)
>  		return ret;
>  	}
>  
> -	if (hdev->bus == BUS_USB) {
> +	if (hid_is_usb(hdev)) {
>  		struct usb_interface *intf = to_usb_interface(hdev->dev.parent);
>  
>  		if (intf->cur_altsetting->desc.bInterfaceNumber != 1)

[Severity: High]
This is a pre-existing issue, but are we missing hid_hw_stop() calls on the
error paths later in rapoo_probe()?

If devm_input_allocate_device() fails, the code returns -ENOMEM directly:

	input = devm_input_allocate_device(&hdev->dev);
	if (!input)
		return -ENOMEM;

Similarly, if input_register_device() fails, it returns the error:

	ret = input_register_device(input);
	if (ret)
		return ret;

When returning these errors after hid_hw_start() has succeeded, the HID core
probe fails and sets hdev->driver to NULL, but hid_hw_stop() is never called
to stop the hardware and kill active URBs.

When the device is later unplugged, usbhid_disconnect() frees the usbhid
structure, but skips hid_hw_stop() because hdev->driver is NULL.

Because active USB URBs are not killed, when the USB core later flushes the
endpoints and cancels the URBs, the completion handler hid_irq_in() attempts
to clear state bits in the already-freed usbhid structure, like calling
clear_bit(HID_IN_RUNNING, &usbhid->iofl).

This results in a use-after-free write memory corruption. While this requires
local physical access to the USB port and specific conditions like memory
pressure to induce a probe failure, it guarantees memory corruption when
triggered.

Would it be better to add an error label that calls hid_hw_stop() when the
input device allocation or registration fails?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260703-hid-usbcheck-v2-0-c5ed7bc94772@google.com?part=3