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

[Tzung-Bi Shih <tzungbi@kernel.org>]

On Wed, Aug 26, 2026 at 06:20:35AM +0000, sashiko-bot@kernel.org wrote:
> 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.

They are real issues.  Sent [1] for fixing if they are still relevant.

[1] https://lore.kernel.org/linux-watchdog/20260827044700.554333-1-tzungbi@kernel.org

> > -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().

https://lore.kernel.org/linux-watchdog/20260827044700.554333-2-tzungbi@kernel.org

> [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.

https://lore.kernel.org/linux-watchdog/20260827044700.554333-3-tzungbi@kernel.org