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

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

[rentao.bupt]

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>
---
 drivers/watchdog/aspeed_wdt.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

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

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

[rentao.bupt]

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>
---
 drivers/watchdog/aspeed_wdt.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

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

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

[rentao.bupt]

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>
---
 drivers/watchdog/aspeed_wdt.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

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


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Joel Stanley]

On Fri, 16 Apr 2021 at 00:12, <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).
>
> Fixes: efa859f7d786 ("watchdog: Add Aspeed watchdog driver")
> Reported-by: Amithash Prasad <amithash@fb.com>
> Signed-off-by: Tao Ren <rentao.bupt@gmail.com>
> ---
>  drivers/watchdog/aspeed_wdt.c | 5 ++---
>  1 file changed, 2 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/watchdog/aspeed_wdt.c b/drivers/watchdog/aspeed_wdt.c
> index 7e00960651fa..9f77272dc906 100644
> --- a/drivers/watchdog/aspeed_wdt.c
> +++ b/drivers/watchdog/aspeed_wdt.c
> @@ -145,9 +145,8 @@ static int aspeed_wdt_set_timeout(struct watchdog_device *wdd,
>         struct aspeed_wdt *wdt = to_aspeed_wdt(wdd);
>         u32 actual;
>
> -       wdd->timeout = timeout;
> -
> -       actual = min(timeout, wdd->max_hw_heartbeat_ms * 1000);
> +       actual = min(timeout, wdd->max_hw_heartbeat_ms / 1000);

The unit of timeout is seconds. You're comparing to ms/1000, which are
microseconds. I think the existing test is correct?

As far as integer overflow is concerned, max_hw_heartbeat_ms is an
unsigned int. We set it to 4294967, which *1000 = 0xfffffed8. This
should be fine.

> +       wdd->timeout = actual;

This might be the correct thing to do though. I'll defer to the
watchdog maintainers for their input.

Cheers,

Joel

>
>         writel(actual * WDT_RATE_1MHZ, wdt->base + WDT_RELOAD_VALUE);
>         writel(WDT_RESTART_MAGIC, wdt->base + WDT_RESTART);
> --
> 2.17.1
>

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

[Joel Stanley]

On Fri, 16 Apr 2021 at 00:12, <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).
>
> Fixes: efa859f7d786 ("watchdog: Add Aspeed watchdog driver")
> Reported-by: Amithash Prasad <amithash@fb.com>
> Signed-off-by: Tao Ren <rentao.bupt@gmail.com>
> ---
>  drivers/watchdog/aspeed_wdt.c | 5 ++---
>  1 file changed, 2 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/watchdog/aspeed_wdt.c b/drivers/watchdog/aspeed_wdt.c
> index 7e00960651fa..9f77272dc906 100644
> --- a/drivers/watchdog/aspeed_wdt.c
> +++ b/drivers/watchdog/aspeed_wdt.c
> @@ -145,9 +145,8 @@ static int aspeed_wdt_set_timeout(struct watchdog_device *wdd,
>         struct aspeed_wdt *wdt = to_aspeed_wdt(wdd);
>         u32 actual;
>
> -       wdd->timeout = timeout;
> -
> -       actual = min(timeout, wdd->max_hw_heartbeat_ms * 1000);
> +       actual = min(timeout, wdd->max_hw_heartbeat_ms / 1000);

The unit of timeout is seconds. You're comparing to ms/1000, which are
microseconds. I think the existing test is correct?

As far as integer overflow is concerned, max_hw_heartbeat_ms is an
unsigned int. We set it to 4294967, which *1000 = 0xfffffed8. This
should be fine.

> +       wdd->timeout = actual;

This might be the correct thing to do though. I'll defer to the
watchdog maintainers for their input.

Cheers,

Joel

>
>         writel(actual * WDT_RATE_1MHZ, wdt->base + WDT_RELOAD_VALUE);
>         writel(WDT_RESTART_MAGIC, wdt->base + WDT_RESTART);
> --
> 2.17.1
>

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

[Joel Stanley]

On Fri, 16 Apr 2021 at 00:12, <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).
>
> Fixes: efa859f7d786 ("watchdog: Add Aspeed watchdog driver")
> Reported-by: Amithash Prasad <amithash@fb.com>
> Signed-off-by: Tao Ren <rentao.bupt@gmail.com>
> ---
>  drivers/watchdog/aspeed_wdt.c | 5 ++---
>  1 file changed, 2 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/watchdog/aspeed_wdt.c b/drivers/watchdog/aspeed_wdt.c
> index 7e00960651fa..9f77272dc906 100644
> --- a/drivers/watchdog/aspeed_wdt.c
> +++ b/drivers/watchdog/aspeed_wdt.c
> @@ -145,9 +145,8 @@ static int aspeed_wdt_set_timeout(struct watchdog_device *wdd,
>         struct aspeed_wdt *wdt = to_aspeed_wdt(wdd);
>         u32 actual;
>
> -       wdd->timeout = timeout;
> -
> -       actual = min(timeout, wdd->max_hw_heartbeat_ms * 1000);
> +       actual = min(timeout, wdd->max_hw_heartbeat_ms / 1000);

The unit of timeout is seconds. You're comparing to ms/1000, which are
microseconds. I think the existing test is correct?

As far as integer overflow is concerned, max_hw_heartbeat_ms is an
unsigned int. We set it to 4294967, which *1000 = 0xfffffed8. This
should be fine.

> +       wdd->timeout = actual;

This might be the correct thing to do though. I'll defer to the
watchdog maintainers for their input.

Cheers,

Joel

>
>         writel(actual * WDT_RATE_1MHZ, wdt->base + WDT_RELOAD_VALUE);
>         writel(WDT_RESTART_MAGIC, wdt->base + WDT_RESTART);
> --
> 2.17.1
>

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

[Joel Stanley]

On Fri, 16 Apr 2021 at 00:12, <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).
>
> Fixes: efa859f7d786 ("watchdog: Add Aspeed watchdog driver")
> Reported-by: Amithash Prasad <amithash@fb.com>
> Signed-off-by: Tao Ren <rentao.bupt@gmail.com>
> ---
>  drivers/watchdog/aspeed_wdt.c | 5 ++---
>  1 file changed, 2 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/watchdog/aspeed_wdt.c b/drivers/watchdog/aspeed_wdt.c
> index 7e00960651fa..9f77272dc906 100644
> --- a/drivers/watchdog/aspeed_wdt.c
> +++ b/drivers/watchdog/aspeed_wdt.c
> @@ -145,9 +145,8 @@ static int aspeed_wdt_set_timeout(struct watchdog_device *wdd,
>         struct aspeed_wdt *wdt = to_aspeed_wdt(wdd);
>         u32 actual;
>
> -       wdd->timeout = timeout;
> -
> -       actual = min(timeout, wdd->max_hw_heartbeat_ms * 1000);
> +       actual = min(timeout, wdd->max_hw_heartbeat_ms / 1000);

The unit of timeout is seconds. You're comparing to ms/1000, which are
microseconds. I think the existing test is correct?

As far as integer overflow is concerned, max_hw_heartbeat_ms is an
unsigned int. We set it to 4294967, which *1000 = 0xfffffed8. This
should be fine.

> +       wdd->timeout = actual;

This might be the correct thing to do though. I'll defer to the
watchdog maintainers for their input.

Cheers,

Joel

>
>         writel(actual * WDT_RATE_1MHZ, wdt->base + WDT_RELOAD_VALUE);
>         writel(WDT_RESTART_MAGIC, wdt->base + WDT_RESTART);
> --
> 2.17.1
>

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Guenter Roeck]

