Messages in this thread Patch in this message | | | Date | Fri, 28 Mar 2008 07:26:26 -0500 | From | David Fries <> | Subject | [PATCH 16/35] W1: w1_therm fix user buffer overflow and cat |
| |
This switches w1_therm from bin_attribute to device_attribute which fixes a buffer overflow and some bad behavior.
slaves/w1_therm.c 1.8 Switching the sysfs read from bin_attribute to device_attribute. The data is far under PAGE_SIZE so the binary interface isn't required. As the device_attribute interface will make one call to w1_therm_read per file open and buffer, the result is, the following problems go away.
buffer overflow: Execute a short read on w1_slave and w1_therm_read_bin would still return the full string size worth of data clobbering the user space buffer when it returned. Switching to device_attribute avoids the buffer overflow problems. With the snprintf formatted output dealing with short reads without doing a conversion per read would have been difficult. bad behavior: `cat w1_slave` would cause two temperature conversions to take place. Previously the code assumed W1_SLAVE_DATA_SIZE would be returned with each read. It would not return 0 unless the offset was less than W1_SLAVE_DATA_SIZE. The result was the first read did a temperature conversion, filled the buffer and returned, the offset in the second read would be less than W1_SLAVE_DATA_SIZE and also fill the buffer and return, the third read would finnally have a big enough offset to return 0 and cause cat to stop. Now w1_therm_read will be called at most once per open.
w1.h 1.8 w1_therm is no longer using the bin_attribute so the W1_SLAVE_DATA_SIZE is no longer being used.
Signed-off-by: David Fries <david@fries.net> --- drivers/w1/slaves/w1_therm.c | 53 ++++++++++++++--------------------------- drivers/w1/w1.h | 1 - 2 files changed, 18 insertions(+), 36 deletions(-)
diff --git a/drivers/w1/slaves/w1_therm.c b/drivers/w1/slaves/w1_therm.c index dd26db2..ca47421 100644 --- a/drivers/w1/slaves/w1_therm.c +++ b/drivers/w1/slaves/w1_therm.c @@ -42,26 +42,20 @@ static u8 bad_roms[][9] = { {} }; -static ssize_t w1_therm_read_bin(struct kobject *, struct bin_attribute *, - char *, loff_t, size_t); +static ssize_t w1_therm_read(struct device *device, + struct device_attribute *attr, char *buf); -static struct bin_attribute w1_therm_bin_attr = { - .attr = { - .name = "w1_slave", - .mode = S_IRUGO, - }, - .size = W1_SLAVE_DATA_SIZE, - .read = w1_therm_read_bin, -}; +static struct device_attribute w1_therm_attr = + __ATTR(w1_slave, S_IRUGO, w1_therm_read, NULL); static int w1_therm_add_slave(struct w1_slave *sl) { - return sysfs_create_bin_file(&sl->dev.kobj, &w1_therm_bin_attr); + return device_create_file(&sl->dev, &w1_therm_attr); } static void w1_therm_remove_slave(struct w1_slave *sl) { - sysfs_remove_bin_file(&sl->dev.kobj, &w1_therm_bin_attr); + device_remove_file(&sl->dev, &w1_therm_attr); } static struct w1_family_ops w1_therm_fops = { @@ -160,30 +154,19 @@ static int w1_therm_check_rom(u8 rom[9]) return 0; } -static ssize_t w1_therm_read_bin(struct kobject *kobj, - struct bin_attribute *bin_attr, - char *buf, loff_t off, size_t count) +static ssize_t w1_therm_read(struct device *device, + struct device_attribute *attr, char *buf) { - struct w1_slave *sl = kobj_to_w1_slave(kobj); + struct w1_slave *sl = dev_to_w1_slave(device); struct w1_master *dev = sl->master; u8 rom[9], crc, verdict; int i, max_trying = 10; + ssize_t c=PAGE_SIZE; mutex_lock(&sl->master->mutex); - if (off > W1_SLAVE_DATA_SIZE) { - count = 0; - goto out; - } - if (off + count > W1_SLAVE_DATA_SIZE) { - count = 0; - goto out; - } - - memset(buf, 0, count); memset(rom, 0, sizeof(rom)); - count = 0; verdict = 0; crc = 0; @@ -200,7 +183,7 @@ static ssize_t w1_therm_read_bin(struct kobject *kobj, w1_write_8(dev, W1_READ_SCRATCHPAD); if ((count = w1_read_block(dev, rom, 9)) != 9) { - dev_warn(&dev->dev, "w1_read_block() returned %d instead of 9.\n", count); + dev_warn(device, "w1_read_block() returned %u instead of 9.\n", count); } crc = w1_calc_crc8(rom, 8); @@ -215,22 +198,22 @@ static ssize_t w1_therm_read_bin(struct kobject *kobj, } for (i = 0; i < 9; ++i) - count += sprintf(buf + count, "%02x ", rom[i]); - count += sprintf(buf + count, ": crc=%02x %s\n", + c -= snprintf(buf + PAGE_SIZE - c, c, "%02x ", rom[i]); + c -= snprintf(buf + PAGE_SIZE - c, c, ": crc=%02x %s\n", crc, (verdict) ? "YES" : "NO"); if (verdict) memcpy(sl->rom, rom, sizeof(sl->rom)); else - dev_warn(&dev->dev, "18S20 doesn't respond to CONVERT_TEMP.\n"); + dev_warn(device, "18S20 doesn't respond to CONVERT_TEMP.\n"); for (i = 0; i < 9; ++i) - count += sprintf(buf + count, "%02x ", sl->rom[i]); + c -= snprintf(buf + PAGE_SIZE - c, c, "%02x ", sl->rom[i]); - count += sprintf(buf + count, "t=%d\n", w1_convert_temp(rom, sl->family->fid)); -out: + c -= snprintf(buf + PAGE_SIZE - c, c, "t=%d\n", + w1_convert_temp(rom, sl->family->fid)); mutex_unlock(&dev->mutex); - return count; + return PAGE_SIZE - c; } static int __init w1_therm_init(void) diff --git a/drivers/w1/w1.h b/drivers/w1/w1.h index bad7c7c..ab2944a 100644 --- a/drivers/w1/w1.h +++ b/drivers/w1/w1.h @@ -46,7 +46,6 @@ struct w1_reg_num #include "w1_family.h" #define W1_MAXNAMELEN 32 -#define W1_SLAVE_DATA_SIZE 128 #define W1_SEARCH 0xF0 #define W1_ALARM_SEARCH 0xEC -- 1.4.4.4[unhandled content-type:application/pgp-signature] | |