Skip to content

Commit

Permalink
W1: w1_therm fix user buffer overflow and cat
Browse files Browse the repository at this point in the history
Fixed data reading bug by replacing binary attribute with device one.

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.

Signed-off-by: David Fries <david@fries.net>
Signed-off-by: Evgeniy Polyakov <johnpol@2ka.mipt.ru>
Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org>
  • Loading branch information
David Fries authored and Linus Torvalds committed Oct 16, 2008
1 parent 07e0034 commit 347ba8a
Show file tree
Hide file tree
Showing 2 changed files with 20 additions and 36 deletions.
55 changes: 20 additions & 35 deletions drivers/w1/slaves/w1_therm.c
Original file line number Diff line number Diff line change
Expand Up @@ -50,26 +50,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 = {
Expand Down Expand Up @@ -168,30 +162,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;

Expand All @@ -211,7 +194,9 @@ 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);
Expand All @@ -226,22 +211,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)
Expand Down
1 change: 0 additions & 1 deletion drivers/w1/w1.h
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down

0 comments on commit 347ba8a

Please sign in to comment.