| Message ID | 20180918125314.GA12752@embeddedor.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | iio: adc: Fix potential integer overflow | expand |
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;
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
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
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.
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. >
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 >
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;
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(-)