Skip to contentSkip to conversation

hwmon: (pc87360) Bounds check data->innr usage

PATCH In review Kees Cook 3 participants 3 messages Nov 30, 2023

Subsystems (2)
Hardware monitoring PC87360 hardware monitoring driver
lore
Share & tools
lore

Kees Cook

author
#0 lore raw

PATCH 1/1 hwmon: (pc87360) Bounds check data->innr usage

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 <[email protected]>
Cc Jean Delvare <[email protected]>
Cc Guenter Roeck <[email protected]>
Cc [email protected]
Signed-off-by Kees Cook <[email protected]>
---
 drivers/hwmon/pc87360.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/hwmon/pc87360.c +1 -1
index 926ea1fe133c..db80394ba854 100644
@@ -323,7 +323,7 @@ static struct pc87360_data *pc87360_update_device(struct device *dev)
 		}
 
 		/* Voltages */
-		for (i = 0; i < data->innr; i++) {
+		for (i = 0; i < min(data->innr, ARRAY_SIZE(data->in)); i++) {
 			data->in_status[i] = pc87360_read_value(data, LD_IN, i,
 					     PC87365_REG_IN_STATUS);
 			/* Clear bits */
Signature · 1 line
2.34.1
Reviewed-by on v1 1/1
#1 lore raw
Quoted 17 lines · Kees Cook

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 */
       |            ^~~~~~
Quoted 5 lines
Cc Jim Cromie <[email protected]>
Cc Jean Delvare <[email protected]>
Cc Guenter Roeck <[email protected]>
Cc [email protected]
Signed-off-by Kees Cook <[email protected]>

Looks good to me.

Reviewed-by Gustavo A. R. Silva <[email protected]>

Thanks!
--
Gustavo

Quoted 17 lines
---
  drivers/hwmon/pc87360.c | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)
drivers/hwmon/pc87360.c +1 -1
index 926ea1fe133c..db80394ba854 100644
@@ -323,7 +323,7 @@ static struct pc87360_data *pc87360_update_device(struct device *dev)
  		}
 
  		/* Voltages */
-		for (i = 0; i < data->innr; i++) {
+		for (i = 0; i < min(data->innr, ARRAY_SIZE(data->in)); i++) {
  			data->in_status[i] = pc87360_read_value(data, LD_IN, i,
  					     PC87365_REG_IN_STATUS);
  			/* Clear bits */
maintainer
#2 lore raw
Quoted 26 lines · Gustavo A. R. Silva
Quoted 17 lines · Kees Cook

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 */
       |            ^~~~~~

Quoted 5 lines
Cc Jim Cromie <[email protected]>
Cc Jean Delvare <[email protected]>
Cc Guenter Roeck <[email protected]>
Cc [email protected]
Signed-off-by Kees Cook <[email protected]>
Quoted 4 lines

Looks good to me.

Reviewed-by Gustavo A. R. Silva <[email protected]>

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