* [PATCH] PM / Domains: Fix integer overflows on u32 bit multiplies
@ 2021-02-07 22:46 ` Colin King
0 siblings, 0 replies; 5+ messages in thread
From: Colin King @ 2021-02-07 22:46 UTC (permalink / raw)
To: Rafael J . Wysocki, Kevin Hilman, Ulf Hansson, Pavel Machek,
Len Brown, Greg Kroah-Hartman, Marc Titinger, Lina Iyer,
linux-pm
Cc: kernel-janitors, linux-kernel
From: Colin Ian King <colin.king@canonical.com>
There are three occurrances of u32 variables being multiplied by
1000 using 32 bit multiplies and the result being assigned to a
64 bit signed integer. These can potentially lead to a 32 bit
overflows, so fix this by casting 1000 to a UL first to force
a 64 bit multiply hence avoiding the overflow.
Addresses-Coverity: ("Unintentional integer overflow")
Fixes: 30f604283e05 ("PM / Domains: Allow domain power states to be read from DT")
Signed-off-by: Colin Ian King <colin.king@canonical.com>
---
drivers/base/power/domain.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
index aaf6c83b5cf6..ddeff69126ff 100644
--- a/drivers/base/power/domain.c
+++ b/drivers/base/power/domain.c
@@ -2831,10 +2831,10 @@ static int genpd_parse_state(struct genpd_power_state *genpd_state,
err = of_property_read_u32(state_node, "min-residency-us", &residency);
if (!err)
- genpd_state->residency_ns = 1000 * residency;
+ genpd_state->residency_ns = 1000UL * residency;
- genpd_state->power_on_latency_ns = 1000 * exit_latency;
- genpd_state->power_off_latency_ns = 1000 * entry_latency;
+ genpd_state->power_on_latency_ns = 1000UL * exit_latency;
+ genpd_state->power_off_latency_ns = 1000UL * entry_latency;
genpd_state->fwnode = &state_node->fwnode;
return 0;
--
2.29.2
^ permalink raw reply related [flat|nested] 5+ messages in thread* Re: [PATCH] PM / Domains: Fix integer overflows on u32 bit multiplies
2021-02-07 22:46 ` Colin King
@ 2021-02-08 7:54 ` Pavel Machek
-1 siblings, 0 replies; 5+ messages in thread
From: Pavel Machek @ 2021-02-08 7:54 UTC (permalink / raw)
To: Colin King
Cc: Rafael J . Wysocki, Kevin Hilman, Ulf Hansson, Len Brown,
Greg Kroah-Hartman, Marc Titinger, Lina Iyer, linux-pm,
kernel-janitors, linux-kernel
[-- Attachment #1: Type: text/plain, Size: 1758 bytes --]
On Sun 2021-02-07 22:46:48, Colin King wrote:
> From: Colin Ian King <colin.king@canonical.com>
>
> There are three occurrances of u32 variables being multiplied by
> 1000 using 32 bit multiplies and the result being assigned to a
> 64 bit signed integer. These can potentially lead to a 32 bit
> overflows, so fix this by casting 1000 to a UL first to force
> a 64 bit multiply hence avoiding the overflow.
Ummm. No?
a) Can you imagine any situation where they result in overflow?
b) How does casting to UL help on 32 bit system?
Best regards,
Pavel
> Addresses-Coverity: ("Unintentional integer overflow")
> Fixes: 30f604283e05 ("PM / Domains: Allow domain power states to be read from DT")
> Signed-off-by: Colin Ian King <colin.king@canonical.com>
> ---
> drivers/base/power/domain.c | 6 +++---
> 1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
> index aaf6c83b5cf6..ddeff69126ff 100644
> --- a/drivers/base/power/domain.c
> +++ b/drivers/base/power/domain.c
> @@ -2831,10 +2831,10 @@ static int genpd_parse_state(struct genpd_power_state *genpd_state,
>
> err = of_property_read_u32(state_node, "min-residency-us", &residency);
> if (!err)
> - genpd_state->residency_ns = 1000 * residency;
> + genpd_state->residency_ns = 1000UL * residency;
>
> - genpd_state->power_on_latency_ns = 1000 * exit_latency;
> - genpd_state->power_off_latency_ns = 1000 * entry_latency;
> + genpd_state->power_on_latency_ns = 1000UL * exit_latency;
> + genpd_state->power_off_latency_ns = 1000UL * entry_latency;
> genpd_state->fwnode = &state_node->fwnode;
>
> return 0;
--
http://www.livejournal.com/~pavelmachek
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH] PM / Domains: Fix integer overflows on u32 bit multiplies
@ 2021-02-08 7:54 ` Pavel Machek
0 siblings, 0 replies; 5+ messages in thread
From: Pavel Machek @ 2021-02-08 7:54 UTC (permalink / raw)
To: Colin King
Cc: Rafael J . Wysocki, Kevin Hilman, Ulf Hansson, Len Brown,
Greg Kroah-Hartman, Marc Titinger, Lina Iyer, linux-pm,
kernel-janitors, linux-kernel
[-- Attachment #1: Type: text/plain, Size: 1758 bytes --]
On Sun 2021-02-07 22:46:48, Colin King wrote:
> From: Colin Ian King <colin.king@canonical.com>
>
> There are three occurrances of u32 variables being multiplied by
> 1000 using 32 bit multiplies and the result being assigned to a
> 64 bit signed integer. These can potentially lead to a 32 bit
> overflows, so fix this by casting 1000 to a UL first to force
> a 64 bit multiply hence avoiding the overflow.
Ummm. No?
a) Can you imagine any situation where they result in overflow?
b) How does casting to UL help on 32 bit system?
Best regards,
Pavel
> Addresses-Coverity: ("Unintentional integer overflow")
> Fixes: 30f604283e05 ("PM / Domains: Allow domain power states to be read from DT")
> Signed-off-by: Colin Ian King <colin.king@canonical.com>
> ---
> drivers/base/power/domain.c | 6 +++---
> 1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
> index aaf6c83b5cf6..ddeff69126ff 100644
> --- a/drivers/base/power/domain.c
> +++ b/drivers/base/power/domain.c
> @@ -2831,10 +2831,10 @@ static int genpd_parse_state(struct genpd_power_state *genpd_state,
>
> err = of_property_read_u32(state_node, "min-residency-us", &residency);
> if (!err)
> - genpd_state->residency_ns = 1000 * residency;
> + genpd_state->residency_ns = 1000UL * residency;
>
> - genpd_state->power_on_latency_ns = 1000 * exit_latency;
> - genpd_state->power_off_latency_ns = 1000 * entry_latency;
> + genpd_state->power_on_latency_ns = 1000UL * exit_latency;
> + genpd_state->power_off_latency_ns = 1000UL * entry_latency;
> genpd_state->fwnode = &state_node->fwnode;
>
> return 0;
--
http://www.livejournal.com/~pavelmachek
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] PM / Domains: Fix integer overflows on u32 bit multiplies
2021-02-07 22:46 ` Colin King
(?)
(?)
@ 2021-03-18 18:57 ` Rafael J. Wysocki
-1 siblings, 0 replies; 5+ messages in thread
From: Rafael J. Wysocki @ 2021-03-18 18:57 UTC (permalink / raw)
To: Colin King
Cc: Rafael J . Wysocki, Kevin Hilman, Ulf Hansson, Pavel Machek,
Len Brown, Greg Kroah-Hartman, Marc Titinger, Lina Iyer,
Linux PM, kernel-janitors, Linux Kernel Mailing List
On Sun, Feb 7, 2021 at 11:47 PM Colin King <colin.king@canonical.com> wrote:
>
> From: Colin Ian King <colin.king@canonical.com>
>
> There are three occurrances of u32 variables being multiplied by
> 1000 using 32 bit multiplies and the result being assigned to a
> 64 bit signed integer. These can potentially lead to a 32 bit
> overflows, so fix this by casting 1000 to a UL first to force
> a 64 bit multiply hence avoiding the overflow.
>
> Addresses-Coverity: ("Unintentional integer overflow")
> Fixes: 30f604283e05 ("PM / Domains: Allow domain power states to be read from DT")
> Signed-off-by: Colin Ian King <colin.king@canonical.com>
> ---
> drivers/base/power/domain.c | 6 +++---
> 1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
> index aaf6c83b5cf6..ddeff69126ff 100644
> --- a/drivers/base/power/domain.c
> +++ b/drivers/base/power/domain.c
> @@ -2831,10 +2831,10 @@ static int genpd_parse_state(struct genpd_power_state *genpd_state,
>
> err = of_property_read_u32(state_node, "min-residency-us", &residency);
> if (!err)
> - genpd_state->residency_ns = 1000 * residency;
> + genpd_state->residency_ns = 1000UL * residency;
Wouldn't it be better to use NSEC_PER_USEC here and below?
>
> - genpd_state->power_on_latency_ns = 1000 * exit_latency;
> - genpd_state->power_off_latency_ns = 1000 * entry_latency;
> + genpd_state->power_on_latency_ns = 1000UL * exit_latency;
> + genpd_state->power_off_latency_ns = 1000UL * entry_latency;
> genpd_state->fwnode = &state_node->fwnode;
>
> return 0;
> --
> 2.29.2
>
^ permalink raw reply [flat|nested] 5+ messages in thread