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

[PATCH v3 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 completion that serialises the
request_firmware_nowait() and remove():

Before calling request_firmware_nowait(), a completion serialises the
calls. If remove() has started, the asynchronous loading will no longer be
started and all waiters will be signaled. The completion is completed at
the end of the callback or if request_firmware_nowait() fails.

In mxt_remove(), the completion of pending request_firmware_nowait() is
awaited and all waiters will be signaled.

Signed-off-by: Hendrik Noack <hendrik-noack@gmx.de>
---
Changes in v2:
- this fix is added to the patch series (thanks sashiko.dev)

---
Changes in v3:
- remove unnecessary lock

---
 drivers/input/touchscreen/atmel_mxt_ts.c | 34 +++++++++++++++++++++++-
 1 file changed, 33 insertions(+), 1 deletion(-)

diff --git a/drivers/input/touchscreen/atmel_mxt_ts.c b/drivers/input/touchscreen/atmel_mxt_ts.c
index a9e86ad7ed5e..a88dc7e6827c 100644
--- a/drivers/input/touchscreen/atmel_mxt_ts.c
+++ b/drivers/input/touchscreen/atmel_mxt_ts.c
@@ -347,6 +347,10 @@ struct mxt_data {
 	/* for config update handling */
 	struct completion crc_completion;
 
+	/* for loading config */
+	struct completion config_completion;
+	bool shutting_down;
+
 	u32 *t19_keymap;
 	unsigned int t19_num_keys;
 
@@ -2221,8 +2225,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 +2278,26 @@ static int mxt_initialize(struct mxt_data *data)
 	if (error)
 		return error;
 
+	wait_for_completion(&data->config_completion);
+
+	if (data->shutting_down) {
+		/* complete() only signals one waiter, signal all waiters once during shutdown */
+		if (!completion_done(&data->config_completion)) {
+			complete_all(&data->config_completion);
+			dev_info(&client->dev, "Shutting down, not loading new config: %d\n",
+				 error);
+		}
+
+		return -EBUSY;
+	}
+
 	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 +3258,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 +3366,14 @@ static void mxt_remove(struct i2c_client *client)
 {
 	struct mxt_data *data = i2c_get_clientdata(client);
 
+	data->shutting_down = true;
+
+	wait_for_completion(&data->config_completion);
+
+	/* complete() only signals one waiter, signal all waiters once during shutdown */
+	if (!completion_done(&data->config_completion))
+		complete_all(&data->config_completion);
+
 	disable_irq(data->irq);
 	mxt_free_input_device(data);
 	mxt_free_object_table(data);
-- 
2.43.0

[PATCH v3 2/2] Input: atmel_mxt_ts: Allow per-machine config via DT compatible

[Hendrik Noack]

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

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

  atmel/maxtouch.<compatible>.cfg

If such a file exists, it is used as the machine-specific config. If the
system is not DT-based or if the per-machine 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

---
Changes in v3:
- fix bypass of of_node_put (thanks sashiko.dev)

---
 drivers/input/touchscreen/atmel_mxt_ts.c | 69 +++++++++++++++++++++---
 1 file changed, 63 insertions(+), 6 deletions(-)

diff --git a/drivers/input/touchscreen/atmel_mxt_ts.c b/drivers/input/touchscreen/atmel_mxt_ts.c
index a88dc7e6827c..0081fa5c7802 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 */
@@ -2232,6 +2234,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;
+
+	/* Get first string of the machine compatible prop */
+	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;
@@ -2291,12 +2351,9 @@ static int mxt_initialize(struct mxt_data *data)
 		return -EBUSY;
 	}
 
-	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",
-			error);
+		dev_err(&client->dev, "Failed to invoke config loader: %d\n", error);
 		complete(&data->config_completion);
 		return error;
 	}
-- 
2.43.0

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

[sashiko-bot]

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

New issues:
- [Low] The `shutting_down` flag and `complete_all()` logic introduced to serialize shutdown are completely unreachable dead code and contain misleading logging.

Pre-existing issues:
- [High] Concurrent sysfs operations lack synchronization, leading to severe use-after-free and double-free vulnerabilities.
- [High] The `wait_for_completion(&data->config_completion)` in `mxt_initialize()` is placed too late, failing to protect against concurrent execution with the firmware callback when triggered via sysfs.
- [High] Accessing the `update_fw` sysfs attribute before probe completes leads to a NULL pointer dereference.
--

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

Input: atmel_mxt_ts: Fix async config use-after-free

The driver starts asynchronous config loading from probe and sysfs. If the
driver is removed while a request is pending, devres frees mxt_data, leading
to a use-after-free in the callback. The patch introduces a completion to
serialize request_firmware_nowait() and remove().

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

[ ... ]

> @@ -2271,12 +2278,26 @@ static int mxt_initialize(struct mxt_data *data)
>  	if (error)
>  		return error;
>  
> +	wait_for_completion(&data->config_completion);