On 4/15/21 5:12 PM, rentao.bupt at 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).
> 
> Fixes: efa859f7d786 ("watchdog: Add Aspeed watchdog driver")
> Reported-by: Amithash Prasad <amithash@fb.com>
> Signed-off-by: Tao Ren <rentao.bupt@gmail.com>
> ---
>  drivers/watchdog/aspeed_wdt.c | 5 ++---
>  1 file changed, 2 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/watchdog/aspeed_wdt.c b/drivers/watchdog/aspeed_wdt.c
> index 7e00960651fa..9f77272dc906 100644
> --- a/drivers/watchdog/aspeed_wdt.c
> +++ b/drivers/watchdog/aspeed_wdt.c
> @@ -145,9 +145,8 @@ static int aspeed_wdt_set_timeout(struct watchdog_device *wdd,
>  	struct aspeed_wdt *wdt = to_aspeed_wdt(wdd);
>  	u32 actual;
>  
> -	wdd->timeout = timeout;
> -
> -	actual = min(timeout, wdd->max_hw_heartbeat_ms * 1000);
> +	actual = min(timeout, wdd->max_hw_heartbeat_ms / 1000);
> +	wdd->timeout = actual;
>  
>  	writel(actual * WDT_RATE_1MHZ, wdt->base + WDT_RELOAD_VALUE);
>  	writel(WDT_RESTART_MAGIC, wdt->base + WDT_RESTART);
> 

If the provided timeout is larger than the supported hardware timeout,
the watchdog core will ping the hardware on behalf of userspace.
The above code would defeat that mechanism for no good reason.

NACK.

Guenter

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

[Guenter Roeck]

On 4/15/21 5:12 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).
> 
> Fixes: efa859f7d786 ("watchdog: Add Aspeed watchdog driver")
> Reported-by: Amithash Prasad <amithash@fb.com>
> Signed-off-by: Tao Ren <rentao.bupt@gmail.com>
> ---
>  drivers/watchdog/aspeed_wdt.c | 5 ++---
>  1 file changed, 2 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/watchdog/aspeed_wdt.c b/drivers/watchdog/aspeed_wdt.c
> index 7e00960651fa..9f77272dc906 100644
> --- a/drivers/watchdog/aspeed_wdt.c
> +++ b/drivers/watchdog/aspeed_wdt.c
> @@ -145,9 +145,8 @@ static int aspeed_wdt_set_timeout(struct watchdog_device *wdd,
>  	struct aspeed_wdt *wdt = to_aspeed_wdt(wdd);
>  	u32 actual;
>  
> -	wdd->timeout = timeout;
> -
> -	actual = min(timeout, wdd->max_hw_heartbeat_ms * 1000);
> +	actual = min(timeout, wdd->max_hw_heartbeat_ms / 1000);
> +	wdd->timeout = actual;
>  
>  	writel(actual * WDT_RATE_1MHZ, wdt->base + WDT_RELOAD_VALUE);
>  	writel(WDT_RESTART_MAGIC, wdt->base + WDT_RESTART);
> 

If the provided timeout is larger than the supported hardware timeout,
the watchdog core will ping the hardware on behalf of userspace.
The above code would defeat that mechanism for no good reason.

NACK.

Guenter

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

[Guenter Roeck]

On 4/15/21 5:12 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).
> 
> Fixes: efa859f7d786 ("watchdog: Add Aspeed watchdog driver")
> Reported-by: Amithash Prasad <amithash@fb.com>
> Signed-off-by: Tao Ren <rentao.bupt@gmail.com>
> ---
>  drivers/watchdog/aspeed_wdt.c | 5 ++---
>  1 file changed, 2 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/watchdog/aspeed_wdt.c b/drivers/watchdog/aspeed_wdt.c
> index 7e00960651fa..9f77272dc906 100644
> --- a/drivers/watchdog/aspeed_wdt.c
> +++ b/drivers/watchdog/aspeed_wdt.c
> @@ -145,9 +145,8 @@ static int aspeed_wdt_set_timeout(struct watchdog_device *wdd,
>  	struct aspeed_wdt *wdt = to_aspeed_wdt(wdd);
>  	u32 actual;
>  
> -	wdd->timeout = timeout;
> -
> -	actual = min(timeout, wdd->max_hw_heartbeat_ms * 1000);
> +	actual = min(timeout, wdd->max_hw_heartbeat_ms / 1000);
> +	wdd->timeout = actual;
>  
>  	writel(actual * WDT_RATE_1MHZ, wdt->base + WDT_RELOAD_VALUE);
>  	writel(WDT_RESTART_MAGIC, wdt->base + WDT_RESTART);
> 

If the provided timeout is larger than the supported hardware timeout,
the watchdog core will ping the hardware on behalf of userspace.
The above code would defeat that mechanism for no good reason.

NACK.

Guenter

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Tao Ren]

