[PATCH] Input: synaptics-rmi4 - reject concurrent firmware updates

[PATCH] Input: synaptics-rmi4 - reject concurrent firmware updates

[Shuangpeng Bai]

The update_fw sysfs store callback can execute concurrently. Each
invocation runs rmi_firmware_update(), which tears down and rebuilds the
RMI function list.

If two writes overlap, both rmi_free_function_list() calls can walk the
same list and select the same rmi_function entry. One invocation can delete
and unregister the entry while the other still uses its iterator's fn
pointer, resulting in use-after-free or list corruption. This was
reproduced as list_del corruption in rmi_free_function_list().

Firmware flashing is an exclusive operation, and running a second update
concurrently has no useful semantics. Add a per-device update_mutex and
try to acquire it immediately before starting the update. Return -EBUSY if
another update is already active, and hold the mutex until the update has
completed so their function-list teardown cannot overlap.

The status attribute does not acquire update_mutex, so update_fw_status can
still be polled while an update is running.

Fixes: 29fd0ec2bdbe ("Input: synaptics-rmi4 - add support for F34 device reflash")
Cc: stable@vger.kernel.org
Signed-off-by: Shuangpeng Bai <shuangpeng.kernel@gmail.com>
---
 drivers/input/rmi4/rmi_driver.c | 1 +
 drivers/input/rmi4/rmi_f34.c    | 8 +++++---
 include/linux/rmi.h             | 1 +
 3 files changed, 7 insertions(+), 3 deletions(-)

diff --git a/drivers/input/rmi4/rmi_driver.c b/drivers/input/rmi4/rmi_driver.c
index 5d49a9021c7d..3a5cb17e938f 100644
--- a/drivers/input/rmi4/rmi_driver.c
+++ b/drivers/input/rmi4/rmi_driver.c
@@ -1218,6 +1218,7 @@ static int rmi_driver_probe(struct device *dev)
 
 	mutex_init(&data->irq_mutex);
 	mutex_init(&data->enabled_mutex);
+	mutex_init(&data->update_mutex);
 
 	retval = rmi_probe_interrupts(data);
 	if (retval)
diff --git a/drivers/input/rmi4/rmi_f34.c b/drivers/input/rmi4/rmi_f34.c
index f1947f03b06a..04f12165cab0 100644
--- a/drivers/input/rmi4/rmi_f34.c
+++ b/drivers/input/rmi4/rmi_f34.c
@@ -468,13 +468,15 @@ static ssize_t rmi_driver_update_fw_store(struct device *dev,
 	if (error)
 		return error;
 
+	if (!mutex_trylock(&data->update_mutex))
+		return -EBUSY;
+
 	dev_info(dev, "Flashing %s\n", fw_name);
 
 	error = rmi_firmware_update(data, fw);
-	if (error)
-		return error;
 
-	return count;
+	mutex_unlock(&data->update_mutex);
+	return error ?: count;
 }
 
 static DEVICE_ATTR(update_fw, 0200, NULL, rmi_driver_update_fw_store);
