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] [day] [month] [year] [list]
Date: Thu, 30 Nov 2023 12:31:26 -0800
From: Guenter Roeck <linux@...ck-us.net>
To: "Gustavo A. R. Silva" <gustavo@...eddedor.com>,
 Kees Cook <keescook@...omium.org>, Jim Cromie <jim.cromie@...il.com>
Cc: Jean Delvare <jdelvare@...e.com>, linux-hwmon@...r.kernel.org,
 linux-kernel@...r.kernel.org, linux-hardening@...r.kernel.org
Subject: Re: [PATCH] hwmon: (pc87360) Bounds check data->innr usage

On 11/30/23 12:11, Gustavo A. R. Silva wrote:
> 
> 
> On 11/30/23 14:02, Kees Cook wrote:
>> Without visibility into the initializers for data->innr, GCC suspects
>> using it as an index could walk off the end of the various 14-element
>> arrays in data. Perform an explicit clamp to the array size. Silences
>> the following warning with GCC 12+:
>>
>> ../drivers/hwmon/pc87360.c: In function 'pc87360_update_device':
>> ../drivers/hwmon/pc87360.c:341:49: warning: writing 1 byte into a region of size 0 [-Wstringop-overflow=]
>>    341 |                                 data->in_max[i] = pc87360_read_value(data,
>>        |                                 ~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~
>>    342 |                                                   LD_IN, i,
>>        |                                                   ~~~~~~~~~
>>    343 |                                                   PC87365_REG_IN_MAX);
>>        |                                                   ~~~~~~~~~~~~~~~~~~~
>> ../drivers/hwmon/pc87360.c:209:12: note: at offset 255 into destination object 'in_max' of size 14
>>    209 |         u8 in_max[14];          /* Register value */
>>        |            ^~~~~~
>>
>> Cc: Jim Cromie <jim.cromie@...il.com>
>> Cc: Jean Delvare <jdelvare@...e.com>
>> Cc: Guenter Roeck <linux@...ck-us.net>
>> Cc: linux-hwmon@...r.kernel.org
>> Signed-off-by: Kees Cook <keescook@...omium.org>
> 
> Looks good to me.
> 
> Reviewed-by: Gustavo A. R. Silva <gustavoars@...nel.org>
> 

Guess I'll apply it, even though it is quite pointless. But arguing against
such changes seems like shouting into the wind, so whatever.

There are several other similar "unchecked" loops, including loops for
fannr and tempnr. The driver would misbehave badly if any of those would
ever be outside the valid range, both when accessing the hardware and
writing into various arrays.

Guenter


Powered by blists - more mailing lists