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

[Stefan Wahren]

Hi Guenter,

Am 27.10.24 um 18:35 schrieb Guenter Roeck:
> On 10/27/24 08:54, Stefan Wahren wrote:
>> Am 27.10.24 um 14:36 schrieb 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.
>> It's funny because I tried this on a i.MX93 board but it didn't work for
>> me. But I must confess that I didn't spend much time in the
>> investigation.
>
> I can't test it, but something like the diff below should do.
Thanks, I will give it a try but it will take some time.

Regards
>
> Guenter
>
> ---
> diff --git a/drivers/watchdog/imx7ulp_wdt.c
> b/drivers/watchdog/imx7ulp_wdt.c
> index 0f13a3053357..e672d27af63e 100644
> --- a/drivers/watchdog/imx7ulp_wdt.c
> +++ b/drivers/watchdog/imx7ulp_wdt.c
> @@ -187,11 +187,16 @@ static int imx7ulp_wdt_set_timeout(struct
> watchdog_device *wdog,
>                                    unsigned int timeout)
>  {
>         struct imx7ulp_wdt_device *wdt = watchdog_get_drvdata(wdog);
> -       u32 toval = wdt->hw->wdog_clock_rate * timeout;
> +       u32 toval;
>         u32 val;
>         int ret;
>         u32 loop = RETRY_MAX;
>
> +       if (timeout > 0xffff / wdt->hw->wdog_clock_rate)
> +               toval = 0xffff;
> +       else
> +               toval = wdt->hw->wdog_clock_rate * timeout;
> +
>         do {
>                 ret = _imx7ulp_wdt_set_timeout(wdt, toval);
>                 val = readl(wdt->base + WDOG_TOVAL);
> @@ -338,7 +343,6 @@ static int imx7ulp_wdt_probe(struct
> platform_device *pdev)
>         wdog->info = &imx7ulp_wdt_info;
>         wdog->ops = &imx7ulp_wdt_ops;
>         wdog->min_timeout = 1;
> -       wdog->max_timeout = MAX_TIMEOUT;
>         wdog->parent = dev;
>         wdog->timeout = DEFAULT_TIMEOUT;
>
> @@ -348,6 +352,7 @@ static int imx7ulp_wdt_probe(struct
> platform_device *pdev)
>         watchdog_set_drvdata(wdog, imx7ulp_wdt);
>
>         imx7ulp_wdt->hw = of_device_get_match_data(dev);
> +       wdog->max_hw_heartbeat_ms = 0xffff * 1000 /
> imx7ulp_wdt->hw->wdog_clock_rate;
>         ret = imx7ulp_wdt_init(imx7ulp_wdt, wdog->timeout *
> imx7ulp_wdt->hw->wdog_clock_rate);
>         if (ret)
>                 return ret;
>
>