diff --git a/include/linux/rmi.h b/include/linux/rmi.h
index ab7eea01ab42..d56c23240868 100644
--- a/include/linux/rmi.h
+++ b/include/linux/rmi.h
@@ -340,6 +340,7 @@ struct rmi_driver_data {
 
 	struct rmi_function *f01_container;
 	struct rmi_function *f34_container;
+	struct mutex update_mutex;
 	bool bootloader_mode;
 
 	int num_of_irq_regs;
-- 
2.43.0

Re: [PATCH] Input: synaptics-rmi4 - reject concurrent firmware updates

[sashiko-bot]

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

Pre-existing issues:
- [High] Lockless polling of sysfs attributes (e.g., `update_fw_status`) concurrently with a firmware update leads to Use-After-Free and NULL pointer dereferences.
- [High] Missing NULL check for `f34` driver data during re-probe in `rmi_firmware_update()` leads to kernel panic.
- [High] Firmware updates unconditionally re-enable IRQs upon completion, bypassing driver teardown intent and exposing destroyed data structures to the IRQ handler.
- [High] If `rmi_driver_probe` fails after creating sysfs attributes, the sysfs group is not removed, leading to a zombie sysfs interface.
- [Medium] Repeated firmware updates permanently leak `struct irq_domain` memory.
- [Medium] The final firmware update status is immediately lost upon failure, causing userspace to falsely read success.
--

--- Patch [1]: [PATCH] Input: synaptics-rmi4 - reject concurrent firmware updates ---
commit 9b19909f9bcb9b5bdbd8eed13e175804af90a281
Author: Shuangpeng Bai <shuangpeng.kernel@gmail.com>

Input: synaptics-rmi4 - reject concurrent firmware updates

This patch introduces a mutex to reject concurrent firmware updates. The
update_mutex is added to prevent multiple overlapping calls to
rmi_firmware_update() from tearing down and rebuilding the RMI function
list simultaneously.

> diff --git a/drivers/input/rmi4/rmi_driver.c b/drivers/input/rmi4/rmi_driver.c
> index 5d49a9021c7d0..3a5cb17e938fe 100644
> --- a/drivers/input/rmi4/rmi_driver.c
> +++ b/drivers/input/rmi4/rmi_driver.c
> @@ -1218,6 +1218,7 @@ static int rmi_driver_probe(struct device *dev)
>  
>  	mutex_init(&data->irq_mutex);
>  	mutex_init(&data->enabled_mutex);
> +	mutex_init(&data->update_mutex);
>  
>  	retval = rmi_probe_interrupts(data);
>  	if (retval)

[Severity: High]
This is a pre-existing issue, but if rmi_driver_probe() fails later in the
function (for example, in rmi_irq_init), does the error path leak the sysfs
group?

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

The error path doesn't seem to call rmi_f34_remove_sysfs(), which leaves
active world-readable sysfs attributes on the device while the devres
framework frees the driver data. Can this lead to a use-after-free when
reading those attributes?

[Severity: Medium]
This isn't a bug introduced by this patch, but does rmi_probe_interrupts()
leak the struct irq_domain memory when called repeatedly?

During rmi_firmware_update(), rmi_probe_interrupts() is called multiple
times to transition into and out of bootloader mode:

drivers/input/rmi4/rmi_driver.c:rmi_probe_interrupts() {
    ...
	data->irqdomain = irq_domain_create_linear(fwnode, irq_count,
						   &irq_domain_simple_ops,
						   data);
    ...
}

It seems irq_domain_create_linear() is called unconditionally, overwriting
data->irqdomain without removing the previous one. Does this cause a
permanent memory leak of the irqdomain during firmware updates?

[ ... ]

> diff --git a/drivers/input/rmi4/rmi_f34.c b/drivers/input/rmi4/rmi_f34.c
> index f1947f03b06af..04f12165cab05 100644
> --- a/drivers/input/rmi4/rmi_f34.c
> +++ b/drivers/input/rmi4/rmi_f34.c
> @@ -468,13 +468,15 @@ static ssize_t rmi_driver_update_fw_store(struct device *dev,
>  	if (error)
>  		return error;
>  
> +	if (!mutex_trylock(&data->update_mutex))
> +		return -EBUSY;
> +
>  	dev_info(dev, "Flashing %s\n", fw_name);
>  
>  	error = rmi_firmware_update(data, fw);

[Severity: High]
This is a pre-existing issue, but does lockless polling of the
update_fw_status attribute concurrently with rmi_firmware_update() lead to
a use-after-free?

During a firmware update, rmi_firmware_update() tears down the function
list, which frees data->f34_container in rmi_free_function_list() before
setting it to null:

drivers/input/rmi4/rmi_driver.c:rmi_free_function_list() {
    ...
		rmi_unregister_function(fn);
    ...
}

If a concurrent reader accesses the update_fw_status attribute, could it
fetch data->f34_container locklessly and dereference its .dev field while
it is being destroyed?

[Severity: Medium]
This is a pre-existing issue, but is the final firmware update status lost
upon failure?

At the end of rmi_firmware_update(), if the flash operation fails, the
error code is saved:

drivers/input/rmi4/rmi_f34.c:rmi_firmware_update() {
    ...
	if (ret) {
		f34->update_status = ret;
    ...
	rmi_free_function_list(rmi_dev);
    ...
}

However, it seems rmi_free_function_list() is called immediately after,
which destroys the f34 structure. When userspace reads the status later,
would it observe a newly zero-initialized f34_data context, effectively
losing the actual error code and falsely reporting success?

[Severity: High]
This is a pre-existing issue, but is there a missing null check for f34
during the re-probe in rmi_firmware_update()?

drivers/input/rmi4/rmi_f34.c:rmi_firmware_update() {
    ...
	f34 = dev_get_drvdata(&data->f34_container->dev);

	/* Perform firmware update */
	if (f34->bl_version >= 7)
    ...
}

If the internal f34 probe fails during rmi_init_functions(), the device
might remain unbound with a null driver data pointer. Could dereferencing
f34->bl_version unconditionally trigger a kernel panic here?

[Severity: High]
This is a pre-existing issue, but do firmware updates unconditionally
re-enable interrupts, potentially exposing destroyed data structures?

If device removal happens concurrently with a firmware update,
rmi_driver_remove() disables interrupts and blocks on sysfs removal waiting
for the update to complete:

drivers/input/rmi4/rmi_driver.c:rmi_driver_remove() {
    ...
	rmi_disable_irq(rmi_dev, false);
	rmi_f34_remove_sysfs(rmi_dev);
    ...
}

However, rmi_firmware_update() unconditionally calls rmi_enable_irq()
before returning:

drivers/input/rmi4/rmi_f34.c:rmi_firmware_update() {
    ...
	rmi_enable_irq(rmi_dev, false);
    ...
}

Does this bypass the driver teardown intent and allow interrupts to fire
while rmi_driver_remove() proceeds to tear down the function lists, causing
a use-after-free?

> -	if (error)
> -		return error;
>  
> -	return count;
> +	mutex_unlock(&data->update_mutex);
> +	return error ?: count;
>  }
>  
>  static DEVICE_ATTR(update_fw, 0200, NULL, rmi_driver_update_fw_store);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260817182205.906592-1-shuangpeng.kernel@gmail.com?part=1