Re: [PATCH] hwmon: (gpio-fan) fix use-after-free in alarm work

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

Pre-existing issues:
- [High] The shared interrupt handler `fan_alarm_irq_handler` unconditionally returns `IRQ_NONE` despite successfully handling the interrupt by scheduling work.
- [High] The driver's devres management of `pm_runtime_disable` is unbalanced, leading to PM state leaks on unbind without control GPIOs, or corrupted PM state across probe failures.
- [High] Error paths in `set_fan_speed` and `gpio_fan_probe` cause software state to desynchronize from runtime PM state, triggering repeated usage count underflow warnings.
--

--- Patch [1]: [PATCH] hwmon: (gpio-fan) fix use-after-free in alarm work ---
commit 3d50f01d30cc7a7c52ecf0ad47ea90b3b37fc216
Author: Fan Wu <[email protected]>

hwmon: (gpio-fan) fix use-after-free in alarm work

fan_alarm_irq_handler() queues fan_data->alarm_work, but nothing cancels it.
Replace INIT_WORK() with devm_work_autocancel() to safely cancel the work
during devres cleanup.

> diff --git a/drivers/hwmon/gpio-fan.c b/drivers/hwmon/gpio-fan.c
> index 084828e1e2817..7f36e5f6f2230 100644
> --- a/drivers/hwmon/gpio-fan.c
> +++ b/drivers/hwmon/gpio-fan.c
> @@ -84,6 +85,7 @@ static DEVICE_ATTR_RO(fan1_alarm);
>  static int fan_alarm_init(struct gpio_fan_data *fan_data)
>  {
>  	int alarm_irq;
> +	int err;
>  	struct device *dev = fan_data->dev;
>  
>  	/*
> @@ -94,7 +96,11 @@ static int fan_alarm_init(struct gpio_fan_data *fan_data)
>  	if (alarm_irq <= 0)
>  		return 0;
>  
> -	INIT_WORK(&fan_data->alarm_work, fan_alarm_notify);
> +	err = devm_work_autocancel(dev, &fan_data->alarm_work,
> +				   fan_alarm_notify);
> +	if (err)
> +		return err;

[Severity: High]
This is a pre-existing issue, but does returning an error here expose an
unbalanced PM runtime state?

If fan_alarm_init() fails here, probe aborts and devres cleanup will execute
gpio_fan_stop(). Looking at gpio_fan_stop(), it unconditionally calls
pm_runtime_disable() on fan_data->dev even if pm_runtime_enable() was never
reached in probe.

Since gpio_fan_probe() registers gpio_fan_stop() via devm_add_action_or_reset()
before calling pm_runtime_enable(), couldn't this failure cause
pm_runtime_disable() to be called without a prior enable? This would increment
the disable depth to 2, permanently disabling runtime PM on subsequent deferred
probes.

Additionally, if the device has no control GPIOs, gpio_fan_stop() is never
registered. probe() unconditionally calls pm_runtime_enable(), but on unbind
pm_runtime_disable() is never called, leaking the PM state.

[Severity: High]
This is also a pre-existing issue, but can error paths in set_fan_speed() and
gpio_fan_probe() cause runtime PM usage count underflows?

In set_fan_speed(), if pm_runtime_put_sync() fails, it returns early:

drivers/hwmon/gpio-fan.c:set_fan_speed() {
    ...
    if (ret < 0 && ret != -ENOSYS)
        return ret;
    ...
}

This skips updating fan_data->speed_index = 0. The usage count was decremented,
but the software state assumes the fan is still active. Could subsequent calls
to stop the fan redundantly call pm_runtime_put_sync(), causing an underflow?

Similarly, in gpio_fan_probe(), if the fan is initially active (speed_index > 0)
but pm_runtime_resume_and_get(&pdev->dev) fails, probe returns an error without
clearing speed_index. During devres unwind, gpio_fan_stop() calls
set_fan_speed(0). Because speed_index > 0, it calls pm_runtime_put_sync() on a
usage count of 0, triggering an underflow warning.

> +
>  	irq_set_irq_type(alarm_irq, IRQ_TYPE_EDGE_BOTH);
>  	return devm_request_irq(dev, alarm_irq, fan_alarm_irq_handler,
>  				IRQF_SHARED, "GPIO fan alarm", fan_data);
>  }

[Severity: High]
This isn't a bug introduced by this patch, but does fan_alarm_irq_handler()
correctly return its IRQ status?

Looking at fan_alarm_irq_handler() in drivers/hwmon/gpio-fan.c:

drivers/hwmon/gpio-fan.c:fan_alarm_irq_handler() {
    ...
    schedule_work(&fan_data->alarm_work);

    return IRQ_NONE;
}

Since the handler unconditionally returns IRQ_NONE despite successfully
queueing the work, and the interrupt is registered as IRQF_SHARED, could the
kernel's spurious interrupt detector permanently disable the shared interrupt
line once the unhandled count exceeds the threshold?

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