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

[PATCH 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
(v2 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.

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.

[1] https://patchwork.kernel.org/project/linux-input/patch/00a489f38b240624dcb5a4bae36a53fcba9cfb47.1787549195.git.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 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

[PATCH 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
Signed-off-by: Wei Jie Law <98lawweijie@gmail.com>
---
 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..c0e91a372eff 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);
+		put_device(&fn->dev);
+		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 2/2] Input: synaptics-rmi4 - reject a PDT that grows between scans

[sashiko-bot]

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

Pre-existing issues:
- [High] The irq_domain allocated by irq_domain_create_linear() is leaked on error paths during probe and firmware updates.
- [High] The RMI firmware sysfs group created by rmi_f34_create_sysfs() is leaked on error paths during probe, leading to a NULL pointer dereference.
- [High] Failure in rmi_f34_create_sysfs during probe skips child device cleanup, leading to resource leaks and NULL pointer dereferences.
--

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

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

This patch fixes an out-of-bounds access in rmi_create_function(). It adds
a validation check for fn->irq_pos + fn->num_of_irqs to ensure that a device
that grows its interrupt count between scans cannot walk past the end of the
flexible array. The probe will now fail instead of overflowing memory.

> diff --git a/drivers/input/rmi4/rmi_driver.c b/drivers/input/rmi4/rmi_driver.c
> index 5d49a9021c7d0..c0e91a372eff4 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);
> +		put_device(&fn->dev);
> +		return -EINVAL;
> +	}
> +
>  	for (i = 0; i < fn->num_of_irqs; i++)
>  		set_bit(fn->irq_pos + i, fn->irq_mask);
>  

[Severity: High]
This isn't a bug introduced by this patch, but does rmi_driver_probe() leak
the irqdomain allocated in rmi_probe_interrupts() on failure paths?

drivers/input/rmi4/rmi_driver.c:rmi_driver_probe() {
...
	retval = rmi_probe_interrupts(data);
	if (retval)
		goto err;
...
err_destroy_functions:
	rmi_free_function_list(rmi_dev);
err:
	return retval;
}

If any subsequent initialization step in rmi_driver_probe() fails, the error
path bypasses irq_domain_remove(), permanently leaking the domain. 

Does this leak also occur during firmware updates when rmi_probe_interrupts()
is called repeatedly without cleaning up the previous irqdomain?

drivers/input/rmi4/rmi_f34.c:rmi_firmware_update() {
...
	ret = rmi_probe_interrupts(data);
	if (ret)
		return ret;
...
}

[Severity: High]
This is a pre-existing issue, but does the error path in rmi_driver_probe()
leave the f34 sysfs group active, potentially leading to a null pointer
dereference?

