Re: [PATCH] watchdog: starfive: fix runtime PM usage counter leak in pm_start

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

Pre-existing issues:
- [Critical] Unprotected hardware access in get_timeleft sysfs callback allows unprivileged local DoS.
- [High] The patch fixes a runtime PM usage counter leak when pm_runtime_get_sync() fails, but leaves an identical leak when the subsequent starfive_wdt_start() fails.
- [High] Unclocked hardware register accesses in suspend, shutdown, and remove paths cause bus faults.
- [High] Leaked runtime PM usage counter and missing pm_runtime_disable() in starfive_wdt_probe() error paths.
- [High] Runtime PM usage counter and clocks are permanently leaked in remove if early_enable is true and the watchdog was never opened.
- [High] The hardware watchdog is not restarted on system resume if early_enable is true but the device was not opened.
--

--- Patch [1]: [PATCH] watchdog: starfive: fix runtime PM usage counter leak in pm_start ---
commit 6f33b73ef6f14abdd5d861e7b791ec5774bc787e
Author: manushprajwal <[email protected]>

watchdog: starfive: fix runtime PM usage counter leak in pm_start

This commit fixes a runtime PM usage counter leak in starfive_wdt_pm_start().
It ensures pm_runtime_put_noidle() is called when pm_runtime_get_sync()
returns an error.

> diff --git a/drivers/watchdog/starfive-wdt.c b/drivers/watchdog/starfive-wdt.c
> index af55adc4a3c69..ecb704b2e9a3d 100644
> --- a/drivers/watchdog/starfive-wdt.c
> +++ b/drivers/watchdog/starfive-wdt.c
> @@ -373,8 +373,10 @@ static int starfive_wdt_pm_start(struct watchdog_device *wdd)
>  	struct starfive_wdt *wdt = watchdog_get_drvdata(wdd);
>  	int ret = pm_runtime_get_sync(wdd->parent);
>  
> -	if (ret < 0)
> +	if (ret < 0) {
> +		pm_runtime_put_noidle(wdd->parent);
>  		return ret;
> +	}
>  
>  	return starfive_wdt_start(wdt);
>  }

[Severity: High]
This is a pre-existing issue, but does this still leak the runtime PM usage
counter if starfive_wdt_start() fails?

If starfive_wdt_start() returns a negative error (for example, if it
times out waiting for an interrupt clear), the error is returned directly.
Since the pm_runtime_get_sync() succeeded, doesn't this return path skip
dropping the PM reference, permanently preventing the device from suspending?


[Severity: High]
This is a pre-existing issue, but do the suspend, remove, and shutdown paths
access hardware registers while the device is suspended?

In starfive_wdt_suspend(), the registers are accessed unconditionally:

    drivers/watchdog/starfive-wdt.c:starfive_wdt_suspend() {
        ...
        wdt->reload = starfive_wdt_get_count(wdt);
        /* Note that WTCNT doesn't need to be saved. */
        starfive_wdt_stop(wdt);
        ...
    }

If the watchdog was not opened by userspace and early_enable is false, the
device resides in a runtime-suspended state with clocks disabled. Could
accessing its registers in this state trigger a synchronous external abort
(bus fault)? Similarly, the remove and shutdown paths also call
starfive_wdt_stop() directly.


[Severity: High]
This is a pre-existing issue, but do the error paths in starfive_wdt_probe()
leak the runtime PM usage counter or miss disabling PM?

If pm_runtime_resume_and_get() fails:

    drivers/watchdog/starfive-wdt.c:starfive_wdt_probe() {
        ...
        ret = pm_runtime_resume_and_get(&pdev->dev);
        if (ret < 0)
            return ret;
        ...
    }

Should this call pm_runtime_disable() before returning?

Additionally, if a subsequent step fails, it jumps to err_exit:

    drivers/watchdog/starfive-wdt.c:starfive_wdt_probe() {
        ...
    err_exit:
        starfive_wdt_disable_clock(wdt);
        pm_runtime_disable(&pdev->dev);

        return ret;
    }

Does this path need to call pm_runtime_put_noidle() to drop the usage
counter before disabling PM?


[Severity: High]
This is a pre-existing issue, but does starfive_wdt_remove() leak the
runtime PM reference and clocks if early_enable is true?

If early_enable is set, the probe function leaves the PM usage count at 1.
If userspace never opens the watchdog, WDOG_ACTIVE remains false.
During remove:

    drivers/watchdog/starfive-wdt.c:starfive_wdt_remove() {
        ...
        if (pm_runtime_enabled(&pdev->dev))
            pm_runtime_disable(&pdev->dev);
        ...
    }

Since watchdog_unregister_device() bypasses the stop callback when
WDOG_ACTIVE is false, and remove() only disables runtime PM without dropping
the usage counter, does this prevent the device from ever suspending and
leave the clocks running indefinitely?


[Severity: High]
This is a pre-existing issue, but does starfive_wdt_resume() fail to
restart the hardware if early_enable is true but the device is unopened?

    drivers/watchdog/starfive-wdt.c:starfive_wdt_resume() {
        ...
        if (watchdog_active(&wdt->wdd))
            return starfive_wdt_start(wdt);

        return 0;
    }

If the watchdog was started via early_enable but not opened by userspace,
watchdog_active() is false. Since starfive_wdt_suspend() unconditionally
stops the hardware, does this leave the system without hardware watchdog
protection after resuming?


[Severity: Critical]
This is a pre-existing issue, but does starfive_wdt_get_timeleft()
allow an unprivileged local denial of service?

    drivers/watchdog/starfive-wdt.c:starfive_wdt_get_timeleft() {
        ...
        count = starfive_wdt_get_count(wdt);
        ...
    }

This function is called when a user reads the 0444 sysfs attribute
timeleft. If the watchdog is closed and the device is runtime-suspended,
its clocks are disabled. Could an unprivileged user reading this sysfs
attribute cause an immediate bus fault (kernel panic) by accessing
unclocked registers?

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