[PATCH 2/2] watchdog: imx7ulp_wdt: Add TOVAL range check

[Guenter Roeck]

On 10/27/24 03:53, Stefan Wahren wrote:
> The WDOG Timeout Value (TOVAL) is a 16 bit value, which is stored
> at the beginning of a 32 bit register. So add a range check to
> prevent writing in the reserved register area.
> 
> Signed-off-by: Stefan Wahren <wahrenst at gmx.net>
> ---
>   drivers/watchdog/imx7ulp_wdt.c | 8 ++++++++
>   1 file changed, 8 insertions(+)
> 
> diff --git a/drivers/watchdog/imx7ulp_wdt.c b/drivers/watchdog/imx7ulp_wdt.c
> index 0f92d2217088..a7574f9c9150 100644
> --- a/drivers/watchdog/imx7ulp_wdt.c
> +++ b/drivers/watchdog/imx7ulp_wdt.c
> @@ -48,6 +48,8 @@
> 
>   #define RETRY_MAX 5
> 
> +#define TOVAL_MAX	0xFFFF
> +
>   static bool nowayout = WATCHDOG_NOWAYOUT;
>   module_param(nowayout, bool, 0000);
>   MODULE_PARM_DESC(nowayout, "Watchdog cannot be stopped once started (default="
> @@ -192,6 +194,9 @@ static int imx7ulp_wdt_set_timeout(struct watchdog_device *wdog,
>   	int ret;
>   	u32 loop = RETRY_MAX;
> 
> +	if (toval > TOVAL_MAX)
> +		return -EINVAL;
> +

The whole idea of having max_timeout in struct watchdog_device is to avoid the need
for this check. max_timeout should be set to 0xffff / wdt->hw->wdog_clock_rate.
It is currently set to 128. With wdt->hw->wdog_clock_rate set to either 125 or 1000,
it can indeed overflow. However, checking the value above is wrong. max_timeout should
be initialized correctly instead.

Even better would be to set max_hw_heartbeat_ms and let the watchdog core handle
larger timeouts.

Another question is why the driver enables a clock but doesn't use its actual
frequency.

Guenter