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

[Guenter Roeck <linux@roeck-us.net>]

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@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.

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;