Commit 347ba8a588c3e49f357291e5a1ac38a11d7e052d

Authored by David Fries
Committed by Linus Torvalds
1 parent 07e003417b

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>

Showing 2 changed files with 20 additions and 36 deletions Side-by-side Diff

drivers/w1/slaves/w1_therm.c
... ... @@ -50,26 +50,20 @@
50 50 {}
51 51 };
52 52  
53   -static ssize_t w1_therm_read_bin(struct kobject *, struct bin_attribute *,
54   - char *, loff_t, size_t);
  53 +static ssize_t w1_therm_read(struct device *device,
  54 + struct device_attribute *attr, char *buf);
55 55  
56   -static struct bin_attribute w1_therm_bin_attr = {
57   - .attr = {
58   - .name = "w1_slave",
59   - .mode = S_IRUGO,
60   - },
61   - .size = W1_SLAVE_DATA_SIZE,
62   - .read = w1_therm_read_bin,
63   -};
  56 +static struct device_attribute w1_therm_attr =
  57 + __ATTR(w1_slave, S_IRUGO, w1_therm_read, NULL);
64 58  
65 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 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 69 static struct w1_family_ops w1_therm_fops = {
76 70  
77 71  
78 72  
79 73  
... ... @@ -168,30 +162,19 @@
168 162 return 0;
169 163 }
170 164  
171   -static ssize_t w1_therm_read_bin(struct kobject *kobj,
172   - struct bin_attribute *bin_attr,
173   - char *buf, loff_t off, size_t count)
  165 +static ssize_t w1_therm_read(struct device *device,
  166 + struct device_attribute *attr, char *buf)
174 167 {
175   - struct w1_slave *sl = kobj_to_w1_slave(kobj);
  168 + struct w1_slave *sl = dev_to_w1_slave(device);
176 169 struct w1_master *dev = sl->master;
177 170 u8 rom[9], crc, verdict;
178 171 int i, max_trying = 10;
  172 + ssize_t c = PAGE_SIZE;
179 173  
180 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 176 memset(rom, 0, sizeof(rom));
193 177  
194   - count = 0;
195 178 verdict = 0;
196 179 crc = 0;
197 180  
... ... @@ -211,7 +194,9 @@
211 194  
212 195 w1_write_8(dev, W1_READ_SCRATCHPAD);
213 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 202 crc = w1_calc_crc8(rom, 8);
218 203  
219 204  
220 205  
221 206  
... ... @@ -226,22 +211,22 @@
226 211 }
227 212  
228 213 for (i = 0; i < 9; ++i)
229   - count += sprintf(buf + count, "%02x ", rom[i]);
230   - count += sprintf(buf + count, ": crc=%02x %s\n",
  214 + c -= snprintf(buf + PAGE_SIZE - c, c, "%02x ", rom[i]);
  215 + c -= snprintf(buf + PAGE_SIZE - c, c, ": crc=%02x %s\n",
231 216 crc, (verdict) ? "YES" : "NO");
232 217 if (verdict)
233 218 memcpy(sl->rom, rom, sizeof(sl->rom));
234 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 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));
241   -out:
  225 + c -= snprintf(buf + PAGE_SIZE - c, c, "t=%d\n",
  226 + w1_convert_temp(rom, sl->family->fid));
242 227 mutex_unlock(&dev->mutex);
243 228  
244   - return count;
  229 + return PAGE_SIZE - c;
245 230 }
246 231  
247 232 static int __init w1_therm_init(void)
... ... @@ -46,7 +46,6 @@
46 46 #include "w1_family.h"
47 47  
48 48 #define W1_MAXNAMELEN 32
49   -#define W1_SLAVE_DATA_SIZE 128
50 49  
51 50 #define W1_SEARCH 0xF0
52 51 #define W1_ALARM_SEARCH 0xEC