e5e9f44c causes buffer under-runs

DISCUSSION Dan Carpenter 3 participants 4 messages Mar 3, 2010

Subsystems (2)

Kernel janitors Hardware monitoring

lore

Share & tools

lore

Dan Carpenter

author

2010-03-03 07:09 UTC #0 lore raw

Smatch complains about: e5e9f44c2 i2c: Drop I2C_CLIENT_INSMOD_2 to 8

The original define had an "any_chip", but this patch removed it.

-#define I2C_CLIENT_INSMOD_8(chip1, chip2, chip3, chip4, chip5, chip6, chip7, chip8) \
-enum chips { any_chip, chip1, chip2, chip3, chip4, chip5, chip6, \
- chip7, chip8 }

This causes some buffer under runs.

drivers/hwmon/fschmd.c +1040 fschmd_detect(33) warn: array offset '(kind-1)' can be negative (-1) drivers/hwmon/tmp401.c +527 tmp401_detect(37) warn: array offset '(kind-1)' can be negative (-1) drivers/hwmon/tmp421.c +255 tmp421_detect(30) warn: array offset '(kind-1)' can be negative (-1) drivers/hwmon/tmp421.c +256 tmp421_detect(31) warn: array offset '(kind-1)' can be negative (-1)

(From a test version of smatch).

regards,
dan carpenter

Jean Delvare

2010-03-03 12:43 UTC #1 lore raw

Hi Dan,

Quoted 8 lines · Dan Carpenter

Smatch complains about: e5e9f44c2 i2c: Drop I2C_CLIENT_INSMOD_2 to 8
The original define had an "any_chip", but this patch removed it.
(quoted diff) +0 -3
-#define I2C_CLIENT_INSMOD_8(chip1, chip2, chip3, chip4, chip5, chip6, chip7, chip8) \
-enum chips { any_chip, chip1, chip2, chip3, chip4, chip5, chip6,       \
-            chip7, chip8 }

Quoted 8 lines

This causes some buffer under runs.
drivers/hwmon/fschmd.c +1040 fschmd_detect(33) warn: array offset '(kind-1)' can be negative (-1) drivers/hwmon/tmp401.c +527 tmp401_detect(37) warn: array offset '(kind-1)' can be negative (-1) drivers/hwmon/tmp421.c +255 tmp421_detect(30) warn: array offset '(kind-1)' can be negative (-1) drivers/hwmon/tmp421.c +256 tmp421_detect(31) warn: array offset '(kind-1)' can be negative (-1)
(From a test version of smatch).

Thanks a lot for reporting. I was well aware that these drivers would need special care when getting rid of the I2C_CLIENT_INSMOD_* macros, and I _thought_ I had taken care, I even seem to remember changing the code in question... but apparently I managed to lose the changes somewhere in the process. Sigh.

The following patch should fix it. Please apply it, run smatch again and confirm the warnings are gone.

Hans, please confirm that I got the fschmd driver right. Andre, please confirm that I got the tmp421 driver right (I decided to change the device data to be explicitly the channel count, otherwise the code was simply too confusing.) As for the tmp401 driver, it is simple enough that I am confident I got it right.

* * * * *

From: Jean Delvare <[email protected]>
Subject: hwmon: Fix off-by-one kind values

Recent changes on the I2C front have left off-by-one array indexes in 3 hwmon drivers. Fix them.

Reported-by Dan Carpenter <[email protected]>

Signed-off-by Jean Delvare <[email protected]>

Cc Hans de Goede <[email protected]>

Cc Andre Prendel <[email protected]>

---
 drivers/hwmon/fschmd.c |   15 ++++++---------
 drivers/hwmon/tmp401.c |    7 +++----
 drivers/hwmon/tmp421.c |   18 +++++++++---------
 3 files changed, 18 insertions(+), 22 deletions(-)

drivers/hwmon/fschmd.c +6 -9

