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

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

[Wei Jie Law]

rmi_event() queues hdata->reset_work for any pointer/mouse usage as soon
as RMI_DEVICE is set, but rmi_remove() only cancels the work when
RMI_STARTED is set:

	if ((hdata->device_flags & RMI_DEVICE)
	    && test_bit(RMI_STARTED, &hdata->flags)) {
		clear_bit(RMI_STARTED, &hdata->flags);
		cancel_work_sync(&hdata->reset_work);
		rmi_unregister_transport_device(&hdata->xport);
	}

RMI_STARTED is set at the very end of rmi_input_configured(), so a device
that advertises the RMI report IDs - which is what makes rmi_probe() set
RMI_DEVICE - but then makes rmi_input_configured() fail leaves the work
schedulable and never cancelled.  Never answering the SET_REPORT that
rmi_set_mode() issues is enough, i.e. exactly the unreachable-device case
the guard was added for.  rmi_probe() still returns 0 there, because
hidraw claims the device, so the driver stays bound and rmi_event() keeps
running.

struct rmi_data is devm_kzalloc()'d on &hdev->dev, so hid_device_remove()
releases it as soon as ->remove() returns, with the work still queued or
still running.  The workqueue then reads and writes the freed object:
process_one_work() stores into work->data, list_del_init()s work->entry,
and loads work->func out of freed memory before calling it, while
rmi_reset_work() dereferences hdata->hdev, which the same unplug freed.

  BUG: KASAN: slab-use-after-free in process_one_work+0xd96/0x10b0
  Read of size 8 at addr ffff8881095c0540 by task kworker/0:4/3058
  [...]
  Allocated by task 450:
   devm_kmalloc+0x7c/0x220
   rmi_probe+0x36/0xcf0 [hid_rmi]
   hid_device_probe+0x286/0x430
  Freed by task 9278:
   kfree+0x125/0x420
   release_nodes+0xf0/0x260
   devres_release_group+0x23a/0x3a0
   hid_device_remove+0xf5/0x220

The read is of hdata->reset_work.func, 320 bytes into the freed 512-byte
region, which process_one_work() calls straight afterwards.  Without KASAN
the same reproducer oopses in rmi_reset_work() and leaves the kworker
"exited with irqs disabled".

Cancel the work unconditionally in rmi_remove(), and do not queue it
before rmi_input_configured() has succeeded or after rmi_remove() has
cleared RMI_STARTED.

A report can still pass the RMI_STARTED test in rmi_event() just before
rmi_remove() clears the bit and queue the work after that cancel, so cancel
once more after hid_hw_stop() has stopped the report flow, and make
rmi_reset_work() bail out when RMI_STARTED is clear - by then the transport
device it would reset has been unregistered.

Fixes: 8725aa4fa7de ("HID: rmi: Check that the RMI_STARTED bit is set before unregistering the RMI transport device")
Cc: stable@vger.kernel.org
Signed-off-by: Wei Jie Law <98lawweijie@gmail.com>
---
This is independent of the pending "HID: rmi: fix OOB access with
undersized RMI reports" v3 [1] -- they touch different functions, and
either order applies cleanly.  They are worth taking together, though.

That patch makes a zero-length READ_DATA reply fail rmi_hid_read_block()
with -EIO instead of spinning in it, so rmi_scan_pdt(), and hence
rmi_input_configured(), now fail where they previously hung with the
device lock held.  That failure leaves RMI_DEVICE set and RMI_STARTED
clear -- the state this patch is about -- and, because the probe no
longer hangs, it also lets the unplug that triggers the use-after-free
complete.  The route described above, never answering the SET_REPORT
that rmi_set_mode() issues, reaches the same state on an unpatched tree,
so this is not a regression from that patch; it is a second way in, and
an argument for the two landing in the same release.

[1] https://lore.kernel.org/all/20260824122708.76168-1-98lawweijie@gmail.com/

 drivers/hid/hid-rmi.c | 40 ++++++++++++++++++++++++++++++++++------
 1 file changed, 34 insertions(+), 6 deletions(-)

diff --git a/drivers/hid/hid-rmi.c b/drivers/hid/hid-rmi.c
index d4af17fdba46..13a301404ff8 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);
 }
@@ -412,7 +420,13 @@ static int rmi_event(struct hid_device *hdev, struct hid_field *field,
 				return 1;
 		}
 
-		schedule_work(&data->reset_work);
+		/*
+		 * Only reset a device that finished rmi_input_configured();
+		 * before that, and after rmi_remove() has cleared the bit,
+		 * struct rmi_data may go away under the work.
+		 */
+		if (test_bit(RMI_STARTED, &data->flags))
+			schedule_work(&data->reset_work);
 		return 1;
 	}
 
@@ -739,15 +753,29 @@ static int rmi_probe(struct hid_device *hdev, const struct hid_device_id *id)
 static void rmi_remove(struct hid_device *hdev)
 {
 	struct rmi_data *hdata = hid_get_drvdata(hdev);
+	bool started = test_and_clear_bit(RMI_STARTED, &hdata->flags);
 
-	if ((hdata->device_flags & RMI_DEVICE)
-	    && test_bit(RMI_STARTED, &hdata->flags)) {
-		clear_bit(RMI_STARTED, &hdata->flags);
-		cancel_work_sync(&hdata->reset_work);
+	/*
+	 * reset_work lives inside the devm-allocated hdata, which is freed as
+	 * soon as this returns, so it has to be cancelled whether or not the
+	 * device ever reached the RMI_STARTED state.  Cancel it here, while
+	 * the transport device it resets is still registered.
+	 */
+	cancel_work_sync(&hdata->reset_work);
+
+	if ((hdata->device_flags & RMI_DEVICE) && started)
 		rmi_unregister_transport_device(&hdata->xport);
-	}
 
 	hid_hw_stop(hdev);
+
+	/*
+	 * A report that passed the RMI_STARTED test in rmi_event() just before
+	 * the clear above can queue the work again after that first cancel.
+	 * Such a work item does nothing, but it still has to be reaped before
+	 * hdata goes away.  hid_hw_stop() has stopped the report flow, so no
+	 * further queueing is possible by now.
+	 */
+	cancel_work_sync(&hdata->reset_work);
 }
 
 static const struct hid_device_id rmi_id[] = {
-- 
2.43.0

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

[sashiko-bot]

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

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

[Wei Jie LAW]

From: Wei Jie Law <98lawweijie@gmail.com>

Superseded by v2:
https://lore.kernel.org/linux-input/20260825103144.12356-1-98lawweijie@gmail.com/

Please use that one instead.  The only difference is the Assisted-by
tags that Documentation/process/coding-assistants.rst asks for -- AI
tools were used in finding and fixing this bug and I should have
acknowledged that in the original posting.  The diff is unchanged.

Please mark this one superseded.

Thanks, and sorry for the noise,
Wei Jie