Re: [PATCH 1/2] HID: alps: unregister DualPoint Stick input device on remove

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

New issues:
- [High] If `input_register_device()` fails for `data->input2` in `alps_input_configured()`, the driver frees `input2` but leaves `data->input2` as a dangling pointer. The newly added `alps_remove()` unconditionally uses this dangling pointer, causing a Use-After-Free.

Pre-existing issues:
- [High] Untrusted USB devices can trigger a NULL pointer dereference in `u1_raw_event()` by sending a StickPointer report when the device was not initialized with one.
--

--- Patch [1]: [PATCH 1/2] HID: alps: unregister DualPoint Stick input device on remove ---
commit 93188845b9160c2ea2789c012e11b634ca66357c
Author: Chen Changcheng <[email protected]>

HID: alps: unregister DualPoint Stick input device on remove

alps_input_configured() allocates a second input device ("DualPoint
Stick") with input_allocate_device() and registers it, but the
alps_driver struct has no .remove handler and input2 is not tracked in
hdev->inputs. The default remove path only iterates hdev->inputs, so
input2 is never unregistered and leaks on every device removal.
This patch adds a .remove handler that stops the device first and then
unregisters input2.

> diff --git a/drivers/hid/hid-alps.c b/drivers/hid/hid-alps.c
> index 67179e3fe39b7..370635f5b7043 100644
> --- a/drivers/hid/hid-alps.c
> +++ b/drivers/hid/hid-alps.c

[ ... ]

> @@ -823,6 +823,24 @@ static int alps_probe(struct hid_device *hdev, const struct hid_device_id *id)
>  	return 0;
>  }
>  
> +static void alps_remove(struct hid_device *hdev)
> +{
> +	struct alps_dev *data = hid_get_drvdata(hdev);
> +
> +	/*
> +	 * input2 ("DualPoint Stick") is allocated separately and is not
> +	 * tracked in hdev->inputs, so the default remove path
> +	 * (hid_hw_stop -> hidinput_disconnect) does not unregister it.
> +	 *
> +	 * Stop the device first so that no URB callback can touch input2
> +	 * while it is being unregistered, then drop it explicitly.
> +	 */
> +	hid_hw_stop(hdev);
> +
> +	if (data->input2)
> +		input_unregister_device(data->input2);

[Severity: High]
If input_register_device() fails in alps_input_configured(), input2 is freed
but data->input2 is not set to NULL:

drivers/hid/hid-alps.c:alps_input_configured() {
    ...
		if (input_register_device(data->input2)) {
			input_free_device(input2);
			ret = -ENOENT;
			goto exit;
		}
    ...
}

Because the driver has a .raw_event handler, hid_connect() considers
the device bound and ignores the failure in hidinput_connect(), allowing
probe to succeed.

When the device is later removed, could this result in a use-after-free
when alps_remove() attempts to unregister the dangling data->input2
pointer?

[Severity: High]
This isn't a bug introduced by this patch, but if a malicious or
malfunctioning device indicates it doesn't support the StickPointer
(has_sp == 0), data->input2 is never allocated.

If that device then sends a raw report with U1_SP_ABSOLUTE_REPORT_ID (0x06),
would this trigger a NULL pointer dereference in u1_raw_event()?

drivers/hid/hid-alps.c:u1_raw_event() {
    ...
	case U1_SP_ABSOLUTE_REPORT_ID:
		sp_x = get_unaligned_le16(data+2);
		sp_y = get_unaligned_le16(data+4);

		sp_x = sp_x / 8;
		sp_y = sp_y / 8;

		input_report_rel(hdata->input2, REL_X, sp_x);
    ...
}

> +}
> +

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1