@@ -267,7 +267,7 @@ struct fschmd_data {
 	struct list_head list; /* member of the watchdog_data_list */
 	struct kref kref;
 	struct miscdevice watchdog_miscdev;
-	int kind;
+	enum chips kind;
 	unsigned long watchdog_is_open;
 	char watchdog_expect_close;
 	char watchdog_name[10]; /* must be unique to avoid sysfs conflict */

@@ -325,8 +325,7 @@ static ssize_t show_in_value(struct devi
 	int index = to_sensor_dev_attr(devattr)->index;
 	struct fschmd_data *data = fschmd_update_device(dev);
 
-	/* fscher / fschrc - 1 as data->kind is an array index, not a chips */
-	if (data->kind = (fscher - 1) || data->kind >= (fschrc - 1))
+	if (data->kind = fscher || data->kind >= fschrc)
 		return sprintf(buf, "%d\n", (data->volt[index] * dmi_vref *
 			dmi_mult[index]) / 255 + dmi_offset[index]);
 	else

@@ -492,7 +491,7 @@ static ssize_t show_pwm_auto_point1_pwm(
 	int val = data->fan_min[index];
 
 	/* 0 = allow turning off (except on the syl), 1-255 = 50-100% */
-	if (val || data->kind = fscsyl - 1)
+	if (val || data->kind = fscsyl)
 		val = val / 2 + 128;
 
 	return sprintf(buf, "%d\n", val);

@@ -506,7 +505,7 @@ static ssize_t store_pwm_auto_point1_pwm
 	unsigned long v = simple_strtoul(buf, NULL, 10);
 
 	/* reg: 0 = allow turning off (except on the syl), 1-255 = 50-100% */
-	if (v || data->kind = fscsyl - 1) {
+	if (v || data->kind = fscsyl) {
 		v = SENSORS_LIMIT(v, 128, 255);
 		v = (v - 128) * 2 + 1;
 	}

@@ -1037,7 +1036,7 @@ static int fschmd_detect(struct i2c_clie
 	else
 		return -ENODEV;
 
-	strlcpy(info->type, fschmd_id[kind - 1].name, I2C_NAME_SIZE);
+	strlcpy(info->type, fschmd_id[kind].name, I2C_NAME_SIZE);
 
 	return 0;
 }

@@ -1065,6 +1064,7 @@ static int fschmd_probe(struct i2c_clien
 	   (where the client is found through a data ptr instead of the
 	   otherway around) */
 	data->client = client;
+	data->kind = kind;
 
 	if (kind = fscpos) {
 		/* The Poseidon has hardwired temp limits, fill these

@@ -1085,9 +1085,6 @@ static int fschmd_probe(struct i2c_clien
 		}
 	}
 
-	/* i2c kind goes from 1-6, we want from 0-5 to address arrays */
-	data->kind = kind - 1;
-
 	/* Read in some never changing registers */
 	data->revision = i2c_smbus_read_byte_data(client, FSCHMD_REG_REVISION);
 	data->global_control = i2c_smbus_read_byte_data(client,

drivers/hwmon/tmp401.c +3 -4

@@ -134,7 +134,7 @@ struct tmp401_data {
 	struct mutex update_lock;
 	char valid; /* zero until following fields are valid */
 	unsigned long last_updated; /* in jiffies */
-	int kind;
+	enum chips kind;
 
 	/* register values */
 	u8 status;

@@ -524,7 +524,7 @@ static int tmp401_detect(struct i2c_clie
 	if (reg > 15)
 		return -ENODEV;
 
-	strlcpy(info->type, tmp401_id[kind - 1].name, I2C_NAME_SIZE);
+	strlcpy(info->type, tmp401_id[kind].name, I2C_NAME_SIZE);
 
 	return 0;
 }

@@ -572,8 +572,7 @@ static int tmp401_probe(struct i2c_clien
 		goto exit_remove;
 	}
 
-	dev_info(&client->dev, "Detected TI %s chip\n",
-		 names[data->kind - 1]);
+	dev_info(&client->dev, "Detected TI %s chip\n", names[data->kind]);
 
 	return 0;

drivers/hwmon/tmp421.c +9 -9

@@ -61,9 +61,9 @@ static const u8 TMP421_TEMP_LSB[4]		= {
 #define TMP423_DEVICE_ID			0x23
 
 static const struct i2c_device_id tmp421_id[] = {
-	{ "tmp421", tmp421 },
-	{ "tmp422", tmp422 },
-	{ "tmp423", tmp423 },
+	{ "tmp421", 2 },
+	{ "tmp422", 3 },
+	{ "tmp423", 4 },
 	{ }
 };
 MODULE_DEVICE_TABLE(i2c, tmp421_id);

@@ -73,7 +73,7 @@ struct tmp421_data {
 	struct mutex update_lock;
 	char valid;
 	unsigned long last_updated;
-	int kind;
+	int channels;
 	u8 config;
 	s16 temp[4];
 };

@@ -107,7 +107,7 @@ static struct tmp421_data *tmp421_update
 		data->config = i2c_smbus_read_byte_data(client,
 			TMP421_CONFIG_REG_1);
 
-		for (i = 0; i <= data->kind; i++) {
+		for (i = 0; i < data->channels; i++) {
 			data->temp[i] = i2c_smbus_read_byte_data(client,
 				TMP421_TEMP_MSB[i]) << 8;
 			data->temp[i] |= i2c_smbus_read_byte_data(client,

@@ -166,7 +166,7 @@ static mode_t tmp421_is_visible(struct k
 	devattr = container_of(a, struct device_attribute, attr);
 	index = to_sensor_dev_attr(devattr)->index;
 
-	if (data->kind > index)
+	if (index < data->channels)
 		return a->mode;
 
 	return 0;

@@ -252,9 +252,9 @@ static int tmp421_detect(struct i2c_clie
 		return -ENODEV;
 	}
 
-	strlcpy(info->type, tmp421_id[kind - 1].name, I2C_NAME_SIZE);
+	strlcpy(info->type, tmp421_id[kind].name, I2C_NAME_SIZE);
 	dev_info(&adapter->dev, "Detected TI %s chip at 0x%02x\n",
-		 names[kind - 1], client->addr);
+		 names[kind], client->addr);
 
 	return 0;
 }

@@ -271,7 +271,7 @@ static int tmp421_probe(struct i2c_clien
 
 	i2c_set_clientdata(client, data);
 	mutex_init(&data->update_lock);
-	data->kind = id->driver_data;
+	data->channels = id->driver_data;
 
 	err = tmp421_init_client(client);
 	if (err)

Signature · 1 line

Jean Delvare

Dan Carpenter

author

2010-03-03 16:59 UTC #2 lore raw

In reply to Dan Carpenter

Quoted 26 lines · Jean Delvare

Hi Dan,
Quoted 8 lines · Dan Carpenter
Smatch complains about: e5e9f44c2 i2c: Drop I2C_CLIENT_INSMOD_2 to 8
The original define had an "any_chip", but this patch removed it.
(quoted diff) +0 -3
-#define I2C_CLIENT_INSMOD_8(chip1, chip2, chip3, chip4, chip5, chip6, chip7, chip8) \
-enum chips { any_chip, chip1, chip2, chip3, chip4, chip5, chip6,       \
-            chip7, chip8 }
Quoted 8 lines
This causes some buffer under runs.
drivers/hwmon/fschmd.c +1040 fschmd_detect(33) warn: array offset '(kind-1)' can be negative (-1) drivers/hwmon/tmp401.c +527 tmp401_detect(37) warn: array offset '(kind-1)' can be negative (-1) drivers/hwmon/tmp421.c +255 tmp421_detect(30) warn: array offset '(kind-1)' can be negative (-1) drivers/hwmon/tmp421.c +256 tmp421_detect(31) warn: array offset '(kind-1)' can be negative (-1)
(From a test version of smatch).
Thanks a lot for reporting. I was well aware that these drivers would need special care when getting rid of the I2C_CLIENT_INSMOD_* macros, and I _thought_ I had taken care, I even seem to remember changing the code in question... but apparently I managed to lose the changes somewhere in the process. Sigh.

Quoted 3 lines

The following patch should fix it. Please apply it, run smatch again and confirm the warnings are gone.

Looks good. You cleaned up the code nicely.

regards,
dan carpenter

Andre Prendel

2010-03-03 20:59 UTC #3 lore raw

In reply to Dan Carpenter

On Wed, Mar 03, 2010 at 01:43:56PM +0100, Jean Delvare wrote:
Hi,

Quoted 26 lines

Hi Dan,
Quoted 8 lines · Dan Carpenter
Smatch complains about: e5e9f44c2 i2c: Drop I2C_CLIENT_INSMOD_2 to 8
The original define had an "any_chip", but this patch removed it.
(quoted diff) +0 -3
-#define I2C_CLIENT_INSMOD_8(chip1, chip2, chip3, chip4, chip5, chip6, chip7, chip8) \
-enum chips { any_chip, chip1, chip2, chip3, chip4, chip5, chip6,       \
-            chip7, chip8 }
Quoted 8 lines
This causes some buffer under runs.
drivers/hwmon/fschmd.c +1040 fschmd_detect(33) warn: array offset '(kind-1)' can be negative (-1) drivers/hwmon/tmp401.c +527 tmp401_detect(37) warn: array offset '(kind-1)' can be negative (-1) drivers/hwmon/tmp421.c +255 tmp421_detect(30) warn: array offset '(kind-1)' can be negative (-1) drivers/hwmon/tmp421.c +256 tmp421_detect(31) warn: array offset '(kind-1)' can be negative (-1)
(From a test version of smatch).
Thanks a lot for reporting. I was well aware that these drivers would need special care when getting rid of the I2C_CLIENT_INSMOD_* macros, and I _thought_ I had taken care, I even seem to remember changing the code in question... but apparently I managed to lose the changes somewhere in the process. Sigh.

Quoted 8 lines

The following patch should fix it. Please apply it, run smatch again and confirm the warnings are gone.
Hans, please confirm that I got the fschmd driver right. Andre, please confirm that I got the tmp421 driver right (I decided to change the device data to be explicitly the channel count, otherwise the code was simply too confusing.) As for the tmp401 driver, it is simple enough that I am confident I got it right.

The patch for tmp401 looks good. As you said it's the simple one. The changes for tmp421 looks good too but I want to investigate it in detail and give it a test. Unfortunatly I don't have a recent kernel so I'm building one right now. Tomorrow you will get the result of my tests.

Regards,
Andre

Quoted 186 lines