[PATCH] watchdog: airoha: validate heartbeat module parameter

[PATCH] watchdog: airoha: validate heartbeat module parameter

[Wayen Yan]

The probe function directly assigns the heartbeat module parameter to
wdog_dev->timeout without any validation. If heartbeat exceeds
max_timeout, the product timeout * wdt_freq (computed in
airoha_wdt_start()) can overflow the 32-bit WDT_TIMER_LOAD_VALUE
register, causing the watchdog to expire immediately and reboot the
system.

Fix by using watchdog_init_timeout() which validates the timeout
against [min_timeout, max_timeout] range, and falls back to the
device tree "timeout-sec" property if the module parameter is invalid.

Fixes: 3cf67f3769b8 ("watchdog: Add support for Airoha EN7851 watchdog")
Signed-off-by: Wayen Yan <win847@gmail.com>
---
 drivers/watchdog/airoha_wdt.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/watchdog/airoha_wdt.c b/drivers/watchdog/airoha_wdt.c
index dc8ca11c14d8..67160d760d84 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;
 
+	watchdog_init_timeout(wdog_dev, heartbeat, dev);
 	watchdog_set_drvdata(wdog_dev, airoha_wdt);
 	watchdog_set_nowayout(wdog_dev, nowayout);
 	watchdog_stop_on_unregister(wdog_dev);
-- 
2.51.0

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

[sashiko-bot]

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

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

[Wayen Yan]

Please do not apply the above patch. It introduces a regression:

The patch removes the direct assignment "wdog_dev->timeout = heartbeat"
and relies on watchdog_init_timeout() instead. However, since wdog_dev
is allocated via devm_kzalloc(), timeout is initialized to 0. If the
heartbeat module parameter is invalid and there is no "timeout-sec" DT
property, watchdog_init_timeout() returns -EINVAL and leaves timeout
at 0. When the watchdog is subsequently started, airoha_wdt_start()
writes 0 * wdt_freq = 0 to WDT_TIMER_LOAD_VALUE, causing an immediate
hardware reboot — which is worse than the original overflow bug.

I will send a v2 patch that:
1. Pre-initializes timeout to WDT_HEARTBEAT before calling
   watchdog_init_timeout(), so the safe default is preserved on failure
2. Sets min_timeout = 1 to prevent timeout = 0 via ioctl

Sorry for the noise.

Wayen