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

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

[Hendrik Noack]

The Atmel maXTouch driver starts asynchronous config loading from both
probe and a sysfs trigger via request_firmware_nowait() and passes the
devm-managed struct mxt_data as the callback context. If the driver is
removed while an asynchronous request is still pending, devres will free
mxt_data before the firmware callback runs. When the callback eventually
executes, it dereferences the freed mxt_data pointer, leading to a
use-after-free.

This is resolved by introducing a lock and a completion that serialise the
request_firmware_nowait() and remove():

Before calling request_firmware_nowait(), a mutex guards the waiting and
reinitialisation of the completion. If remove() has started, the
asynchronous loading will no longer be started. The completion is
completed at the end of the callback or if request_firmware_nowait()
fails.

In mxt_remove(), the same mutex guards the start of remove(). Afterwards,
the completion of pending request_firmware_nowait() is awaited.

Signed-off-by: Hendrik Noack <hendrik-noack@gmx.de>
---
Changes in v2:
- this fix is added to the patch series (thanks sashiko.dev)
---
 drivers/input/touchscreen/atmel_mxt_ts.c | 29 +++++++++++++++++++++++-
 1 file changed, 28 insertions(+), 1 deletion(-)

diff --git a/drivers/input/touchscreen/atmel_mxt_ts.c b/drivers/input/touchscreen/atmel_mxt_ts.c
index a9e86ad7ed5e..07690c3bff06 100644
--- a/drivers/input/touchscreen/atmel_mxt_ts.c
+++ b/drivers/input/touchscreen/atmel_mxt_ts.c
@@ -347,6 +347,11 @@ struct mxt_data {
 	/* for config update handling */
 	struct completion crc_completion;
 
+	/* for loading config */
+	struct completion config_completion;
+	struct mutex config_lock;
+	bool shutting_down;
+
 	u32 *t19_keymap;
 	unsigned int t19_num_keys;
 
@@ -2221,8 +2226,11 @@ static int mxt_configure_objects(struct mxt_data *data,
 
 static void mxt_config_cb(const struct firmware *cfg, void *ctx)
 {
-	mxt_configure_objects(ctx, cfg);
+	struct mxt_data *data = ctx;
+
+	mxt_configure_objects(data, cfg);
 	release_firmware(cfg);
+	complete(&data->config_completion);
 }
 
 static int mxt_initialize(struct mxt_data *data)
@@ -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);
+
+		if (data->shutting_down)
+			return -EBUSY;
+
+		reinit_completion(&data->config_completion);
+	}
+
 	error = request_firmware_nowait(THIS_MODULE, true, MXT_CFG_NAME,
 					&client->dev, GFP_KERNEL, data,
 					mxt_config_cb);
 	if (error) {
 		dev_err(&client->dev, "Failed to invoke firmware loader: %d\n",
 			error);
+		complete(&data->config_completion);
 		return error;
 	}
 
@@ -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;
 
 	data->suspend_mode = dmi_check_system(chromebook_T9_suspend_dmi) ?
 		MXT_SUSPEND_T9_CTRL : MXT_SUSPEND_DEEP_SLEEP;
@@ -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);
 	mxt_free_object_table(data);
-- 
2.43.0

[PATCH v2 2/2] Input: atmel_mxt_ts: Allow per-board config via DT compatible

[Hendrik Noack]

The Atmel maXTouch config is board-dependent: different panels and layouts
require different configs even when the same controller and firmware is
used.

Extend the driver to support per-board config files keyed by the device
tree compatible string. For DT-based systems, the driver now looks up the
primary compatible of the board and searches for a config file named:

  atmel/maxtouch,<compatible>.cfg

If such a file exists, it is used as the board-specific config. If the
system is not DT-based or if the per-board file is not present, the driver
reverts to the original behaviour and searches for a config file named:

  maxtouch.cfg

Signed-off-by: Hendrik Noack <hendrik-noack@gmx.de>
---
Changes in v2:
- fix leak of fw_name (thanks sashiko.dev)
- adopt to added patch
---
 drivers/input/touchscreen/atmel_mxt_ts.c | 68 ++++++++++++++++++++++--
 1 file changed, 63 insertions(+), 5 deletions(-)

diff --git a/drivers/input/touchscreen/atmel_mxt_ts.c b/drivers/input/touchscreen/atmel_mxt_ts.c
index 07690c3bff06..a9e9f151d042 100644
--- a/drivers/input/touchscreen/atmel_mxt_ts.c
+++ b/drivers/input/touchscreen/atmel_mxt_ts.c
@@ -36,7 +36,9 @@
 
 /* Firmware files */
 #define MXT_FW_NAME		"maxtouch.fw"
-#define MXT_CFG_NAME		"maxtouch.cfg"
+#define MXT_CFG_FOLDER		"atmel"
+#define MXT_CFG_NAME		"maxtouch"
+#define MXT_CFG_EXTENSION	".cfg"
 #define MXT_CFG_MAGIC		"OBP_RAW V1"
 
 /* Registers */
@@ -2233,6 +2235,64 @@ static void mxt_config_cb(const struct firmware *cfg, void *ctx)
 	complete(&data->config_completion);
 }
 
