Re: [PATCH v3] HID: rmi: fix OOB access with undersized RMI reports

[sashiko-bot@kernel.org]

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

New issues:
- [High] NULL pointer dereference introduced by the patch in rmi_probe().

Pre-existing issues:
- [High] Use-After-Free due to improper teardown order in rmi_remove().
- [High] Resource leak and Use-After-Free in rmi_probe error path.
- [High] TOCTOU race condition in chunked reads causes data corruption.
- [High] Global state pollution of rmi_hid_pdata in rmi_probe.
--

--- Patch [1]: [PATCH v3] HID: rmi: fix OOB access with undersized RMI reports ---
commit f98155b3d4bb0b8765a10c106b1cf4d13fb70009
Author: Wei Jie Law <98lawweijie@gmail.com>

HID: rmi: fix OOB access with undersized RMI reports

The hid-rmi driver sizes its writeReport/readReport buffer purely from
the report descriptor supplied by the device, with no minimum bound.
This patch adds bounds checking to prevent out-of-bounds accesses during
reads and writes.

Link: https://lore.kernel.org/linux-input/20260822121007.153988-1-98lawweijie@gmail.com/
Link: https://lore.kernel.org/linux-input/00a489f38b240624dcb5a4bae36a53fcba9cfb47.1787549195.git.98lawweijie@gmail.com/

> diff --git a/drivers/hid/hid-rmi.c b/drivers/hid/hid-rmi.c
> index 2bd781f1e0f53..21076521e35e7 100644
> --- a/drivers/hid/hid-rmi.c
> +++ b/drivers/hid/hid-rmi.c

[ ... ]

> @@ -696,6 +717,17 @@ static int rmi_probe(struct hid_device *hdev, const struct hid_device_id *id)
>  
>  	data->output_report_size = hid_report_len(output_report);
>  
> +	/*
> +	 * The write reports built by this driver occupy 6 bytes and the read
> +	 * handshake looks at the first 3 bytes of an input report, so refuse
> +	 * to drive a device whose reports cannot hold them.
> +	 */
> +	if (data->output_report_size < 6 || data->input_report_size < 3) {
> +		hid_err(hdev, "rmi reports too small (out=%u in=%u)\n",
> +			data->output_report_size, data->input_report_size);
> +		goto start;
> +	}

[Severity: High]
Does this goto bypass the allocation of data->writeReport?

If a device is dynamically bound using the new_id sysfs interface, and the
driver_data provided has BIT(0) (RMI_DEVICE) set, data->device_flags will
already have RMI_DEVICE set from id->driver_data earlier in rmi_probe().

When this jumps to the start label, hid_hw_start() will call
rmi_input_configured() which checks the RMI_DEVICE flag. Since it is set,
it will proceed to call rmi_set_page(), which dereferences
data->writeReport[0].

Since data->writeReport was never allocated, could this cause a NULL pointer
dereference?

>  	data->device_flags |= RMI_DEVICE;
>  	alloc_size = data->output_report_size + data->input_report_size;
>  


[Severity: High]
This is a pre-existing issue, but is there a use-after-free in rmi_remove()
due to the teardown order?

In rmi_remove(), cancel_work_sync(&hdata->reset_work) is called before
hid_hw_stop(hdev). Since hid_hw_stop() is what ultimately stops incoming
hardware events, a hardware interrupt could still occur after the worker is
cancelled.

This incoming event invokes rmi_event(), which unconditionally calls:

    schedule_work(&data->reset_work);
    return 1;

Once rmi_remove() returns, devres cleanup frees the hdata structure. The
newly scheduled work item could then execute rmi_reset_work() and
dereference the freed hdata memory.


[Severity: High]
This is a pre-existing issue, but does the rmi_probe() error path leak the
transport device?

During rmi_probe(), hid_hw_start() is called, which invokes the driver's
rmi_input_configured() callback. This callback registers the transport
device:

    ret = rmi_register_transport_device(&data->xport);

If hid_hw_start() subsequently fails (for example, during hidraw
registration), rmi_probe() returns the error without unregistering the
transport device:

    ret = hid_hw_start(hdev, HID_CONNECT_DEFAULT);
    if (ret) {
        hid_err(hdev, "hw start failed\n");
        return ret;
    }

Devres will free the hdata structure, but the RMI core still retains a
pointer to the now-freed hdata->xport device, which could lead to a
use-after-free.


[Severity: High]
This is a pre-existing issue, but is there a race condition in chunked reads
that could cause data corruption?

When rmi_hid_read_block() requests a read larger than what fits in a single
input report, the device responds with multiple consecutive reports. Each
report triggers rmi_read_data_event() in interrupt context, which executes:

    memcpy(hdata->readReport, data,
           min((u32)size, hdata->input_report_size));
    set_bit(RMI_READ_DATA_PENDING, &hdata->flags);
    wake_up(&hdata->wait);

There appears to be no synchronization to prevent a subsequent report from
overwriting hdata->readReport before the consumer thread in
rmi_hid_read_block() wakes up to consume the previous chunk:

    while (bytes_read < len) {
        if (!wait_event_timeout(data->wait,
            test_bit(RMI_READ_DATA_PENDING, &data->flags),

If the device sends reports faster than the thread gets scheduled, could
data be overwritten and silently lost?


[Severity: High]
This is a pre-existing issue, but does rmi_probe() modify global state
instead of device-specific state?

rmi_hid_pdata is defined as a static global template structure. However,
in rmi_probe(), if the device has physical buttons, it modifies the global
template directly:

    if (data->device_flags & RMI_DEVICE_HAS_PHYS_BUTTONS)
        rmi_hid_pdata.gpio_data.disable = true;

    data->xport.dev = hdev->dev.parent;
    data->xport.pdata = rmi_hid_pdata;

This permanently mutates the global template. Could all subsequently probed
devices erroneously inherit gpio_data.disable = true, even if they do not
have physical buttons?

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