[PATCH] Input: synaptics-rmi4 - remove the F34 sysfs group when probe fails

[PATCH] Input: synaptics-rmi4 - remove the F34 sysfs group when probe fails

[Wei Jie Law]

rmi_driver_probe() creates the F34 firmware attribute group directly on
the rmi_device's kobject:

	retval = rmi_f34_create_sysfs(rmi_dev);
	if (retval)
		goto err;

but rmi_f34_remove_sysfs() is only ever called from
rmi_driver_remove(), which does not run when probe fails.  Every error
path taken after that point -- input_register_device(), rmi_irq_init(),
rmi_enable_sensor() -- therefore leaves the group in place on a device
that never finished binding.

The driver core clears the device's driver data when probe fails, so
what is left behind is a set of world-readable attributes whose show()
handlers dereference that now-NULL pointer:

	static ssize_t rmi_driver_bootloader_id_show(struct device *dev, ...)
	{
		struct rmi_driver_data *data = dev_get_drvdata(dev);
		struct rmi_function *fn;

		fn = data->f34_container;

bootloader_id, configuration_id and update_fw_status are all mode 0444,
so any local user can dereference it.  Reaching the failure does not
need privileges either: a device that simply stops answering a register
read makes rmi_enable_sensor() fail, which is past the group creation.

An emulated RMI4 touchpad over /dev/uhid that leaves the second read of
the F01 interrupt-status register unanswered, on v6.12.105 with
CONFIG_KASAN=y:

  rmi4_physical rmi4-00: Failed to read irqs, code=-11
  rmi4_physical rmi4-00: probe with driver rmi4_physical failed with error -11

and then, from an ordinary user:

  $ cat /sys/bus/rmi4/devices/rmi4-00/bootloader_id

  Oops: general protection fault, probably for non-canonical address
        0xdffffc0000000004: 0000 [#1] PREEMPT SMP KASAN NOPTI
  KASAN: null-ptr-deref in range [0x0000000000000020-0x0000000000000027]
  CPU: 0 UID: 1000 PID: 1851 Comm: cat Tainted: G            E
  RIP: 0010:rmi_driver_bootloader_id_show+0x4d/0x1b0 [rmi_core]
   dev_attr_show+0x46/0xc0
   sysfs_kf_seq_show+0x1f1/0x3c0
   seq_read_iter+0x2f8/0x1150
   vfs_read+0x699/0xa00

one oops per attribute.

Add an err_remove_sysfs label that drops the group, and route the error
paths below rmi_f34_create_sysfs() through it.  rmi_f34_create_sysfs()
failing on its own account goes to err_destroy_functions instead, which
also repairs a second leak on that path: it used to "goto err" and skip
rmi_free_function_list() entirely, leaving the function devices
registered on the RMI bus with a parent whose driver data is gone.  The
resulting order matches rmi_driver_remove().

After this change the same device leaves no attributes behind, the reads
fail with -ENOENT, and no oops is reported; a well-behaved device still
probes and keeps its F34 group.

Fixes: 29fd0ec2bdbe ("Input: synaptics-rmi4 - add support for F34 device reflash")
Cc: stable@vger.kernel.org
Signed-off-by: Wei Jie Law <98lawweijie@gmail.com>
---
 drivers/input/rmi4/rmi_driver.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/drivers/input/rmi4/rmi_driver.c b/drivers/input/rmi4/rmi_driver.c
index 5d49a9021c7d..144b203e636a 100644
--- a/drivers/input/rmi4/rmi_driver.c
+++ b/drivers/input/rmi4/rmi_driver.c
@@ -1255,7 +1255,7 @@ static int rmi_driver_probe(struct device *dev)
 
 	retval = rmi_f34_create_sysfs(rmi_dev);
 	if (retval)
-		goto err;
+		goto err_destroy_functions;
 
 	if (data->input) {
 		rmi_driver_set_input_name(rmi_dev, data->input);
@@ -1264,14 +1264,14 @@ static int rmi_driver_probe(struct device *dev)
 			if (retval) {
 				dev_err(dev, "%s: Failed to register input device.\n",
 					__func__);
-				goto err_destroy_functions;
+				goto err_remove_sysfs;
 			}
 		}
 	}
 
 	retval = rmi_irq_init(rmi_dev);
 	if (retval < 0)
-		goto err_destroy_functions;
+		goto err_remove_sysfs;
 
 	if (data->f01_container->dev.driver) {
 		/* Driver already bound, so enable ATTN now. */
@@ -1284,6 +1284,8 @@ static int rmi_driver_probe(struct device *dev)
 
 err_disable_irq:
 	rmi_disable_irq(rmi_dev, false);
+err_remove_sysfs:
+	rmi_f34_remove_sysfs(rmi_dev);
 err_destroy_functions:
 	rmi_free_function_list(rmi_dev);
 err:
-- 
2.43.0

Re: [PATCH] Input: synaptics-rmi4 - remove the F34 sysfs group when probe fails

[sashiko-bot]

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

Pre-existing issues:
- [High] The non-devm managed IRQ domain allocated in rmi_probe_interrupts() is permanently leaked on probe error paths and repeatedly during firmware updates, leading to a Use-After-Free.
- [High] Missing mutual exclusion in the sysfs interface allows concurrent firmware updates to trigger kernel panics via list corruption and double-frees.
- [High] Modifying input->name with a devm-allocated string for a transport-provided input device causes a Use-After-Free.
- [Medium] TOCTOU race between rmi_set_attn_data and rmi_disable_irq leads to a memory leak in attn_fifo.
--

--- Patch [1]: [PATCH] Input: synaptics-rmi4 - remove the F34 sysfs group when probe fails ---
commit 8215764a60df6f91a5ee0d3e6244015a81ede7e2
Author: Wei Jie Law <98lawweijie@gmail.com>

Input: synaptics-rmi4 - remove the F34 sysfs group when probe fails

This patch adds an error path to drop the F34 sysfs group if probe fails
after it is created, preventing a null pointer dereference on the
attribute's show handler. It also fixes a leak by ensuring
rmi_free_function_list() is called when rmi_f34_create_sysfs() fails.

> diff --git a/drivers/input/rmi4/rmi_driver.c b/drivers/input/rmi4/rmi_driver.c
> --- a/drivers/input/rmi4/rmi_driver.c
> +++ b/drivers/input/rmi4/rmi_driver.c
> @@ -1255,7 +1255,7 @@ static int rmi_driver_probe(struct device *dev)
>  
>  	retval = rmi_f34_create_sysfs(rmi_dev);
>  	if (retval)
> -		goto err;
> +		goto err_destroy_functions;
>  
>  	if (data->input) {
>  		rmi_driver_set_input_name(rmi_dev, data->input);

[Severity: High]
This is a pre-existing issue, but does assigning a devm-allocated string to
the transport-provided input device cause a use-after-free?

When the transport layer provides an existing input device:

rmi_driver_probe() {
        if (rmi_dev->xport->input) {
                data->input = rmi_dev->xport->input;
        }
        ...
}

The device outlives rmi_dev. However, rmi_driver_set_input_name() assigns a
devm-allocated string to input->name:

rmi_driver_set_input_name() {
        name = devm_kasprintf(&rmi_dev->dev, ...);
        input->name = name;
}

If probe fails or the driver unbinds, the devm string is freed while the
input device is still alive, leaving a dangling pointer for userspace readers.

[ ... ]

>  err_disable_irq:
>  	rmi_disable_irq(rmi_dev, false);

[Severity: Medium]
This isn't a bug introduced by this patch, but is there a race condition
between rmi_disable_irq() and rmi_set_attn_data() that could leak memory?

In rmi_set_attn_data(), there is a lockless check of drvdata->enabled:

rmi_set_attn_data() {
        if (!drvdata->enabled)
                return;
        fifo_data = kmemdup(data, size, GFP_ATOMIC);
        ...
}

Meanwhile, rmi_disable_irq() sets enabled to false and drains the FIFO
under a mutex:

rmi_disable_irq() {
        while (!kfifo_is_empty(&data->attn_fifo)) {
                count = kfifo_get(&data->attn_fifo, &attn_data);
                if (count)
                        kfree(attn_data.data);
        }
}

Could a concurrent enqueue from a hardware interrupt occur right after the
drain, permanently leaking the dynamically allocated attn_data elements?

> +err_remove_sysfs:
> +	rmi_f34_remove_sysfs(rmi_dev);
>  err_destroy_functions:
>  	rmi_free_function_list(rmi_dev);

[Severity: High]
This is a pre-existing issue, but does calling rmi_free_function_list()
lack necessary serialization during firmware updates?

The sysfs store handler rmi_driver_update_fw_store() calls
rmi_firmware_update() directly without acquiring any driver-level lock.
When that update calls rmi_free_function_list(), it tears down the function
list locklessly:

rmi_free_function_list() {
        list_for_each_entry_safe_reverse(fn, tmp,
                                         &data->function_list, node) {
                list_del(&fn->node);
                rmi_unregister_function(fn);
        }
}

Could concurrent writes to the update_fw sysfs attribute trigger list
corruption or double-frees since sysfs store callbacks do not serialize
concurrent execution?

[Severity: High]
This isn't a bug introduced by this patch, but does this error path leak
the IRQ domain allocated earlier?

In rmi_probe_interrupts(), the IRQ domain is created without devres management:

rmi_probe_interrupts() {
        data->irqdomain = irq_domain_create_linear(fwnode, irq_count,
                                                   &irq_domain_simple_ops, data);
}

However, these error paths do not call irq_domain_remove(). Also, since
rmi_firmware_update() re-probes interrupts, does it allocate a new IRQ domain
without freeing the previous one, potentially leading to a use-after-free
when the devm-allocated data pointer is freed?

>  err:
>  	return retval;
>  }

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

Re: [PATCH] Input: synaptics-rmi4 - remove the F34 sysfs group when probe fails

[Wei Jie LAW]

From: Wei Jie Law <98lawweijie@gmail.com>

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

Please use that one instead.  The only difference is the Assisted-by
tags that Documentation/process/coding-assistants.rst asks for -- AI
tools were used in finding and fixing this bug and I should have
acknowledged that in the original posting.  The diff is unchanged.

Please mark this one superseded.

Thanks, and sorry for the noise,
Wei Jie