[PATCH 2/2] watchdog: imx7ulp_wdt: Add TOVAL range check
Guenter Roeck
linux at roeck-us.net
Sun Oct 27 06:36:57 PDT 2024
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
More information about the linux-arm-kernel
mailing list