[PATCH] iio: adc: Fix potential integer overflow

[PATCH] iio: adc: Fix potential integer overflow

[Gustavo A. R. Silva]

Cast factor to s64 in order to give the compiler complete information
about the proper arithmetic to use and avoid a potential integer
overflow. Notice that such variable is being used in a context
that expects an expression of type s64 (64 bits, signed).

Addresses-Coverity-ID: 1324146 ("Unintentional integer overflow")
Fixes: e13d757279bb ("iio: adc: Add QCOM SPMI PMIC5 ADC driver")
Signed-off-by: Gustavo A. R. Silva <gustavo@embeddedor.com>
---
 drivers/iio/adc/qcom-vadc-common.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/iio/adc/qcom-vadc-common.c b/drivers/iio/adc/qcom-vadc-common.c
index dcd7fb5..e360e27 100644
--- a/drivers/iio/adc/qcom-vadc-common.c
+++ b/drivers/iio/adc/qcom-vadc-common.c
@@ -282,7 +282,7 @@ static int qcom_vadc_scale_code_voltage_factor(u16 adc_code,
 	voltage = div64_s64(voltage, data->full_scale_code_volt);
 	if (voltage > 0) {
 		voltage *= prescale->den;
-		temp = prescale->num * factor;
+		temp = prescale->num * (s64)factor;
 		voltage = div64_s64(voltage, temp);
 	} else {
 		voltage = 0;
-- 
2.7.4

Re: [PATCH] iio: adc: Fix potential integer overflow

[Jonathan Cameron]

On Tue, 18 Sep 2018 07:53:14 -0500
"Gustavo A. R. Silva" <gustavo@embeddedor.com> wrote:

> Cast factor to s64 in order to give the compiler complete information
> about the proper arithmetic to use and avoid a potential integer
> overflow. Notice that such variable is being used in a context
> that expects an expression of type s64 (64 bits, signed).
> 
> Addresses-Coverity-ID: 1324146 ("Unintentional integer overflow")
> Fixes: e13d757279bb ("iio: adc: Add QCOM SPMI PMIC5 ADC driver")
> Signed-off-by: Gustavo A. R. Silva <gustavo@embeddedor.com>
> ---
>  drivers/iio/adc/qcom-vadc-common.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/iio/adc/qcom-vadc-common.c b/drivers/iio/adc/qcom-vadc-common.c
> index dcd7fb5..e360e27 100644
> --- a/drivers/iio/adc/qcom-vadc-common.c
> +++ b/drivers/iio/adc/qcom-vadc-common.c
> @@ -282,7 +282,7 @@ static int qcom_vadc_scale_code_voltage_factor(u16 adc_code,
>  	voltage = div64_s64(voltage, data->full_scale_code_volt);
>  	if (voltage > 0) {
>  		voltage *= prescale->den;
> -		temp = prescale->num * factor;
> +		temp = prescale->num * (s64)factor;
So factor is an unsigned int so could be 32 bits.  In reality it only
takes a small set of values between 1 and 1000

Maximum numerator is 10 so a maximum of 10,000.

Hence this is a false positive, be it one that would be very hard
for a static checker to identify.

So that moves it from a fix to a warning suppression change.
I have no problem with those, but description needs to reflect that.

Let me know if I've missed something, if not I'm happy to apply
this and will put some text in the message to explain the above
reasoning.

Thanks,

Jonathan

>  		voltage = div64_s64(voltage, temp);
>  	} else {
>  		voltage = 0;

Re: [PATCH] iio: adc: Fix potential integer overflow

[Gustavo A. R. Silva]

On 9/22/18 8:42 AM, Jonathan Cameron wrote:
> On Tue, 18 Sep 2018 07:53:14 -0500
> "Gustavo A. R. Silva" <gustavo@embeddedor.com> wrote:
> 
>> Cast factor to s64 in order to give the compiler complete information
>> about the proper arithmetic to use and avoid a potential integer
>> overflow. Notice that such variable is being used in a context
>> that expects an expression of type s64 (64 bits, signed).
>>
>> Addresses-Coverity-ID: 1324146 ("Unintentional integer overflow")
>> Fixes: e13d757279bb ("iio: adc: Add QCOM SPMI PMIC5 ADC driver")
>> Signed-off-by: Gustavo A. R. Silva <gustavo@embeddedor.com>
>> ---
>>  drivers/iio/adc/qcom-vadc-common.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/iio/adc/qcom-vadc-common.c b/drivers/iio/adc/qcom-vadc-common.c
>> index dcd7fb5..e360e27 100644
>> --- a/drivers/iio/adc/qcom-vadc-common.c
>> +++ b/drivers/iio/adc/qcom-vadc-common.c
>> @@ -282,7 +282,7 @@ static int qcom_vadc_scale_code_voltage_factor(u16 adc_code,
>>  	voltage = div64_s64(voltage, data->full_scale_code_volt);
>>  	if (voltage > 0) {
>>  		voltage *= prescale->den;
>> -		temp = prescale->num * factor;
>> +		temp = prescale->num * (s64)factor;
> So factor is an unsigned int so could be 32 bits.  In reality it only
> takes a small set of values between 1 and 1000
> 
> Maximum numerator is 10 so a maximum of 10,000.
> 
> Hence this is a false positive, be it one that would be very hard
> for a static checker to identify.
> 
> So that moves it from a fix to a warning suppression change.
> I have no problem with those, but description needs to reflect that.
> 
> Let me know if I've missed something, if not I'm happy to apply
> this and will put some text in the message to explain the above
> reasoning.
> 

Hi Jonathan,

I think you are right. Plase, feel free to update the commit log.

Thanks
--
Gustavo

Re: [PATCH] iio: adc: Fix potential integer overflow

[Himanshu Jha]

Hi Gustavo,

On Tue, Sep 18, 2018 at 07:53:14AM -0500, Gustavo A. R. Silva wrote:
> Cast factor to s64 in order to give the compiler complete information
> about the proper arithmetic to use and avoid a potential integer
> overflow. Notice that such variable is being used in a context
> that expects an expression of type s64 (64 bits, signed).
> 
> Addresses-Coverity-ID: 1324146 ("Unintentional integer overflow")
> Fixes: e13d757279bb ("iio: adc: Add QCOM SPMI PMIC5 ADC driver")
> Signed-off-by: Gustavo A. R. Silva <gustavo@embeddedor.com>
> ---
>  drivers/iio/adc/qcom-vadc-common.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/iio/adc/qcom-vadc-common.c b/drivers/iio/adc/qcom-vadc-common.c
> index dcd7fb5..e360e27 100644
> --- a/drivers/iio/adc/qcom-vadc-common.c
> +++ b/drivers/iio/adc/qcom-vadc-common.c
> @@ -282,7 +282,7 @@ static int qcom_vadc_scale_code_voltage_factor(u16 adc_code,
>  	voltage = div64_s64(voltage, data->full_scale_code_volt);
>  	if (voltage > 0) {
>  		voltage *= prescale->den;
> -		temp = prescale->num * factor;
> +		temp = prescale->num * (s64)factor;

As Jonathan pointed it is a false positive, let me share some more
insight on this particular set of warnings.

`num` is u32 and `factor` is unsigned int(u32 on most implementations).

So, if multiplication b/w them exceeds UNIT_MAX then that is perfectly
defined behavior in C. And often called "wrapping".
https://port70.net/~nsz/c/c11/n1570.html#6.2.5p9

And *if* it exceeds UNIT_MAX, then it is certainly wrong arthimetic
implementation by the author.

On the other hand, if it were the case signed int overflow then
certainly it is undefined behavior and called "overflow".

And here `temp` is guaranteed to not overflow!

But I don't understand what issue are you trying to resolve here and I'm
interested in this particular set of warnings because I too get coverity
scan reports on the same although I only search for IIO drivers issues.


Thanks
-- 
Himanshu Jha
Undergraduate Student
Department of Electronics & Communication
Guru Tegh Bahadur Institute of Technology

Re: [PATCH] iio: adc: Fix potential integer overflow

[Lars-Peter Clausen]

On 09/22/2018 03:42 PM, Jonathan Cameron wrote:
> On Tue, 18 Sep 2018 07:53:14 -0500
> "Gustavo A. R. Silva" <gustavo@embeddedor.com> wrote:
> 
>> Cast factor to s64 in order to give the compiler complete information
>> about the proper arithmetic to use and avoid a potential integer
>> overflow. Notice that such variable is being used in a context
>> that expects an expression of type s64 (64 bits, signed).
>>
>> Addresses-Coverity-ID: 1324146 ("Unintentional integer overflow")
>> Fixes: e13d757279bb ("iio: adc: Add QCOM SPMI PMIC5 ADC driver")
>> Signed-off-by: Gustavo A. R. Silva <gustavo@embeddedor.com>
>> ---
>>  drivers/iio/adc/qcom-vadc-common.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/iio/adc/qcom-vadc-common.c b/drivers/iio/adc/qcom-vadc-common.c
>> index dcd7fb5..e360e27 100644
>> --- a/drivers/iio/adc/qcom-vadc-common.c
>> +++ b/drivers/iio/adc/qcom-vadc-common.c
>> @@ -282,7 +282,7 @@ static int qcom_vadc_scale_code_voltage_factor(u16 adc_code,
>>  	voltage = div64_s64(voltage, data->full_scale_code_volt);
>>  	if (voltage > 0) {
>>  		voltage *= prescale->den;
>> -		temp = prescale->num * factor;
>> +		temp = prescale->num * (s64)factor;
> So factor is an unsigned int so could be 32 bits.  In reality it only
> takes a small set of values between 1 and 1000
> 
> Maximum numerator is 10 so a maximum of 10,000.
> 
> Hence this is a false positive, be it one that would be very hard
> for a static checker to identify.

I think the reason why it complains is because temp is s64. So it infers
that the idea was that the result of the multiplication can be larger
than 64 bit. For 32bit * 32bit -> 32bit it should not complain.

> 
> So that moves it from a fix to a warning suppression change.
> I have no problem with those, but description needs to reflect that.

Maybe just change the type of temp to u32. There is also
mul_u64_u32_div() which could be used here to further simplify things.

Re: [PATCH] iio: adc: Fix potential integer overflow

[Lars-Peter Clausen]

On 09/24/2018 07:18 PM, Lars-Peter Clausen wrote:
> On 09/22/2018 03:42 PM, Jonathan Cameron wrote:
>> On Tue, 18 Sep 2018 07:53:14 -0500
>> "Gustavo A. R. Silva" <gustavo@embeddedor.com> wrote:
>>
>>> Cast factor to s64 in order to give the compiler complete information
>>> about the proper arithmetic to use and avoid a potential integer
>>> overflow. Notice that such variable is being used in a context
>>> that expects an expression of type s64 (64 bits, signed).
>>>
>>> Addresses-Coverity-ID: 1324146 ("Unintentional integer overflow")
>>> Fixes: e13d757279bb ("iio: adc: Add QCOM SPMI PMIC5 ADC driver")
>>> Signed-off-by: Gustavo A. R. Silva <gustavo@embeddedor.com>
>>> ---
>>>  drivers/iio/adc/qcom-vadc-common.c | 2 +-
>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/iio/adc/qcom-vadc-common.c b/drivers/iio/adc/qcom-vadc-common.c
>>> index dcd7fb5..e360e27 100644
>>> --- a/drivers/iio/adc/qcom-vadc-common.c
>>> +++ b/drivers/iio/adc/qcom-vadc-common.c
>>> @@ -282,7 +282,7 @@ static int qcom_vadc_scale_code_voltage_factor(u16 adc_code,
>>>  	voltage = div64_s64(voltage, data->full_scale_code_volt);
>>>  	if (voltage > 0) {
>>>  		voltage *= prescale->den;
>>> -		temp = prescale->num * factor;
>>> +		temp = prescale->num * (s64)factor;
>> So factor is an unsigned int so could be 32 bits.  In reality it only
>> takes a small set of values between 1 and 1000
>>
>> Maximum numerator is 10 so a maximum of 10,000.
>>
>> Hence this is a false positive, be it one that would be very hard
>> for a static checker to identify.
> 
> I think the reason why it complains is because temp is s64. So it infers
> that the idea was that the result of the multiplication can be larger
> than 64 bit. For 32bit * 32bit -> 32bit it should not complain.

"lager than 32 bit"

> 
>>
>> So that moves it from a fix to a warning suppression change.
>> I have no problem with those, but description needs to reflect that.
> 
> Maybe just change the type of temp to u32. There is also
> mul_u64_u32_div() which could be used here to further simplify things.
> 

Re: [PATCH] iio: adc: Fix potential integer overflow

[Jonathan Cameron]

On Mon, 24 Sep 2018 19:19:34 +0200
Lars-Peter Clausen <lars@metafoo.de> wrote:

> On 09/24/2018 07:18 PM, Lars-Peter Clausen wrote:
> > On 09/22/2018 03:42 PM, Jonathan Cameron wrote:  
> >> On Tue, 18 Sep 2018 07:53:14 -0500
> >> "Gustavo A. R. Silva" <gustavo@embeddedor.com> wrote:
> >>  
> >>> Cast factor to s64 in order to give the compiler complete information
> >>> about the proper arithmetic to use and avoid a potential integer
> >>> overflow. Notice that such variable is being used in a context
> >>> that expects an expression of type s64 (64 bits, signed).
> >>>
> >>> Addresses-Coverity-ID: 1324146 ("Unintentional integer overflow")
> >>> Fixes: e13d757279bb ("iio: adc: Add QCOM SPMI PMIC5 ADC driver")
> >>> Signed-off-by: Gustavo A. R. Silva <gustavo@embeddedor.com>
> >>> ---
> >>>  drivers/iio/adc/qcom-vadc-common.c | 2 +-
> >>>  1 file changed, 1 insertion(+), 1 deletion(-)
> >>>
> >>> diff --git a/drivers/iio/adc/qcom-vadc-common.c b/drivers/iio/adc/qcom-vadc-common.c
> >>> index dcd7fb5..e360e27 100644
> >>> --- a/drivers/iio/adc/qcom-vadc-common.c
> >>> +++ b/drivers/iio/adc/qcom-vadc-common.c
> >>> @@ -282,7 +282,7 @@ static int qcom_vadc_scale_code_voltage_factor(u16 adc_code,
> >>>  	voltage = div64_s64(voltage, data->full_scale_code_volt);
> >>>  	if (voltage > 0) {
> >>>  		voltage *= prescale->den;
> >>> -		temp = prescale->num * factor;
> >>> +		temp = prescale->num * (s64)factor;  
> >> So factor is an unsigned int so could be 32 bits.  In reality it only
> >> takes a small set of values between 1 and 1000
> >>
> >> Maximum numerator is 10 so a maximum of 10,000.
> >>
> >> Hence this is a false positive, be it one that would be very hard
> >> for a static checker to identify.  
> > 
> > I think the reason why it complains is because temp is s64. So it infers
> > that the idea was that the result of the multiplication can be larger
> > than 64 bit. For 32bit * 32bit -> 32bit it should not complain.  
> 
> "lager than 32 bit"
> 
> >   
> >>
> >> So that moves it from a fix to a warning suppression change.
> >> I have no problem with those, but description needs to reflect that.  
> > 
> > Maybe just change the type of temp to u32. There is also
> > mul_u64_u32_div() which could be used here to further simplify things.
> >   
That would be a nice improvement to this patch.  Gustavo,
if you don't mind doing an updated version that would be great.
If not I'll get to it sooner or later.

Thanks,

Jonathan

>