| Message ID | 20210416021337.18715-1-rentao.bupt@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | [v2] watchdog: aspeed: fix integer overflow in set_timeout handler | expand |
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); >
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 --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);