| Message ID | 20231130200207.work.679-kees@kernel.org (mailing list archive) |
|---|---|
| State | Accepted |
| Headers | show |
| Series | hwmon: (pc87360) Bounds check data->innr usage | expand |
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@gmail.com> > Cc: Jean Delvare <jdelvare@suse.com> > Cc: Guenter Roeck <linux@roeck-us.net> > Cc: linux-hwmon@vger.kernel.org > Signed-off-by: Kees Cook <keescook@chromium.org> Looks good to me. Reviewed-by: Gustavo A. R. Silva <gustavoars@kernel.org> Thanks! -- Gustavo > --- > drivers/hwmon/pc87360.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/hwmon/pc87360.c b/drivers/hwmon/pc87360.c > index 926ea1fe133c..db80394ba854 100644 > --- a/drivers/hwmon/pc87360.c > +++ b/drivers/hwmon/pc87360.c > @@ -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 */
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@gmail.com> >> Cc: Jean Delvare <jdelvare@suse.com> >> Cc: Guenter Roeck <linux@roeck-us.net> >> Cc: linux-hwmon@vger.kernel.org >> Signed-off-by: Kees Cook <keescook@chromium.org> > > Looks good to me. > > Reviewed-by: Gustavo A. R. Silva <gustavoars@kernel.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
diff --git a/drivers/hwmon/pc87360.c b/drivers/hwmon/pc87360.c index 926ea1fe133c..db80394ba854 100644 --- a/drivers/hwmon/pc87360.c +++ b/drivers/hwmon/pc87360.c @@ -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 */
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@gmail.com> Cc: Jean Delvare <jdelvare@suse.com> Cc: Guenter Roeck <linux@roeck-us.net> Cc: linux-hwmon@vger.kernel.org Signed-off-by: Kees Cook <keescook@chromium.org> --- drivers/hwmon/pc87360.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)