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