* Re: e5e9f44c causes buffer under-runs
2010-03-03 7:09 e5e9f44c causes buffer under-runs Dan Carpenter
@ 2010-03-03 12:43 ` Jean Delvare
2010-03-03 16:59 ` Dan Carpenter
2010-03-03 20:59 ` [lm-sensors] " Andre Prendel
2 siblings, 0 replies; 4+ messages in thread
From: Jean Delvare @ 2010-03-03 12:43 UTC (permalink / raw)
To: kernel-janitors
Hi Dan,
On Wed, 3 Mar 2010 10:09:14 +0300, Dan Carpenter wrote:
> 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).
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 <khali@linux-fr.org>
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 <error27@gmail.com>
Signed-off-by: Jean Delvare <khali@linux-fr.org>
Cc: Hans de Goede <hdegoede@redhat.com>
Cc: Andre Prendel <andre.prendel@gmx.de>
---
drivers/hwmon/fschmd.c | 15 ++++++---------
drivers/hwmon/tmp401.c | 7 +++----
drivers/hwmon/tmp421.c | 18 +++++++++---------
3 files changed, 18 insertions(+), 22 deletions(-)
--- linux-2.6.34-rc0.orig/drivers/hwmon/fschmd.c 2010-03-03 11:22:50.000000000 +0100
+++ linux-2.6.34-rc0/drivers/hwmon/fschmd.c 2010-03-03 12:15:07.000000000 +0100
@@ -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,
--- linux-2.6.34-rc0.orig/drivers/hwmon/tmp401.c 2010-02-25 09:12:22.000000000 +0100
+++ linux-2.6.34-rc0/drivers/hwmon/tmp401.c 2010-03-03 11:40:35.000000000 +0100
@@ -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;
--- linux-2.6.34-rc0.orig/drivers/hwmon/tmp421.c 2010-02-25 09:12:22.000000000 +0100
+++ linux-2.6.34-rc0/drivers/hwmon/tmp421.c 2010-03-03 12:56:05.000000000 +0100
@@ -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)
--
Jean Delvare
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: e5e9f44c causes buffer under-runs
2010-03-03 7:09 e5e9f44c causes buffer under-runs Dan Carpenter
2010-03-03 12:43 ` Jean Delvare
@ 2010-03-03 16:59 ` Dan Carpenter
2010-03-03 20:59 ` [lm-sensors] " Andre Prendel
2 siblings, 0 replies; 4+ messages in thread
From: Dan Carpenter @ 2010-03-03 16:59 UTC (permalink / raw)
To: kernel-janitors
On Wed, Mar 03, 2010 at 01:43:56PM +0100, Jean Delvare wrote:
> Hi Dan,
>
> On Wed, 3 Mar 2010 10:09:14 +0300, Dan Carpenter wrote:
> > 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).
>
> 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.
>
Looks good. You cleaned up the code nicely.
regards,
dan carpenter
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [lm-sensors] e5e9f44c causes buffer under-runs
2010-03-03 7:09 e5e9f44c causes buffer under-runs Dan Carpenter
2010-03-03 12:43 ` Jean Delvare
2010-03-03 16:59 ` Dan Carpenter
@ 2010-03-03 20:59 ` Andre Prendel
2 siblings, 0 replies; 4+ messages in thread
From: Andre Prendel @ 2010-03-03 20:59 UTC (permalink / raw)
To: kernel-janitors
On Wed, Mar 03, 2010 at 01:43:56PM +0100, Jean Delvare wrote:
Hi,
> Hi Dan,
>
> On Wed, 3 Mar 2010 10:09:14 +0300, Dan Carpenter wrote:
> > 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).
>
> 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.
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
> * * * * *
>
> From: Jean Delvare <khali@linux-fr.org>
> 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 <error27@gmail.com>
> Signed-off-by: Jean Delvare <khali@linux-fr.org>
> Cc: Hans de Goede <hdegoede@redhat.com>
> Cc: Andre Prendel <andre.prendel@gmx.de>
> ---
> drivers/hwmon/fschmd.c | 15 ++++++---------
> drivers/hwmon/tmp401.c | 7 +++----
> drivers/hwmon/tmp421.c | 18 +++++++++---------
> 3 files changed, 18 insertions(+), 22 deletions(-)
>
> --- linux-2.6.34-rc0.orig/drivers/hwmon/fschmd.c 2010-03-03 11:22:50.000000000 +0100
> +++ linux-2.6.34-rc0/drivers/hwmon/fschmd.c 2010-03-03 12:15:07.000000000 +0100
> @@ -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,
> --- linux-2.6.34-rc0.orig/drivers/hwmon/tmp401.c 2010-02-25 09:12:22.000000000 +0100
> +++ linux-2.6.34-rc0/drivers/hwmon/tmp401.c 2010-03-03 11:40:35.000000000 +0100
> @@ -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;
>
> --- linux-2.6.34-rc0.orig/drivers/hwmon/tmp421.c 2010-02-25 09:12:22.000000000 +0100
> +++ linux-2.6.34-rc0/drivers/hwmon/tmp421.c 2010-03-03 12:56:05.000000000 +0100
> @@ -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)
>
>
> --
> Jean Delvare
>
> _______________________________________________
> lm-sensors mailing list
> lm-sensors@lm-sensors.org
> http://lists.lm-sensors.org/mailman/listinfo/lm-sensors
^ permalink raw reply [flat|nested] 4+ messages in thread