[PATCH v3 0/2] Input: synaptics-rmi4 - fix two device-controlled out-of-bounds writes

[PATCH v3 0/2] Input: synaptics-rmi4 - fix two device-controlled out-of-bounds writes

[Wei Jie LAW]

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

Hi Dmitry,

While putting together a reproducer for an out-of-bounds bug in hid-rmi
([1]; a v4 fixing a hole in its own error path is being sent alongside
this series) I found two further memory-safety problems in the shared
RMI4 core.  Both are driven entirely by data the *device* supplies --
its Page Description Table -- so they are reachable from a malicious USB
HID device with no code running on the victim, and equally from I2C and
SMBus RMI4 devices.  Neither depends on the hid-rmi bug; they are in
drivers/input/rmi4/ and need fixing separately.

Both are present in mainline and in every stable tree I looked at.

1/2 is an off-by-one: RMI_PDT_INT_SOURCE_COUNT_MASK is 0x07, so
interrupt_source_count can be 7, but struct rmi_function declares
int irq[RMI_FN_MAX_IRQS] with RMI_FN_MAX_IRQS == 6 and two loops walk it
up to fn->num_of_irqs.  irq[6] is the storage of the next member,
unsigned int irq_pos, so the function's position in the interrupt bitmap
is silently replaced with a Linux virq number.  UBSAN flags all five
stores plus the read on the unregister path.

2/2 is a time-of-check/time-of-use across two reads of the device: the
PDT is scanned three times and re-read from the device every time,
irq_mask[] is sized by the counting scan and filled by the creating scan,
and nothing verifies the two agree.  KASAN catches the resulting
set_bit() walking off the end of the flexible array at the tail of every
struct rmi_function.

2/2 also closes the device-driven route into an older problem in
rmi_create_function_irq(): irq_create_mapping() is called without
checking for failure, and it returns 0, not an error code.  A device that
declares one interrupt in the counting scan and four in the creating scan
makes the mapping fail, and irq_set_chip_data(0, fn) plus
irq_set_chip_and_handler(0, &rmi_irq_chip, handle_simple_irq) then
replace the chip and flow handler of IRQ 0 -- the timer, on x86:

  =========== /proc/interrupts, before ===========
    0:          9          0   IO-APIC   2-edge      timer
  =========== /proc/interrupts, after ============
    0:          9          0      rmi4   2  timer

  error: hwirq 0x1 is too large for unknown-2
  WARNING: CPU: 0 PID: 56 at kernel/irq/irqdomain.c:688
           irq_domain_associate_locked+0x2b4/0x390
  genirq: Flags mismatch irq 0. 00002000 (rmi4-00.fn01) vs. 00215a00 (timer)

With 2/2 applied the same device is rejected before any mapping is
attempted and IRQ 0 is untouched.  A standalone check of the
irq_create_mapping() return value still looks worthwhile, but it is a
separate change and I did not want to bury it in this series.

How this was verified:

Linux v6.12.69 (CONFIG_UBSAN_BOUNDS=y, booted slub_debug=FZPU) and
v6.12.105 (CONFIG_KASAN=y + CONFIG_KASAN_INLINE=y, CONFIG_UBSAN_BOUNDS=y,
booted kasan_multi_shot -- generic KASAN otherwise reports only the first
error per boot), x86_64.  An emulated Synaptics RMI4 device publishes a
Page Description Table crafted for each case.  Two independent
reproducers, giving identical results:

  - a /dev/uhid program -- no hardware, fully deterministic, and the easy
    one to run;
  - the same device over dummy_hcd + raw-gadget with Facedancer, so the
    reports really traverse usbcore -> usbhid -> hid-rmi.

Each bug was exercised on its own cold boot, because heap state left by a
previous run changes what the out-of-bounds read returns and UBSAN
reports each call site only once per boot.

