Re: [PATCH] Input: synaptics-rmi4 - fix input_dev->name use-after-free on unplug

[sashiko-bot@kernel.org]

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

New issues:
- [High] TOCTOU race condition in sysfs name read leading to Use-After-Free.

Pre-existing issues:
- [High] Memory leak of IRQ domain on probe error and firmware update.
- [High] Use-After-Free due to bypassed function cleanup on sysfs creation failure.
- [High] Resource leak and Use-After-Free due to dangling sysfs group on probe failure.
- [High] NULL Pointer Dereference on F01 driver probe failure.
- [Medium] TOCTOU race condition causing memory leak of attention FIFO data.
--

--- Patch [1]: [PATCH] Input: synaptics-rmi4 - fix input_dev->name use-after-free on unplug ---
commit 9cd37129e649f20d52a9bc378f92b844e6f83fbd
Author: Wei Jie Law <98lawweijie@gmail.com>

Input: synaptics-rmi4 - fix input_dev->name use-after-free on unplug

rmi_driver_probe() reuses the transport driver's input device when it has
one, and then renames it out of memory owned by the RMI device. In the
borrowed case the name outlives its allocation. This commit puts the name
back to a static string when this driver lets go of an input device it
does not own.

> diff --git a/drivers/input/rmi4/rmi_driver.c b/drivers/input/rmi4/rmi_driver.c
> index 5d49a9021c7d0..8696a6aa0fa9f 100644
> --- a/drivers/input/rmi4/rmi_driver.c
> +++ b/drivers/input/rmi4/rmi_driver.c
> @@ -369,6 +369,24 @@ static void rmi_driver_set_input_name(struct rmi_device *rmi_dev,
>  	input->name = name;
>  }

[Severity: High]
This isn't a bug introduced by this patch, but can this lead to a NULL pointer
dereference if the F01 driver probe fails?

In rmi_driver_set_input_name(), rmi_f01_get_product_ID() is called on
data->f01_container before verifying if the driver successfully bound:

drivers/input/rmi4/rmi_driver.c:rmi_driver_set_input_name() {
    ...
    const char *device_name = rmi_f01_get_product_ID(data->f01_container);
    ...
}

If the F01 hardware fails to communicate properly, the F01 driver's drvdata
might remain NULL. When rmi_f01_get_product_ID() fetches the drvdata and
dereferences it:

drivers/input/rmi4/rmi_f01.c:rmi_f01_get_product_ID() {
    ...
    struct f01_data *f01 = dev_get_drvdata(&fn->dev);
    return f01->properties.product_id;
}

Could this result in a kernel panic on initialization?

>  
> +/*
> + * Let go of an input device that belongs to the transport driver.  It outlives
> + * us, but rmi_driver_set_input_name() pointed its name at devres memory of
> + * ours that is freed as soon as we are done - input_register_device() and both
> + * the add and the remove uevent print that name - so put the name back to a
> + * string with static storage duration before dropping the reference.
> + */
> +static void rmi_driver_put_input(struct rmi_device *rmi_dev,
> +				 struct rmi_driver_data *data)
> +{
> +	if (!data->input || data->input != rmi_dev->xport->input)
> +		return;
> +
> +	data->input->name = SYNAPTICS_INPUT_DEVICE_NAME;

[Severity: High]
Is this non-atomic assignment safe against concurrent lockless sysfs reads?

If a concurrent sysfs read, such as input_dev_show_name(), fetches the old
devm_kasprintf allocated pointer just before this swap, could it dereference
the pointer after rmi_driver_remove() completes and devres frees the string?

Could this lead to a use-after-free if the sysfs reader is delayed and calls
sysfs_emit() with the freed pointer?

> +	input_put_device(data->input);
> +	data->input = NULL;
> +}
> +

[ ... ]

> @@ -1026,6 +1044,8 @@ static int rmi_driver_remove(struct device *dev)
>  	rmi_f34_remove_sysfs(rmi_dev);
>  	rmi_free_function_list(rmi_dev);
>  
> +	rmi_driver_put_input(rmi_dev, data);
> +
>  	irq_domain_remove(data->irqdomain);
>  	data->irqdomain = NULL;
>  

[Severity: Medium]
This is a pre-existing issue, but is there a race condition between
rmi_set_attn_data() and rmi_disable_irq() that could leak attention FIFO data?

In rmi_set_attn_data(), drvdata->enabled is checked locklessly before
allocating data and pushing it to the FIFO:

drivers/input/rmi4/rmi_driver.c:rmi_set_attn_data() {
    ...
    if (!drvdata->enabled)
        return;

    fifo_data = kmemdup(data, size, GFP_ATOMIC);
    ...
}

Concurrently, rmi_disable_irq(), which is called earlier during removal,
acquires the enabled_mutex, sets enabled to false, and flushes the FIFO:

drivers/input/rmi4/rmi_driver.c:rmi_disable_irq() {
    ...
    while (!kfifo_is_empty(&data->attn_fifo)) {
        count = kfifo_get(&data->attn_fifo, &attn_data);
    ...
}

If rmi_set_attn_data() reads enabled as true just before it is set to false,
could it append the allocated item to the FIFO after rmi_disable_irq() has
already flushed it? Since no further flushes occur upon driver removal, would
this dynamically allocated memory be leaked?

[ ... ]

> @@ -1287,6 +1307,7 @@ static int rmi_driver_probe(struct device *dev)
>  err_destroy_functions:
>  	rmi_free_function_list(rmi_dev);
>  err:

[Severity: High]
This isn't a bug introduced by this patch, but does the error path bypass
function cleanup if rmi_f34_create_sysfs() fails?

In rmi_driver_probe(), if rmi_f34_create_sysfs() fails, the code executes
a goto to the err label:

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

This bypasses the err_destroy_functions label, leaving the child RMI function
devices registered. When the driver core cleans up the probe failure, it frees
the devres-managed data. Would any subsequent callbacks on the leaked child
devices dereference the freed data and cause a use-after-free?


[Severity: High]
This is a pre-existing issue, but does this error path leave a dangling sysfs
group if input_register_device() fails?

If input_register_device() fails later in rmi_driver_probe(), the code jumps
to err_destroy_functions, but rmi_f34_remove_sysfs() is never called:

drivers/input/rmi4/rmi_driver.c:rmi_driver_probe() {
    ...
    retval = input_register_device(data->input);
    if (retval) {
        ...
        goto err_destroy_functions;
    }
    ...
}

Since the transport device outlives the probe failure, do the sysfs attributes
remain active? Could reading these attributes trigger callbacks that access
the freed devm-managed data, resulting in a use-after-free?


[Severity: High]
This is also a pre-existing issue, but does jumping to the err label leak the
IRQ domain?

In rmi_probe_interrupts(), irq_domain_create_linear() allocates an IRQ domain
which is not managed by devres:

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

If a subsequent step in rmi_driver_probe() fails and jumps to the err or
err_destroy_functions labels, irq_domain_remove() is omitted. Additionally,
since rmi_firmware_update() calls rmi_probe_interrupts() on every flash
operation without destroying the previously created domain, does this result
in repeated memory leaks?

> +	rmi_driver_put_input(rmi_dev, data);
>  	return retval;
>  }

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