* [PATCH] cpufreq: intel_pstate: fix possible integer overflow @ 2013-10-20 3:31 Geyslan G. Bem 2013-10-21 15:56 ` Dirk Brandewie 0 siblings, 1 reply; 6+ messages in thread From: Geyslan G. Bem @ 2013-10-20 3:31 UTC (permalink / raw) To: kernel-br Cc: Geyslan G. Bem, Rafael J. Wysocki, Viresh Kumar, open list:CPU FREQUENCY DRI..., open list:CPU FREQUENCY DRI..., open list The expression 'pstate << 8' is evaluated using 32-bit arithmetic while 'val' expects an expression of type u64. Signed-off-by: Geyslan G. Bem <geyslan@gmail.com> --- drivers/cpufreq/intel_pstate.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/cpufreq/intel_pstate.c b/drivers/cpufreq/intel_pstate.c index badf620..43446b5 100644 --- a/drivers/cpufreq/intel_pstate.c +++ b/drivers/cpufreq/intel_pstate.c @@ -395,7 +395,7 @@ static void intel_pstate_set_pstate(struct cpudata *cpu, int pstate) trace_cpu_frequency(pstate * 100000, cpu->cpu); cpu->pstate.current_pstate = pstate; - val = pstate << 8; + val = (u64)pstate << 8; if (limits.no_turbo) val |= (u64)1 << 32; -- 1.8.4 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] cpufreq: intel_pstate: fix possible integer overflow 2013-10-20 3:31 [PATCH] cpufreq: intel_pstate: fix possible integer overflow Geyslan G. Bem @ 2013-10-21 15:56 ` Dirk Brandewie 2013-10-21 22:47 ` Rafael J. Wysocki 0 siblings, 1 reply; 6+ messages in thread From: Dirk Brandewie @ 2013-10-21 15:56 UTC (permalink / raw) To: Geyslan G. Bem, kernel-br Cc: Rafael J. Wysocki, Viresh Kumar, open list:CPU FREQUENCY DRI..., open list:CPU FREQUENCY DRI..., open list On 10/19/2013 08:31 PM, Geyslan G. Bem wrote: > The expression 'pstate << 8' is evaluated using 32-bit arithmetic while > 'val' expects an expression of type u64. > > Signed-off-by: Geyslan G. Bem <geyslan@gmail.com> Acked-by: Dirk Brandewie <dirk.j.brandewie@intel.com> > --- > drivers/cpufreq/intel_pstate.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/cpufreq/intel_pstate.c b/drivers/cpufreq/intel_pstate.c > index badf620..43446b5 100644 > --- a/drivers/cpufreq/intel_pstate.c > +++ b/drivers/cpufreq/intel_pstate.c > @@ -395,7 +395,7 @@ static void intel_pstate_set_pstate(struct cpudata *cpu, int pstate) > trace_cpu_frequency(pstate * 100000, cpu->cpu); > > cpu->pstate.current_pstate = pstate; > - val = pstate << 8; > + val = (u64)pstate << 8; > if (limits.no_turbo) > val |= (u64)1 << 32; > > ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] cpufreq: intel_pstate: fix possible integer overflow 2013-10-21 15:56 ` Dirk Brandewie @ 2013-10-21 22:47 ` Rafael J. Wysocki 2013-10-21 22:43 ` Dirk Brandewie 0 siblings, 1 reply; 6+ messages in thread From: Rafael J. Wysocki @ 2013-10-21 22:47 UTC (permalink / raw) To: Dirk Brandewie Cc: Geyslan G. Bem, kernel-br, Viresh Kumar, open list:CPU FREQUENCY DRI..., open list:CPU FREQUENCY DRI..., open list On Monday, October 21, 2013 08:56:22 AM Dirk Brandewie wrote: > On 10/19/2013 08:31 PM, Geyslan G. Bem wrote: > > The expression 'pstate << 8' is evaluated using 32-bit arithmetic while > > 'val' expects an expression of type u64. > > > > Signed-off-by: Geyslan G. Bem <geyslan@gmail.com> > Acked-by: Dirk Brandewie <dirk.j.brandewie@intel.com> Actually, isn't (pstate << 8) guaranteed not to overflow? > > --- > > drivers/cpufreq/intel_pstate.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/drivers/cpufreq/intel_pstate.c b/drivers/cpufreq/intel_pstate.c > > index badf620..43446b5 100644 > > --- a/drivers/cpufreq/intel_pstate.c > > +++ b/drivers/cpufreq/intel_pstate.c > > @@ -395,7 +395,7 @@ static void intel_pstate_set_pstate(struct cpudata *cpu, int pstate) > > trace_cpu_frequency(pstate * 100000, cpu->cpu); > > > > cpu->pstate.current_pstate = pstate; > > - val = pstate << 8; > > + val = (u64)pstate << 8; > > if (limits.no_turbo) > > val |= (u64)1 << 32; > > > > > -- I speak only for myself. Rafael J. Wysocki, Intel Open Source Technology Center. ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] cpufreq: intel_pstate: fix possible integer overflow 2013-10-21 22:47 ` Rafael J. Wysocki @ 2013-10-21 22:43 ` Dirk Brandewie 2013-10-21 23:06 ` Rafael J. Wysocki 0 siblings, 1 reply; 6+ messages in thread From: Dirk Brandewie @ 2013-10-21 22:43 UTC (permalink / raw) To: Rafael J. Wysocki Cc: Geyslan G. Bem, kernel-br, Viresh Kumar, open list:CPU FREQUENCY DRI..., open list:CPU FREQUENCY DRI..., open list On 10/21/2013 03:47 PM, Rafael J. Wysocki wrote: > On Monday, October 21, 2013 08:56:22 AM Dirk Brandewie wrote: >> On 10/19/2013 08:31 PM, Geyslan G. Bem wrote: >>> The expression 'pstate << 8' is evaluated using 32-bit arithmetic while >>> 'val' expects an expression of type u64. >>> >>> Signed-off-by: Geyslan G. Bem <geyslan@gmail.com> >> Acked-by: Dirk Brandewie <dirk.j.brandewie@intel.com> > > Actually, isn't (pstate << 8) guaranteed not to overflow? > Yes, I was assuming this was caught by a static checking tool. I didn't see a downside to giving the compilier complete information. >>> --- >>> drivers/cpufreq/intel_pstate.c | 2 +- >>> 1 file changed, 1 insertion(+), 1 deletion(-) >>> >>> diff --git a/drivers/cpufreq/intel_pstate.c b/drivers/cpufreq/intel_pstate.c >>> index badf620..43446b5 100644 >>> --- a/drivers/cpufreq/intel_pstate.c >>> +++ b/drivers/cpufreq/intel_pstate.c >>> @@ -395,7 +395,7 @@ static void intel_pstate_set_pstate(struct cpudata *cpu, int pstate) >>> trace_cpu_frequency(pstate * 100000, cpu->cpu); >>> >>> cpu->pstate.current_pstate = pstate; >>> - val = pstate << 8; >>> + val = (u64)pstate << 8; >>> if (limits.no_turbo) >>> val |= (u64)1 << 32; >>> >>> >> ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] cpufreq: intel_pstate: fix possible integer overflow 2013-10-21 22:43 ` Dirk Brandewie @ 2013-10-21 23:06 ` Rafael J. Wysocki [not found] ` <CANOOhLRCC360exKKePsJGxYqx3ULZf1Ac0y16hsnPQNP69OHwA@mail.gmail.com> 0 siblings, 1 reply; 6+ messages in thread From: Rafael J. Wysocki @ 2013-10-21 23:06 UTC (permalink / raw) To: Dirk Brandewie Cc: Geyslan G. Bem, kernel-br, Viresh Kumar, open list:CPU FREQUENCY DRI..., open list:CPU FREQUENCY DRI..., open list On Monday, October 21, 2013 03:43:51 PM Dirk Brandewie wrote: > On 10/21/2013 03:47 PM, Rafael J. Wysocki wrote: > > On Monday, October 21, 2013 08:56:22 AM Dirk Brandewie wrote: > >> On 10/19/2013 08:31 PM, Geyslan G. Bem wrote: > >>> The expression 'pstate << 8' is evaluated using 32-bit arithmetic while > >>> 'val' expects an expression of type u64. > >>> > >>> Signed-off-by: Geyslan G. Bem <geyslan@gmail.com> > >> Acked-by: Dirk Brandewie <dirk.j.brandewie@intel.com> > > > > Actually, isn't (pstate << 8) guaranteed not to overflow? > > > > Yes, I was assuming this was caught by a static checking tool. What was caught by the tool was the fact that 1UL << 32 might overflow on 32-bit, so using BIT(32) wasn't correct. > I didn't see a downside to giving the compilier complete information. Well, in that case the function's argument should be u64 rather than int. Either you know that it won't overflow, in which case the explicit type casting doesn't change anything, or you are not sure, in which case it's better to use u64 as the original type anyway in my opinion. Thanks! -- I speak only for myself. Rafael J. Wysocki, Intel Open Source Technology Center. ^ permalink raw reply [flat|nested] 6+ messages in thread
[parent not found: <CANOOhLRCC360exKKePsJGxYqx3ULZf1Ac0y16hsnPQNP69OHwA@mail.gmail.com>]
* Re: [PATCH] cpufreq: intel_pstate: fix possible integer overflow [not found] ` <CANOOhLRCC360exKKePsJGxYqx3ULZf1Ac0y16hsnPQNP69OHwA@mail.gmail.com> @ 2013-10-22 10:25 ` Geyslan Gregório Bem 0 siblings, 0 replies; 6+ messages in thread From: Geyslan Gregório Bem @ 2013-10-22 10:25 UTC (permalink / raw) To: Dirk Brandewie Cc: Rafael J. Wysocki, Viresh Kumar, open list:CPU FREQUENCY DRI..., open list:CPU FREQUENCY DRI..., open list 2013/10/21 Dirk Brandewie <dirk.brandewie@gmail.com>: > > > On Monday, October 21, 2013, Rafael J. Wysocki wrote: >> >> On Monday, October 21, 2013 03:43:51 PM Dirk Brandewie wrote: >> > On 10/21/2013 03:47 PM, Rafael J. Wysocki wrote: >> > > On Monday, October 21, 2013 08:56:22 AM Dirk Brandewie wrote: >> > >> On 10/19/2013 08:31 PM, Geyslan G. Bem wrote: >> > >>> The expression 'pstate << 8' is evaluated using 32-bit arithmetic >> > >>> while >> > >>> 'val' expects an expression of type u64. >> > >>> >> > >>> Signed-off-by: Geyslan G. Bem <geyslan@gmail.com> >> > >> Acked-by: Dirk Brandewie <dirk.j.brandewie@intel.com> >> > > >> > > Actually, isn't (pstate << 8) guaranteed not to overflow? >> > > >> > >> > Yes, I was assuming this was caught by a static checking tool. >> Yes, it was caught by Coverity, and I didn't debug the possibles pstate's. >> What was caught by the tool was the fact that 1UL << 32 might overflow on >> 32-bit, so using BIT(32) wasn't correct. This is the entire output: CID 1108110 (#1 of 1): Unintentional integer overflow (OVERFLOW_BEFORE_WIDEN)overflow_before_widen: Potentially overflowing expression "pstate << 8" with type "int" (32 bits, signed) is evaluated using 32-bit arithmetic before being used in a context which expects an expression of type "u64" (64 bits, unsigned). To avoid overflow, cast the left operand to "u64" before performing the left shift. >> >> > I didn't see a downside to giving the compilier complete information. >> >> Well, in that case the function's argument should be u64 rather than int. >> >> Either you know that it won't overflow, in which case the explicit type >> casting doesn't change anything, or you are not sure, in which case it's >> better to use u64 as the original type anyway in my opinion. > > > pstate << 8 can't overflow we can drop this I realized that are five calls to intel_pstate_set_pstate() /drivers/cpufreq/intel_pstate.c:410 /drivers/cpufreq/intel_pstate.c:417 /drivers/cpufreq/intel_pstate.c:432 /drivers/cpufreq/intel_pstate.c:514 /drivers/cpufreq/intel_pstate.c:566 I really don't know if the values that pstate assumes could cause overflow. And I really don't know if these values may change in the future. So I have assumed that the most careful is to cast, making the code error proof. But you know the code, thus know what is better. ;) Cheers. > >> >> Thanks! >> >> -- >> I speak only for myself. >> Rafael J. Wysocki, Intel Open Source Technology Center. ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2013-10-22 10:25 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2013-10-20 3:31 [PATCH] cpufreq: intel_pstate: fix possible integer overflow Geyslan G. Bem
2013-10-21 15:56 ` Dirk Brandewie
2013-10-21 22:47 ` Rafael J. Wysocki
2013-10-21 22:43 ` Dirk Brandewie
2013-10-21 23:06 ` Rafael J. Wysocki
[not found] ` <CANOOhLRCC360exKKePsJGxYqx3ULZf1Ac0y16hsnPQNP69OHwA@mail.gmail.com>
2013-10-22 10:25 ` Geyslan Gregório Bem
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox