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

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

[Wei Jie Law]

Two fixes for out-of-bounds writes in the RMI4 core that a device can
trigger through its Page Description Table.

1/2 rejects functions declaring 7 interrupt sources, a value the RMI4
specification reserves; the driver otherwise writes past fn->irq[].

2/2 rejects a PDT whose interrupt counts grow between the counting scan
and the function-creating scan; the driver otherwise sets bits past the
end of fn->irq_mask[].

Changes in v5:
 - 1/2: keep RMI_FN_MAX_IRQS at 6 and reject a count of 7 in
   rmi_scan_pdt_page(), after the RMI4_END_OF_PDT() check, instead of
   growing irq[] (Dmitry).  New subject.
 - 2/2: unchanged.

v4: https://lore.kernel.org/linux-input/20260825103123.12216-1-98lawweijie@gmail.com/

Wei Jie Law (2):
  Input: synaptics-rmi4 - reject functions with more than 6 interrupt
    sources
  Input: synaptics-rmi4 - reject a PDT that grows between scans

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

-- 
2.43.0

[PATCH v5 1/2] Input: synaptics-rmi4 - reject functions with more than 6 interrupt sources

[Wei Jie Law]

The interrupt source count in a PDT entry is a 3-bit field, but the RMI4
specification only uses values 0-6 as a count; 7 is reserved to mean
"more than 6 interrupt sources", which the driver does not support.  The
driver takes the value as is, and a function claiming 7 sources makes
rmi_create_function_irq() and rmi_unregister_function() access
fn->irq[6], one past the end of the array, clobbering fn->irq_pos:

  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]'

Reject such entries in rmi_scan_pdt_page().  The check has to come after
the RMI4_END_OF_PDT() test: an unpopulated entry reads as 0xff, which
also decodes to 7 interrupt sources, and must still terminate the scan.

Fixes: 24d28e4f1271 ("Input: synaptics-rmi4 - convert irq distribution to irq_domain")
Cc: stable@vger.kernel.org
Suggested-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
Assisted-by: LLM
Signed-off-by: Wei Jie Law <98lawweijie@gmail.com>
---
 drivers/input/rmi4/rmi_driver.c | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/drivers/input/rmi4/rmi_driver.c b/drivers/input/rmi4/rmi_driver.c
index 5d49a9021c7d..443ba4106352 100644
--- a/drivers/input/rmi4/rmi_driver.c
+++ b/drivers/input/rmi4/rmi_driver.c
@@ -526,6 +526,19 @@ static int rmi_scan_pdt_page(struct rmi_device *rmi_dev,
 		if (RMI4_END_OF_PDT(pdt_entry.function_number))
 			break;
 
+		/*
+		 * A count of 7 means "more than 6 interrupt sources", which is
+		 * not supported.  Checked after RMI4_END_OF_PDT() because an
+		 * unpopulated (0xff) entry also decodes to 7.
+		 */
+		if (pdt_entry.interrupt_source_count > RMI_FN_MAX_IRQS) {
+			dev_err(&rmi_dev->dev,
+				"F%02X: invalid interrupt source count %u\n",
+				pdt_entry.function_number,
+				pdt_entry.interrupt_source_count);
+			return -EINVAL;
+		}
+
 		retval = callback(rmi_dev, ctx, &pdt_entry);
 		if (retval != RMI_SCAN_CONTINUE)
 			return retval;
-- 
2.43.0

[PATCH v5 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 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
Assisted-by: LLM
Signed-off-by: Wei Jie Law <98lawweijie@gmail.com>
---
 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 443ba4106352..b2ed5991b197 100644
--- a/drivers/input/rmi4/rmi_driver.c
+++ b/drivers/input/rmi4/rmi_driver.c
@@ -898,6 +898,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 v5 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:
- [Medium] input: synaptics-rmi4: multiple resource leaks in rmi_driver_probe error paths
--

commit 81cc67e5a61f0cfdb2ce1738f5778c94caef2b1c
Author: Wei Jie Law <98lawweijie@gmail.com>

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

This commit introduces a validation check in rmi_create_function() to prevent
out-of-bounds writes if a device's interrupt count grows between Page
Description Table scans. It fails the probe by returning -EINVAL before
allocating memory for the invalid function.

> diff --git a/drivers/input/rmi4/rmi_driver.c b/drivers/input/rmi4/rmi_driver.c
> index 443ba41063524..b2ed5991b1970 100644
> --- a/drivers/input/rmi4/rmi_driver.c
> +++ b/drivers/input/rmi4/rmi_driver.c
> @@ -898,6 +898,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;

[Severity: Medium]
Returning -EINVAL here aborts the device probe and exposes regressions where
the IRQ domain, function devices, and sysfs nodes are not properly freed in
the error handling paths of rmi_driver_probe() and its helpers.

First, does this error path permanently leak the IRQ domain? The domain is
allocated without devm in rmi_probe_interrupts():

drivers/input/rmi4/rmi_driver.c:rmi_probe_interrupts() {
	...
	data->irqdomain = irq_domain_create_linear(fwnode, irq_count,
						   &irq_domain_simple_ops,
						   data);
	...
	data->irq_memory = devm_kcalloc(dev, size, 4, GFP_KERNEL);
	if (!data->irq_memory) {
		dev_err(dev, "Failed to allocate memory for irq masks.\n");
		return -ENOMEM;
	}
	...
}

If devm_kcalloc() fails, it returns without calling irq_domain_remove().
Similarly, if rmi_create_function() returns -EINVAL (the new error path
added by this patch), the teardown block in rmi_driver_probe() fails to remove
the domain:

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

Second, does jumping directly to the err label in rmi_driver_probe() leak
the RMI function devices?

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

If rmi_f34_create_sysfs() fails, the jump to the err label skips the
err_destroy_functions label and rmi_free_function_list(rmi_dev).

Finally, does the err_destroy_functions teardown block leave orphaned sysfs
nodes?

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

If rmi_irq_init() or input_register_device() fails, the jump to
err_destroy_functions does not call rmi_f34_remove_sysfs(rmi_dev),
leaking the sysfs entries created earlier.

> +	}
> +
>  	fn = rmi_alloc_function(rmi_dev, pdt->function_number);
>  	if (!fn) {
>  		dev_err(dev, "Failed to allocate memory for F%02X\n",

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