drivers/input/rmi4/rmi_driver.c:rmi_driver_probe() {
...
	retval = rmi_f34_create_sysfs(rmi_dev);
	if (retval)
		goto err;

	if (data->input) {
		rmi_driver_set_input_name(rmi_dev, data->input);
		if (!rmi_dev->xport->input) {
			retval = input_register_device(data->input);
			if (retval) {
				dev_err(dev, "%s: Failed to register input device.\n",
					__func__);
				goto err_destroy_functions;
			}
...
err_destroy_functions:
	rmi_free_function_list(rmi_dev);
err:
	return retval;
}

If input_register_device() fails, the code jumps to err_destroy_functions,
which cleans up the function list but neglects to call
rmi_f34_remove_sysfs().

When the probe fails, the driver core sets the device's drvdata to null. If
userspace accesses these stale files, callbacks will call dev_get_drvdata()
and dereference a null pointer.

[Severity: High]
This isn't a bug introduced by this patch, but does a failure in
rmi_f34_create_sysfs() bypass the cleanup for child functions?

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;
...
err_destroy_functions:
	rmi_free_function_list(rmi_dev);
err:
	return retval;
}

If rmi_f34_create_sysfs() fails, the code jumps directly to the err label,
completely skipping the err_destroy_functions cleanup for the already
registered child functions.

When probe fails, the driver core clears drvdata and devres frees data,
leaving the child devices active and causing a null pointer dereference if
accessed from userspace.

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

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

[sashiko-bot]

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

Pre-existing issues:
- [High] The function devices created during probe are leaked and can trigger a Use-After-Free if `rmi_f34_create_sysfs()` fails.
- [High] Failure of `irq_create_mapping()` returns 0, which is unchecked and passed directly to IRQ manipulation functions, hijacking or corrupting system IRQ 0.
- [Medium] The IRQ domain created during probe is leaked on almost all error paths in `rmi_driver_probe()`.
--

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

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

This commit fixes an out-of-bounds array access by expanding the irq array
size from 6 to 7 to match the maximum interrupt source count specified by the
RMI4 Page Description Table entry.

> 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
>  
>  /**
>   * struct rmi_function - represents the implementation of an RMI4

[Severity: High]
This isn't a bug introduced by this patch, but I noticed a potential
use-after-free and memory leak in the error handling of rmi_driver_probe():

drivers/input/rmi4/rmi_driver.c:rmi_driver_probe() {
    ...
	retval = rmi_f34_create_sysfs(rmi_dev);
	if (retval)
		goto err;
    ...
err_destroy_functions:
	rmi_free_function_list(rmi_dev);
err:
	return retval;
}

If rmi_f34_create_sysfs() fails, the code jumps to the err label, skipping
the call to rmi_free_function_list(rmi_dev). Doesn't this leave the function
devices orphaned on the RMI bus with a soon-to-be-freed parent drvdata,
leading to a use-after-free if a driver later attempts to bind to them?

[Severity: Medium]
This isn't a bug introduced by this patch, but I noticed the IRQ domain might
be leaking on probe error paths:

drivers/input/rmi4/rmi_driver.c:rmi_driver_probe() {
    ...
	retval = rmi_probe_interrupts(data);
	if (retval)
		goto err;
    ...
err_destroy_functions:
	rmi_free_function_list(rmi_dev);
err:
	return retval;
}

The rmi_probe_interrupts() function allocates a linear IRQ domain using
irq_domain_create_linear(), which is not managed by devres. If any subsequent
initialization step fails, the probe function returns without calling
irq_domain_remove(). Is it expected that the IRQ domain is leaked here?

[Severity: High]
This isn't a bug introduced by this patch, but I noticed a potential problem
in how mapped IRQs are handled:

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);
    ...
}

If irq_create_mapping() fails due to memory or descriptor exhaustion, it
returns 0. Is it safe to pass 0 directly to irq_set_chip_data() and
irq_set_chip_and_handler() without checking the return value? Could this
overwrite the chip and handler for system IRQ 0?

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

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

[Wei Jie Law]

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

This patch is unchanged in v2 -- the series was resent because of 2/2,
which disposed of a rejected struct rmi_function with put_device() at a
point where device_initialize() had not run yet.

Please mark this one superseded.

Thanks,
Wei Jie

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

[Wei Jie Law]

Superseded by v2 2/2:
https://lore.kernel.org/linux-input/20260824122733.76321-3-98lawweijie@gmail.com/

Please do not apply this version.  The rejection path it adds calls
put_device(&fn->dev) before rmi_register_function(), which is where
device_initialize() happens, so fn->dev is still all zeroes at that
point.  kobject_put() then warns that the kobject was never initialised,
refcount_dec_and_test() warns about the underflow, and because the
saturated refcount makes kref_put() return false the release never runs
-- so the function is leaked on every rejection, i.e. on exactly the
malicious device this patch is meant to reject:

  rmi4_physical rmi4-00: F40: interrupt count changed between PDT scans (pos 1 + 6 > 1)
  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

v2 uses kfree() instead.

Please mark this one superseded.

Thanks,
Wei Jie