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

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] This problem wasn't introduced by this patch, but the IRQ domain allocated in `rmi_probe_interrupts()` is leaked on probe error paths and during firmware updates, leading to a Use-After-Free.
- [High] This problem wasn't introduced by this patch, but if `rmi_f34_create_sysfs()` fails during probe, the error path bypasses RMI function cleanup, leading to leaked child devices and Use-After-Free.
- [High] This problem wasn't introduced by this patch, but missing sysfs cleanup on subsequent probe failures leaves dangling attributes that access freed memory.
--

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

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

rmi_driver_probe() walks the Page Description Table three times, and each
walk reads the table back from the device. Validate the counts before the 
function is allocated and fail the probe instead.

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

[Severity: High]
This isn't a bug introduced by this patch, but the IRQ domain allocated in
rmi_probe_interrupts() is leaked on probe error paths and during firmware
updates, leading to a use-after-free.

Can this sequence actually occur? Looking at rmi_driver_probe() in
rmi_driver.c, if any step after rmi_probe_interrupts() fails, we jump
to err, but the irqdomain is never removed:

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

Since the irqdomain's host_data points to data which is devm-managed,
data will be freed on probe failure, leaving a dangling pointer in the
globally registered irqdomain.

Additionally, this happens during firmware updates:

drivers/input/rmi4/rmi_f34.c:rmi_firmware_update() {
...
	rmi_free_function_list(rmi_dev);

	ret = rmi_probe_interrupts(data);
...
}

Will this overwrite the irqdomain during reprobing without removing the
old one, leaking it again?


[Severity: High]
This isn't a bug introduced by this patch, but if rmi_f34_create_sysfs()
fails during probe, the error path bypasses RMI function cleanup, leading
to leaked child devices and a use-after-free.

Looking at rmi_driver_probe() again:

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 executes goto err, bypassing
err_destroy_functions. 

Since child RMI function devices are manually registered via device_add
and are not devres-managed, won't bypassing their cleanup leave them
permanently in the system? 

When probe aborts, devres frees data. Could interacting with the orphaned
child devices then cause a use-after-free?


[Severity: High]
This isn't a bug introduced by this patch, but missing sysfs cleanup on
subsequent probe failures leaves dangling attributes that access freed
memory.

Looking at rmi_driver_probe():

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

	if (data->input) {
...
			retval = input_register_device(data->input);
			if (retval) {
...
				goto err_destroy_functions;
			}
...
err_destroy_functions:
	rmi_free_function_list(rmi_dev);
err:
	return retval;
}

If a step fails after rmi_f34_create_sysfs() (like input_register_device()),
the code jumps to err_destroy_functions, which does not call
rmi_f34_remove_sysfs(). 

Since data is freed by devres on probe failure, won't userspace accessing
these dangling sysfs files dereference the freed data pointer?

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