Re: [PATCH] HID: rmi: fix use-after-free of struct rmi_data via reset_work

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [Critical] The driver allocates `writeReport` and `readReport` buffers based on unvalidated `hid_report_len()` values from the device's HID descriptor, leading to heap out-of-bounds accesses in `rmi_hid_write_block()` and `rmi_hid_read_block()`.
- [High] If the RMI4 core driver fails to bind to the registered transport device, `rmi_reset_attn_mode()` blindly dereferences a NULL `rmi_dev->driver` pointer when triggered by a mouse report.
- [High] Preexisting Use-After-Free of `input_dev` if `input_register_device()` fails during `hid_hw_start()`.
--

--- Patch [1]: [PATCH] HID: rmi: fix use-after-free of struct rmi_data via reset_work ---
commit 46d084fef573949d9f74d0919ad4a8a2ea0ce1cc
Author: Wei Jie Law <98lawweijie@gmail.com>

HID: rmi: fix use-after-free of struct rmi_data via reset_work

This commit fixes a use-after-free issue where rmi_remove() only cancels
the reset_work when RMI_STARTED is set, leaving it schedulable if probe fails.
It unconditionally cancels the work in rmi_remove() and adds extra checks
to prevent the work from running after the device is unregistered.

> diff --git a/drivers/hid/hid-rmi.c b/drivers/hid/hid-rmi.c
> index 2bd781f1e0f53..4df6d8cd2dc3c 100644
> --- a/drivers/hid/hid-rmi.c
> +++ b/drivers/hid/hid-rmi.c
> @@ -313,6 +313,14 @@ static void rmi_reset_work(struct work_struct *work)
>  	struct rmi_data *hdata = container_of(work, struct rmi_data,
>  						reset_work);
>  
> +	/*
> +	 * A report that raced with rmi_remove() may have queued us after it
> +	 * cleared RMI_STARTED, i.e. after the transport device we would reset
> +	 * has been unregistered.
> +	 */
> +	if (!test_bit(RMI_STARTED, &hdata->flags))
> +		return;
> +
>  	/* switch the device to RMI if we receive a generic mouse report */
>  	rmi_reset_attn_mode(hdata->hdev);

[Severity: High]
This is a pre-existing issue, but can rmi_reset_attn_mode() dereference a
null rmi_dev->driver pointer here if the RMI core driver fails to bind to
the registered transport device?

If the RMI physical driver fails to probe, device_add() still succeeds.
The hid-rmi driver might mistakenly treat this as success, set the
RMI_STARTED flag, and leave the device active:

drivers/hid/hid-rmi.c:rmi_input_configured() {
    ...
    ret = rmi_register_transport_device(&data->xport);
    if (ret < 0) {
    ...
    set_bit(RMI_STARTED, &data->flags);
    ...
}

When the device later sends a mouse report, it triggers this worker that
calls into rmi_reset_attn_mode() and blindly dereferences
rmi_dev->driver->reset_handler:

drivers/hid/hid-rmi.c:rmi_reset_attn_mode() {
    ...
    if (test_bit(RMI_STARTED, &data->flags))
        ret = rmi_dev->driver->reset_handler(rmi_dev);
    ...
}

>  }
[ ... ]
> @@ -739,15 +753,29 @@ static int rmi_probe(struct hid_device *hdev, const struct hid_device_id *id)

[Severity: Critical]
This is a pre-existing issue, but does this driver allocate writeReport and
readReport buffers based on unvalidated hid_report_len() values from the
device's hid descriptor?

If an attacker plugs in a malicious usb device with a crafted hid report
descriptor that advertises an output report size smaller than the fixed
header written by the driver, it seems we might get a heap buffer overflow.

For example, in rmi_probe():

drivers/hid/hid-rmi.c:rmi_probe() {
    ...
    alloc_size = data->output_report_size + data->input_report_size;
    data->writeReport = devm_kzalloc(&hdev->dev, alloc_size, GFP_KERNEL);
    ...
}

Then in rmi_hid_write_block(), the driver blindly writes a 4-byte header
and a payload to data->writeReport without verifying that the allocated
buffer is large enough:

drivers/hid/hid-rmi.c:rmi_hid_write_block() {
    ...
    data->writeReport[0] = RMI_WRITE_REPORT_ID;
    ...
    memcpy(&data->writeReport[4], buf, len);
    ...
}

[Severity: High]
This is also a pre-existing issue, but does this code cause a use-after-free
of the input_dev structure if input_register_device() fails?

The hid-rmi driver copies the input_dev pointer and registers the transport
device before input_register_device() is called by the hid core:

drivers/hid/hid-rmi.c:rmi_input_configured() {
    ...
    data->xport.input = input;
    ...
}

If input_register_device() fails, the hid core unwinds and frees the
input_dev:

drivers/hid/hid-input.c:hidinput_connect() {
    ...
    if (input_register_device(hidinput->input))
        goto out_unwind;
    ...
out_unwind:
    /* unwind the ones we already registered */
    hidinput_disconnect(hid);
    ...
}

Because hid-rmi implements a raw_event handler, the device remains bound.
When the device subsequently sends hardware interrupts, the transport driver
unconditionally dereferences the freed input pointer:

drivers/input/rmi4/rmi_driver.c:rmi_process_interrupt_requests() {
    ...
    if (data->input)
        input_sync(data->input);
    ...
}

>  static void rmi_remove(struct hid_device *hdev)
>  {
[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260825034854.63555-1-98lawweijie@gmail.com?part=1