From: Andre Prendel <andre.prendel@gmx.de>
To: kernel-janitors@vger.kernel.org
Subject: Re: [lm-sensors] e5e9f44c causes buffer under-runs
Date: Wed, 03 Mar 2010 20:59:14 +0000 [thread overview]
Message-ID: <20100303205913.GA4995@andre-laptop> (raw)
In-Reply-To: <20100303070914.GE5086@bicker>
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
WARNING: multiple messages have this Message-ID (diff)
From: Andre Prendel <andre.prendel@gmx.de>
To: kernel-janitors@vger.kernel.org
Subject: Re: [lm-sensors] e5e9f44c causes buffer under-runs
Date: Wed, 03 Mar 2010 20:59:14 +0000 [thread overview]
Message-ID: <20100303205913.GA4995@andre-laptop> (raw)
In-Reply-To: <20100303070914.GE5086@bicker>
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
_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors
next prev parent reply other threads:[~2010-03-03 20:59 UTC|newest]
Thread overview: 8+ messages / expand[flat|nested] mbox.gz Atom feed top
2010-03-03 7:09 e5e9f44c causes buffer under-runs Dan Carpenter
2010-03-03 7:09 ` [lm-sensors] " Dan Carpenter
2010-03-03 12:43 ` Jean Delvare
2010-03-03 12:43 ` [lm-sensors] " Jean Delvare
2010-03-03 16:59 ` Dan Carpenter
2010-03-03 16:59 ` [lm-sensors] " Dan Carpenter
2010-03-03 20:59 ` Andre Prendel [this message]
2010-03-03 20:59 ` Andre Prendel
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20100303205913.GA4995@andre-laptop \
--to=andre.prendel@gmx.de \
--cc=kernel-janitors@vger.kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.