On Fri, Jan 27, 2023 at 02:37:45PM -0800, Kees Cook wrote:
> The index into various register arrays was not bounds checked. Add
> checking. Seen under GCC 13:
>
> drivers/hwmon/lm85.c: In function 'pwm_auto_pwm_minctl_store':
> drivers/hwmon/lm85.c:1110:26: warning: array subscript [0, 31] is outside array bounds of 'struct lm85_autofan[3]' [-Warray-bounds=]
> 1110 | if (data->autofan[nr].min_off)
> | ~~~~~~~~~~~~~^~~~
> drivers/hwmon/lm85.c:317:29: note: while referencing 'autofan'
> 317 | struct lm85_autofan autofan[3];
> | ^~~~~~~
>
This is a false positive. The value can never be >= 3.
It is derived from the last value of the following
SENSOR_DEVICE_ATTR_RW() entries.
I resist making changes like this to the code just because
the compiler can not determine the range of a variable.
It blows up code size amd makes it hard to read just to
make the compiler happy.
Guenter
> Cc: Jean Delvare <jdelvare@xxxxxxxx>
> Cc: Guenter Roeck <linux@xxxxxxxxxxxx>
> Cc: linux-hwmon@xxxxxxxxxxxxxxx
> Signed-off-by: Kees Cook <keescook@xxxxxxxxxxxx>
> ---
> drivers/hwmon/lm85.c | 221 +++++++++++++++++++++++++++---------
> include/linux/hwmon-sysfs.h | 2 +-
> 2 files changed, 167 insertions(+), 56 deletions(-)
>
> diff --git a/drivers/hwmon/lm85.c b/drivers/hwmon/lm85.c
> index 8d33c2484755..367a77660dda 100644
> --- a/drivers/hwmon/lm85.c
> +++ b/drivers/hwmon/lm85.c
> @@ -552,29 +552,40 @@ static struct lm85_data *lm85_update_device(struct device *dev)
> static ssize_t fan_show(struct device *dev, struct device_attribute *attr,
> char *buf)
> {
> - int nr = to_sensor_dev_attr(attr)->index;
> + unsigned int nr = to_sensor_dev_attr(attr)->index;
> struct lm85_data *data = lm85_update_device(dev);
> - return sprintf(buf, "%d\n", FAN_FROM_REG(data->fan[nr]));
> + int val = 0;
> +
> + if (nr < ARRAY_SIZE(data->fan))
> + val = FAN_FROM_REG(data->fan[nr]);
> + return sprintf(buf, "%d\n", val);
> }
>
> static ssize_t fan_min_show(struct device *dev, struct device_attribute *attr,
> char *buf)
> {
> - int nr = to_sensor_dev_attr(attr)->index;
> + unsigned int nr = to_sensor_dev_attr(attr)->index;
> struct lm85_data *data = lm85_update_device(dev);
> - return sprintf(buf, "%d\n", FAN_FROM_REG(data->fan_min[nr]));
> + int val = 0;
> +
> + if (nr < ARRAY_SIZE(data->fan_min))
> + val = FAN_FROM_REG(data->fan_min[nr]);
> + return sprintf(buf, "%d\n", val);
> }
>
> static ssize_t fan_min_store(struct device *dev,
> struct device_attribute *attr, const char *buf,
> size_t count)
> {
> - int nr = to_sensor_dev_attr(attr)->index;
> + unsigned int nr = to_sensor_dev_attr(attr)->index;
> struct lm85_data *data = dev_get_drvdata(dev);
> struct i2c_client *client = data->client;
> unsigned long val;
> int err;
>
> + if (nr >= ARRAY_SIZE(data->fan_min))
> + return -EINVAL;
> +
> err = kstrtoul(buf, 10, &val);
> if (err)
> return err;
> @@ -683,20 +694,27 @@ static SENSOR_DEVICE_ATTR_RO(fan4_alarm, alarm, 13);
> static ssize_t pwm_show(struct device *dev, struct device_attribute *attr,
> char *buf)
> {
> - int nr = to_sensor_dev_attr(attr)->index;
> + unsigned int nr = to_sensor_dev_attr(attr)->index;
> struct lm85_data *data = lm85_update_device(dev);
> - return sprintf(buf, "%d\n", PWM_FROM_REG(data->pwm[nr]));
> + int val = 0;
> +
> + if (nr < ARRAY_SIZE(data->pwm))
> + val = PWM_FROM_REG(data->pwm[nr]);
> + return sprintf(buf, "%d\n", val);
> }
>
> static ssize_t pwm_store(struct device *dev, struct device_attribute *attr,
> const char *buf, size_t count)
> {
> - int nr = to_sensor_dev_attr(attr)->index;
> + unsigned int nr = to_sensor_dev_attr(attr)->index;
> struct lm85_data *data = dev_get_drvdata(dev);
> struct i2c_client *client = data->client;
> unsigned long val;
> int err;
>
> + if (nr >= ARRAY_SIZE(data->pwm))
> + return -EINVAL;
> +
> err = kstrtoul(buf, 10, &val);
> if (err)
> return err;
> @@ -711,11 +729,12 @@ static ssize_t pwm_store(struct device *dev, struct device_attribute *attr,
> static ssize_t pwm_enable_show(struct device *dev,
> struct device_attribute *attr, char *buf)
> {
> - int nr = to_sensor_dev_attr(attr)->index;
> + unsigned int nr = to_sensor_dev_attr(attr)->index;
> struct lm85_data *data = lm85_update_device(dev);
> - int pwm_zone, enable;
> + int pwm_zone = -1, enable;
>
> - pwm_zone = ZONE_FROM_REG(data->autofan[nr].config);
> + if (nr < ARRAY_SIZE(data->autofan))
> + pwm_zone = ZONE_FROM_REG(data->autofan[nr].config);
> switch (pwm_zone) {
> case -1: /* PWM is always at 100% */
> enable = 0;
> @@ -734,13 +753,16 @@ static ssize_t pwm_enable_store(struct device *dev,
> struct device_attribute *attr,
> const char *buf, size_t count)
> {
> - int nr = to_sensor_dev_attr(attr)->index;
> + unsigned int nr = to_sensor_dev_attr(attr)->index;
> struct lm85_data *data = dev_get_drvdata(dev);
> struct i2c_client *client = data->client;
> u8 config;
> unsigned long val;
> int err;
>
> + if (nr >= ARRAY_SIZE(data->autofan))
> + return -EINVAL;
> +
> err = kstrtoul(buf, 10, &val);
> if (err)
> return err;
> @@ -777,10 +799,13 @@ static ssize_t pwm_enable_store(struct device *dev,
> static ssize_t pwm_freq_show(struct device *dev,
> struct device_attribute *attr, char *buf)
> {
> - int nr = to_sensor_dev_attr(attr)->index;
> + unsigned int nr = to_sensor_dev_attr(attr)->index;
> struct lm85_data *data = lm85_update_device(dev);
> int freq;
>
> + if (nr >= ARRAY_SIZE(data->pwm_freq))
> + return -EINVAL;
> +
> if (IS_ADT7468_HFPWM(data))
> freq = 22500;
> else
> @@ -794,12 +819,15 @@ static ssize_t pwm_freq_store(struct device *dev,
> struct device_attribute *attr, const char *buf,
> size_t count)
> {
> - int nr = to_sensor_dev_attr(attr)->index;
> + unsigned int nr = to_sensor_dev_attr(attr)->index;
> struct lm85_data *data = dev_get_drvdata(dev);
> struct i2c_client *client = data->client;
> unsigned long val;
> int err;
>
> + if (nr >= ARRAY_SIZE(data->pwm_freq))
> + return -EINVAL;
> +
> err = kstrtoul(buf, 10, &val);
> if (err)
> return err;
> @@ -843,10 +871,13 @@ static SENSOR_DEVICE_ATTR_RW(pwm3_freq, pwm_freq, 2);
> static ssize_t in_show(struct device *dev, struct device_attribute *attr,
> char *buf)
> {
> - int nr = to_sensor_dev_attr(attr)->index;
> + unsigned int nr = to_sensor_dev_attr(attr)->index;
> struct lm85_data *data = lm85_update_device(dev);
> - return sprintf(buf, "%d\n", INSEXT_FROM_REG(nr, data->in[nr],
> - data->in_ext[nr]));
> + int val = 0;
> +
> + if (nr < ARRAY_SIZE(data->in))
> + val = INSEXT_FROM_REG(nr, data->in[nr], data->in_ext[nr]);
> + return sprintf(buf, "%d\n", val);
> }
>
> static ssize_t in_min_show(struct device *dev, struct device_attribute *attr,
> @@ -854,18 +885,25 @@ static ssize_t in_min_show(struct device *dev, struct device_attribute *attr,
> {
> int nr = to_sensor_dev_attr(attr)->index;
> struct lm85_data *data = lm85_update_device(dev);
> - return sprintf(buf, "%d\n", INS_FROM_REG(nr, data->in_min[nr]));
> + int val = 0;
> +
> + if (nr < ARRAY_SIZE(data->in_min))
> + val = INS_FROM_REG(nr, data->in_min[nr]);
> + return sprintf(buf, "%d\n", val);
> }
>
> static ssize_t in_min_store(struct device *dev, struct device_attribute *attr,
> const char *buf, size_t count)
> {
> - int nr = to_sensor_dev_attr(attr)->index;
> + unsigned int nr = to_sensor_dev_attr(attr)->index;
> struct lm85_data *data = dev_get_drvdata(dev);
> struct i2c_client *client = data->client;
> long val;
> int err;
>
> + if (nr >= ARRAY_SIZE(data->in_min))
> + return -EINVAL;
> +
> err = kstrtol(buf, 10, &val);
> if (err)
> return err;
> @@ -880,20 +918,27 @@ static ssize_t in_min_store(struct device *dev, struct device_attribute *attr,
> static ssize_t in_max_show(struct device *dev, struct device_attribute *attr,
> char *buf)
> {
> - int nr = to_sensor_dev_attr(attr)->index;
> + unsigned int nr = to_sensor_dev_attr(attr)->index;
> struct lm85_data *data = lm85_update_device(dev);
> - return sprintf(buf, "%d\n", INS_FROM_REG(nr, data->in_max[nr]));
> + int val = 0;
> +
> + if (nr < ARRAY_SIZE(data->in_max))
> + val = INS_FROM_REG(nr, data->in_max[nr]);
> + return sprintf(buf, "%d\n", val);
> }
>
> static ssize_t in_max_store(struct device *dev, struct device_attribute *attr,
> const char *buf, size_t count)
> {
> - int nr = to_sensor_dev_attr(attr)->index;
> + unsigned int nr = to_sensor_dev_attr(attr)->index;
> struct lm85_data *data = dev_get_drvdata(dev);
> struct i2c_client *client = data->client;
> long val;
> int err;
>
> + if (nr >= ARRAY_SIZE(data->in_max))
> + return -EINVAL;
> +
> err = kstrtol(buf, 10, &val);
> if (err)
> return err;
> @@ -935,30 +980,40 @@ static SENSOR_DEVICE_ATTR_RW(in7_max, in_max, 7);
> static ssize_t temp_show(struct device *dev, struct device_attribute *attr,
> char *buf)
> {
> - int nr = to_sensor_dev_attr(attr)->index;
> + unsigned int nr = to_sensor_dev_attr(attr)->index;
> struct lm85_data *data = lm85_update_device(dev);
> - return sprintf(buf, "%d\n", TEMPEXT_FROM_REG(data->temp[nr],
> - data->temp_ext[nr]));
> + int val = 0;
> +
> + if (nr < ARRAY_SIZE(data->temp))
> + val = TEMPEXT_FROM_REG(data->temp[nr], data->temp_ext[nr]);
> + return sprintf(buf, "%d\n", val);
> }
>
> static ssize_t temp_min_show(struct device *dev,
> struct device_attribute *attr, char *buf)
> {
> - int nr = to_sensor_dev_attr(attr)->index;
> + unsigned int nr = to_sensor_dev_attr(attr)->index;
> struct lm85_data *data = lm85_update_device(dev);
> - return sprintf(buf, "%d\n", TEMP_FROM_REG(data->temp_min[nr]));
> + int val = 0;
> +
> + if (nr < ARRAY_SIZE(data->temp_min))
> + val = TEMP_FROM_REG(data->temp_min[nr]);
> + return sprintf(buf, "%d\n", val);
> }
>
> static ssize_t temp_min_store(struct device *dev,
> struct device_attribute *attr, const char *buf,
> size_t count)
> {
> - int nr = to_sensor_dev_attr(attr)->index;
> + unsigned int nr = to_sensor_dev_attr(attr)->index;
> struct lm85_data *data = dev_get_drvdata(dev);
> struct i2c_client *client = data->client;
> long val;
> int err;
>
> + if (nr >= ARRAY_SIZE(data->temp_min))
> + return -EINVAL;
> +
> err = kstrtol(buf, 10, &val);
> if (err)
> return err;
> @@ -976,21 +1031,28 @@ static ssize_t temp_min_store(struct device *dev,
> static ssize_t temp_max_show(struct device *dev,
> struct device_attribute *attr, char *buf)
> {
> - int nr = to_sensor_dev_attr(attr)->index;
> + unsigned int nr = to_sensor_dev_attr(attr)->index;
> struct lm85_data *data = lm85_update_device(dev);
> - return sprintf(buf, "%d\n", TEMP_FROM_REG(data->temp_max[nr]));
> + int val = 0;
> +
> + if (nr < ARRAY_SIZE(data->temp_max))
> + val = TEMP_FROM_REG(data->temp_max[nr]);
> + return sprintf(buf, "%d\n", val);
> }
>
> static ssize_t temp_max_store(struct device *dev,
> struct device_attribute *attr, const char *buf,
> size_t count)
> {
> - int nr = to_sensor_dev_attr(attr)->index;
> + unsigned int nr = to_sensor_dev_attr(attr)->index;
> struct lm85_data *data = dev_get_drvdata(dev);
> struct i2c_client *client = data->client;
> long val;
> int err;
>
> + if (nr >= ARRAY_SIZE(data->temp_max))
> + return -EINVAL;
> +
> err = kstrtol(buf, 10, &val);
> if (err)
> return err;
> @@ -1021,21 +1083,28 @@ static ssize_t pwm_auto_channels_show(struct device *dev,
> struct device_attribute *attr,
> char *buf)
> {
> - int nr = to_sensor_dev_attr(attr)->index;
> + unsigned int nr = to_sensor_dev_attr(attr)->index;
> struct lm85_data *data = lm85_update_device(dev);
> - return sprintf(buf, "%d\n", ZONE_FROM_REG(data->autofan[nr].config));
> + int val = 0;
> +
> + if (nr < ARRAY_SIZE(data->autofan))
> + val = ZONE_FROM_REG(data->autofan[nr].config);
> + return sprintf(buf, "%d\n", val);
> }
>
> static ssize_t pwm_auto_channels_store(struct device *dev,
> struct device_attribute *attr,
> const char *buf, size_t count)
> {
> - int nr = to_sensor_dev_attr(attr)->index;
> + unsigned int nr = to_sensor_dev_attr(attr)->index;
> struct lm85_data *data = dev_get_drvdata(dev);
> struct i2c_client *client = data->client;
> long val;
> int err;
>
> + if (nr >= ARRAY_SIZE(data->autofan))
> + return -EINVAL;
> +
> err = kstrtol(buf, 10, &val);
> if (err)
> return err;
> @@ -1052,21 +1121,28 @@ static ssize_t pwm_auto_channels_store(struct device *dev,
> static ssize_t pwm_auto_pwm_min_show(struct device *dev,
> struct device_attribute *attr, char *buf)
> {
> - int nr = to_sensor_dev_attr(attr)->index;
> + unsigned int nr = to_sensor_dev_attr(attr)->index;
> struct lm85_data *data = lm85_update_device(dev);
> - return sprintf(buf, "%d\n", PWM_FROM_REG(data->autofan[nr].min_pwm));
> + int val = 0;
> +
> + if (nr < ARRAY_SIZE(data->autofan))
> + val = PWM_FROM_REG(data->autofan[nr].min_pwm);
> + return sprintf(buf, "%d\n", val);
> }
>
> static ssize_t pwm_auto_pwm_min_store(struct device *dev,
> struct device_attribute *attr,
> const char *buf, size_t count)
> {
> - int nr = to_sensor_dev_attr(attr)->index;
> + unsigned int nr = to_sensor_dev_attr(attr)->index;
> struct lm85_data *data = dev_get_drvdata(dev);
> struct i2c_client *client = data->client;
> unsigned long val;
> int err;
>
> + if (nr >= ARRAY_SIZE(data->autofan))
> + return -EINVAL;
> +
> err = kstrtoul(buf, 10, &val);
> if (err)
> return err;
> @@ -1083,22 +1159,29 @@ static ssize_t pwm_auto_pwm_minctl_show(struct device *dev,
> struct device_attribute *attr,
> char *buf)
> {
> - int nr = to_sensor_dev_attr(attr)->index;
> + unsigned int nr = to_sensor_dev_attr(attr)->index;
> struct lm85_data *data = lm85_update_device(dev);
> - return sprintf(buf, "%d\n", data->autofan[nr].min_off);
> + int val = 0;
> +
> + if (nr < ARRAY_SIZE(data->autofan))
> + val = data->autofan[nr].min_off;
> + return sprintf(buf, "%d\n", val);
> }
>
> static ssize_t pwm_auto_pwm_minctl_store(struct device *dev,
> struct device_attribute *attr,
> const char *buf, size_t count)
> {
> - int nr = to_sensor_dev_attr(attr)->index;
> + unsigned int nr = to_sensor_dev_attr(attr)->index;
> struct lm85_data *data = dev_get_drvdata(dev);
> struct i2c_client *client = data->client;
> u8 tmp;
> long val;
> int err;
>
> + if (nr >= ARRAY_SIZE(data->autofan))
> + return -EINVAL;
> +
> err = kstrtol(buf, 10, &val);
> if (err)
> return err;
> @@ -1130,23 +1213,30 @@ static ssize_t temp_auto_temp_off_show(struct device *dev,
> struct device_attribute *attr,
> char *buf)
> {
> - int nr = to_sensor_dev_attr(attr)->index;
> + unsigned int nr = to_sensor_dev_attr(attr)->index;
> struct lm85_data *data = lm85_update_device(dev);
> - return sprintf(buf, "%d\n", TEMP_FROM_REG(data->zone[nr].limit) -
> - HYST_FROM_REG(data->zone[nr].hyst));
> + int val = 0;
> +
> + if (nr < ARRAY_SIZE(data->zone))
> + val = TEMP_FROM_REG(data->zone[nr].limit) -
> + HYST_FROM_REG(data->zone[nr].hyst);
> + return sprintf(buf, "%d\n", val);
> }
>
> static ssize_t temp_auto_temp_off_store(struct device *dev,
> struct device_attribute *attr,
> const char *buf, size_t count)
> {
> - int nr = to_sensor_dev_attr(attr)->index;
> + unsigned int nr = to_sensor_dev_attr(attr)->index;
> struct lm85_data *data = dev_get_drvdata(dev);
> struct i2c_client *client = data->client;
> int min;
> long val;
> int err;
>
> + if (nr >= ARRAY_SIZE(data->zone))
> + return -EINVAL;
> +
> err = kstrtol(buf, 10, &val);
> if (err)
> return err;
> @@ -1170,21 +1260,28 @@ static ssize_t temp_auto_temp_min_show(struct device *dev,
> struct device_attribute *attr,
> char *buf)
> {
> - int nr = to_sensor_dev_attr(attr)->index;
> + unsigned int nr = to_sensor_dev_attr(attr)->index;
> struct lm85_data *data = lm85_update_device(dev);
> - return sprintf(buf, "%d\n", TEMP_FROM_REG(data->zone[nr].limit));
> + int val = 0;
> +
> + if (nr < ARRAY_SIZE(data->zone))
> + val = TEMP_FROM_REG(data->zone[nr].limit);
> + return sprintf(buf, "%d\n", val);
> }
>
> static ssize_t temp_auto_temp_min_store(struct device *dev,
> struct device_attribute *attr,
> const char *buf, size_t count)
> {
> - int nr = to_sensor_dev_attr(attr)->index;
> + unsigned int nr = to_sensor_dev_attr(attr)->index;
> struct lm85_data *data = dev_get_drvdata(dev);
> struct i2c_client *client = data->client;
> long val;
> int err;
>
> + if (nr >= ARRAY_SIZE(data->zone))
> + return -EINVAL;
> +
> err = kstrtol(buf, 10, &val);
> if (err)
> return err;
> @@ -1194,7 +1291,7 @@ static ssize_t temp_auto_temp_min_store(struct device *dev,
> lm85_write_value(client, LM85_REG_AFAN_LIMIT(nr),
> data->zone[nr].limit);
>
> -/* Update temp_auto_max and temp_auto_range */
> + /* Update temp_auto_max and temp_auto_range */
> data->zone[nr].range = RANGE_TO_REG(
> TEMP_FROM_REG(data->zone[nr].max_desired) -
> TEMP_FROM_REG(data->zone[nr].limit));
> @@ -1210,23 +1307,30 @@ static ssize_t temp_auto_temp_max_show(struct device *dev,
> struct device_attribute *attr,
> char *buf)
> {
> - int nr = to_sensor_dev_attr(attr)->index;
> + unsigned int nr = to_sensor_dev_attr(attr)->index;
> struct lm85_data *data = lm85_update_device(dev);
> - return sprintf(buf, "%d\n", TEMP_FROM_REG(data->zone[nr].limit) +
> - RANGE_FROM_REG(data->zone[nr].range));
> + int val = 0;
> +
> + if (nr < ARRAY_SIZE(data->zone))
> + val = TEMP_FROM_REG(data->zone[nr].limit) +
> + RANGE_FROM_REG(data->zone[nr].range);
> + return sprintf(buf, "%d\n", val);
> }
>
> static ssize_t temp_auto_temp_max_store(struct device *dev,
> struct device_attribute *attr,
> const char *buf, size_t count)
> {
> - int nr = to_sensor_dev_attr(attr)->index;
> + unsigned int nr = to_sensor_dev_attr(attr)->index;
> struct lm85_data *data = dev_get_drvdata(dev);
> struct i2c_client *client = data->client;
> int min;
> long val;
> int err;
>
> + if (nr >= ARRAY_SIZE(data->zone))
> + return -EINVAL;
> +
> err = kstrtol(buf, 10, &val);
> if (err)
> return err;
> @@ -1247,21 +1351,28 @@ static ssize_t temp_auto_temp_crit_show(struct device *dev,
> struct device_attribute *attr,
> char *buf)
> {
> - int nr = to_sensor_dev_attr(attr)->index;
> + unsigned int nr = to_sensor_dev_attr(attr)->index;
> struct lm85_data *data = lm85_update_device(dev);
> - return sprintf(buf, "%d\n", TEMP_FROM_REG(data->zone[nr].critical));
> + int val = 0;
> +
> + if (nr < ARRAY_SIZE(data->zone))
> + val = TEMP_FROM_REG(data->zone[nr].critical);
> + return sprintf(buf, "%d\n", val);
> }
>
> static ssize_t temp_auto_temp_crit_store(struct device *dev,
> struct device_attribute *attr,
> const char *buf, size_t count)
> {
> - int nr = to_sensor_dev_attr(attr)->index;
> + unsigned int nr = to_sensor_dev_attr(attr)->index;
> struct lm85_data *data = dev_get_drvdata(dev);
> struct i2c_client *client = data->client;
> long val;
> int err;
>
> + if (nr >= ARRAY_SIZE(data->zone))
> + return -EINVAL;
> +
> err = kstrtol(buf, 10, &val);
> if (err)
> return err;
> diff --git a/include/linux/hwmon-sysfs.h b/include/linux/hwmon-sysfs.h
> index d896713359cd..f0505d10bfad 100644
> --- a/include/linux/hwmon-sysfs.h
> +++ b/include/linux/hwmon-sysfs.h
> @@ -10,7 +10,7 @@
> #include <linux/device.h>
> #include <linux/kstrtox.h>
>
> -struct sensor_device_attribute{
> +struct sensor_device_attribute {
> struct device_attribute dev_attr;
> int index;
> };
> --
> 2.34.1
>