[Severity: High]
This is a pre-existing issue, but does wait_for_completion() happen too late
here to protect against concurrent execution with the firmware callback when
triggered via sysfs?

When mxt_update_fw_store() is invoked, it first calls mxt_load_fw(), which
clears data->info via mxt_free_object_table(), before mxt_initialize() is
even called.

It then calls mxt_initialize(), which waits for the completion. However, if
a previously initiated mxt_config_cb() is executing concurrently, it uses
data->info inside mxt_configure_objects().

Since mxt_load_fw() frees and clears this pointer without waiting for the
async callback to finish, mxt_config_cb() will suffer a null pointer
dereference or use-after-free. The wait seems placed too late to prevent
this sysfs race.

[Severity: High]
This is a pre-existing issue, but do concurrent sysfs operations lack
synchronization?

The driver exposes sysfs attributes like update_fw without internal
serialization. The sysfs framework permits multiple threads to write to the
same attribute concurrently.

mxt_update_fw_store() directly invokes mxt_load_fw(), which destructively
modifies device state by calling mxt_free_object_table().

Without a mutex protecting this path, concurrent writes to update_fw will
cause multiple threads to enter these teardown functions simultaneously,
leading to a double free of data->raw_info_block and data->msg_buf.

> +
> +	if (data->shutting_down) {
> +		/* complete() only signals one waiter, signal all waiters once during shutdown */
> +		if (!completion_done(&data->config_completion)) {
> +			complete_all(&data->config_completion);
> +			dev_info(&client->dev, "Shutting down, not loading new config: %d\n",
> +				 error);

[Severity: Low]
Is this block actually reachable? mxt_initialize() is only invoked by
mxt_probe() and the sysfs attribute mxt_update_fw_store().

The driver core strictly serializes mxt_probe() and mxt_remove() via the
device lock. For the sysfs path, the driver core drains active sysfs
operations via device_remove_groups() before invoking the bus remove()
callback (which calls mxt_remove()).

Because of this, it seems mxt_initialize() and mxt_remove() can never run
concurrently.

Also, error here is mathematically guaranteed to be 0 since we just passed
if (error) return error, making the log message semantically misleading.

> +		}
> +
> +		return -EBUSY;
> +	}
> +

[ ... ]

> @@ -3237,6 +3258,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: High]
This is a pre-existing issue, but could accessing the update_fw sysfs
attribute before probe completes lead to a null pointer dereference?

Sysfs attributes defined via mxt_driver.driver.dev_groups are registered by
the driver core (device_add_groups()) before the driver's probe function is
called.

An early write to update_fw during this window executes
mxt_update_fw_store() before i2c_set_clientdata() is called in mxt_probe().
In this scenario, dev_get_drvdata(dev) returns a null pointer, which is then
immediately dereferenced in mxt_load_fw():

drivers/input/touchscreen/atmel_mxt_ts.c:mxt_load_fw() {
    ...
    if (!data->in_bootloader) {
    ...
}

Would this cause a kernel panic?

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

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

[sashiko-bot]

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

Pre-existing issues:
- [High] Concurrent execution of 'mxt_load_fw' via the 'update_fw' sysfs attribute lacks locking, leading to double-free of the input device, TOCTOU races, and interleaved I2C commands.
--

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

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

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

> diff --git a/drivers/input/touchscreen/atmel_mxt_ts.c b/drivers/input/touchscreen/atmel_mxt_ts.c
> index a88dc7e6827c7..5ac123d4ed036 100644
> --- a/drivers/input/touchscreen/atmel_mxt_ts.c
> +++ b/drivers/input/touchscreen/atmel_mxt_ts.c
[ ... ]
> @@ -2291,12 +2351,9 @@ static int mxt_initialize(struct mxt_data *data)
>  		return -EBUSY;
>  	}
>  
> -	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",
> -			error);
> +		dev_err(&client->dev, "Failed to invoke config loader: %d\n", error);
>  		complete(&data->config_completion);
>  		return error;
>  	}

[Severity: High]
This is a pre-existing issue, but I noticed that mxt_update_fw_store()
invokes mxt_load_fw() without any locking to serialize concurrent execution.

Since sysfs write operations don't hold a global lock like device_lock,
could multiple threads concurrently writing to the update_fw attribute race
when checking and setting data->in_bootloader?

If so, could both threads concurrently execute mxt_free_input_device(),
observe a non-null data->input_dev, and invoke input_unregister_device() on
the same pointer before it is set to NULL?

This seems like it might lead to a double-free or use-after-free since
input_unregister_device() drops the reference count.

Could this also race with PM callbacks like mxt_suspend() attempting to
acquire input_dev->mutex, operating on freed memory if
mxt_free_input_device() is executed concurrently?

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