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

[sashiko-bot@kernel.org]

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