Re: [PATCH v2 1/2] Input: atmel_mxt_ts: Fix async config use-after-free

[sashiko-bot@kernel.org]

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

New issues:
- [Critical] The newly added `config_lock` mutex in `struct mxt_data` is used without being initialized.
- [High] If `mxt_initialize()` aborts due to `shutting_down == true`, it consumes the completion but fails to restore it, leading to a permanent hang.
- [High] The `config_lock` mutex is held across a potentially unbounded wait, defeating the purpose of the `shutting_down` flag.
- [Medium] A time-of-check to time-of-use (TOCTOU) race allows asynchronous firmware loading to start after device removal has begun.

Pre-existing issues:
- [High] Concurrent sysfs writes to the `update_fw` attribute can trigger a double free of the input device.
--

commit a6627582981aef52fbfe146ee37db19f67d402dd
Author: Hendrik Noack <hendrik-noack@gmx.de>
Input: atmel_mxt_ts: Fix async config use-after-free

This commit attempts to fix a use-after-free by adding a mutex and a
completion to serialize the asynchronous firmware loading and the device
removal process.

> diff --git a/drivers/input/touchscreen/atmel_mxt_ts.c b/drivers/input/touchscreen/atmel_mxt_ts.c
> index a9e86ad7ed5ef..07690c3bff06e 100644
> --- a/drivers/input/touchscreen/atmel_mxt_ts.c
> +++ b/drivers/input/touchscreen/atmel_mxt_ts.c

[ ... ]

> @@ -2271,12 +2279,22 @@ static int mxt_initialize(struct mxt_data *data)
>  	if (error)
>  		return error;
>  
> +	scoped_guard(mutex, &data->config_lock) {
> +		wait_for_completion(&data->config_completion);

[Severity: High]
Is it safe to hold the config_lock mutex across this wait_for_completion()
call? 

Since the lock is held while waiting for a potentially long firmware load,
and mxt_remove() needs this same lock to set shutting_down to true, would
this cause mxt_remove() to block indefinitely, defeating the early shutdown
check?

> +
> +		if (data->shutting_down)
> +			return -EBUSY;

[Severity: High]
If shutting_down is true, the wait_for_completion() above has already
decremented the completion's counter. 

Returning -EBUSY here without calling complete(&data->config_completion)
leaves the completion consumed. When mxt_remove() later calls
wait_for_completion(), will it hang forever waiting for a completion that
is never restored?

> +
> +		reinit_completion(&data->config_completion);
> +	}
> +
>  	error = request_firmware_nowait(THIS_MODULE, true, MXT_CFG_NAME,
>  					&client->dev, GFP_KERNEL, data,
>  					mxt_config_cb);

[Severity: Medium]
Because request_firmware_nowait() is called after the config_lock is
released, is there a race condition here? 

If mxt_remove() runs immediately after the lock is dropped, it will acquire
the lock and set shutting_down to true, but request_firmware_nowait() will
still be invoked, starting a new asynchronous load while the device is
actively being removed.

[ ... ]

> @@ -3237,6 +3255,9 @@ static int mxt_probe(struct i2c_client *client)
>  	init_completion(&data->bl_completion);
>  	init_completion(&data->reset_completion);
>  	init_completion(&data->crc_completion);
> +	init_completion(&data->config_completion);
> +	complete(&data->config_completion);
> +	data->shutting_down = false;

[Severity: Critical]
Does the config_lock mutex need to be initialized here? 

The mxt_data structure is allocated with devm_kzalloc(), which 
zero-initializes the memory. Using an uninitialized mutex with 
scoped_guard() will cause a NULL pointer dereference when contention 
occurs. Should there be a call to mutex_init(&data->config_lock)?

[ ... ]

> @@ -3342,6 +3363,12 @@ static void mxt_remove(struct i2c_client *client)
>  {
>  	struct mxt_data *data = i2c_get_clientdata(client);
>  
> +	scoped_guard(mutex, &data->config_lock) {
> +		data->shutting_down = true;
> +	}
> +
> +	wait_for_completion(&data->config_completion);
> +
>  	disable_irq(data->irq);
>  	mxt_free_input_device(data);

[Severity: High]
This is a pre-existing issue, but looking at mxt_free_input_device(), it
checks if data->input_dev is non-NULL and then unregisters it.

If concurrent sysfs writes to the update_fw attribute occur, multiple
threads can enter mxt_load_fw() and call mxt_free_input_device() at the
same time. Since sysfs doesn't serialize these writes and there is no
internal locking, could both threads pass the NULL check before either
sets it to NULL, resulting in a use-after-free and double-free of the
input device?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260523152332.86864-1-hendrik-noack@gmx.de?part=1