linux-kernel - Re: [PATCH] iio: adc: Fix potential integer overflow

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20180922144230.7d5e6c80@archlinux>
Date:   Sat, 22 Sep 2018 14:42:30 +0100
From:   Jonathan Cameron <jic23@...nel.org>
To:     "Gustavo A. R. Silva" <gustavo@...eddedor.com>
Cc:     Hartmut Knaack <knaack.h@....de>,
        Lars-Peter Clausen <lars@...afoo.de>,
        Peter Meerwald-Stadler <pmeerw@...erw.net>,
        linux-iio@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] iio: adc: Fix potential integer overflow

On Tue, 18 Sep 2018 07:53:14 -0500
"Gustavo A. R. Silva" <gustavo@...eddedor.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@...eddedor.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;