Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Subject: Re: [PATCH] iio: adc: Fix potential integer overflow
To:     Jonathan Cameron <jic23@kernel.org>,
        "Gustavo A. R. Silva" <gustavo@embeddedor.com>
Cc:     Hartmut Knaack <knaack.h@gmx.de>,
        Peter Meerwald-Stadler <pmeerw@pmeerw.net>,
        linux-iio@vger.kernel.org, linux-kernel@vger.kernel.org
References: <20180918125314.GA12752@embeddedor.com>
 <20180922144230.7d5e6c80@archlinux>
From:   Lars-Peter Clausen <lars@metafoo.de>
Message-ID: <b16657d2-250e-2cef-5aca-2c32b3fac8a4@metafoo.de>
Date:   Mon, 24 Sep 2018 19:18:22 +0200
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101
 Thunderbird/52.9.1
MIME-Version: 1.0
In-Reply-To: <20180922144230.7d5e6c80@archlinux>
Content-Type: text/plain; charset=utf-8
Content-Language: en-US
Content-Transfer-Encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

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.