Re: [PATCH v3 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 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