* [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 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 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: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
* 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