+static int mxt_invoke_config_loader(struct mxt_data *data, bool device_specific);
+
+static void mxt_board_config_cb(const struct firmware *cfg, void *ctx)
+{
+	if (!cfg) {
+		struct mxt_data *data = ctx;
+		int error;
+
+		error = mxt_invoke_config_loader(data, false);
+		/* request_firmware_nowait() succeeded and mxt_config_cb() will be called at end */
+		if (!error)
+			return;
+
+		dev_err(&data->client->dev, "Failed to invoke general config loader: %d\n", error);
+	}
+
+	mxt_config_cb(cfg, ctx);
+}
+
+static int mxt_invoke_config_loader(struct mxt_data *data, bool device_specific)
+{
+	struct device_node *root;
+	char *board_type = NULL;
+	char *fw_name;
+	void (*cb)(const struct firmware *fw, void *context);
+	int error;
+
+	root = of_find_node_by_path("/");
+	if (device_specific && root) {
+		const char *tmp;
+
+		if (!of_property_read_string_index(root, "compatible", 0, &tmp))
+			board_type = kstrdup(tmp, GFP_KERNEL);
+
+		of_node_put(root);
+	}
+
+	if (board_type) {
+		/* get rid of '/' in the compatible string to be able to find the FW */
+		strreplace(board_type, '/', '-');
+
+		fw_name = kasprintf(GFP_KERNEL, "%s/%s.%s%s", MXT_CFG_FOLDER, MXT_CFG_NAME,
+				    board_type, MXT_CFG_EXTENSION);
+		cb = mxt_board_config_cb;
+		kfree(board_type);
+	} else {
+		fw_name = kasprintf(GFP_KERNEL, "%s%s", MXT_CFG_NAME, MXT_CFG_EXTENSION);
+		cb = mxt_config_cb;
+	}
+	if (!fw_name)
+		return -ENOMEM;
+
+	error = request_firmware_nowait(THIS_MODULE, true, fw_name, &data->client->dev, GFP_KERNEL,
+					data, cb);
+	kfree(fw_name);
+	return error;
+}
+
 static int mxt_initialize(struct mxt_data *data)
 {
 	struct i2c_client *client = data->client;
@@ -2288,11 +2348,9 @@ static int mxt_initialize(struct mxt_data *data)
 		reinit_completion(&data->config_completion);
 	}
 
-	error = request_firmware_nowait(THIS_MODULE, true, MXT_CFG_NAME,
-					&client->dev, GFP_KERNEL, data,
-					mxt_config_cb);
+	error = mxt_invoke_config_loader(data, true);
 	if (error) {
-		dev_err(&client->dev, "Failed to invoke firmware loader: %d\n",
+		dev_err(&client->dev, "Failed to invoke config loader: %d\n",
 			error);
 		complete(&data->config_completion);
 		return error;
-- 
2.43.0

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

[sashiko-bot]

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

Re: [PATCH v2 2/2] Input: atmel_mxt_ts: Allow per-board config via DT compatible

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] The OF device node reference for the root node is leaked when falling back to the generic configuration loader.
- [Medium] Mismatch between the constructed firmware filename and the filename documented in the commit message.
--

commit e3439c3fb5cf6c8c740fae5a27659ea3cb9e7436
Author: Hendrik Noack <hendrik-noack@gmx.de>

Input: atmel_mxt_ts: Allow per-board config via DT compatible

This commit modifies the Atmel maXTouch driver to load a board-specific
configuration file based on the device tree compatible string. If the specific
configuration file is not found, it falls back to the original generic
configuration behavior.

> Extend the driver to support per-board config files keyed by the device
> tree compatible string. For DT-based systems, the driver now looks up the
> primary compatible of the board and searches for a config file named:
> 
>   atmel/maxtouch,<compatible>.cfg

[Severity: Medium]
The documentation in the commit message uses a comma before the compatible
string, but the code implementation constructs the filename using a dot.

Is it intended to use a dot here, and if so, could the commit message
be updated to match the code?

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

[ ... ]

> @@ -2233,6 +2235,64 @@ static void mxt_config_cb(const struct firmware *cfg, void *ctx)

[ ... ]

> +static int mxt_invoke_config_loader(struct mxt_data *data, bool device_specific)
> +{
> +	struct device_node *root;
> +	char *board_type = NULL;
> +	char *fw_name;
> +	void (*cb)(const struct firmware *fw, void *context);
> +	int error;
> +
> +	root = of_find_node_by_path("/");
> +	if (device_specific && root) {
> +		const char *tmp;
> +
> +		if (!of_property_read_string_index(root, "compatible", 0, &tmp))
> +			board_type = kstrdup(tmp, GFP_KERNEL);
> +
> +		of_node_put(root);
> +	}

[Severity: Medium]
Does this leak the root device node reference when device_specific is false?

The call to of_find_node_by_path() unconditionally acquires a reference,
but of_node_put() is only called inside the if block. The fallback
configuration path triggered from mxt_board_config_cb() passes false
for device_specific, which would bypass the release.

> +
> +	if (board_type) {
> +		/* get rid of '/' in the compatible string to be able to find the FW */
> +		strreplace(board_type, '/', '-');
> +
> +		fw_name = kasprintf(GFP_KERNEL, "%s/%s.%s%s", MXT_CFG_FOLDER, MXT_CFG_NAME,
> +				    board_type, MXT_CFG_EXTENSION);

[Severity: Medium]
Does this format string construct the filename with a dot instead of the comma
described in the commit message?

This format string expands to atmel/maxtouch.<compatible>.cfg which might lead
to unexpected firmware loading failures for users following the naming scheme
documented in the commit.

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