* [PATCH v3 2/2] Input: atmel_mxt_ts: Allow per-machine config via DT compatible
2026-05-28 7:43 [PATCH v3 1/2] Input: atmel_mxt_ts: Fix async config use-after-free Hendrik Noack
@ 2026-05-28 7:43 ` Hendrik Noack
2026-05-28 9:28 ` sashiko-bot
2026-05-28 8:48 ` [PATCH v3 1/2] Input: atmel_mxt_ts: Fix async config use-after-free sashiko-bot
1 sibling, 1 reply; 4+ messages in thread
From: Hendrik Noack @ 2026-05-28 7:43 UTC (permalink / raw)
To: Nick Dyer, Dmitry Torokhov; +Cc: Hendrik Noack, linux-input, linux-kernel
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
^ permalink raw reply related [flat|nested] 4+ messages in thread* Re: [PATCH v3 1/2] Input: atmel_mxt_ts: Fix async config use-after-free
2026-05-28 7:43 [PATCH v3 1/2] Input: atmel_mxt_ts: Fix async config use-after-free Hendrik Noack
2026-05-28 7:43 ` [PATCH v3 2/2] Input: atmel_mxt_ts: Allow per-machine config via DT compatible Hendrik Noack
@ 2026-05-28 8:48 ` sashiko-bot
1 sibling, 0 replies; 4+ messages in thread
From: sashiko-bot @ 2026-05-28 8:48 UTC (permalink / raw)
To: Hendrik Noack; +Cc: dmitry.torokhov, linux-input
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
^ permalink raw reply [flat|nested] 4+ messages in thread