Re: [PATCH 09/17] hwmon: (nct7802) Fix overflows seen when writing into limit attributes

[Guenter Roeck <linux@roeck-us.net>]

Hi Jean,

On 12/09/2016 01:49 AM, Jean Delvare wrote:
> Hi Guenter,
>
> On Sun,  4 Dec 2016 20:55:32 -0800, Guenter Roeck wrote:
>> Fix overflows seen when writing voltage and temperature limit attributes.
>>
>> The value passed to DIV_ROUND_CLOSEST() needs to be clamped.
>>
>> Signed-off-by: Guenter Roeck <linux@roeck-us.net>
>
> I think you can also add:
>
> Fixes: 3434f3783580 ("hwmon: Driver for Nuvoton NCT7802Y")
>
>> ---
>>  drivers/hwmon/nct7802.c | 7 ++++---
>>  1 file changed, 4 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/hwmon/nct7802.c b/drivers/hwmon/nct7802.c
>> index 3ce33d244cc0..6fe71cfe0320 100644
>> --- a/drivers/hwmon/nct7802.c
>> +++ b/drivers/hwmon/nct7802.c
>> @@ -326,8 +326,9 @@ static int nct7802_write_voltage(struct nct7802_data *data, int nr, int index,
>>  	int shift = 8 - REG_VOLTAGE_LIMIT_MSB_SHIFT[index - 1][nr];
>>  	int err;
>>
>> -	voltage = DIV_ROUND_CLOSEST(voltage, nct7802_vmul[nr]);
>> -	voltage = clamp_val(voltage, 0, 0x3ff);
>> +	voltage = DIV_ROUND_CLOSEST(clamp_val(voltage, 0,
>> +					      0x3ff * nct7802_vmul[nr]),
>> +				    nct7802_vmul[nr]);
>
> As for previous patches, I think separate lines make the code more
> readable.
>
Ok

>>
>>  	mutex_lock(&data->access_lock);
>>  	err = regmap_write(data->regmap,
>> @@ -402,7 +403,7 @@ static ssize_t store_temp(struct device *dev, struct device_attribute *attr,
>>  	if (err < 0)
>>  		return err;
>>
>> -	val = clamp_val(DIV_ROUND_CLOSEST(val, 1000), -128, 127);
>> +	val = DIV_ROUND_CLOSEST(clamp_val(val, -127000, 127000), 1000);
>
> You change the low limit from -128 to -127 without justification.
>
There is none. This is a bug.

>>
>>  	err = regmap_write(data->regmap, nr, val & 0xff);
>>  	return err ? : count;
>
> Looking at function nct7802_write_fan_min() I think an overflow can
> happen here too, as DIV_ROUND_CLOSEST() is called before clamp_val().
> Any reason why you did not fix that one?
>
Not really. The call is
	DIV_ROUND_CLOSEST(1350000U, limit);
and thus won't overflow.

Thanks,
Guenter