Linux-Watchdog Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH v2] watchdog: aspeed: fix integer overflow in set_timeout handler
@ 2021-04-16  2:13 rentao.bupt
  2021-04-16  5:07 ` Guenter Roeck
  0 siblings, 1 reply; 3+ messages in thread
From: rentao.bupt @ 2021-04-16  2:13 UTC (permalink / raw)
  To: Wim Van Sebroeck, Guenter Roeck, Joel Stanley, Andrew Jeffery,
	linux-watchdog, linux-arm-kernel, linux-aspeed, linux-kernel,
	openbmc, Tao Ren, Amithash Prasad
  Cc: Tao Ren

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

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


^ permalink raw reply	[flat|nested] 3+ messages in thread

* Re: [PATCH v2] watchdog: aspeed: fix integer overflow in set_timeout handler
  2021-04-16  2:13 [PATCH v2] watchdog: aspeed: fix integer overflow in set_timeout handler rentao.bupt
@ 2021-04-16  5:07 ` Guenter Roeck
  2021-04-16 18:25   ` Tao Ren
  0 siblings, 1 reply; 3+ messages in thread
From: Guenter Roeck @ 2021-04-16  5:07 UTC (permalink / raw)
  To: rentao.bupt, Wim Van Sebroeck, Joel Stanley, Andrew Jeffery,
	linux-watchdog, linux-arm-kernel, linux-aspeed, linux-kernel,
	openbmc, Tao Ren, Amithash Prasad

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


^ permalink raw reply	[flat|nested] 3+ messages in thread

* Re: [PATCH v2] watchdog: aspeed: fix integer overflow in set_timeout handler
  2021-04-16  5:07 ` Guenter Roeck
@ 2021-04-16 18:25   ` Tao Ren
  0 siblings, 0 replies; 3+ messages in thread
From: Tao Ren @ 2021-04-16 18:25 UTC (permalink / raw)
  To: Guenter Roeck
  Cc: Wim Van Sebroeck, Joel Stanley, Andrew Jeffery, linux-watchdog,
	linux-arm-kernel, linux-aspeed, linux-kernel, openbmc, Tao Ren,
	Amithash Prasad

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

^ permalink raw reply	[flat|nested] 3+ messages in thread

end of thread, back to index

Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-04-16  2:13 [PATCH v2] watchdog: aspeed: fix integer overflow in set_timeout handler rentao.bupt
2021-04-16  5:07 ` Guenter Roeck
2021-04-16 18:25   ` Tao Ren

Linux-Watchdog Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-watchdog/0 linux-watchdog/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-watchdog linux-watchdog/ https://lore.kernel.org/linux-watchdog \
		linux-watchdog@vger.kernel.org
	public-inbox-index linux-watchdog

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-watchdog


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git