Re: [PATCH] HID: logitech-dj: forward Bolt receiver input to the dj child device

[sashiko-bot@kernel.org]

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

New issues:
- [High] The shared `djrcv_dev->unnumbered_application` field is overwritten when multiple interfaces of a Bolt receiver use unnumbered reports, leading to corrupted input routing.

Pre-existing issues:
- [Critical] Worker teardown race leading to a Use-After-Free of `djrcv_dev` when the device is unbound or disconnected.
--

commit 8ddfaefb25fbb11d3135bd6028112cdebd298409
Author: Grégoire Stein <greyxor@protonmail.com>

    HID: logitech-dj: forward Bolt receiver input to the dj child device

This patch drops the special case for Bolt receivers and sets no_dj_interfaces
to 3, allowing interfaces 0 and 1 to be claimed by the dj driver so their
reports get properly forwarded to the child device.

> diff --git a/drivers/hid/hid-logitech-dj.c b/drivers/hid/hid-logitech-dj.c
> index 1d619d2345e12..5a8f63ced627c 100644
> --- a/drivers/hid/hid-logitech-dj.c
> +++ b/drivers/hid/hid-logitech-dj.c
> @@ -1907,9 +1907,6 @@ static int logi_dj_probe(struct hid_device *hdev,
>  	 * treat these as logitech-dj interfaces then this causes input events
>  	 * reported through this extra interface to not be reported correctly.
>  	 * To avoid this, we treat these as generic-hid devices.
> -	 *
> -	 * Bolt receivers only use LOGITECH_DJ_INTERFACE_NUMBER for receiver
> -	 * reporting. Treat all other Bolt interfaces as generic-hid devices.
>  	 */
>  	switch (id->driver_data) {
>  	case recvr_type_dj:		no_dj_interfaces = 3; break;
> @@ -1920,23 +1917,14 @@ static int logi_dj_probe(struct hid_device *hdev,
>  	case recvr_type_27mhz:		no_dj_interfaces = 2; break;
>  	case recvr_type_bluetooth:	no_dj_interfaces = 2; break;
>  	case recvr_type_dinovo:		no_dj_interfaces = 2; break;
> +	case recvr_type_bolt:		no_dj_interfaces = 3; break;
>  	}
>  	if (hid_is_usb(hdev)) {
>  		intf = to_usb_interface(hdev->dev.parent);
> -		if (intf) {
> -			bool generic_hid_interface;
> -
> -			if (id->driver_data == recvr_type_bolt)
> -				generic_hid_interface =
> -					intf->altsetting->desc.bInterfaceNumber !=
> -					LOGITECH_DJ_INTERFACE_NUMBER;
> -			else
> -				generic_hid_interface =
> -					intf->altsetting->desc.bInterfaceNumber >= no_dj_interfaces;
> -			if (generic_hid_interface) {
> -				hdev->quirks |= HID_QUIRK_INPUT_PER_APP;
> -				return hid_hw_start(hdev, HID_CONNECT_DEFAULT);
> -			}
> +		if (intf && intf->altsetting->desc.bInterfaceNumber >=
> +							no_dj_interfaces) {
> +			hdev->quirks |= HID_QUIRK_INPUT_PER_APP;
> +			return hid_hw_start(hdev, HID_CONNECT_DEFAULT);
>  		}
>  	}

[Severity: High]
Does this code overwrite the shared djrcv_dev->unnumbered_application field
when multiple interfaces of a Bolt receiver use unnumbered reports?

By dropping the special case for Bolt, both the keyboard (interface 0) and
mouse (interface 1) bind to hid-logitech-dj. Because they share the same
dj_receiver_dev struct, the sequentially probed mouse interface overwrites
unnumbered_application with HID_GD_MOUSE:

drivers/hid/hid-logitech-dj.c:logi_dj_probe() {
    ...
    if (!input_report_enum->numbered)
        djrcv_dev->unnumbered_application = rep->application;
    ...
}

When unnumbered keyboard reports arrive on interface 0, does this cause them to
be incorrectly routed and formatted as mouse reports in logi_dj_raw_event()?

[Severity: Critical]
This is a pre-existing issue, but is there a use-after-free regression during
device teardown in logi_dj_remove()?

In logi_dj_remove(), the asynchronous work is cancelled before the hardware
is stopped:

drivers/hid/hid-logitech-dj.c:logi_dj_remove() {
    ...
    cancel_work_sync(&djrcv_dev->work);

    hid_hw_close(hdev);
    hid_hw_stop(hdev);
    ...
}

Since the hardware polling is not yet stopped, an incoming hardware interrupt
can re-queue the work via schedule_work() in
logi_dj_recv_queue_unknown_work() just before djrcv_dev is freed by
dj_put_receiver_dev().

Does this leave a window for incoming interrupts to queue freed work items,
leading to a use-after-free when the delayedwork_callback executes and
accesses djrcv_dev->lock?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260901-bolt-scroll-fix-v1-1-58bca7ae487f@protonmail.com?part=1