Re: [PATCH v2] watchdog: aspeed: fix integer overflow in set_timeout handler

[Tao Ren]

On Thu, Apr 15, 2021 at 10:07:32PM -0700, Guenter Roeck wrote:
> On 4/15/21 7:13 PM, rentao.b...@gmail.com wrote:
> > From: Tao Ren 
> > 
> > Fix the time comparison (timeout vs. max_hw_heartbeat_ms) in set_timeout
> > handler to avoid potential integer overflow when the supplied timeout is
> > greater than aspeed's maximum allowed timeout (4294 seconds).
> > 
> 
> I think this is the wrong focus: What this fixes is the wrong hardware
> timeout calculation. Again, I think that the wrong calculation leads to
> the overflow should not be the focus of this patch, though it can of
> course be mentioned.
> 
> I'll leave it up to Wim to decide if he wants to apply the patch with the
> current explanation.
> 
> Thanks,
> Guenter

Sorry I didn't get your point correctly, and I guess it was because of
my lack of knowledge in timeout/max_hw_heartbeat_ms/worker (hopefully
my understanding is correct now :))

Let me drop this patch and send a new one with different subject and
description soon.


Cheers,

Tao

Re: [PATCH v2] watchdog: aspeed: fix integer overflow in set_timeout handler

[Guenter Roeck]

On 4/15/21 7:13 PM, rentao.b...@gmail.com wrote:
> From: Tao Ren 
> 
> Fix the time comparison (timeout vs. max_hw_heartbeat_ms) in set_timeout
> handler to avoid potential integer overflow when the supplied timeout is
> greater than aspeed's maximum allowed timeout (4294 seconds).
> 

I think this is the wrong focus: What this fixes is the wrong hardware
timeout calculation. Again, I think that the wrong calculation leads to
the overflow should not be the focus of this patch, though it can of
course be mentioned.

I'll leave it up to Wim to decide if he wants to apply the patch with the
current explanation.

Thanks,
Guenter

> Fixes: efa859f7d786 ("watchdog: Add Aspeed watchdog driver")
> Reported-by: Amithash Prasad 
> Signed-off-by: Tao Ren 
> ---
>  Changes in v2:
>- do not touch "wdd->timeout": only "max_hw_heartbeat_ms * 1000" is
>  updated to "max_hw_heartbeat_ms / 1000".
> 
>  drivers/watchdog/aspeed_wdt.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/watchdog/aspeed_wdt.c b/drivers/watchdog/aspeed_wdt.c
> index 7e00960651fa..507fd815d767 100644
> --- a/drivers/watchdog/aspeed_wdt.c
> +++ b/drivers/watchdog/aspeed_wdt.c
> @@ -147,7 +147,7 @@ static int aspeed_wdt_set_timeout(struct watchdog_device 
> *wdd,
>  
>   wdd->timeout = timeout;
>  
> - actual = min(timeout, wdd->max_hw_heartbeat_ms * 1000);
> + actual = min(timeout, wdd->max_hw_heartbeat_ms / 1000);
>  
>   writel(actual * WDT_RATE_1MHZ, wdt->base + WDT_RELOAD_VALUE);
>   writel(WDT_RESTART_MAGIC, wdt->base + WDT_RESTART);
> 

[PATCH v2] watchdog: aspeed: fix integer overflow in set_timeout handler

[rentao . bupt]

From: Tao Ren 

Fix the time comparison (timeout vs. max_hw_heartbeat_ms) in set_timeout
handler to avoid potential integer overflow when the supplied timeout is
greater than aspeed's maximum allowed timeout (4294 seconds).

Fixes: efa859f7d786 ("watchdog: Add Aspeed watchdog driver")
Reported-by: Amithash Prasad 
Signed-off-by: Tao Ren 
---
 Changes in v2:
   - do not touch "wdd->timeout": only "max_hw_heartbeat_ms * 1000" is
 updated to "max_hw_heartbeat_ms / 1000".

 drivers/watchdog/aspeed_wdt.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/watchdog/aspeed_wdt.c b/drivers/watchdog/aspeed_wdt.c
index 7e00960651fa..507fd815d767 100644
--- a/drivers/watchdog/aspeed_wdt.c
+++ b/drivers/watchdog/aspeed_wdt.c
@@ -147,7 +147,7 @@ static int aspeed_wdt_set_timeout(struct watchdog_device 
*wdd,
 
wdd->timeout = timeout;
 
-   actual = min(timeout, wdd->max_hw_heartbeat_ms * 1000);
+   actual = min(timeout, wdd->max_hw_heartbeat_ms / 1000);
 
writel(actual * WDT_RATE_1MHZ, wdt->base + WDT_RELOAD_VALUE);
writel(WDT_RESTART_MAGIC, wdt->base + WDT_RESTART);
-- 
2.17.1