[PATCH 2/2] watchdog: imx7ulp_wdt: Add TOVAL range check
Stefan Wahren
wahrenst at gmx.net
Mon Oct 28 09:52:03 PDT 2024
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;
>
>
More information about the linux-arm-kernel
mailing list