[PATCH] cpufreq: intel_pstate: fix possible integer overflow

[PATCH] cpufreq: intel_pstate: fix possible integer overflow

[Geyslan G. Bem]

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

[PATCH] cpufreq: intel_pstate: fix possible integer overflow

[Geyslan G. Bem]

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

Re: [PATCH] cpufreq: intel_pstate: fix possible integer overflow

[Dirk Brandewie]

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;
>
>

Re: [PATCH] cpufreq: intel_pstate: fix possible integer overflow

[Rafael J. Wysocki]

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.

Re: [PATCH] cpufreq: intel_pstate: fix possible integer overflow

[Dirk Brandewie]

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;
>>>
>>>
>>

Re: [PATCH] cpufreq: intel_pstate: fix possible integer overflow

[Rafael J. Wysocki]

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.

Re: [PATCH] cpufreq: intel_pstate: fix possible integer overflow

[Geyslan Gregório Bem]

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.