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

[PATCH v2] 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
Assisted-by: Claude:claude-opus-5
Assisted-by: GLM:glm-5.3
Signed-off-by: Wei Jie Law <98lawweijie@gmail.com>
---
Changes in v2:
 - No code change: the diff is identical to v1.  Adds the Assisted-by
   tags required by Documentation/process/coding-assistants.rst.

This is independent of the pending "HID: rmi: fix OOB access with
undersized RMI reports" [1] (whose v5, also a tags-only respin, is being
sent alongside this) -- 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/20260825060954.104890-1-98lawweijie@gmail.com/

v1: https://lore.kernel.org/linux-input/20260825034854.63555-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 v2] HID: rmi: fix use-after-free of struct rmi_data via reset_work

[sashiko-bot]

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

Pre-existing issues:
- [High] Out-of-bounds read in `rmi_hid_read_block()` due to unvalidated device input.
- [High] Global state contamination in `rmi_probe()` affecting subsequently probed devices.
--

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

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

This patch resolves a use-after-free vulnerability involving struct rmi_data
and its reset_work. It unconditionally cancels the work in rmi_remove() and
prevents new work from being queued inappropriately during device removal.

> 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

[ ... ]

drivers/hid/hid-rmi.c:rmi_hid_read_block() {
    ...
			read_input_count = data->readReport[1];
			memcpy(buf + bytes_read, &data->readReport[2],
				min(read_input_count, bytes_needed));
    ...
}

[Severity: High]
This isn't a bug introduced by this patch, but can this code cause an
out-of-bounds read?

A malicious device could advertise a small input_report_size via its HID
descriptor but send an input report with a large length field in data[1].

Because rmi_hid_read_block() uses this untrusted data->readReport[1] as
read_input_count, could it bypass boundary checks against the buffer's
actual allocated size during the memcpy?


drivers/hid/hid-rmi.c:rmi_probe() {
    ...
	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;
    ...
}

[Severity: High]
This isn't a bug introduced by this patch, but does this code modify the
global static rmi_hid_pdata struct instead of a device-specific local copy?

If a device with the RMI_DEVICE_HAS_PHYS_BUTTONS flag is probed, it sets
gpio_data.disable = true on the global struct.

If a different RMI device without this flag is subsequently probed on the
same system, could it incorrectly inherit the disabled GPIO state from this
contaminated global template?

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