diff mbox series

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

Message ID 20210416021337.18715-1-rentao.bupt@gmail.com (mailing list archive)
State New
Headers show
Series [v2] watchdog: aspeed: fix integer overflow in set_timeout handler | expand

Commit Message

Tao Ren April 16, 2021, 2:13 a.m. UTC
From: Tao Ren <rentao.bupt@gmail.com>

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 <amithash@fb.com>
Signed-off-by: Tao Ren <rentao.bupt@gmail.com>
---
 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(-)

Comments

Guenter Roeck April 16, 2021, 5:07 a.m. UTC | #1
On 4/15/21 7:13 PM, rentao.bupt@gmail.com wrote:
> From: Tao Ren <rentao.bupt@gmail.com>
> 
> 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 <amithash@fb.com>
> Signed-off-by: Tao Ren <rentao.bupt@gmail.com>
> ---
>  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);
>
Tao Ren April 16, 2021, 6:25 p.m. UTC | #2
On Thu, Apr 15, 2021 at 10:07:32PM -0700, Guenter Roeck wrote:
> On 4/15/21 7:13 PM, rentao.bupt@gmail.com wrote:
> > From: Tao Ren <rentao.bupt@gmail.com>
> > 
> > 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
diff mbox series

Patch

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);