[PATCH] Input: synaptics-rmi4 - fix NULL dereference in the PM callbacks

[PATCH] Input: synaptics-rmi4 - fix NULL dereference in the PM callbacks

[Yogesh Gaur]

rmi_driver_suspend() and rmi_driver_resume() are exported for the
transport drivers; hid-rmi calls them straight from its ->suspend and
->resume. Both reach code that dereferences the driver data right away:

	static int rmi_suspend_functions(struct rmi_device *rmi_dev)
	{
		struct rmi_driver_data *data = dev_get_drvdata(&rmi_dev->dev);
		...
		list_for_each_entry(entry, &data->function_list, node) {

That data is installed by rmi_driver_probe(), which binds only after the
transport driver has already published rmi_dev via
rmi_register_transport_device(). A runtime PM suspend of the underlying
USB interface in that window dereferences NULL:

	KASAN: null-ptr-deref in range [0x0000000000000078-0x000000000000007f]
	RIP: 0010:dev_get_drvdata include/linux/device.h:989 [inline]
	RIP: 0010:rmi_suspend_functions drivers/input/rmi4/rmi_driver.c:283 [inline]
	RIP: 0010:rmi_driver_suspend+0x30/0x180 drivers/input/rmi4/rmi_driver.c:994
	Call Trace:
	 <TASK>
	 rmi_suspend+0xa7/0xf0 drivers/hid/hid-rmi.c:448
	 hid_suspend+0x4a3/0x530 drivers/hid/usbhid/hid-core.c:1618
	 usb_suspend_interface drivers/usb/core/driver.c:1323 [inline]
	 usb_suspend_both+0x285/0x1040 drivers/usb/core/driver.c:1446
	 usb_runtime_suspend+0x58/0x110 drivers/usb/core/driver.c:2000
	 rpm_callback drivers/base/power/runtime.c:460 [inline]
	 rpm_suspend+0x8a0/0x17a0 drivers/base/power/runtime.c:698
	 pm_runtime_work+0x132/0x1b0 drivers/base/power/runtime.c
	 </TASK>

rmi_disable_irq() and rmi_enable_irq(), reached a few lines further on,
look the driver data up the same way and then take
&data->enabled_mutex, so they would fault too.

There is nothing to suspend or resume before the driver has bound, so
return success early while the driver data is not there yet.

Fixes: 2b6a321da9a2 ("Input: synaptics-rmi4 - add support for Synaptics RMI4 devices")
Reported-by: syzbot+09103639e39c989e3ed3@syzkaller.appspotmail.com
Closes: https://syzkaller.appspot.com/bug?extid=09103639e39c989e3ed3
Signed-off-by: Yogesh Gaur <yogeshgaur.83@gmail.com>
---
 drivers/input/rmi4/rmi_driver.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/drivers/input/rmi4/rmi_driver.c b/drivers/input/rmi4/rmi_driver.c
index 5d49a9021c7d..cff9209ec0b3 100644
--- a/drivers/input/rmi4/rmi_driver.c
+++ b/drivers/input/rmi4/rmi_driver.c
@@ -991,6 +991,14 @@ int rmi_driver_suspend(struct rmi_device *rmi_dev, bool enable_wake)
 {
 	int retval;
 
+	/*
+	 * The transport driver registers rmi_dev before this driver has bound
+	 * to it, so a PM transition can reach here while the driver data does
+	 * not exist yet. There is nothing to suspend in that case.
+	 */
+	if (!dev_get_drvdata(&rmi_dev->dev))
+		return 0;
+
 	retval = rmi_suspend_functions(rmi_dev);
 	if (retval)
 		dev_warn(&rmi_dev->dev, "Failed to suspend functions: %d\n",
@@ -1005,6 +1013,9 @@ int rmi_driver_resume(struct rmi_device *rmi_dev, bool clear_wake)
 {
 	int retval;
 
+	if (!dev_get_drvdata(&rmi_dev->dev))
+		return 0;
+
 	rmi_enable_irq(rmi_dev, clear_wake);
 
 	retval = rmi_resume_functions(rmi_dev);
-- 
2.55.0.windows.5

Re: [PATCH] Input: synaptics-rmi4 - fix NULL dereference in the PM callbacks

[sashiko-bot]

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

New issues:
- [High] Use-After-Free during concurrent device unbind and runtime suspend due to flawed lockless check.

Pre-existing issues:
- [Critical] Slab Out-Of-Bounds read in `rmi_hid_read_block()` due to unvalidated `read_input_count`.
- [Critical] Heap Buffer Overflow in `rmi_hid_write_block()` due to unvalidated write length.
- [High] NULL pointer dereference in `rmi_set_attn_data()` due to unbound driver.
- [High] Use-After-Free due to inverted teardown order of asynchronous reset work in `hid-rmi.c`.
- [High] Memory and resource leaks in `rmi_driver_probe()` error handling paths.
--

commit 8770ae261623b4b40a29cfb4b477b2a4585aca4a
Author: Yogesh Gaur <yogeshgaur.83@gmail.com>

Input: synaptics-rmi4 - fix NULL dereference in the PM callbacks

rmi_driver_suspend() and rmi_driver_resume() are exported for the
transport drivers; hid-rmi calls them straight from its ->suspend and
->resume. Both reach code that dereferences the driver data right away:
[ ... ]

> diff --git a/drivers/input/rmi4/rmi_driver.c b/drivers/input/rmi4/rmi_driver.c
> index 5d49a9021c7d0..cff9209ec0b37 100644
> --- a/drivers/input/rmi4/rmi_driver.c
> +++ b/drivers/input/rmi4/rmi_driver.c
> @@ -991,6 +991,14 @@ int rmi_driver_suspend(struct rmi_device *rmi_dev, bool enable_wake)
>  {
>  	int retval;
>  
> +	/*
> +	 * The transport driver registers rmi_dev before this driver has bound
> +	 * to it, so a PM transition can reach here while the driver data does
> +	 * not exist yet. There is nothing to suspend in that case.
> +	 */
> +	if (!dev_get_drvdata(&rmi_dev->dev))
> +		return 0;
> +

[Severity: High]
Can this lockless check pass during a concurrent device unbind, leading to a
use-after-free?

If the parent USB device runtime suspends concurrently with a sysfs driver
unbind, this check might pass because dev_get_drvdata() is only cleared
by the driver core after rmi_driver_remove() completes. 

In this scenario, rmi_suspend_functions() will iterate over data->function_list
without locks:

drivers/input/rmi4/rmi_driver.c:rmi_suspend_functions() {
    ...
    list_for_each_entry(entry, &data->function_list, node) {
    ...
}

While rmi_driver_remove() concurrently calls rmi_free_function_list(),
which modifies and frees the elements:

drivers/input/rmi4/rmi_driver.c:rmi_free_function_list() {
    ...
    list_del(&fn->node);
    rmi_unregister_function(fn);
    ...
}

Could this cause list corruption and a use-after-free?


[Severity: High]
This isn't a bug introduced by this patch, but since we are fixing NULL pointer
dereferences due to unbound drivers, does the event reporting path suffer from
the exact same issue?

If a hardware event arrives immediately after rmi_register_transport_device()
succeeds but before rmi_driver_probe() completes, hid-rmi's rmi_input_event()
unconditionally forwards the data to rmi_set_attn_data() because RMI_STARTED
is already set:

drivers/hid/hid-rmi.c:rmi_input_event() {
    ...
    rmi_set_attn_data(rmi_dev, data[1], &data[2], size - 2);
    ...
}

Then rmi_set_attn_data() fetches drvdata (which would be NULL) and
dereferences it:

drivers/input/rmi4/rmi_driver.c:rmi_set_attn_data() {
    ...
    struct rmi_driver_data *drvdata = dev_get_drvdata(&rmi_dev->dev);
    ...
    if (!drvdata->enabled)
    ...
}

Could this crash the kernel?


[Severity: High]
This is a pre-existing issue, but does the teardown order of asynchronous reset
work in hid-rmi.c cause a use-after-free?

In rmi_remove(), cancel_work_sync() is called before hid_hw_stop():

drivers/hid/hid-rmi.c:rmi_remove() {
    ...
    cancel_work_sync(&hdata->reset_work);
    rmi_unregister_transport_device(&hdata->xport);
    ...
    hid_hw_stop(hdev);
}

During this window, the hardware is still active. If a mouse report arrives,
rmi_event() triggers and re-queues the asynchronous work:

drivers/hid/hid-rmi.c:rmi_event() {
    ...
    schedule_work(&data->reset_work);
    return 1;
}

After remove() returns and devres frees hdata, the worker executes on freed
memory. Should hid_hw_stop() be called before canceling the work?


[Severity: High]
This is a pre-existing issue, but are there memory and resource leaks in the
error handling paths of rmi_driver_probe()?

If rmi_f34_create_sysfs() fails, the code jumps directly 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 skips the 'err_destroy_functions' label, omitting the function list
and irqdomain cleanup:

drivers/input/rmi4/rmi_driver.c:rmi_driver_probe() {
    ...
err_destroy_functions:
    rmi_free_function_list(rmi_dev);
err:
    return retval;
}

Similarly, if a step like rmi_irq_init() fails, it jumps to
err_destroy_functions, which frees the function list but bypasses sysfs
removal and irqdomain cleanup.


[Severity: Critical]
This is a pre-existing issue, but is there a slab out-of-bounds read in
rmi_hid_read_block() due to an unvalidated read_input_count?

rmi_hid_read_block() trusts the hardware-provided byte data->readReport[1]
as the read_input_count:

drivers/hid/hid-rmi.c:rmi_hid_read_block() {
    ...
    read_input_count = data->readReport[1];
    memcpy(buf + bytes_read, &data->readReport[2],
           min(read_input_count, bytes_needed));
    ...
}

If a malicious device sets this to a large value (e.g., 255) and the driver
requests a large read, memcpy will copy up to 255 bytes from
&data->readReport[2]. Since data->readReport is allocated based on the device's
advertised input_report_size, could this read past the end of the slab
allocation into adjacent memory?


[Severity: Critical]
This is a pre-existing issue, but is there a heap buffer overflow in
rmi_hid_write_block() due to an unvalidated write length?

rmi_hid_write_block() receives 'len' and copies it into data->writeReport[4]:

drivers/hid/hid-rmi.c:rmi_hid_write_block() {
    ...
    data->writeReport[1] = len;
    data->writeReport[2] = addr & 0xFF;
    data->writeReport[3] = (addr >> 8) & 0xFF;
    memcpy(&data->writeReport[4], buf, len);
    ...
}

The size of the data->writeReport buffer (data->output_report_size) is
determined entirely by the HID report descriptor. If a malicious device
advertises a very small output report size (e.g., 5 bytes), and the driver
attempts a standard block write (e.g., 16 bytes), could this memcpy write
past the bounds of data->writeReport?

>  	retval = rmi_suspend_functions(rmi_dev);
>  	if (retval)
>  		dev_warn(&rmi_dev->dev, "Failed to suspend functions: %d\n",

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260908165925.2030-1-yogeshgaur.83@gmail.com?part=1