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

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 [off-list ref]

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.

[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);
    ...
}

 }

[ ... ]

[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