Re: [PATCH] watchdog: airoha: validate heartbeat module parameter

[sashiko-bot@kernel.org]

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

New issues:
- [High] The patch removes the direct assignment of `wdog_dev->timeout = heartbeat;` to fix an overflow issue, and relies on `watchdog_init_timeout()` for validation. However, if the `heartbeat` parameter is invalid and there is no `timeout-sec` DT property, `watchdog_init_timeout()` returns `-EINVAL` and leaves `wdog_dev->timeout` unmodified. Because `wdog_dev` is allocated via `devm_kzalloc()`, `timeout` is 0. The driver ignores the `-EINVAL` return value and registers the watchdog with a timeout of 0. When `/dev/watchdog` is subsequently opened, `airoha_wdt_start()` writes `0 * airoha_wdt->wdt_freq` (0) to `WDT_TIMER_LOAD_VALUE`. This instantly expires the timer and triggers an immediate hardware reboot. The driver must pre-initialize `wdog_dev->timeout` to a valid default (e.g., `WDT_HEARTBEAT`) before calling `watchdog_init_timeout()`.

Pre-existing issues:
- [High] This problem wasn't introduced by this patch, but the driver leaves `wdog_dev->min_timeout` uninitialized (0). The watchdog core's `watchdog_timeout_invalid()` relies on `min_timeout` to validate lower bounds. With `min_timeout` set to 0, userspace can successfully execute `ioctl(WDIOC_SETTIMEOUT)` with a timeout of 0. The `airoha_wdt_set_timeout()` function assigns this 0 to `wdog_dev->timeout` and restarts the hardware timer. This causes `airoha_wdt_start()` to write 0 to the timer load register, which triggers an instantaneous hardware reset without proper shutdown. The driver should explicitly set `wdog_dev->min_timeout = 1`.
--

--- Patch [1]: [PATCH] watchdog: airoha: validate heartbeat module parameter ---
commit 1b1b24f68ee022a802b613038f9562b2f86dffc9
Author: Wayen Yan <win847@gmail.com>

watchdog: airoha: validate heartbeat module parameter

This patch replaces a direct assignment of the heartbeat module parameter
with a call to watchdog_init_timeout() to properly validate the timeout
range. This prevents an integer overflow in the timer load calculation
that could otherwise cause the watchdog to expire immediately.

> diff --git a/drivers/watchdog/airoha_wdt.c b/drivers/watchdog/airoha_wdt.c
> index dc8ca11c14d81..67160d760d844 100644
> --- a/drivers/watchdog/airoha_wdt.c
> +++ b/drivers/watchdog/airoha_wdt.c
> @@ -150,13 +150,13 @@ static int airoha_wdt_probe(struct platform_device *pdev)
>  
>  	/* Initialize struct watchdog device */
>  	wdog_dev = &airoha_wdt->wdog_dev;
> -	wdog_dev->timeout = heartbeat;
>  	wdog_dev->info = &airoha_wdt_info;
>  	wdog_dev->ops = &airoha_wdt_ops;
>  	/* Bus 300MHz, watchdog 150MHz, 28 seconds */
>  	wdog_dev->max_timeout = FIELD_MAX(WDT_TIMER_VAL) / airoha_wdt->wdt_freq;
>  	wdog_dev->parent = dev;

[Severity: High]
This is a pre-existing issue, but should the driver also explicitly 
initialize wdog_dev->min_timeout?

Because min_timeout is currently left uninitialized (0), the watchdog core's
watchdog_timeout_invalid() function allows a timeout of 0. If userspace
calls ioctl(WDIOC_SETTIMEOUT) with 0, airoha_wdt_set_timeout() will assign 0
to wdog_dev->timeout.

This results in airoha_wdt_start() writing 0 to the timer load register,
which triggers an immediate, ungraceful hardware reset:

drivers/watchdog/airoha_wdt.c:airoha_wdt_start() {
        ...
        val = wdog_dev->timeout * airoha_wdt->wdt_freq;
        writel(val, airoha_wdt->base + REG_WDT_TIMER_LOAD_VALUE);
        ...
}

Setting wdog_dev->min_timeout = 1 would prevent userspace from configuring a
zero timeout.

> +	watchdog_init_timeout(wdog_dev, heartbeat, dev);

[Severity: High]
If watchdog_init_timeout() fails, does this leave wdog_dev->timeout set 
to 0?

The kzalloc allocation zero-initializes wdog_dev, and the patch removes
the direct assignment of wdog_dev->timeout. If the heartbeat parameter is
invalid and there is no timeout-sec device tree property, 
watchdog_init_timeout() returns -EINVAL and leaves wdog_dev->timeout as 0.

Because the return value is ignored, the watchdog registers successfully.
When a userspace daemon subsequently opens /dev/watchdog, airoha_wdt_start() 
runs:

drivers/watchdog/airoha_wdt.c:airoha_wdt_start() {
        ...
        val = wdog_dev->timeout * airoha_wdt->wdt_freq;
        writel(val, airoha_wdt->base + REG_WDT_TIMER_LOAD_VALUE);
        ...
}

This calculates a load value of 0, immediately expiring the timer and
rebooting the system.

Would it be better to initialize wdog_dev->timeout to a valid default
value before calling watchdog_init_timeout(), or explicitly handle the 
error return value?

>  	watchdog_set_drvdata(wdog_dev, airoha_wdt);
>  	watchdog_set_nowayout(wdog_dev, nowayout);
>  	watchdog_stop_on_unregister(wdog_dev);

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