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

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

[Wei Jie Law]

Hi Dmitry,

While putting together a reproducer for an out-of-bounds bug in hid-rmi
(v3 posted separately, [1]) 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.

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 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/

[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 | 18 ++++++++++++++++++
 2 files changed, 24 insertions(+), 3 deletions(-)

-- 
2.43.0

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

[Wei Jie Law]

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

Re: [PATCH v2 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] Missing error check for `irq_create_mapping()` return value.
--

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

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

The commit fixes an out-of-bounds array access in the rmi4 driver by
increasing RMI_FN_MAX_IRQS from 6 to 7. A device can legitimately declare
up to 7 interrupt sources, causing the driver to overwrite memory when
indexing the irq array.

> 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 @@

[ ... ]

> -#define RMI_FN_MAX_IRQS	6
> +#define RMI_FN_MAX_IRQS	7

[Severity: High]
This isn't a bug introduced by this patch, but I noticed a potential problem
in how the interrupts are mapped in rmi_create_function_irq().

drivers/input/rmi4/rmi_bus.c:rmi_create_function_irq() {
        ...
        fn->irq[i] = irq_create_mapping(drvdata->irqdomain,
                                        fn->irq_pos + i);

        irq_set_chip_data(fn->irq[i], fn);
        irq_set_chip_and_handler(fn->irq[i], &rmi_irq_chip,
                                 handle_simple_irq);
        ...
}

Does this code need to check if irq_create_mapping() returns 0?

If memory allocation fails or the IRQ domain is exhausted during device probe,
it seems we might pass 0 directly to irq_set_chip_data() and
irq_set_chip_and_handler().

On architectures where IRQ 0 is a valid and critical system interrupt, like
the x86 system timer, could overwriting the descriptor cause system
instability?

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

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

[Wei Jie Law]

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 position before using it and fail the probe instead.  The
check is exact, not conservative: when both scans see the same table,
fn->irq_pos + fn->num_of_irqs is the running total that produced
data->irq_count, so it never fires for a device that behaves.

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
The rejection happens before rmi_register_function(), i.e. before
device_initialize() has touched fn->dev, so the partially built function
is disposed of with kfree(): put_device() on a kobject that was never
initialised warns and then leaves the object behind, because the
saturated refcount stops the release from ever running.

Signed-off-by: Wei Jie Law <98lawweijie@gmail.com>
---
Changes in v2:
 - Dispose of the rejected function with kfree() instead of
   put_device().  See the cover letter.

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

diff --git a/drivers/input/rmi4/rmi_driver.c b/drivers/input/rmi4/rmi_driver.c
index 5d49a9021c7d..8dcf088a7a81 100644
--- a/drivers/input/rmi4/rmi_driver.c
+++ b/drivers/input/rmi4/rmi_driver.c
@@ -899,6 +899,24 @@ static int rmi_create_function(struct rmi_device *rmi_dev,
 	fn->irq_pos = *current_irq_count;
 	*current_irq_count += fn->num_of_irqs;
 
+	/*
+	 * irq_mask[] was sized from the interrupt count collected by the
+	 * earlier rmi_count_irqs() scan of the PDT.  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 these set_bit() calls past the end
+	 * of the flexible array.  Refuse the function instead.
+	 */
+	if (fn->num_of_irqs > RMI_FN_MAX_IRQS ||
+	    fn->irq_pos + fn->num_of_irqs > data->irq_count) {
+		dev_err(dev,
+			"F%02X: interrupt count changed between PDT scans (pos %u + %u > %d)\n",
+			pdt->function_number, fn->irq_pos, fn->num_of_irqs,
+			data->irq_count);
+		kfree(fn);
+		return -EINVAL;
+	}
+
 	for (i = 0; i < fn->num_of_irqs; i++)
 		set_bit(fn->irq_pos + i, fn->irq_mask);
 
-- 
2.43.0

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

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Memory leak and kobject lifecycle violation on the error path in `rmi_create_function()`.
--

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

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

This commit adds a validation check in rmi_create_function() to reject
devices whose interrupt counts grow between PDT scans, preventing an
out-of-bounds bitmap write. When the check fails, it frees the function
using kfree().

> The rejection happens before rmi_register_function(), i.e. before
> device_initialize() has touched fn->dev, so the partially built function
> is disposed of with kfree(): put_device() on a kobject that was never
> initialised warns and then leaves the object behind, because the
> saturated refcount stops the release from ever running.

[Severity: High]
Is this assumption accurate? Looking at rmi_alloc_function() which is called
right at the beginning of rmi_create_function(), it appears
device_initialize() is actually called during allocation:

drivers/input/rmi4/rmi_bus.c:rmi_alloc_function() {
    ...
    device_initialize(&fn->dev);
    dev_set_name(&fn->dev, "%s.fn%02x", dev_name(&rmi_dev->dev), id);
    ...
}

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

[ ... ]

> @@ -899,6 +899,24 @@ static int rmi_create_function(struct rmi_device *rmi_dev,
>  	fn->irq_pos = *current_irq_count;
>  	*current_irq_count += fn->num_of_irqs;
>  
> +	/*
> +	 * irq_mask[] was sized from the interrupt count collected by the
> +	 * earlier rmi_count_irqs() scan of the PDT.  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 these set_bit() calls past the end
> +	 * of the flexible array.  Refuse the function instead.
> +	 */
> +	if (fn->num_of_irqs > RMI_FN_MAX_IRQS ||
> +	    fn->irq_pos + fn->num_of_irqs > data->irq_count) {
> +		dev_err(dev,
> +			"F%02X: interrupt count changed between PDT scans (pos %u + %u > %d)\n",
> +			pdt->function_number, fn->irq_pos, fn->num_of_irqs,
> +			data->irq_count);
> +		kfree(fn);

[Severity: High]
Does this kfree() bypass the kobject lifecycle rules and cause a memory leak?

Since rmi_alloc_function() already initialized the kobject and dynamically
allocated the device name string via dev_set_name(), shouldn't we use
put_device(&fn->dev) here instead of kfree()?

Calling kfree() directly skips kobject_cleanup(), which permanently leaks
the heap-allocated name string and violates the driver core requirement that
initialized kobjects must be freed via their release method.

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

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