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

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

[Wei Jie Law]

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:

	if (rmi_dev->xport->input) {
		data->input = rmi_dev->xport->input;
	...
	name = devm_kasprintf(&rmi_dev->dev, GFP_KERNEL,
			      "Synaptics %s", device_name);
	if (!name)
		return;

	input->name = name;

In the borrowed case the name outlives its allocation.  hid-rmi is the
transport that hits it: rmi_input_configured() hands us its hidinput
device, and rmi_remove() then tears the RMI device down first:

	rmi_unregister_transport_device(&hdata->xport);
	hid_hw_stop(hdev);

The first line unbinds this driver, so devres frees the name.  The second
reaches input_unregister_device(), whose device_del() emits the
KOBJ_REMOVE uevent, and input_dev_uevent() does

	if (dev->name)
		INPUT_ADD_HOTPLUG_VAR("NAME=\"%s\"", dev->name);

so vsnprintf() walks the freed string and copies it into the uevent that
is broadcast to userspace.  With slub_debug=FZPU the remove event carries

  NAME="kkkkkkkkkkkkkkkkkkkkkkk\xa5\xbb\xbb\xbb\xbb\xbb\xbb\xbb\xbb..."

i.e. POISON_FREE, POISON_END, the redzone and the SLUB tracking metadata
past the end of the object - the read runs to the first NUL, so it leaves
the object as well.

  BUG: KASAN: slab-use-after-free in string+0x2a9/0x330
  Read of size 1 at addr ffff88810a379d31 by task name/8056
  [...]
   string+0x2a9/0x330
   vsnprintf+0x5ec/0x1680
   add_uevent_var+0x165/0x390
   input_dev_uevent+0x14c/0x750
   dev_uevent+0x26e/0x6e0
   kobject_uevent_env+0x4ed/0x11b0
   device_del+0x5ac/0x940
   input_unregister_device+0x88/0xc0
   hidinput_disconnect+0x144/0x3e0
   hid_disconnect+0xf7/0x160
   hid_hw_stop+0x13/0x70
   hid_device_remove+0xc4/0x220
  [...]
  Allocated by task 7810:
   devm_kasprintf+0xb0/0xe0
   rmi_driver_probe+0x3e0/0xbf0 [rmi_core]
   rmi_register_transport_device+0x19e/0x3e0 [rmi_core]
   rmi_input_configured+0x184/0x2e0 [hid_rmi]
  Freed by task 8056:
   release_nodes+0xf0/0x260
   devres_release_all+0x106/0x170
   device_unbind_cleanup+0x16/0x1a0
   device_release_driver_internal+0x3f9/0x550
   rmi_unregister_transport_device+0x34/0x50 [rmi_core]
   rmi_remove+0xd0/0x100 [hid_rmi]

This is not limited to the RMI_DEVICE_HAS_PHYS_BUTTONS models: any RMI4
sensor reaches it, because rmi_init_functions() runs inside
rmi_input_configured() and F11/F12/F30 call input_set_capability() on the
borrowed device, so hidinput_connect() finds it populated and registers
it.  11 to 14 reports per three unplugs here.

The same string is read on the add side too.
rmi_register_transport_device() returns 0 whenever device_add() succeeded,
so a probe failure of this driver - rmi_enable_sensor() failing on a
register read is enough, and a HID device that stops answering can arrange
that - does not stop the transport: devres frees the name, and
hidinput_connect() then goes on to call input_register_device(), which
prints the name and emits KOBJ_ADD.

  BUG: KASAN: slab-use-after-free in string+0x2a9/0x330
  [...]
   input_register_device+0x710/0xe10
   hidinput_connect+0x1466/0x2a50
  [...]
  Freed by task 4394:
   release_nodes+0xf0/0x260
   devres_release_all+0x106/0x170
   device_unbind_cleanup+0x16/0x1a0
   really_probe+0x388/0x930

Moving the allocation onto the input device does not help - device_del()
releases devres before it emits the uevent - so put the name back to a
string with static storage duration when this driver lets go of an input
device it does not own, both on remove and on the probe error paths.

That means writing to the borrowed device after hidinput_connect() may
already have thrown it away: if none of the RMI functions populated it,
hidinput_connect() calls hidinput_cleanup_hidinput() and frees it while
data->input still points there.  Take a reference for as long as we keep
the pointer, which also stops rmi_process_interrupt_requests() from
input_sync()ing a freed device.

Fixes: 2b6a321da9a2 ("Input: synaptics-rmi4 - add support for Synaptics RMI4 devices")
Cc: stable@vger.kernel.org
Signed-off-by: Wei Jie Law <98lawweijie@gmail.com>
---
 drivers/input/rmi4/rmi_driver.c | 23 ++++++++++++++++++++++-
 1 file changed, 22 insertions(+), 1 deletion(-)

diff --git a/drivers/input/rmi4/rmi_driver.c b/drivers/input/rmi4/rmi_driver.c
index 5d49a9021c7d..8696a6aa0fa9 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;
 }
 
+/*
+ * 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;
+	input_put_device(data->input);
+	data->input = NULL;
+}
+
 static int rmi_driver_set_irq_bits(struct rmi_device *rmi_dev,
 				   unsigned long *mask)
 {
@@ -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;
 
@@ -1231,7 +1251,7 @@ static int rmi_driver_probe(struct device *dev)
 		 * One example is some HID touchpads report "pass-through"
 		 * button events are not reported by rmi registers.
 		 */
-		data->input = rmi_dev->xport->input;
+		data->input = input_get_device(rmi_dev->xport->input);
 	} else {
 		data->input = devm_input_allocate_device(dev);
 		if (!data->input) {
@@ -1287,6 +1307,7 @@ static int rmi_driver_probe(struct device *dev)
 err_destroy_functions:
 	rmi_free_function_list(rmi_dev);
 err:
+	rmi_driver_put_input(rmi_dev, data);
 	return retval;
 }
 
-- 
2.43.0

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

[sashiko-bot]

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

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

[Wei Jie LAW]

From: Wei Jie Law <98lawweijie@gmail.com>

Superseded by v2:
https://lore.kernel.org/linux-input/20260825103139.12314-1-98lawweijie@gmail.com/

Please use that one instead.  The only difference is the Assisted-by
tags that Documentation/process/coding-assistants.rst asks for -- AI
tools were used in finding and fixing this bug and I should have
acknowledged that in the original posting -- plus a trim of the commit
message.  The diff is unchanged.

Please mark this one superseded.

Thanks, and sorry for the noise,
Wei Jie