On Thu, Apr 15, 2021 at 05:50:32PM -0700, Guenter Roeck wrote:
> On 4/15/21 5:12 PM, rentao.bupt at 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).
> > 
> > Fixes: efa859f7d786 ("watchdog: Add Aspeed watchdog driver")
> > Reported-by: Amithash Prasad <amithash@fb.com>
> > Signed-off-by: Tao Ren <rentao.bupt@gmail.com>
> > ---
> >  drivers/watchdog/aspeed_wdt.c | 5 ++---
> >  1 file changed, 2 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/watchdog/aspeed_wdt.c b/drivers/watchdog/aspeed_wdt.c
> > index 7e00960651fa..9f77272dc906 100644
> > --- a/drivers/watchdog/aspeed_wdt.c
> > +++ b/drivers/watchdog/aspeed_wdt.c
> > @@ -145,9 +145,8 @@ static int aspeed_wdt_set_timeout(struct watchdog_device *wdd,
> >  	struct aspeed_wdt *wdt = to_aspeed_wdt(wdd);
> >  	u32 actual;
> >  
> > -	wdd->timeout = timeout;
> > -
> > -	actual = min(timeout, wdd->max_hw_heartbeat_ms * 1000);
> > +	actual = min(timeout, wdd->max_hw_heartbeat_ms / 1000);
> > +	wdd->timeout = actual;
> >  
> >  	writel(actual * WDT_RATE_1MHZ, wdt->base + WDT_RELOAD_VALUE);
> >  	writel(WDT_RESTART_MAGIC, wdt->base + WDT_RESTART);
> > 
> 
> If the provided timeout is larger than the supported hardware timeout,
> the watchdog core will ping the hardware on behalf of userspace.
> The above code would defeat that mechanism for no good reason.
> 
> NACK.
> 
> Guenter

Thanks Guenter for Joel for the quick review!

The integer overflow happens at (actual * WDT_RATE_1MHZ). For example,
if a user tries to set timeout to 4295 seconds, then the hardware would
be programmed to timeout after about 32 milliseconds. I would say this
behavior is not expected?


Cheers,

Tao

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

[Tao Ren]

On Thu, Apr 15, 2021 at 05:50:32PM -0700, Guenter Roeck wrote:
> On 4/15/21 5:12 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).
> > 
> > Fixes: efa859f7d786 ("watchdog: Add Aspeed watchdog driver")
> > Reported-by: Amithash Prasad <amithash@fb.com>
> > Signed-off-by: Tao Ren <rentao.bupt@gmail.com>
> > ---
> >  drivers/watchdog/aspeed_wdt.c | 5 ++---
> >  1 file changed, 2 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/watchdog/aspeed_wdt.c b/drivers/watchdog/aspeed_wdt.c
> > index 7e00960651fa..9f77272dc906 100644
> > --- a/drivers/watchdog/aspeed_wdt.c
> > +++ b/drivers/watchdog/aspeed_wdt.c
> > @@ -145,9 +145,8 @@ static int aspeed_wdt_set_timeout(struct watchdog_device *wdd,
> >  	struct aspeed_wdt *wdt = to_aspeed_wdt(wdd);
> >  	u32 actual;
> >  
> > -	wdd->timeout = timeout;
> > -
> > -	actual = min(timeout, wdd->max_hw_heartbeat_ms * 1000);
> > +	actual = min(timeout, wdd->max_hw_heartbeat_ms / 1000);
> > +	wdd->timeout = actual;
> >  
> >  	writel(actual * WDT_RATE_1MHZ, wdt->base + WDT_RELOAD_VALUE);
> >  	writel(WDT_RESTART_MAGIC, wdt->base + WDT_RESTART);
> > 
> 
> If the provided timeout is larger than the supported hardware timeout,
> the watchdog core will ping the hardware on behalf of userspace.
> The above code would defeat that mechanism for no good reason.
> 
> NACK.
> 
> Guenter

Thanks Guenter for Joel for the quick review!

The integer overflow happens at (actual * WDT_RATE_1MHZ). For example,
if a user tries to set timeout to 4295 seconds, then the hardware would
be programmed to timeout after about 32 milliseconds. I would say this
behavior is not expected?


Cheers,

Tao

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

[Tao Ren]

On Thu, Apr 15, 2021 at 05:50:32PM -0700, Guenter Roeck wrote:
> On 4/15/21 5:12 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).
> > 
> > Fixes: efa859f7d786 ("watchdog: Add Aspeed watchdog driver")
> > Reported-by: Amithash Prasad <amithash@fb.com>
> > Signed-off-by: Tao Ren <rentao.bupt@gmail.com>
> > ---
> >  drivers/watchdog/aspeed_wdt.c | 5 ++---
> >  1 file changed, 2 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/watchdog/aspeed_wdt.c b/drivers/watchdog/aspeed_wdt.c
> > index 7e00960651fa..9f77272dc906 100644
> > --- a/drivers/watchdog/aspeed_wdt.c
> > +++ b/drivers/watchdog/aspeed_wdt.c
> > @@ -145,9 +145,8 @@ static int aspeed_wdt_set_timeout(struct watchdog_device *wdd,
> >  	struct aspeed_wdt *wdt = to_aspeed_wdt(wdd);
> >  	u32 actual;
> >  
> > -	wdd->timeout = timeout;
> > -
> > -	actual = min(timeout, wdd->max_hw_heartbeat_ms * 1000);
> > +	actual = min(timeout, wdd->max_hw_heartbeat_ms / 1000);
> > +	wdd->timeout = actual;
> >  
> >  	writel(actual * WDT_RATE_1MHZ, wdt->base + WDT_RELOAD_VALUE);
> >  	writel(WDT_RESTART_MAGIC, wdt->base + WDT_RESTART);
> > 
> 
> If the provided timeout is larger than the supported hardware timeout,
> the watchdog core will ping the hardware on behalf of userspace.
> The above code would defeat that mechanism for no good reason.
> 
> NACK.
> 
> Guenter

Thanks Guenter for Joel for the quick review!

The integer overflow happens at (actual * WDT_RATE_1MHZ). For example,
if a user tries to set timeout to 4295 seconds, then the hardware would
be programmed to timeout after about 32 milliseconds. I would say this
behavior is not expected?


Cheers,

Tao

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

[Tao Ren]

On Thu, Apr 15, 2021 at 05:50:32PM -0700, Guenter Roeck wrote:
> On 4/15/21 5:12 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).
> > 
> > Fixes: efa859f7d786 ("watchdog: Add Aspeed watchdog driver")
> > Reported-by: Amithash Prasad <amithash@fb.com>
> > Signed-off-by: Tao Ren <rentao.bupt@gmail.com>
> > ---
> >  drivers/watchdog/aspeed_wdt.c | 5 ++---
> >  1 file changed, 2 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/watchdog/aspeed_wdt.c b/drivers/watchdog/aspeed_wdt.c
> > index 7e00960651fa..9f77272dc906 100644
> > --- a/drivers/watchdog/aspeed_wdt.c
> > +++ b/drivers/watchdog/aspeed_wdt.c
> > @@ -145,9 +145,8 @@ static int aspeed_wdt_set_timeout(struct watchdog_device *wdd,
> >  	struct aspeed_wdt *wdt = to_aspeed_wdt(wdd);
> >  	u32 actual;
> >  
> > -	wdd->timeout = timeout;
> > -
> > -	actual = min(timeout, wdd->max_hw_heartbeat_ms * 1000);
> > +	actual = min(timeout, wdd->max_hw_heartbeat_ms / 1000);
> > +	wdd->timeout = actual;
> >  
> >  	writel(actual * WDT_RATE_1MHZ, wdt->base + WDT_RELOAD_VALUE);
> >  	writel(WDT_RESTART_MAGIC, wdt->base + WDT_RESTART);
> > 
> 
> If the provided timeout is larger than the supported hardware timeout,
> the watchdog core will ping the hardware on behalf of userspace.
> The above code would defeat that mechanism for no good reason.
> 
> NACK.
> 
> Guenter

Thanks Guenter for Joel for the quick review!

The integer overflow happens at (actual * WDT_RATE_1MHZ). For example,
if a user tries to set timeout to 4295 seconds, then the hardware would
be programmed to timeout after about 32 milliseconds. I would say this
behavior is not expected?


Cheers,

Tao

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Guenter Roeck]

