Re: [PATCH 4/8] watchdog: msc313e_wdt: Convert to DEFINE_SIMPLE_DEV_PM_OPS()

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Missing platform_set_drvdata() or dev_set_drvdata() in the probe function leads to a guaranteed NULL pointer dereference during suspend/resume operations.
- [High] The driver accesses hardware registers without ensuring the required clock is enabled, which can lead to a synchronous bus fault.
--

commit 906c6a1c7ebc485b90aee7c86e2994040111e762
Author: Triet Hoang <triet.hoang.dev@gmail.com>

watchdog: msc313e_wdt: Convert to DEFINE_SIMPLE_DEV_PM_OPS()
    
Convert deprecated SIMPLE_DEV_PM_OPS() to DEFINE_SIMPLE_DEV_PM_OPS()
and pm_sleep_ptr(). This lets us drop the __maybe_unused annotations
from the resume callback.

> diff --git a/drivers/watchdog/msc313e_wdt.c b/drivers/watchdog/msc313e_wdt.c
> index 90171431fc594..b8edf85f10236 100644
> --- a/drivers/watchdog/msc313e_wdt.c
> +++ b/drivers/watchdog/msc313e_wdt.c
> @@ -133,7 +133,7 @@ static int msc313e_wdt_probe(struct platform_device *pdev)
>  	return devm_watchdog_register_device(dev, &priv->wdev);
>  }
>  
> -static int __maybe_unused msc313e_wdt_suspend(struct device *dev)
> +static int msc313e_wdt_suspend(struct device *dev)
>  {
>  	struct msc313e_wdt_priv *priv = dev_get_drvdata(dev);

[Severity: High]
This isn't a bug introduced by this patch, but does this result in a NULL
pointer dereference when the system suspends?

The PM core invokes msc313e_wdt_suspend() and passes the platform device as the
dev parameter. However, msc313e_wdt_probe() never calls platform_set_drvdata()
or dev_set_drvdata() for the platform device:

drivers/watchdog/msc313e_wdt.c:msc313e_wdt_probe() {
    ...
	watchdog_set_drvdata(&priv->wdev, priv);

	watchdog_init_timeout(&priv->wdev, timeout, dev);
	watchdog_stop_on_reboot(&priv->wdev);
	watchdog_stop_on_unregister(&priv->wdev);

	return devm_watchdog_register_device(dev, &priv->wdev);
}

Because the driver data is uninitialized, dev_get_drvdata(dev) evaluates to
NULL. The callback then computes an offset into this NULL pointer via
&priv->wdev and passes it to watchdog_active(), which unconditionally
dereferences it via test_bit(WDOG_ACTIVE, &wdd->status), causing a fatal
page fault.

The identical NULL pointer dereference logic also executes on the resume path
in msc313e_wdt_resume().

[Severity: High]
This is also a pre-existing issue, but does this driver access hardware
registers without ensuring the required clock is enabled?

The probe function reads from hardware registers without first calling
clk_prepare_enable(), which can crash if the clock is disabled by the
bootloader:

drivers/watchdog/msc313e_wdt.c:msc313e_wdt_probe() {
    ...
	/* If the period is non-zero the WDT is running */
	if (readw(priv->base + REG_WDT_MAX_PRD_L) | (readw(priv->base + REG_WDT_MAX_PRD_H) << 16))
		set_bit(WDOG_HW_RUNNING, &priv->wdev.status);
    ...
}

Additionally, because the clock is never enabled in the probe function, when
the Common Clock Framework's late_initcall executes, it physically disables
the unreferenced clock. If the watchdog is already running at boot time, the
watchdog core will periodically call the background ping callback:

drivers/watchdog/msc313e_wdt.c:msc313e_wdt_ping() {
    ...
	writew(1, priv->base + REG_WDT_CLR);
    ...
}

Writing to these unclocked watchdog registers will trigger a synchronous
bus fault.

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260826060839.203297-1-triet.hoang.dev@gmail.com?part=4