Re: [PATCH] Input: synaptics-rmi4 - fix GPF in suspend and resume when unbound

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

Pre-existing issues:
- [High] The exported function `rmi_set_attn_data()` dereferences `drvdata` without a NULL check, leading to a potential NULL pointer dereference if the physical device probe fails.
- [High] If `rmi_driver_probe()` fails after `rmi_f34_create_sysfs()` succeeds, the error path fails to call `rmi_f34_remove_sysfs()`, leaking the sysfs group. Accessing the leaked sysfs files causes a NULL pointer dereference.
- [Medium] If `rmi_suspend_functions()` fails, `rmi_driver_suspend()` and `rmi_i2c_suspend()` disable IRQs and regulators, but propagate the error to the PM core. The PM core aborts suspend and does not call `resume` for this device, leaving it permanently powered off.
- [Low] Misleading copy-paste error messages in suspend and resume paths.
--

--- Patch [1]: [PATCH] Input: synaptics-rmi4 - fix GPF in suspend and resume when unbound ---
commit 19d89c520c8ddb566dee79a7cfbffc9acc8839a2
Author: Dmitry Torokhov <[email protected]>

Input: synaptics-rmi4 - fix GPF in suspend and resume when unbound

Transport drivers invoke rmi_driver_suspend() and rmi_driver_resume()
on their child rmi_dev device during power management events. This
prevents a GPF by checking if driver data is attached, exiting early
if not present.

Closes: https://syzkaller.appspot.com/bug?extid=09103639e39c989e3ed3

> diff --git a/drivers/input/rmi4/rmi_driver.c b/drivers/input/rmi4/rmi_driver.c
> index 5d49a9021c7d0..a349dfd17519c 100644
> --- a/drivers/input/rmi4/rmi_driver.c
> +++ b/drivers/input/rmi4/rmi_driver.c
> @@ -991,6 +991,15 @@ int rmi_driver_suspend(struct rmi_device *rmi_dev, bool enable_wake)
>  {
>  	int retval;
>  
> +	/*
> +	 * Transport driver will try to suspend RMI device even if physical
> +	 * driver did not bind to the RMI device, because transport device
> +	 * (I2C, SPI) is fully registered and operational. Exit early if
> +	 * there is no driver data attached to the RMI device.
> +	 */
> +	if (!dev_get_drvdata(&rmi_dev->dev))
> +		return 0;
> +
>  	retval = rmi_suspend_functions(rmi_dev);

[Severity: Medium]
This isn't a bug introduced by this patch, but looking at the rest of
rmi_driver_suspend():

rmi_driver_suspend() {
    ...
    retval = rmi_suspend_functions(rmi_dev);
    if (retval)
        dev_warn(&rmi_dev->dev, "Failed to suspend functions: %d\n", retval);
    
    rmi_disable_irq(rmi_dev, enable_wake);
    return retval;
}

If rmi_suspend_functions() fails, the error path still disables the IRQ
and returns the error to the PM core. The I2C transport driver then
unconditionally disables regulators in rmi_i2c_suspend() and also
propagates the error.

Since the PM core aborts suspend and skips calling resume for devices
that return an error, does this leave the device permanently disabled and
unresponsive?

> @@ -1005,6 +1014,10 @@ int rmi_driver_resume(struct rmi_device *rmi_dev, bool clear_wake)
>  {
>  	int retval;
>  
> +	/* Skip if not fully bound to RMI driver */
> +	if (!dev_get_drvdata(&rmi_dev->dev))
> +		return 0;
> +
>  	rmi_enable_irq(rmi_dev, clear_wake);
>  
>  	retval = rmi_resume_functions(rmi_dev);

[Severity: Low]
This is a pre-existing issue, but further down in rmi_driver_resume() there
appears to be a copy-paste error in the warning message:

rmi_driver_resume() {
    ...
    retval = rmi_resume_functions(rmi_dev);
    if (retval)
        dev_warn(&rmi_dev->dev, "Failed to suspend functions: %d\n",
            retval);
    ...
}

Should this warning say "Failed to resume functions"? Similarly, in
rmi_i2c_suspend(), there is a warning that says "Failed to resume device"
when suspend fails.

[Severity: High]
This is a pre-existing issue, but since this patch addresses NULL pointer
dereferences from unbound devices, is there a similar vulnerability in
rmi_set_attn_data()?

rmi_set_attn_data() {
    struct rmi_driver_data *drvdata = dev_get_drvdata(&rmi_dev->dev);
    
    if (!drvdata->enabled)
        return;
    ...
}

If rmi_driver_probe() fails, dev_get_drvdata() is cleared to NULL. However,
the HID transport driver (hid-rmi) sets the RMI_STARTED bit after successful
transport registration, independent of the physical driver probe.

If hardware sends events or malicious data is provided after a probe failure,
can rmi_input_event() bypass the RMI_STARTED check and call
rmi_set_attn_data(), leading to a NULL pointer dereference when accessing
drvdata->enabled?

[Severity: High]
This is another pre-existing issue, but looking at the error handling in
rmi_driver_probe():

rmi_driver_probe() {
    ...
    err_destroy_functions:
        rmi_free_function_list(rmi_dev);
    err:
        return retval;
}

If rmi_driver_probe() fails after successfully calling
rmi_f34_create_sysfs(), it jumps to err_destroy_functions but misses a
call to rmi_f34_remove_sysfs().

Does this leak the sysfs group? If the sysfs group is leaked and the driver
core clears drvdata, could a later user-space read to the world-readable
update_fw_status sysfs attribute trigger a NULL pointer dereference in
rmi_driver_update_fw_status_show() when it checks data->f34_container?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1