On Thu, Apr 15, 2021 at 06:04:45PM -0700, Tao Ren wrote:
> On Thu, Apr 15, 2021 at 05:50:32PM -0700, Guenter Roeck wrote:
> > On 4/15/21 5:12 PM, rentao.bupt at 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).
> > > 
> > > Fixes: efa859f7d786 ("watchdog: Add Aspeed watchdog driver")
> > > Reported-by: Amithash Prasad <amithash@fb.com>
> > > Signed-off-by: Tao Ren <rentao.bupt@gmail.com>
> > > ---
> > >  drivers/watchdog/aspeed_wdt.c | 5 ++---
> > >  1 file changed, 2 insertions(+), 3 deletions(-)
> > > 
> > > diff --git a/drivers/watchdog/aspeed_wdt.c b/drivers/watchdog/aspeed_wdt.c
> > > index 7e00960651fa..9f77272dc906 100644
> > > --- a/drivers/watchdog/aspeed_wdt.c
> > > +++ b/drivers/watchdog/aspeed_wdt.c
> > > @@ -145,9 +145,8 @@ static int aspeed_wdt_set_timeout(struct watchdog_device *wdd,
> > >  	struct aspeed_wdt *wdt = to_aspeed_wdt(wdd);
> > >  	u32 actual;
> > >  
> > > -	wdd->timeout = timeout;
> > > -
> > > -	actual = min(timeout, wdd->max_hw_heartbeat_ms * 1000);
> > > +	actual = min(timeout, wdd->max_hw_heartbeat_ms / 1000);
> > > +	wdd->timeout = actual;
> > >  
> > >  	writel(actual * WDT_RATE_1MHZ, wdt->base + WDT_RELOAD_VALUE);
> > >  	writel(WDT_RESTART_MAGIC, wdt->base + WDT_RESTART);
> > > 
> > 
> > If the provided timeout is larger than the supported hardware timeout,
> > the watchdog core will ping the hardware on behalf of userspace.
> > The above code would defeat that mechanism for no good reason.
> > 
> > NACK.
> > 
> > Guenter
> 
> Thanks Guenter for Joel for the quick review!
> 
> The integer overflow happens at (actual * WDT_RATE_1MHZ). For example,
> if a user tries to set timeout to 4295 seconds, then the hardware would
> be programmed to timeout after about 32 milliseconds. I would say this
> behavior is not expected?

The fix would be

-	actual = min(timeout, wdd->max_hw_heartbeat_ms * 1000);
+	actual = min(timeout, wdd->max_hw_heartbeat_ms / 1000);

without modifying
	wdd->timeout = timeout;

The real problem here is that "wdd->max_hw_heartbeat_ms * 1000"
is simply wrong. The overflow is a side effect of the wrong
calculation, and concentrating on it disguises the real problem.

Guenter

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

[Guenter Roeck]

On Thu, Apr 15, 2021 at 06:04:45PM -0700, Tao Ren wrote:
> On Thu, Apr 15, 2021 at 05:50:32PM -0700, Guenter Roeck wrote:
> > On 4/15/21 5:12 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).
> > > 
> > > Fixes: efa859f7d786 ("watchdog: Add Aspeed watchdog driver")
> > > Reported-by: Amithash Prasad <amithash@fb.com>
> > > Signed-off-by: Tao Ren <rentao.bupt@gmail.com>
> > > ---
> > >  drivers/watchdog/aspeed_wdt.c | 5 ++---
> > >  1 file changed, 2 insertions(+), 3 deletions(-)
> > > 
> > > diff --git a/drivers/watchdog/aspeed_wdt.c b/drivers/watchdog/aspeed_wdt.c
> > > index 7e00960651fa..9f77272dc906 100644
> > > --- a/drivers/watchdog/aspeed_wdt.c
> > > +++ b/drivers/watchdog/aspeed_wdt.c
> > > @@ -145,9 +145,8 @@ static int aspeed_wdt_set_timeout(struct watchdog_device *wdd,
> > >  	struct aspeed_wdt *wdt = to_aspeed_wdt(wdd);
> > >  	u32 actual;
> > >  
> > > -	wdd->timeout = timeout;
> > > -
> > > -	actual = min(timeout, wdd->max_hw_heartbeat_ms * 1000);
> > > +	actual = min(timeout, wdd->max_hw_heartbeat_ms / 1000);
> > > +	wdd->timeout = actual;
> > >  
> > >  	writel(actual * WDT_RATE_1MHZ, wdt->base + WDT_RELOAD_VALUE);
> > >  	writel(WDT_RESTART_MAGIC, wdt->base + WDT_RESTART);
> > > 
> > 
> > If the provided timeout is larger than the supported hardware timeout,
> > the watchdog core will ping the hardware on behalf of userspace.
> > The above code would defeat that mechanism for no good reason.
> > 
> > NACK.
> > 
> > Guenter
> 
> Thanks Guenter for Joel for the quick review!
> 
> The integer overflow happens at (actual * WDT_RATE_1MHZ). For example,
> if a user tries to set timeout to 4295 seconds, then the hardware would
> be programmed to timeout after about 32 milliseconds. I would say this
> behavior is not expected?

The fix would be

-	actual = min(timeout, wdd->max_hw_heartbeat_ms * 1000);
+	actual = min(timeout, wdd->max_hw_heartbeat_ms / 1000);

without modifying
	wdd->timeout = timeout;

The real problem here is that "wdd->max_hw_heartbeat_ms * 1000"
is simply wrong. The overflow is a side effect of the wrong
calculation, and concentrating on it disguises the real problem.

Guenter

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

[Guenter Roeck]

On Thu, Apr 15, 2021 at 06:04:45PM -0700, Tao Ren wrote:
> On Thu, Apr 15, 2021 at 05:50:32PM -0700, Guenter Roeck wrote:
> > On 4/15/21 5:12 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).
> > > 
> > > Fixes: efa859f7d786 ("watchdog: Add Aspeed watchdog driver")
> > > Reported-by: Amithash Prasad <amithash@fb.com>
> > > Signed-off-by: Tao Ren <rentao.bupt@gmail.com>
> > > ---
> > >  drivers/watchdog/aspeed_wdt.c | 5 ++---
> > >  1 file changed, 2 insertions(+), 3 deletions(-)
> > > 
> > > diff --git a/drivers/watchdog/aspeed_wdt.c b/drivers/watchdog/aspeed_wdt.c
> > > index 7e00960651fa..9f77272dc906 100644
> > > --- a/drivers/watchdog/aspeed_wdt.c
> > > +++ b/drivers/watchdog/aspeed_wdt.c
> > > @@ -145,9 +145,8 @@ static int aspeed_wdt_set_timeout(struct watchdog_device *wdd,
> > >  	struct aspeed_wdt *wdt = to_aspeed_wdt(wdd);
> > >  	u32 actual;
> > >  
> > > -	wdd->timeout = timeout;
> > > -
> > > -	actual = min(timeout, wdd->max_hw_heartbeat_ms * 1000);
> > > +	actual = min(timeout, wdd->max_hw_heartbeat_ms / 1000);
> > > +	wdd->timeout = actual;
> > >  
> > >  	writel(actual * WDT_RATE_1MHZ, wdt->base + WDT_RELOAD_VALUE);
> > >  	writel(WDT_RESTART_MAGIC, wdt->base + WDT_RESTART);
> > > 
> > 
> > If the provided timeout is larger than the supported hardware timeout,
> > the watchdog core will ping the hardware on behalf of userspace.
> > The above code would defeat that mechanism for no good reason.
> > 
> > NACK.
> > 
> > Guenter
> 
> Thanks Guenter for Joel for the quick review!
> 
> The integer overflow happens at (actual * WDT_RATE_1MHZ). For example,
> if a user tries to set timeout to 4295 seconds, then the hardware would
> be programmed to timeout after about 32 milliseconds. I would say this
> behavior is not expected?

The fix would be

-	actual = min(timeout, wdd->max_hw_heartbeat_ms * 1000);
+	actual = min(timeout, wdd->max_hw_heartbeat_ms / 1000);

without modifying
	wdd->timeout = timeout;

The real problem here is that "wdd->max_hw_heartbeat_ms * 1000"
is simply wrong. The overflow is a side effect of the wrong
calculation, and concentrating on it disguises the real problem.

Guenter

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

[Guenter Roeck]

On Thu, Apr 15, 2021 at 06:04:45PM -0700, Tao Ren wrote:
> On Thu, Apr 15, 2021 at 05:50:32PM -0700, Guenter Roeck wrote:
> > On 4/15/21 5:12 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).
> > > 
> > > Fixes: efa859f7d786 ("watchdog: Add Aspeed watchdog driver")
> > > Reported-by: Amithash Prasad <amithash@fb.com>
> > > Signed-off-by: Tao Ren <rentao.bupt@gmail.com>
> > > ---
> > >  drivers/watchdog/aspeed_wdt.c | 5 ++---
> > >  1 file changed, 2 insertions(+), 3 deletions(-)
> > > 
> > > diff --git a/drivers/watchdog/aspeed_wdt.c b/drivers/watchdog/aspeed_wdt.c
> > > index 7e00960651fa..9f77272dc906 100644
> > > --- a/drivers/watchdog/aspeed_wdt.c
> > > +++ b/drivers/watchdog/aspeed_wdt.c
> > > @@ -145,9 +145,8 @@ static int aspeed_wdt_set_timeout(struct watchdog_device *wdd,
> > >  	struct aspeed_wdt *wdt = to_aspeed_wdt(wdd);
> > >  	u32 actual;
> > >  
> > > -	wdd->timeout = timeout;
> > > -
> > > -	actual = min(timeout, wdd->max_hw_heartbeat_ms * 1000);
> > > +	actual = min(timeout, wdd->max_hw_heartbeat_ms / 1000);
> > > +	wdd->timeout = actual;
> > >  
> > >  	writel(actual * WDT_RATE_1MHZ, wdt->base + WDT_RELOAD_VALUE);
> > >  	writel(WDT_RESTART_MAGIC, wdt->base + WDT_RESTART);
> > > 
> > 
> > If the provided timeout is larger than the supported hardware timeout,
> > the watchdog core will ping the hardware on behalf of userspace.
> > The above code would defeat that mechanism for no good reason.
> > 
> > NACK.
> > 
> > Guenter
> 
> Thanks Guenter for Joel for the quick review!
> 
> The integer overflow happens at (actual * WDT_RATE_1MHZ). For example,
> if a user tries to set timeout to 4295 seconds, then the hardware would
> be programmed to timeout after about 32 milliseconds. I would say this
> behavior is not expected?

The fix would be

-	actual = min(timeout, wdd->max_hw_heartbeat_ms * 1000);
+	actual = min(timeout, wdd->max_hw_heartbeat_ms / 1000);

without modifying
	wdd->timeout = timeout;

The real problem here is that "wdd->max_hw_heartbeat_ms * 1000"
is simply wrong. The overflow is a side effect of the wrong
calculation, and concentrating on it disguises the real problem.

Guenter

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel