[PATCH v2] lm85: Bounds check to_sensor_dev_attr()->index usage

From: Kees Cook
Date: Fri Feb 03 2023 - 17:33:09 EST

Next message: Ron Economos: "Re: [PATCH 6.1 00/28] 6.1.10-rc1 review"
Previous message: Yosry Ahmed: "Re: [RFC PATCH v1 1/2] mm: vmscan: refactor updating reclaimed pages in reclaim_state"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

The index into various register arrays was not bounds checked. Provide a
simple wrapper to bounds check the index, adding robustness in the face
of memory corruption, unexpected index manipulation, etc. Noticed under
GCC 13 with -Warray-bounds:

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];
| ^~~~~~~

Additionally fix a comment indentation and a coding style issue of
a missing space before the { in the struct sensor_device_attribute
definition.

Cc: Jean Delvare <jdelvare@xxxxxxxx>
Cc: Guenter Roeck <linux@xxxxxxxxxxxx>
Cc: linux-hwmon@xxxxxxxxxxxxxxx
Signed-off-by: Kees Cook <keescook@xxxxxxxxxxxx>
---
v2: create a common helper for bounds checking
v1: https://lore.kernel.org/linux-hardening/20230127223744.never.113-kees@xxxxxxxxxx/
---
drivers/hwmon/lm85.c | 77 +++++++++++++++++++++----------------
include/linux/hwmon-sysfs.h | 2 +-
2 files changed, 44 insertions(+), 35 deletions(-)

diff --git a/drivers/hwmon/lm85.c b/drivers/hwmon/lm85.c
index 8d33c2484755..3cb04e4138b8 100644
--- a/drivers/hwmon/lm85.c
+++ b/drivers/hwmon/lm85.c
@@ -318,6 +318,15 @@ struct lm85_data {
struct lm85_zone zone[3];
};

+#define get_sensor_index(attr, array) \
+({ \
+ unsigned int _nr = to_sensor_dev_attr(attr)->index; \
+ \
+ if (WARN_ON_ONCE(_nr >= ARRAY_SIZE(array))) \
+ _nr = 0; \
+ _nr; \
+})
+
static int lm85_read_value(struct i2c_client *client, u8 reg)
{
int res;
@@ -552,16 +561,16 @@ 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;
struct lm85_data *data = lm85_update_device(dev);
+ unsigned int nr = get_sensor_index(attr, data->fan);
return sprintf(buf, "%d\n", FAN_FROM_REG(data->fan[nr]));
}

static ssize_t fan_min_show(struct device *dev, struct device_attribute *attr,
char *buf)
{
- int nr = to_sensor_dev_attr(attr)->index;
struct lm85_data *data = lm85_update_device(dev);
+ unsigned int nr = get_sensor_index(attr, data->fan_min);
return sprintf(buf, "%d\n", FAN_FROM_REG(data->fan_min[nr]));
}

@@ -569,8 +578,8 @@ 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;
struct lm85_data *data = dev_get_drvdata(dev);
+ unsigned int nr = get_sensor_index(attr, data->fan_min);
struct i2c_client *client = data->client;
unsigned long val;
int err;
@@ -683,16 +692,16 @@ 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;
struct lm85_data *data = lm85_update_device(dev);
+ unsigned int nr = get_sensor_index(attr, data->pwm);
return sprintf(buf, "%d\n", PWM_FROM_REG(data->pwm[nr]));
}

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;
struct lm85_data *data = dev_get_drvdata(dev);
+ unsigned int nr = get_sensor_index(attr, data->pwm);
struct i2c_client *client = data->client;
unsigned long val;
int err;
@@ -711,8 +720,8 @@ 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;
struct lm85_data *data = lm85_update_device(dev);
+ unsigned int nr = get_sensor_index(attr, data->autofan);
int pwm_zone, enable;

pwm_zone = ZONE_FROM_REG(data->autofan[nr].config);
@@ -734,8 +743,8 @@ 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;
struct lm85_data *data = dev_get_drvdata(dev);
+ unsigned int nr = get_sensor_index(attr, data->autofan);
struct i2c_client *client = data->client;
u8 config;
unsigned long val;
@@ -777,8 +786,8 @@ 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;
struct lm85_data *data = lm85_update_device(dev);
+ unsigned int nr = get_sensor_index(attr, data->pwm_freq);
int freq;

if (IS_ADT7468_HFPWM(data))
@@ -794,8 +803,8 @@ 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;
struct lm85_data *data = dev_get_drvdata(dev);
+ unsigned int nr = get_sensor_index(attr, data->pwm_freq);
struct i2c_client *client = data->client;
unsigned long val;
int err;
@@ -843,8 +852,8 @@ 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;
struct lm85_data *data = lm85_update_device(dev);
+ unsigned int nr = get_sensor_index(attr, data->in);
return sprintf(buf, "%d\n", INSEXT_FROM_REG(nr, data->in[nr],
data->in_ext[nr]));
}
@@ -852,16 +861,16 @@ static ssize_t in_show(struct device *dev, struct device_attribute *attr,
static ssize_t in_min_show(struct device *dev, struct device_attribute *attr,
char *buf)
{
- int nr = to_sensor_dev_attr(attr)->index;
struct lm85_data *data = lm85_update_device(dev);
+ unsigned int nr = get_sensor_index(attr, data->in);
return sprintf(buf, "%d\n", INS_FROM_REG(nr, data->in_min[nr]));
}

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;
struct lm85_data *data = dev_get_drvdata(dev);
+ unsigned int nr = get_sensor_index(attr, data->in_min);
struct i2c_client *client = data->client;
long val;
int err;
@@ -880,16 +889,16 @@ 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;
struct lm85_data *data = lm85_update_device(dev);
+ unsigned int nr = get_sensor_index(attr, data->in_max);
return sprintf(buf, "%d\n", INS_FROM_REG(nr, data->in_max[nr]));
}

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;
struct lm85_data *data = dev_get_drvdata(dev);
+ unsigned int nr = get_sensor_index(attr, data->in_max);
struct i2c_client *client = data->client;
long val;
int err;
@@ -935,8 +944,8 @@ 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;
struct lm85_data *data = lm85_update_device(dev);
+ unsigned int nr = get_sensor_index(attr, data->temp);
return sprintf(buf, "%d\n", TEMPEXT_FROM_REG(data->temp[nr],
data->temp_ext[nr]));
}
@@ -944,8 +953,8 @@ static ssize_t temp_show(struct device *dev, struct device_attribute *attr,
static ssize_t temp_min_show(struct device *dev,
struct device_attribute *attr, char *buf)
{
- int nr = to_sensor_dev_attr(attr)->index;
struct lm85_data *data = lm85_update_device(dev);
+ unsigned int nr = get_sensor_index(attr, data->temp_min);
return sprintf(buf, "%d\n", TEMP_FROM_REG(data->temp_min[nr]));
}

@@ -953,8 +962,8 @@ 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;
struct lm85_data *data = dev_get_drvdata(dev);
+ unsigned int nr = get_sensor_index(attr, data->temp_min);
struct i2c_client *client = data->client;
long val;
int err;
@@ -976,8 +985,8 @@ 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;
struct lm85_data *data = lm85_update_device(dev);
+ unsigned int nr = get_sensor_index(attr, data->temp_max);
return sprintf(buf, "%d\n", TEMP_FROM_REG(data->temp_max[nr]));
}

@@ -985,8 +994,8 @@ 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;
struct lm85_data *data = dev_get_drvdata(dev);
+ unsigned int nr = get_sensor_index(attr, data->temp_max);
struct i2c_client *client = data->client;
long val;
int err;
@@ -1021,8 +1030,8 @@ static ssize_t pwm_auto_channels_show(struct device *dev,
struct device_attribute *attr,
char *buf)
{
- int nr = to_sensor_dev_attr(attr)->index;
struct lm85_data *data = lm85_update_device(dev);
+ unsigned int nr = get_sensor_index(attr, data->autofan);
return sprintf(buf, "%d\n", ZONE_FROM_REG(data->autofan[nr].config));
}

@@ -1030,8 +1039,8 @@ 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;
struct lm85_data *data = dev_get_drvdata(dev);
+ unsigned int nr = get_sensor_index(attr, data->autofan);
struct i2c_client *client = data->client;
long val;
int err;
@@ -1052,8 +1061,8 @@ 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;
struct lm85_data *data = lm85_update_device(dev);
+ unsigned int nr = get_sensor_index(attr, data->autofan);
return sprintf(buf, "%d\n", PWM_FROM_REG(data->autofan[nr].min_pwm));
}

@@ -1061,8 +1070,8 @@ 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;
struct lm85_data *data = dev_get_drvdata(dev);
+ unsigned int nr = get_sensor_index(attr, data->autofan);
struct i2c_client *client = data->client;
unsigned long val;
int err;
@@ -1083,8 +1092,8 @@ 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;
struct lm85_data *data = lm85_update_device(dev);
+ unsigned int nr = get_sensor_index(attr, data->autofan);
return sprintf(buf, "%d\n", data->autofan[nr].min_off);
}

@@ -1092,8 +1101,8 @@ 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;
struct lm85_data *data = dev_get_drvdata(dev);
+ unsigned int nr = get_sensor_index(attr, data->autofan);
struct i2c_client *client = data->client;
u8 tmp;
long val;
@@ -1130,8 +1139,8 @@ 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;
struct lm85_data *data = lm85_update_device(dev);
+ unsigned int nr = get_sensor_index(attr, data->zone);
return sprintf(buf, "%d\n", TEMP_FROM_REG(data->zone[nr].limit) -
HYST_FROM_REG(data->zone[nr].hyst));
}
@@ -1140,8 +1149,8 @@ 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;
struct lm85_data *data = dev_get_drvdata(dev);
+ unsigned int nr = get_sensor_index(attr, data->zone);
struct i2c_client *client = data->client;
int min;
long val;
@@ -1170,8 +1179,8 @@ 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;
struct lm85_data *data = lm85_update_device(dev);
+ unsigned int nr = get_sensor_index(attr, data->zone);
return sprintf(buf, "%d\n", TEMP_FROM_REG(data->zone[nr].limit));
}

@@ -1179,8 +1188,8 @@ 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;
struct lm85_data *data = dev_get_drvdata(dev);
+ unsigned int nr = get_sensor_index(attr, data->zone);
struct i2c_client *client = data->client;
long val;
int err;
@@ -1194,7 +1203,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,8 +1219,8 @@ 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;
struct lm85_data *data = lm85_update_device(dev);
+ unsigned int nr = get_sensor_index(attr, data->zone);
return sprintf(buf, "%d\n", TEMP_FROM_REG(data->zone[nr].limit) +
RANGE_FROM_REG(data->zone[nr].range));
}
@@ -1220,8 +1229,8 @@ 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;
struct lm85_data *data = dev_get_drvdata(dev);
+ unsigned int nr = get_sensor_index(attr, data->zone);
struct i2c_client *client = data->client;
int min;
long val;
@@ -1247,8 +1256,8 @@ 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;
struct lm85_data *data = lm85_update_device(dev);
+ unsigned int nr = get_sensor_index(attr, data->zone);
return sprintf(buf, "%d\n", TEMP_FROM_REG(data->zone[nr].critical));
}

@@ -1256,8 +1265,8 @@ 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;
struct lm85_data *data = dev_get_drvdata(dev);
+ unsigned int nr = get_sensor_index(attr, data->zone);
struct i2c_client *client = data->client;
long val;
int 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

Next message: Ron Economos: "Re: [PATCH 6.1 00/28] 6.1.10-rc1 review"
Previous message: Yosry Ahmed: "Re: [RFC PATCH v1 1/2] mm: vmscan: refactor updating reclaimed pages in reclaim_state"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]