diff options
author | David Fries <david@fries.net> | 2008-10-16 01:04:51 -0400 |
---|---|---|
committer | Linus Torvalds <torvalds@linux-foundation.org> | 2008-10-16 14:21:50 -0400 |
commit | 347ba8a588c3e49f357291e5a1ac38a11d7e052d (patch) | |
tree | e3f67be1903df707e60116f841b8082da7facc7b /drivers | |
parent | 07e003417b88deac4b887c98f499fc3b01bc8df0 (diff) |
W1: w1_therm fix user buffer overflow and cat
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>
Diffstat (limited to 'drivers')
-rw-r--r-- | drivers/w1/slaves/w1_therm.c | 55 | ||||
-rw-r--r-- | drivers/w1/w1.h | 1 |
2 files changed, 20 insertions, 36 deletions
diff --git a/drivers/w1/slaves/w1_therm.c b/drivers/w1/slaves/w1_therm.c index e87f464a6fb0..7de99dfd11c8 100644 --- a/drivers/w1/slaves/w1_therm.c +++ b/drivers/w1/slaves/w1_therm.c | |||
@@ -50,26 +50,20 @@ static u8 bad_roms[][9] = { | |||
50 | {} | 50 | {} |
51 | }; | 51 | }; |
52 | 52 | ||
53 | static ssize_t w1_therm_read_bin(struct kobject *, struct bin_attribute *, | 53 | static ssize_t w1_therm_read(struct device *device, |
54 | char *, loff_t, size_t); | 54 | struct device_attribute *attr, char *buf); |
55 | 55 | ||
56 | static struct bin_attribute w1_therm_bin_attr = { | 56 | static struct device_attribute w1_therm_attr = |
57 | .attr = { | 57 | __ATTR(w1_slave, S_IRUGO, w1_therm_read, NULL); |
58 | .name = "w1_slave", | ||
59 | .mode = S_IRUGO, | ||
60 | }, | ||
61 | .size = W1_SLAVE_DATA_SIZE, | ||
62 | .read = w1_therm_read_bin, | ||
63 | }; | ||
64 | 58 | ||
65 | static int w1_therm_add_slave(struct w1_slave *sl) | 59 | static int w1_therm_add_slave(struct w1_slave *sl) |
66 | { | 60 | { |
67 | return sysfs_create_bin_file(&sl->dev.kobj, &w1_therm_bin_attr); | 61 | return device_create_file(&sl->dev, &w1_therm_attr); |
68 | } | 62 | } |
69 | 63 | ||
70 | static void w1_therm_remove_slave(struct w1_slave *sl) | 64 | static void w1_therm_remove_slave(struct w1_slave *sl) |
71 | { | 65 | { |
72 | sysfs_remove_bin_file(&sl->dev.kobj, &w1_therm_bin_attr); | 66 | device_remove_file(&sl->dev, &w1_therm_attr); |
73 | } | 67 | } |
74 | 68 | ||
75 | static struct w1_family_ops w1_therm_fops = { | 69 | static struct w1_family_ops w1_therm_fops = { |
@@ -168,30 +162,19 @@ static int w1_therm_check_rom(u8 rom[9]) | |||
168 | return 0; | 162 | return 0; |
169 | } | 163 | } |
170 | 164 | ||
171 | static ssize_t w1_therm_read_bin(struct kobject *kobj, | 165 | static ssize_t w1_therm_read(struct device *device, |
172 | struct bin_attribute *bin_attr, | 166 | struct device_attribute *attr, char *buf) |
173 | char *buf, loff_t off, size_t count) | ||
174 | { | 167 | { |
175 | struct w1_slave *sl = kobj_to_w1_slave(kobj); | 168 | struct w1_slave *sl = dev_to_w1_slave(device); |
176 | struct w1_master *dev = sl->master; | 169 | struct w1_master *dev = sl->master; |
177 | u8 rom[9], crc, verdict; | 170 | u8 rom[9], crc, verdict; |
178 | int i, max_trying = 10; | 171 | int i, max_trying = 10; |
172 | ssize_t c = PAGE_SIZE; | ||
179 | 173 | ||
180 | mutex_lock(&sl->master->mutex); | 174 | mutex_lock(&sl->master->mutex); |
181 | 175 | ||
182 | if (off > W1_SLAVE_DATA_SIZE) { | ||
183 | count = 0; | ||
184 | goto out; | ||
185 | } | ||
186 | if (off + count > W1_SLAVE_DATA_SIZE) { | ||
187 | count = 0; | ||
188 | goto out; | ||
189 | } | ||
190 | |||
191 | memset(buf, 0, count); | ||
192 | memset(rom, 0, sizeof(rom)); | 176 | memset(rom, 0, sizeof(rom)); |
193 | 177 | ||
194 | count = 0; | ||
195 | verdict = 0; | 178 | verdict = 0; |
196 | crc = 0; | 179 | crc = 0; |
197 | 180 | ||
@@ -211,7 +194,9 @@ static ssize_t w1_therm_read_bin(struct kobject *kobj, | |||
211 | 194 | ||
212 | w1_write_8(dev, W1_READ_SCRATCHPAD); | 195 | w1_write_8(dev, W1_READ_SCRATCHPAD); |
213 | if ((count = w1_read_block(dev, rom, 9)) != 9) { | 196 | if ((count = w1_read_block(dev, rom, 9)) != 9) { |
214 | dev_warn(&dev->dev, "w1_read_block() returned %d instead of 9.\n", count); | 197 | dev_warn(device, "w1_read_block() " |
198 | "returned %u instead of 9.\n", | ||
199 | count); | ||
215 | } | 200 | } |
216 | 201 | ||
217 | crc = w1_calc_crc8(rom, 8); | 202 | crc = w1_calc_crc8(rom, 8); |
@@ -226,22 +211,22 @@ static ssize_t w1_therm_read_bin(struct kobject *kobj, | |||
226 | } | 211 | } |
227 | 212 | ||
228 | for (i = 0; i < 9; ++i) | 213 | for (i = 0; i < 9; ++i) |
229 | count += sprintf(buf + count, "%02x ", rom[i]); | 214 | c -= snprintf(buf + PAGE_SIZE - c, c, "%02x ", rom[i]); |
230 | count += sprintf(buf + count, ": crc=%02x %s\n", | 215 | c -= snprintf(buf + PAGE_SIZE - c, c, ": crc=%02x %s\n", |
231 | crc, (verdict) ? "YES" : "NO"); | 216 | crc, (verdict) ? "YES" : "NO"); |
232 | if (verdict) | 217 | if (verdict) |
233 | memcpy(sl->rom, rom, sizeof(sl->rom)); | 218 | memcpy(sl->rom, rom, sizeof(sl->rom)); |
234 | else | 219 | else |
235 | dev_warn(&dev->dev, "18S20 doesn't respond to CONVERT_TEMP.\n"); | 220 | dev_warn(device, "18S20 doesn't respond to CONVERT_TEMP.\n"); |
236 | 221 | ||
237 | for (i = 0; i < 9; ++i) | 222 | for (i = 0; i < 9; ++i) |
238 | count += sprintf(buf + count, "%02x ", sl->rom[i]); | 223 | c -= snprintf(buf + PAGE_SIZE - c, c, "%02x ", sl->rom[i]); |
239 | 224 | ||
240 | count += sprintf(buf + count, "t=%d\n", w1_convert_temp(rom, sl->family->fid)); | 225 | c -= snprintf(buf + PAGE_SIZE - c, c, "t=%d\n", |
241 | out: | 226 | w1_convert_temp(rom, sl->family->fid)); |
242 | mutex_unlock(&dev->mutex); | 227 | mutex_unlock(&dev->mutex); |
243 | 228 | ||
244 | return count; | 229 | return PAGE_SIZE - c; |
245 | } | 230 | } |
246 | 231 | ||
247 | static int __init w1_therm_init(void) | 232 | static int __init w1_therm_init(void) |
diff --git a/drivers/w1/w1.h b/drivers/w1/w1.h index 00b84ab22808..cdaa6fffbfc7 100644 --- a/drivers/w1/w1.h +++ b/drivers/w1/w1.h | |||
@@ -46,7 +46,6 @@ struct w1_reg_num | |||
46 | #include "w1_family.h" | 46 | #include "w1_family.h" |
47 | 47 | ||
48 | #define W1_MAXNAMELEN 32 | 48 | #define W1_MAXNAMELEN 32 |
49 | #define W1_SLAVE_DATA_SIZE 128 | ||
50 | 49 | ||
51 | #define W1_SEARCH 0xF0 | 50 | #define W1_SEARCH 0xF0 |
52 | #define W1_ALARM_SEARCH 0xEC | 51 | #define W1_ALARM_SEARCH 0xEC |