With both patches applied 1/2 produces no UBSAN reports and the same
device -- F01 declaring the full 7 interrupt sources -- probes normally,
and 2/2 fails the probe cleanly instead of corrupting the heap: three
consecutive runs of the growing-PDT device give three rejections
("F40: interrupt count changed between PDT scans (pos 1 + 6 > 1)",
"Function creation failed with code -22.") and zero KASAN reports,
against 14 from the unpatched core on the same boot, with no kobject or
refcount warnings; devices answering both scans consistently still probe
and report their real product id.

I am happy to post the reproducers, or to send them privately if you
would rather they did not go to a public list.

Changes in v3:

 - 2/2: reject the function *before* allocating it, rather than
   disposing of it afterwards.  The v2 kfree() is correct on today's
   stable trees, but it is wrong on mainline since
   commit 58d42ec10b73 ("Input: rmi4 - refactor function allocation
   and registration"): rmi_alloc_function() has since run
   device_initialize() and dev_set_name() on fn->dev, so kfree() on
   this path would leak the
   name string and skip the kobject cleanup that device_initialize()
   obligates.  put_device() is no alternative: on the pre-refactor trees
   the stable backports target it warns and leaks exactly as v1 did.
   Checking pdt->interrupt_source_count and the running total against
   data->irq_count before rmi_alloc_function() needs no cleanup on any
   tree; the error message and the -EINVAL are unchanged.
 - 1/2 is unchanged.

Changes in v2:

 - 2/2: dispose of the rejected struct rmi_function with kfree() rather
   than put_device().  The rejection happens before
   rmi_register_function(), so device_initialize() has not run yet and
   fn->dev is still all zeroes.  put_device() on it warned twice --

     kobject: '(null)' (00000000cfabc269): is not initialized, yet
              kobject_put() is being called.
     WARNING: CPU: 0 PID: 9 at lib/kobject.c:734 kobject_put+0x1cf/0x4b0
     refcount_t: underflow; use-after-free.
     WARNING: CPU: 0 PID: 9 at lib/refcount.c:28
              refcount_warn_saturate+0xf2/0x150

   -- and then leaked the function, because the saturated refcount stops
   kref_put() from ever running the release.  ftrace over 405 rejections
   counted 810 rmi_create_function against 405 rmi_release_function, one
   orphan per rejection, matching a +407 growth in kmalloc-1k.  With
   kfree() there are no warnings over 206 rejections and the slab count
   is flat.  Thanks to the Sashiko automated review for prompting a
   closer look at that error path.
 - 1/2 is unchanged.

The v1 posting is at
https://lore.kernel.org/linux-input/cover.1787549234.git.98lawweijie@gmail.com/
and v2 at
https://lore.kernel.org/linux-input/20260824122733.76321-1-98lawweijie@gmail.com/

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

Wei Jie Law (2):
  Input: synaptics-rmi4 - fix irq[] overrun with 7 interrupt sources
  Input: synaptics-rmi4 - reject a PDT that grows between scans

 drivers/input/rmi4/rmi_bus.h    |  9 ++++++---
 drivers/input/rmi4/rmi_driver.c | 19 +++++++++++++++++++
 2 files changed, 25 insertions(+), 3 deletions(-)

-- 
2.43.0

[PATCH v3 1/2] Input: synaptics-rmi4 - fix irq[] overrun with 7 interrupt sources

[Wei Jie LAW]

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

rmi_read_pdt_entry() takes the interrupt source count straight out of the
Page Description Table entry the device supplies:

	entry->interrupt_source_count = buf[4] & RMI_PDT_INT_SOURCE_COUNT_MASK;

RMI_PDT_INT_SOURCE_COUNT_MASK is 0x07, so the value can be 7, and
rmi_create_function() copies it verbatim into fn->num_of_irqs.  But
struct rmi_function declares

	int irq[RMI_FN_MAX_IRQS];

with RMI_FN_MAX_IRQS == 6, and both rmi_create_function_irq() and
rmi_unregister_function() index that array up to fn->num_of_irqs.  A
device declaring 7 interrupt sources for a function that has a handler --
F01 always does -- makes the driver write irq[6], which is the storage of
the following member, unsigned int irq_pos.  The function's position in
the interrupt bitmap then holds a Linux virq number, and that value feeds
set_bit(fn->irq_pos, ...) in rmi_f11_probe()/rmi_f12_probe() and the
irq_dispose_mapping() loop on teardown.

UBSAN reports every store in the loop body and the read on the unregister
path:

  UBSAN: array-index-out-of-bounds in drivers/input/rmi4/rmi_bus.c:183:10
  index 6 is out of range for type 'int [6]'
  Workqueue: events uhid_device_add_worker
   dump_stack_lvl+0x64/0x80
   __ubsan_handle_out_of_bounds+0xc8/0x100
   rmi_function_probe+0x1c1/0x210 [rmi_core]
  UBSAN: array-index-out-of-bounds in drivers/input/rmi4/rmi_bus.c:186:28
  UBSAN: array-index-out-of-bounds in drivers/input/rmi4/rmi_bus.c:187:35
  UBSAN: array-index-out-of-bounds in drivers/input/rmi4/rmi_bus.c:189:32
  UBSAN: array-index-out-of-bounds in drivers/input/rmi4/rmi_bus.c:191:54
  UBSAN: array-index-out-of-bounds in drivers/input/rmi4/rmi_bus.c:282:30

Size the array to match the three bit field that feeds it.  Clamping
num_of_irqs instead would silently drop an interrupt source a device is
allowed to declare, and would desynchronise irq_pos for every function
created after it.

Reproduced with an emulated RMI4 device that publishes a single F01 PDT
entry with interrupt_source_count = 7, driven over /dev/uhid and again
over dummy_hcd plus raw-gadget, on v6.12.69 and v6.12.105 with
CONFIG_UBSAN_BOUNDS=y.  No reports after this change, and the same device
now probes normally.

Fixes: 24d28e4f1271 ("Input: synaptics-rmi4 - convert irq distribution to irq_domain")
Cc: stable@vger.kernel.org
Signed-off-by: Wei Jie Law <98lawweijie@gmail.com>
---
 drivers/input/rmi4/rmi_bus.h | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/drivers/input/rmi4/rmi_bus.h b/drivers/input/rmi4/rmi_bus.h
index 90122df21f74..faf2ebb00d52 100644
--- a/drivers/input/rmi4/rmi_bus.h
+++ b/drivers/input/rmi4/rmi_bus.h
@@ -12,10 +12,13 @@
 struct rmi_device;
 
 /*
- * The interrupt source count in the function descriptor can represent up to
- * 6 interrupt sources in the normal manner.
+ * The interrupt source count in the function descriptor is a three bit field
+ * (RMI_PDT_INT_SOURCE_COUNT_MASK), so a device can legitimately declare up to
+ * 7 interrupt sources for a single function.  irq[] must be able to hold all
+ * of them: rmi_create_function_irq() and rmi_unregister_function() both walk
+ * it up to fn->num_of_irqs.
  */
-#define RMI_FN_MAX_IRQS	6
+#define RMI_FN_MAX_IRQS	7
 
 /**
  * struct rmi_function - represents the implementation of an RMI4
-- 
2.43.0

[PATCH v3 2/2] Input: synaptics-rmi4 - reject a PDT that grows between scans

[Wei Jie LAW]

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

rmi_driver_probe() walks the Page Description Table three times, and each
walk reads the table back from the device:

  1. rmi_initial_reset   - issue the reset command in the F01 entry
  2. rmi_count_irqs      - total the interrupt sources
  3. rmi_create_function - create the functions and set their irq bits

Scan 2 fixes data->irq_count, data->num_of_irq_regs and the size of every
per-function irq_mask[].  Scan 3 then accumulates fn->irq_pos and does

	for (i = 0; i < fn->num_of_irqs; i++)
		set_bit(fn->irq_pos + i, fn->irq_mask);

without checking the result against the count that sized the bitmap.
Nothing makes the device answer the third scan the way it answered the
second, so a device that reports one function with one interrupt source
on scan 2 and a long list of functions on scan 3 walks set_bit() past the
end of the flexible array at the tail of every struct rmi_function:

  BUG: KASAN: slab-out-of-bounds in rmi_create_function+0x560/0x930 [rmi_core]
  Write of size 8 at addr ffff888110c92b58 by task kworker/1:2/129
  Workqueue: events uhid_device_add_worker
   kasan_report+0xc6/0x100
   kasan_check_range+0x105/0x1b0
   rmi_create_function+0x560/0x930 [rmi_core]
   rmi_scan_pdt+0x190/0x3f0 [rmi_core]
   rmi_init_functions+0xb8/0x320 [rmi_core]
   rmi_driver_probe+0x31e/0xbf0 [rmi_core]

one report per corrupted function object.  The same unvalidated
fn->irq_pos is used again by the set_bit() and irq_create_mapping() in
rmi_create_function_irq().

Validate the counts before the function is allocated and fail the probe
instead.  The check is exact, not conservative: when both scans see the
same table, the running interrupt total plus the new function's count is
exactly what produced data->irq_count, so it never fires for a device
that behaves.

Placing the check before rmi_alloc_function() is deliberate.  Since
commit 58d42ec10b73 ("Input: rmi4 - refactor function allocation and
registration") that function has already run device_initialize() and
dev_set_name() on fn->dev, so disposing of a rejected function with
kfree() would leak the name string and skip kobject cleanup on those
trees, while put_device() on the same path broke on the pre-refactor
trees that stable backports target: a kobject that device_initialize()
never touched warns and its saturated refcount keeps the object
allocated for good.  Rejecting before the allocation needs no cleanup
on any tree.

Reproduced with an emulated RMI4 device driven over /dev/uhid, and again
over dummy_hcd plus raw-gadget, on v6.12.69 booted slub_debug=FZPU and on
v6.12.105 built with CONFIG_KASAN=y.  After this change the same device
gets

  rmi4_physical rmi4-03: F40: interrupt count changed between PDT
      scans (pos 1 + 6 > 1)
  rmi4_physical rmi4-03: Function creation failed with code -22.

and a device that answers both scans consistently still probes normally.

Fixes: 2b6a321da9a2 ("Input: synaptics-rmi4 - add support for Synaptics RMI4 devices")
Cc: stable@vger.kernel.org
Signed-off-by: Wei Jie Law <98lawweijie@gmail.com>
---
Changes in v3:
 - Validate before rmi_alloc_function() instead of freeing after it; no
   disposal is needed on any tree this way.  See the cover letter.

Changes in v2:
 - Dispose of the rejected function with kfree() instead of
   put_device().  See the cover letter.

 drivers/input/rmi4/rmi_driver.c | 19 +++++++++++++++++++
 1 file changed, 19 insertions(+)

diff --git a/drivers/input/rmi4/rmi_driver.c b/drivers/input/rmi4/rmi_driver.c
index 5d49a9021c7d..f66be55677a9 100644
--- a/drivers/input/rmi4/rmi_driver.c
+++ b/drivers/input/rmi4/rmi_driver.c
@@ -885,6 +885,25 @@ static int rmi_create_function(struct rmi_device *rmi_dev,
 	rmi_dbg(RMI_DEBUG_CORE, dev, "Initializing F%02X.\n",
 			pdt->function_number);
 
+	/*
+	 * irq_mask[] was sized from the interrupt count collected by the
+	 * earlier rmi_count_irqs() scan of the PDT, and irq[] holds
+	 * RMI_FN_MAX_IRQS entries.  Nothing guarantees that this scan sees
+	 * the same table -- the PDT is read back from the device every
+	 * time -- so a device that grows its interrupt counts between the
+	 * two scans would push the set_bit() calls below past the end of
+	 * the flexible array.  Refuse the function instead, before anything
+	 * is allocated for it.
+	 */
+	if (pdt->interrupt_source_count > RMI_FN_MAX_IRQS ||
+	    *current_irq_count + pdt->interrupt_source_count > data->irq_count) {
+		dev_err(dev,
+			"F%02X: interrupt count changed between PDT scans (pos %u + %u > %d)\n",
+			pdt->function_number, *current_irq_count,
+			pdt->interrupt_source_count, data->irq_count);
+		return -EINVAL;
+	}
+
 	fn = rmi_alloc_function(rmi_dev, pdt->function_number);
 	if (!fn) {
 		dev_err(dev, "Failed to allocate memory for F%02X\n",
-- 
2.43.0

Re: [PATCH v3 2/2] Input: synaptics-rmi4 - reject a PDT that grows between scans

[sashiko-bot]

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

Pre-existing issues:
- [High] If rmi_f34_create_sysfs() fails during probe, the error path skips function cleanup, leading to a use-after-free of the driver's private data and leaking registered devices.
- [High] Concurrent sysfs writes to the `update_fw` attribute execute firmware updates without serialization, leading to list corruption, data races, and potential double-frees.
--

--- Patch [2]: [PATCH v3 2/2] Input: synaptics-rmi4 - reject a PDT that grows between scans ---
commit 2fad09409bb9588b1763723ac01bfc86aa11a46f
Author: Wei Jie Law <98lawweijie@gmail.com>

Input: synaptics-rmi4 - reject a PDT that grows between scans

This commit validates the interrupt counts gathered during Page Description
Table (PDT) scans to ensure they haven't grown between consecutive reads from
the device. By checking the counts before allocating the function, it prevents
an out-of-bounds write in the flexible array at the end of struct rmi_function.

> diff --git a/drivers/input/rmi4/rmi_driver.c b/drivers/input/rmi4/rmi_driver.c
> index 5d49a9021c7d0..f66be55677a94 100644
> --- a/drivers/input/rmi4/rmi_driver.c
> +++ b/drivers/input/rmi4/rmi_driver.c

[ ... ]

While reviewing this code, I noticed a couple of pre-existing issues in the
surrounding functions.

In drivers/input/rmi4/rmi_driver.c:rmi_driver_probe():

	retval = rmi_init_functions(data);
	if (retval)
		goto err;

	retval = rmi_f34_create_sysfs(rmi_dev);
	if (retval)
		goto err;

[Severity: High]
This isn't a bug introduced by this patch, but if rmi_f34_create_sysfs() fails
during probe, the error path jumps to 'err', which bypasses
err_destroy_functions.

Does this skip rmi_free_function_list() and leave the registered rmi_function
child devices active? Because rmi_driver_probe() returns an error, the driver
core cleans up the devm-managed struct rmi_driver_data. Any subsequent access
by the still-active child devices could dereference the freed driver data,
leading to a use-after-free.

Should this error path use goto err_destroy_functions instead?


In drivers/input/rmi4/rmi_f34.c:rmi_driver_update_fw_store():

	error = rmi_firmware_update(data, fw);
	if (error)
		return error;

[Severity: High]
This is a pre-existing issue, but does this sysfs attribute lack serialization?

If multiple threads write to the update_fw sysfs file concurrently, they will
execute rmi_firmware_update() simultaneously:

drivers/input/rmi4/rmi_f34.c:rmi_firmware_update() {
	...
	/* Tear down functions and re-probe */
	rmi_free_function_list(rmi_dev);
	...
}

Because rmi_free_function_list() modifies the global data->function_list without
any synchronization, won't concurrent execution corrupt the linked list and lead
to double-frees?

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

Re: [PATCH v3 1/2] Input: synaptics-rmi4 - fix irq[] overrun with 7 interrupt sources

[sashiko-bot]

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

Pre-existing issues:
- [High] A malicious device reporting an interrupt source count of 0 can trigger an out-of-bounds memory write during function initialization in F11 and F12 functions.
--

--- Patch [1]: [PATCH v3 1/2] Input: synaptics-rmi4 - fix irq[] overrun with 7 interrupt sources ---
commit cc9ceca86b18ef61f6b6360e9c31ded31f4d76e9
Author: Wei Jie Law <98lawweijie@gmail.com>

Input: synaptics-rmi4 - fix irq[] overrun with 7 interrupt sources

This commit addresses an array-index-out-of-bounds issue when a device
declares 7 interrupt sources. It increases RMI_FN_MAX_IRQS to 7 to match
the three-bit interrupt source count field in the function descriptor.

> diff --git a/drivers/input/rmi4/rmi_bus.h b/drivers/input/rmi4/rmi_bus.h
> index 90122df21f746..faf2ebb00d521 100644
> --- a/drivers/input/rmi4/rmi_bus.h
> +++ b/drivers/input/rmi4/rmi_bus.h
> @@ -12,10 +12,13 @@
>  struct rmi_device;
>  
>  /*
> - * The interrupt source count in the function descriptor can represent up to
> - * 6 interrupt sources in the normal manner.
> + * The interrupt source count in the function descriptor is a three bit field
> + * (RMI_PDT_INT_SOURCE_COUNT_MASK), so a device can legitimately declare up to
> + * 7 interrupt sources for a single function.  irq[] must be able to hold all
> + * of them: rmi_create_function_irq() and rmi_unregister_function() both walk
> + * it up to fn->num_of_irqs.
>   */
> -#define RMI_FN_MAX_IRQS	6
> +#define RMI_FN_MAX_IRQS	7

[Severity: High]
This is a pre-existing issue, but since we're looking at edge cases for
interrupt source counts, can a malicious or emulated device trigger an
out-of-bounds heap write if it advertises an interrupt_source_count of 0?

In drivers/input/rmi4/rmi_f11.c:rmi_f11_initialize():

    mask_size = BITS_TO_LONGS(drvdata->irq_count) * sizeof(unsigned long);
    f11 = devm_kzalloc(&fn->dev,
                       sizeof(struct f11_data) + mask_size * 2,
                       GFP_KERNEL);
    // ...
    set_bit(fn->irq_pos, f11->abs_mask);
    set_bit(fn->irq_pos + 1, f11->rel_mask);

If drvdata->irq_count is 0, mask_size evaluates to 0, leaving 0 extra
bytes allocated for the masks. However, set_bit() is called unconditionally,
which would write past the end of the f11 allocation.

A similar situation appears to exist in
drivers/input/rmi4/rmi_f12.c:rmi_f12_probe():

    irq_mask_size = BITS_TO_LONGS(drvdata->irq_count);
    // ...
    f12 = devm_kzalloc(&fn->dev,
                       struct_size(f12, irq_mask, irq_mask_size * 2),
                       GFP_KERNEL);
    // ...
    set_bit(fn->irq_pos, f12->irq_mask);
    set_bit(fn->irq_pos + 1, f12->irq_mask + irq_mask_size);

If drvdata->irq_count is 0, the flexible array irq_mask is 0-sized, yet
set_bit() is still executed. Should these functions validate that irq_count
is greater than 0 before allocating and setting bits?

>  
>  /**
>   * struct rmi_function - represents the implementation of an RMI4

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

Re: [PATCH v3 1/2] Input: synaptics-rmi4 - fix irq[] overrun with 7 interrupt sources

[Wei Jie LAW]

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

Superseded by the v4 series:
https://lore.kernel.org/linux-input/20260825103123.12216-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 these bugs and I should have
acknowledged that in the earlier postings.  Neither diff is changed.

Please mark this one superseded.

Thanks, and sorry for the noise,
Wei Jie

Re: [PATCH v3 2/2] Input: synaptics-rmi4 - reject a PDT that grows between scans

[Wei Jie LAW]

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

Superseded by the v4 series:
https://lore.kernel.org/linux-input/20260825103123.12216-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 these bugs and I should have
acknowledged that in the earlier postings.  Neither diff is changed.

Please mark this one superseded.

Thanks, and sorry for the noise,
Wei Jie