net/atm : for sprintf, need check the total write length whether larger than a page.
Subsystems (3)
Hello David Miller:
in net/atm/atm_sysfs.c:
suggest to check the write length whether larger than a page.
the length of parameter buf is one page size (reference: fill_read_buffer at fs/sysfs/file.c) and the count of atm adresses are not limited (reference: atm_dev_ioctl -> atm_add_addr)
thanks.
gchen.
34 static ssize_t show_atmaddress(struct device *cdev,
35 struct device_attribute *attr, char *buf)
36 {
37 unsigned long flags;
38 char *pos = buf;
39 struct atm_dev *adev = to_atm_dev(cdev);
40 struct atm_dev_addr *aaddr;
41 int bin[] = { 1, 2, 10, 6, 1 }, *fmt = bin;
42 int i, j;
43
44 spin_lock_irqsave(&adev->lock, flags);
45 list_for_each_entry(aaddr, &adev->local, entry) {
46 for (i = 0, j = 0; i < ATM_ESA_LEN; ++i, ++j) {
47 if (j == *fmt) {
48 pos += sprintf(pos, ".");
49 ++fmt;
50 j = 0; 51 }
52 pos += sprintf(pos, "%02x",
53 aaddr->addr.sas_addr.prv[i]);
54 }
55 pos += sprintf(pos, "\n");
56 }
57 spin_unlock_irqrestore(&adev->lock, flags);
58
59 return pos - buf;
60 }
61
in net/atm/addr.c
67 int atm_add_addr(struct atm_dev *dev, const struct sockaddr_atmsvc *addr,
68 enum atm_addr_type_t atype)
69 {
70 unsigned long flags;
71 struct atm_dev_addr *this;
72 struct list_head *head;
73 int error;
74
75 error = check_addr(addr);
76 if (error)
77 return error;
78 spin_lock_irqsave(&dev->lock, flags);
79 if (atype == ATM_ADDR_LECS)
80 head = &dev->lecs;
81 else
82 head = &dev->local;
83 list_for_each_entry(this, head, entry) {
84 if (identical(&this->addr, addr)) {
85 spin_unlock_irqrestore(&dev->lock, flags);
86 return -EEXIST;
87 }
88 }
89 this = kmalloc(sizeof(struct atm_dev_addr), GFP_ATOMIC);
90 if (!this) {
91 spin_unlock_irqrestore(&dev->lock, flags);
92 return -ENOMEM;
93 }
94 this->addr = *addr;
95 list_add(&this->entry, head);
96 spin_unlock_irqrestore(&dev->lock, flags);
97 if (head == &dev->local) 98 notify_sigd(dev);
99 return 0;
100 }
101
in net/atm/resources.c
195 int atm_dev_ioctl(unsigned int cmd, void __user *arg, int compat) 196 {
197 void __user *buf; 198 int error, len, number, size = 0; 199 struct atm_dev *dev; 200 struct list_head *p; 201 int *tmp_buf, *tmp_p; 202 int __user *sioc_len; 203 int __user *iobuf_len;
204
205 #ifndef CONFIG_COMPAT
206 compat = 0; /* Just so the compiler _knows_ */
207 #endif
208
209 switch (cmd) {
210 case ATM_GETNAMES:
211 if (compat) {
212 #ifdef CONFIG_COMPAT
213 struct compat_atm_iobuf __user *ciobuf = arg; 214 compat_uptr_t cbuf; 215 iobuf_len = &ciobuf->length; 216 if (get_user(cbuf, &ciobuf->buffer))
217 return -EFAULT;
218 buf = compat_ptr(cbuf);
219 #endif
220 } else {
221 struct atm_iobuf __user *iobuf = arg; 222 iobuf_len = &iobuf->length; 223 if (get_user(buf, &iobuf->buffer))
224 return -EFAULT;
225 }
226 if (get_user(len, iobuf_len))
227 return -EFAULT;
228 mutex_lock(&atm_dev_mutex);
229 list_for_each(p, &atm_devs)
230 size += sizeof(int);
231 if (size > len) {
232 mutex_unlock(&atm_dev_mutex);
233 return -E2BIG;
234 }
235 tmp_buf = kmalloc(size, GFP_ATOMIC);
236 if (!tmp_buf) {
237 mutex_unlock(&atm_dev_mutex);
238 return -ENOMEM;
239 }
240 tmp_p = tmp_buf;
241 list_for_each(p, &atm_devs) {
242 dev = list_entry(p, struct atm_dev, dev_list);
243 *tmp_p++ = dev->number;
244 }
245 mutex_unlock(&atm_dev_mutex);
246 error = ((copy_to_user(buf, tmp_buf, size)) ||247 put_user(size, iobuf_len))
248 ? -EFAULT : 0;
249 kfree(tmp_buf);
250 return error;
251 default:
252 break;
253 }
254
255 if (compat) {
256 #ifdef CONFIG_COMPAT
257 struct compat_atmif_sioc __user *csioc = arg;
258 compat_uptr_t carg;
259
260 sioc_len = &csioc->length;
261 if (get_user(carg, &csioc->arg))
262 return -EFAULT;
263 buf = compat_ptr(carg);
264
265 if (get_user(len, &csioc->length))
266 return -EFAULT;
267 if (get_user(number, &csioc->number))
268 return -EFAULT;
269 #endif
270 } else {
271 struct atmif_sioc __user *sioc = arg;
272
273 sioc_len = &sioc->length;
274 if (get_user(buf, &sioc->arg))
275 return -EFAULT;
276 if (get_user(len, &sioc->length))
277 return -EFAULT;
278 if (get_user(number, &sioc->number))
279 return -EFAULT;
280 }
281
282 dev = try_then_request_module(atm_dev_lookup(number), "atm-device-%d",
283 number);
284 if (!dev)
285 return -ENODEV;
286
287 switch (cmd) {
288 case ATM_GETTYPE:
289 size = strlen(dev->type) + 1;
290 if (copy_to_user(buf, dev->type, size)) {
291 error = -EFAULT;
292 goto done;
293 }
294 break;
295 case ATM_GETESI:
296 size = ESI_LEN;
297 if (copy_to_user(buf, dev->esi, size)) {
298 error = -EFAULT;
299 goto done;
300 }
301 break;
302 case ATM_SETESI:
303 {
304 int i;
305
306 for (i = 0; i < ESI_LEN; i++)
307 if (dev->esi[i]) {
308 error = -EEXIST;
309 goto done;
310 }
311 }
312 /* fall through */
313 case ATM_SETESIF:
314 {315 unsigned char esi[ESI_LEN];
316
317 if (!capable(CAP_NET_ADMIN)) {
318 error = -EPERM;
319 goto done;
320 }
321 if (copy_from_user(esi, buf, ESI_LEN)) {
322 error = -EFAULT;
323 goto done;
324 } 325 memcpy(dev->esi, esi, ESI_LEN); 326 error = ESI_LEN; 327 goto done;
328 }
329 case ATM_GETSTATZ:
330 if (!capable(CAP_NET_ADMIN)) {
331 error = -EPERM;
332 goto done;
333 }
334 /* fall through */
335 case ATM_GETSTAT:
336 size = sizeof(struct atm_dev_stats); 337 error = fetch_stats(dev, buf, cmd == ATM_GETSTATZ); 338 if (error)
339 goto done;
340 break;
341 case ATM_GETCIRANGE:
342 size = sizeof(struct atm_cirange);
343 if (copy_to_user(buf, &dev->ci_range, size)) {
344 error = -EFAULT;
345 goto done;
346 }
347 break;
348 case ATM_GETLINKRATE:
349 size = sizeof(int);
350 if (copy_to_user(buf, &dev->link_rate, size)) {
351 error = -EFAULT;
352 goto done;
353 }
354 break;
355 case ATM_RSTADDR:
356 if (!capable(CAP_NET_ADMIN)) {
357 error = -EPERM;
358 goto done;
359 }
360 atm_reset_addr(dev, ATM_ADDR_LOCAL);
361 break;
362 case ATM_ADDADDR:
363 case ATM_DELADDR:
364 case ATM_ADDLECSADDR:
365 case ATM_DELLECSADDR:
366 {367 struct sockaddr_atmsvc addr;
368
369 if (!capable(CAP_NET_ADMIN)) {
370 error = -EPERM;
371 goto done;
372 }
373
374 if (copy_from_user(&addr, buf, sizeof(addr))) {
375 error = -EFAULT;
376 goto done;
377 }
378 if (cmd == ATM_ADDADDR || cmd == ATM_ADDLECSADDR)
379 error = atm_add_addr(dev, &addr,
380 (cmd == ATM_ADDADDR ?
381 ATM_ADDR_LOCAL : ATM_ADDR_LECS));
382 else
383 error = atm_del_addr(dev, &addr,
384 (cmd == ATM_DELADDR ?
385 ATM_ADDR_LOCAL : ATM_ADDR_LECS));
386 goto done;
387 } ... ... ... ...
Hello Maintainers:
was this suggestion replied ? (it seems not).
and please help to check whether this suggestion is valid.
thanks.
gchen.
Quoted 281 lines · Chen Gang
Hello David Miller:
in
net/atm/atm_sysfs.c:
suggest to check the write length whether larger than a page.
the length of parameter buf is one page size (reference:fill_read_bufferatfs/sysfs/file.c) and the count of atm adresses are not limited (reference:atm_dev_ioctl->atm_add_addr)thanks.
gchen.
34 static
ssize_tshow_atmaddress(struct device *cdev,
35 structdevice_attribute*attr, char *buf)
36 {37 unsigned long flags; 38 char *pos = buf; 39 struct atm_dev *adev = to_atm_dev(cdev); 40 struct atm_dev_addr *aaddr; 41 int bin[] = { 1, 2, 10, 6, 1 }, *fmt = bin; 42 int i, j; 43 44 spin_lock_irqsave(&adev->lock, flags); 45 list_for_each_entry(aaddr, &adev->local, entry) { 46 for (i = 0, j = 0; i < ATM_ESA_LEN; ++i, ++j) { 47 if (j == *fmt) { 48 pos += sprintf(pos, "."); 49 ++fmt; 50 j = 0;51 }
52 pos += sprintf(pos, "%02x",
53 aaddr->addr.sas_addr.prv[i]);
54 }
55 pos +=sprintf(pos, "\n");
56 }
57spin_unlock_irqrestore(&adev->lock, flags);
58
59 return pos - buf;
60 }
61in
net/atm/addr.c67 int atm_add_addr(struct
atm_dev*dev, const structsockaddr_atmsvc*addr,
68 enumatm_addr_type_tatype)
69 {70 unsigned long flags; 71 struct atm_dev_addr *this; 72 struct list_head *head; 73 int error; 74 75 error = check_addr(addr); 76 if (error) 77 return error; 78 spin_lock_irqsave(&dev->lock, flags); 79 if (atype == ATM_ADDR_LECS) 80 head = &dev->lecs; 81 else 82 head = &dev->local; 83 list_for_each_entry(this, head, entry) { 84 if (identical(&this->addr, addr)) { 85 spin_unlock_irqrestore(&dev->lock, flags); 86 return -EEXIST; 87 } 88 } 89 this = kmalloc(sizeof(struct atm_dev_addr), GFP_ATOMIC); 90 if (!this) { 91 spin_unlock_irqrestore(&dev->lock, flags); 92 return -ENOMEM; 93 } 94 this->addr = *addr; 95 list_add(&this->entry, head); 96 spin_unlock_irqrestore(&dev->lock, flags); 97 if (head == &dev->local)98
notify_sigd(dev);
99 return 0;
100 }
101in
net/atm/resources.c195 int
atm_dev_ioctl(unsigned int cmd, void __user *arg, int compat)196 {197 void __user *buf; 198 int error, len, number, size = 0; 199 struct atm_dev *dev; 200 struct list_head *p; 201 int *tmp_buf, *tmp_p; 202 int __user *sioc_len; 203 int __user *iobuf_len;204
205 #ifndefCONFIG_COMPAT
206 compat = 0; /* Just so the compiler _knows_ */
207 #endif
208
209 switch (cmd) {
210 caseATM_GETNAMES:
211 if (compat) {
212 #ifdefCONFIG_COMPAT213 struct compat_atm_iobuf __user *ciobuf = arg; 214 compat_uptr_t cbuf; 215 iobuf_len = &ciobuf->length; 216 if (get_user(cbuf, &ciobuf->buffer))217 return
-EFAULT;
218 buf =compat_ptr(cbuf);
219 #endif
220 } else {221 struct atm_iobuf __user *iobuf = arg; 222 iobuf_len = &iobuf->length; 223 if (get_user(buf, &iobuf->buffer))224 return
-EFAULT;
225 }
226 if (get_user(len, iobuf_len))
227 return-EFAULT;
228mutex_lock(&atm_dev_mutex);
229list_for_each(p, &atm_devs)
230 size += sizeof(int);
231 if (size > len) {
232mutex_unlock(&atm_dev_mutex);
233 return-E2BIG;234 } 235 tmp_buf = kmalloc(size, GFP_ATOMIC); 236 if (!tmp_buf) { 237 mutex_unlock(&atm_dev_mutex); 238 return -ENOMEM; 239 } 240 tmp_p = tmp_buf; 241 list_for_each(p, &atm_devs) { 242 dev = list_entry(p, struct atm_dev, dev_list); 243 *tmp_p++ = dev->number; 244 } 245 mutex_unlock(&atm_dev_mutex); 246 error = ((copy_to_user(buf, tmp_buf, size)) ||247
put_user(size, iobuf_len))
248 ?-EFAULT: 0;
249kfree(tmp_buf);
250 return error;
251 default:
252 break;
253 }
254
255 if (compat) {
256 #ifdefCONFIG_COMPAT
257 structcompat_atmif_sioc__user*csioc = arg;
258compat_uptr_tcarg;
259
260sioc_len=&csioc->length;
261 if (get_user(carg, &csioc->arg))
262 return-EFAULT;
263 buf =compat_ptr(carg);
264
265 if (get_user(len, &csioc->length))
266 return-EFAULT;
267 if (get_user(number, &csioc->number))
268 return-EFAULT;
269 #endif
270 } else {
271 structatmif_sioc__user*sioc = arg;
272
273sioc_len=&sioc->length;
274 if (get_user(buf, &sioc->arg))
275 return-EFAULT;
276 if (get_user(len, &sioc->length))
277 return-EFAULT;
278 if (get_user(number, &sioc->number))
279 return-EFAULT;
280 }
281
282 dev = try_then_request_module(atm_dev_lookup(number), "atm-device-%d",
283 number);
284 if (!dev)
285 return-ENODEV;
286
287 switch (cmd) {
288 caseATM_GETTYPE:
289 size =strlen(dev->type)+ 1;
290 if (copy_to_user(buf, dev->type, size)) {
291 error =-EFAULT;
292 goto done;
293 }
294 break;
295 caseATM_GETESI:
296 size =ESI_LEN;
297 if (copy_to_user(buf, dev->esi, size)) {
298 error =-EFAULT;
299 goto done;
300 }
301 break;
302 caseATM_SETESI:
303 {
304 int i;
305
306 for (i = 0; i <ESI_LEN; i++)
307 if (dev->esi[i]) {
308 error =-EEXIST;
309 goto done;
310 }311 } 312 /* fall through */ 313 case ATM_SETESIF: 314 {315 unsigned char esi[ESI_LEN];
316
317 if (!capable(CAP_NET_ADMIN)) {
318 error =-EPERM;
319 goto done;
320 }
321 if (copy_from_user(esi, buf, ESI_LEN)) {
322 error =-EFAULT;
323 goto done;324 } 325 memcpy(dev->esi, esi, ESI_LEN); 326 error = ESI_LEN; 327 goto done;328 }
329 caseATM_GETSTATZ:
330 if (!capable(CAP_NET_ADMIN)) {
331 error =-EPERM;
332 goto done;
333 }
334 /* fall through */
335 caseATM_GETSTAT:336 size = sizeof(struct atm_dev_stats); 337 error = fetch_stats(dev, buf, cmd == ATM_GETSTATZ); 338 if (error)339 goto done;
340 break;
341 caseATM_GETCIRANGE:
342 size =sizeof(struct atm_cirange);
343 if (copy_to_user(buf, &dev->ci_range, size)) {
344 error =-EFAULT;
345 goto done;
346 }
347 break;
348 caseATM_GETLINKRATE:
349 size = sizeof(int);
350 if (copy_to_user(buf, &dev->link_rate, size)) {
351 error =-EFAULT;
352 goto done;
353 }
354 break;
355 caseATM_RSTADDR:
356 if (!capable(CAP_NET_ADMIN)) {
357 error =-EPERM;
358 goto done;359 } 360 atm_reset_addr(dev, ATM_ADDR_LOCAL); 361 break; 362 case ATM_ADDADDR: 363 case ATM_DELADDR: 364 case ATM_ADDLECSADDR: 365 case ATM_DELLECSADDR: 366 {367 struct
sockaddr_atmsvcaddr;
368
369 if (!capable(CAP_NET_ADMIN)) {
370 error =-EPERM;
371 goto done;
372 }
373
374 if (copy_from_user(&addr, buf, sizeof(addr))) {
375 error =-EFAULT;
376 goto done;
377 }
378 if (cmd ==ATM_ADDADDR|| cmd ==ATM_ADDLECSADDR)
379 error = atm_add_addr(dev, &addr,
380 (cmd ==ATM_ADDADDR?
381ATM_ADDR_LOCAL:ATM_ADDR_LECS));
382 else
383 error = atm_del_addr(dev, &addr,
384 (cmd ==ATM_DELADDR?
385ATM_ADDR_LOCAL:ATM_ADDR_LECS));
386 goto done;387 } ... ... ... ...
Signature · 3 lines
Chen Gang Asianux Corporation
yes this seems like it should be done. maybe this week i will try to put something together unless you already have a patch somewhere.
Quoted 295 lines · On Mon, 03 Dec 2012 16:56:56 +0800 Chen Gang <[email protected]> wrote:
Hello Maintainers:
was this suggestion replied ? (it seems not).
and please help to check whether this suggestion is valid.
thanks.
gchen.
Quoted 281 lines · Chen Gang
Hello David Miller:
in
net/atm/atm_sysfs.c:
suggest to check the write length whether larger than a page.
the length of parameter buf is one page size (reference:fill_read_bufferatfs/sysfs/file.c) and the count of atm adresses are not limited (reference:atm_dev_ioctl->atm_add_addr)thanks.
gchen.
34 static
ssize_tshow_atmaddress(struct device *cdev,
35 structdevice_attribute*attr, char *buf)
36 {37 unsigned long flags; 38 char *pos = buf; 39 struct atm_dev *adev = to_atm_dev(cdev); 40 struct atm_dev_addr *aaddr; 41 int bin[] = { 1, 2, 10, 6, 1 }, *fmt = bin; 42 int i, j; 43 44 spin_lock_irqsave(&adev->lock, flags); 45 list_for_each_entry(aaddr, &adev->local, entry) { 46 for (i = 0, j = 0; i < ATM_ESA_LEN; ++i, ++j) { 47 if (j == *fmt) { 48 pos += sprintf(pos, "."); 49 ++fmt; 50 j = 0;51 }
52 pos += sprintf(pos, "%02x",
53 aaddr->addr.sas_addr.prv[i]);
54 }
55 pos +=sprintf(pos, "\n");
56 }
57spin_unlock_irqrestore(&adev->lock, flags);
58
59 return pos - buf;
60 }
61in
net/atm/addr.c67 int atm_add_addr(struct
atm_dev*dev, const structsockaddr_atmsvc*addr,
68 enumatm_addr_type_tatype)
69 {70 unsigned long flags; 71 struct atm_dev_addr *this; 72 struct list_head *head; 73 int error; 74 75 error = check_addr(addr); 76 if (error) 77 return error; 78 spin_lock_irqsave(&dev->lock, flags); 79 if (atype == ATM_ADDR_LECS) 80 head = &dev->lecs; 81 else 82 head = &dev->local; 83 list_for_each_entry(this, head, entry) { 84 if (identical(&this->addr, addr)) { 85 spin_unlock_irqrestore(&dev->lock, flags); 86 return -EEXIST; 87 } 88 } 89 this = kmalloc(sizeof(struct atm_dev_addr), GFP_ATOMIC); 90 if (!this) { 91 spin_unlock_irqrestore(&dev->lock, flags); 92 return -ENOMEM; 93 } 94 this->addr = *addr; 95 list_add(&this->entry, head); 96 spin_unlock_irqrestore(&dev->lock, flags); 97 if (head == &dev->local)98
notify_sigd(dev);
99 return 0;
100 }
101in
net/atm/resources.c195 int
atm_dev_ioctl(unsigned int cmd, void __user *arg, int compat)196 {197 void __user *buf; 198 int error, len, number, size = 0; 199 struct atm_dev *dev; 200 struct list_head *p; 201 int *tmp_buf, *tmp_p; 202 int __user *sioc_len; 203 int __user *iobuf_len;204
205 #ifndefCONFIG_COMPAT
206 compat = 0; /* Just so the compiler _knows_ */
207 #endif
208
209 switch (cmd) {
210 caseATM_GETNAMES:
211 if (compat) {
212 #ifdefCONFIG_COMPAT213 struct compat_atm_iobuf __user *ciobuf = arg; 214 compat_uptr_t cbuf; 215 iobuf_len = &ciobuf->length; 216 if (get_user(cbuf, &ciobuf->buffer))217 return
-EFAULT;
218 buf =compat_ptr(cbuf);
219 #endif
220 } else {221 struct atm_iobuf __user *iobuf = arg; 222 iobuf_len = &iobuf->length; 223 if (get_user(buf, &iobuf->buffer))224 return
-EFAULT;
225 }
226 if (get_user(len, iobuf_len))
227 return-EFAULT;
228mutex_lock(&atm_dev_mutex);
229list_for_each(p, &atm_devs)
230 size += sizeof(int);
231 if (size > len) {
232mutex_unlock(&atm_dev_mutex);
233 return-E2BIG;234 } 235 tmp_buf = kmalloc(size, GFP_ATOMIC); 236 if (!tmp_buf) { 237 mutex_unlock(&atm_dev_mutex); 238 return -ENOMEM; 239 } 240 tmp_p = tmp_buf; 241 list_for_each(p, &atm_devs) { 242 dev = list_entry(p, struct atm_dev, dev_list); 243 *tmp_p++ = dev->number; 244 } 245 mutex_unlock(&atm_dev_mutex); 246 error = ((copy_to_user(buf, tmp_buf, size)) ||247
put_user(size, iobuf_len))
248 ?-EFAULT: 0;
249kfree(tmp_buf);
250 return error;
251 default:
252 break;
253 }
254
255 if (compat) {
256 #ifdefCONFIG_COMPAT
257 structcompat_atmif_sioc__user*csioc = arg;
258compat_uptr_tcarg;
259
260sioc_len=&csioc->length;
261 if (get_user(carg, &csioc->arg))
262 return-EFAULT;
263 buf =compat_ptr(carg);
264
265 if (get_user(len, &csioc->length))
266 return-EFAULT;
267 if (get_user(number, &csioc->number))
268 return-EFAULT;
269 #endif
270 } else {
271 structatmif_sioc__user*sioc = arg;
272
273sioc_len=&sioc->length;
274 if (get_user(buf, &sioc->arg))
275 return-EFAULT;
276 if (get_user(len, &sioc->length))
277 return-EFAULT;
278 if (get_user(number, &sioc->number))
279 return-EFAULT;
280 }
281
282 dev = try_then_request_module(atm_dev_lookup(number), "atm-device-%d",
283 number);
284 if (!dev)
285 return-ENODEV;
286
287 switch (cmd) {
288 caseATM_GETTYPE:
289 size =strlen(dev->type)+ 1;
290 if (copy_to_user(buf, dev->type, size)) {
291 error =-EFAULT;
292 goto done;
293 }
294 break;
295 caseATM_GETESI:
296 size =ESI_LEN;
297 if (copy_to_user(buf, dev->esi, size)) {
298 error =-EFAULT;
299 goto done;
300 }
301 break;
302 caseATM_SETESI:
303 {
304 int i;
305
306 for (i = 0; i <ESI_LEN; i++)
307 if (dev->esi[i]) {
308 error =-EEXIST;
309 goto done;
310 }311 } 312 /* fall through */ 313 case ATM_SETESIF: 314 {315 unsigned char esi[ESI_LEN];
316
317 if (!capable(CAP_NET_ADMIN)) {
318 error =-EPERM;
319 goto done;
320 }
321 if (copy_from_user(esi, buf, ESI_LEN)) {
322 error =-EFAULT;
323 goto done;324 } 325 memcpy(dev->esi, esi, ESI_LEN); 326 error = ESI_LEN; 327 goto done;328 }
329 caseATM_GETSTATZ:
330 if (!capable(CAP_NET_ADMIN)) {
331 error =-EPERM;
332 goto done;
333 }
334 /* fall through */
335 caseATM_GETSTAT:336 size = sizeof(struct atm_dev_stats); 337 error = fetch_stats(dev, buf, cmd == ATM_GETSTATZ); 338 if (error)339 goto done;
340 break;
341 caseATM_GETCIRANGE:
342 size =sizeof(struct atm_cirange);
343 if (copy_to_user(buf, &dev->ci_range, size)) {
344 error =-EFAULT;
345 goto done;
346 }
347 break;
348 caseATM_GETLINKRATE:
349 size = sizeof(int);
350 if (copy_to_user(buf, &dev->link_rate, size)) {
351 error =-EFAULT;
352 goto done;
353 }
354 break;
355 caseATM_RSTADDR:
356 if (!capable(CAP_NET_ADMIN)) {
357 error =-EPERM;
358 goto done;359 } 360 atm_reset_addr(dev, ATM_ADDR_LOCAL); 361 break; 362 case ATM_ADDADDR: 363 case ATM_DELADDR: 364 case ATM_ADDLECSADDR: 365 case ATM_DELLECSADDR: 366 {367 struct
sockaddr_atmsvcaddr;
368
369 if (!capable(CAP_NET_ADMIN)) {
370 error =-EPERM;
371 goto done;
372 }
373
374 if (copy_from_user(&addr, buf, sizeof(addr))) {
375 error =-EFAULT;
376 goto done;
377 }
378 if (cmd ==ATM_ADDADDR|| cmd ==ATM_ADDLECSADDR)
379 error = atm_add_addr(dev, &addr,
380 (cmd ==ATM_ADDADDR?
381ATM_ADDR_LOCAL:ATM_ADDR_LECS));
382 else
383 error = atm_del_addr(dev, &addr,
384 (cmd ==ATM_DELADDR?
385ATM_ADDR_LOCAL:ATM_ADDR_LECS));
386 goto done;387 } ... ... ... ...
Quoted 8 lines · In message <[email protected]>,Chen Gang writes:
in
net/atm/atm_sysfs.c:
suggest to check the write length whether larger than a page.
the length of parameter buf is one page size (reference:fill_read_bufferatfs/sysfs/file.c) and the count of atm adresses are not limited (reference:atm_dev_ioctl->atm_add_addr)thanks.
gchen.
how about this as a possible fix?
atm: use scnprintf() instead of sprintf()
As reported by Chen Gang <[email protected]>, we should ensure there is enough space when formatting the sysfs buffers.
--- net/atm/atm_sysfs.c | 40 +++++++++++++++------------------------- 1 files changed, 15 insertions(+), 25 deletions(-)
net/atm/atm_sysfs.c +15 -25
@@ -14,49 +14,45 @@ static ssize_t show_type(struct device *cdev, struct device_attribute *attr, char *buf) { struct atm_dev *adev = to_atm_dev(cdev); -return sprintf (buf,"%s\n" , adev->type); + +return scnprintf (buf,PAGE_SIZE ,"%s\n" , adev->type); }static ssize_t show_address (struct device *cdev,struct device_attribute *attr,char *buf) { -char *pos = buf;struct atm_dev *adev =to_atm_dev (cdev); -int i; - -for (i =0 ; i < (ESI_LEN -1 ); i++) - pos +=sprintf (pos,"%02x:" , adev->esi[i]); - pos +=sprintf (pos,"%02x\n" , adev->esi[i]); -return pos - buf; +return scnprintf (buf,PAGE_SIZE ,"%pM\n" , adev->esi); }static ssize_t show_atmaddress (struct device *cdev,struct device_attribute *attr,char *buf) {unsigned long flags; -char *pos = buf;struct atm_dev *adev =to_atm_dev (cdev);struct atm_dev_addr *aaddr;int bin[] = {1 ,2 ,10 ,6 ,1 }, *fmt = bin; -int i, j; +int i, j, count =0 ;spin_lock_irqsave (&adev->lock, flags);list_for_each_entry (aaddr, &adev->local, entry) {for (i =0 , j =0 ; i <ATM_ESA_LEN ; ++i, ++j) {if (j == *fmt) { - pos +=sprintf (pos,"." ); + count +=scnprintf (buf + count, +PAGE_SIZE - count,"." ); ++fmt; j =0 ; } - pos +=sprintf (pos,"%02x" , - aaddr->addr.sas_addr.prv[i]); + count +=scnprintf (buf + count, +PAGE_SIZE - count,"%02x" , + aaddr->addr.sas_addr.prv[i]); } - pos +=sprintf (pos,"\n" ); + count +=scnprintf (buf + count,PAGE_SIZE - count,"\n" ); }spin_unlock_irqrestore (&adev->lock, flags); -return pos - buf; +return count; } static ssize_t show_atmindex(struct device *cdev,
@@ -64,25 +60,21 @@ static ssize_t show_atmindex(struct device *cdev, {struct atm_dev *adev =to_atm_dev (cdev); -return sprintf (buf,"%d\n" , adev->number); +return scnprintf (buf,PAGE_SIZE ,"%d\n" , adev->number); }static ssize_t show_carrier (struct device *cdev,struct device_attribute *attr,char *buf) { -char *pos = buf;struct atm_dev *adev =to_atm_dev (cdev); - pos +=sprintf (pos,"%d\n" , - adev->signal ==ATM_PHY_SIG_LOST ?0 :1 ); - -return pos - buf; +return scnprintf (buf,PAGE_SIZE ,"%d\n" , + adev->signal ==ATM_PHY_SIG_LOST ?0 :1 ); }static ssize_t show_link_rate (struct device *cdev,struct device_attribute *attr,char *buf) { -char *pos = buf;struct atm_dev *adev =to_atm_dev (cdev);int link_rate;
@@ -100,9 +92,7 @@ static ssize_t show_link_rate(struct device *cdev,default : link_rate = adev->link_rate *8 *53 ; } - pos +=sprintf (pos,"%d\n" , link_rate); - -return pos - buf; +return scnprintf (buf,PAGE_SIZE ,"%d\n" , link_rate); } static DEVICE_ATTR(address, S_IRUGO, show_address, NULL);
Signature · 1 line
1.7.7.6
firstly, sorry for reply late (yesterday I have an annual leave for
some personal things).
and thank you for your reply, too.
Quoted 5 lines · Chas Williams (CONTRACTOR)
static
ssize_tshow_address(struct device *cdev,net/atm/atm_sysfs.c +0
-1struct device_attribute *attr, char *buf) { - char *pos = buf;
net/atm/atm_sysfs.c static ssize_t show_type(struct device *cdev, · line 27
Quoted 10 lines
net/atm/atm_sysfs.c +1
-6struct atm_dev *adev =to_atm_dev (cdev); -int i; - -for (i =0 ; i < (ESI_LEN -1 ); i++) - pos +=sprintf (pos,"%02x:" , adev->esi[i]); - pos +=sprintf (pos,"%02x\n" , adev->esi[i]); -return pos - buf; +return scnprintf (buf,PAGE_SIZE ,"%pM\n" , adev->esi); }
"%p" seems print a pointer, not contents of pointer (is it correct ?)
will it change the original display format to outside ?
Quoted 26 lines
static
ssize_tshow_atmaddress(struct device *cdev,net/atm/atm_sysfs.c +5
-5struct device_attribute *attr, char *buf) { unsigned long flags; -char *pos = buf; struct atm_dev *adev = to_atm_dev(cdev); struct atm_dev_addr *aaddr; int bin[] = { 1, 2, 10, 6, 1 }, *fmt = bin; -int i, j; + int i, j, count = 0; spin_lock_irqsave(&adev->lock, flags); list_for_each_entry(aaddr, &adev->local, entry) { for (i = 0, j = 0; i < ATM_ESA_LEN; ++i, ++j) { if (j == *fmt) { - pos +=sprintf (pos,"." ); + count += scnprintf(buf + count, + PAGE_SIZE - count, "."); ++fmt; j = 0; } - pos +=sprintf (pos,"%02x" , - aaddr->addr.sas_addr.prv[i]); + count += scnprintf(buf + count, + PAGE_SIZE - count, "%02x",
net/atm/atm_sysfs.c static ssize_t show_type(struct device *cdev, · line 56
Quoted 10 lines
net/atm/atm_sysfs.c +3
-2+ aaddr->addr.sas_addr.prv[i]); } - pos +=sprintf (pos,"\n" ); + count +=scnprintf (buf + count,PAGE_SIZE - count,"\n" ); }spin_unlock_irqrestore (&adev->lock, flags); -return pos - buf; +return count; }
need we judge whether count >= PAGE_SIZE ?
these are my suggestions, maybe not correct.
:-)
thanks.
Signature · 3 lines
Chen Gang Asianux Corporation
Quoted 3 lines · chas williams - CONTRACTOR
yes this seems like it should be done. maybe this week i will try to put something together unless you already have a patch somewhere.
thank you very much.
and also sorry for replying late (yesterday, I have an annual leave
for some personal things)
Quoted 11 lines · In message <[email protected]>,Chen Gang writes:
net/atm/atm_sysfs.c static ssize_t show_type(struct device *cdev, · line 27
Quoted 8 lines
net/atm/atm_sysfs.c +1
-4-for (i =0 ; i < (ESI_LEN -1 ); i++) - pos +=sprintf (pos,"%02x:" , adev->esi[i]); - pos +=sprintf (pos,"%02x\n" , adev->esi[i]); -return pos - buf; +return scnprintf (buf,PAGE_SIZE ,"%pM\n" , adev->esi); }"%p" seems print a pointer, not contents of pointer (is it correct ?) will it change the original display format to outside ?
%pM means format this pointer as a mac address. it didnt exist when the atm stack was originally written but can be used now to save a bit of messy code.
Quoted 2 lines
Quoted 2 lines
net/atm/atm_sysfs.c +1
-1- pos +=sprintf (pos,"\n" ); + count +=scnprintf (buf + count,PAGE_SIZE - count,"\n" );
...
Quoted 1 line
need we judge whether count >=
PAGE_SIZE?
count will eventually make PAGE_SIZE - count reach 0 at which point, scnprintf() won't be able to write into the buffer.
Quoted 13 lines · Chas Williams (CONTRACTOR)
Quoted 11 lines · In message <[email protected]>,Chen Gang writes:
net/atm/atm_sysfs.c static ssize_t show_type(struct device *cdev, · line 27
Quoted 8 lines
net/atm/atm_sysfs.c +1
-4-for (i =0 ; i < (ESI_LEN -1 ); i++) - pos +=sprintf (pos,"%02x:" , adev->esi[i]); - pos +=sprintf (pos,"%02x\n" , adev->esi[i]); -return pos - buf; +return scnprintf (buf,PAGE_SIZE ,"%pM\n" , adev->esi); }"%p" seems print a pointer, not contents of pointer (is it correct ?) will it change the original display format to outside ?
Quoted 4 lines
%pM means format this pointer as a mac address. it didnt exist when the atm stack was originally written but can be used now to save a bit of messy code.
it is my fault. thank you
:-)
Quoted 7 lines
Quoted 2 lines
Quoted 2 lines
net/atm/atm_sysfs.c +1
-1- pos +=sprintf (pos,"\n" ); + count +=scnprintf (buf + count,PAGE_SIZE - count,"\n" );..
Quoted 1 line
need we judge whether count >=
PAGE_SIZE?count will eventually make
PAGE_SIZE- count reach 0 at which point,scnprintf()won't be able to write into the buffer.
I also think so.
I think, maybe it will be better to break the loop when we already
know that "count >= PAGE_SIZE" (it can save waste looping, although it seems unlikly happen, for example, using unlikly(...) ).
By the way:
will it be better that always let "\n" at the end ?
(if count == PAGE_SIZE in a loop, we can not let "\n" at the end).
I think what I said above are minor, if you think, for this patch, do
not need consider them, it is ok (at least for me, it is true).
:-)
Quoted 6 lines
--
To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Signature · 3 lines
Chen Gang Asianux Corporation
Quoted 9 lines · Chen Gang
Quoted 7 lines
Quoted 7 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
net/atm/atm_sysfs.c +1
-1- pos +=sprintf (pos,"\n" ); + count +=scnprintf (buf + count,PAGE_SIZE - count,"\n" );..
Quoted 1 line
Quoted 1 line
need we judge whether count >=
PAGE_SIZE?count will eventually make
PAGE_SIZE- count reach 0 at which point,scnprintf()won't be able to write into the buffer.I also think so.
Quoted 7 lines
I think, maybe it will be better to break the loop when we already
know that "count >=PAGE_SIZE" (it can save waste looping, although it seems unlikly happen, for example, using unlikly(...) ).By the way:
will it be better that always let "\n" at the end ?
(if count ==PAGE_SIZEin a loop, we can not let "\n" at the end).
oh, sorry ! count will never >= PAGE_SIZE.
I think let "PAGE_SIZE - 2" instead of "PAGE_SIZE" in the loop, so we
can make the room for the end of "\n".
Signature · 3 lines
Chen Gang Asianux Corporation
Quoted 14 lines · Chen Gang
Quoted 13 lines · Chen Gang
Quoted 7 lines
Quoted 7 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
net/atm/atm_sysfs.c +1
-1- pos +=sprintf (pos,"\n" ); + count +=scnprintf (buf + count,PAGE_SIZE - count,"\n" );..
Quoted 1 line
Quoted 1 line
need we judge whether count >=
PAGE_SIZE?count will eventually make
PAGE_SIZE- count reach 0 at which point,scnprintf()won't be able to write into the buffer.I also think so.
I think, maybe it will be better to break the loop when we already
know that "count >=PAGE_SIZE" (it can save waste looping, although it seems unlikly happen, for example, using unlikly(...) ).
Quoted 11 lines
Quoted 3 lines
By the way:
will it be better that always let "\n" at the end ?
(if count ==PAGE_SIZEin a loop, we can not let "\n" at the end).oh, sorry ! count will never >=
PAGE_SIZE.I think let "
PAGE_SIZE- 2" instead of "PAGE_SIZE" in the loop, so we
can make the room for the end of "\n".
sorry, "PAGE_SIZE - 1" is enough, not need "PAGE_SIZE - 2".
Signature · 3 lines
Chen Gang Asianux Corporation
Quoted 7 lines · On Wed, 05 Dec 2012 13:59:26 +0800 Chen Gang <[email protected]> wrote:
Quoted 6 lines · Chen Gang
Quoted 5 lines · Chen Gang
Quoted 5 lines
Quoted 5 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
net/atm/atm_sysfs.c +1
-1- pos +=sprintf (pos,"\n" ); + count +=scnprintf (buf + count,PAGE_SIZE - count,"\n" );..
Quoted 1 line
Quoted 1 line
need we judge whether count >=
PAGE_SIZE?
Quoted 7 lines
Quoted 7 lines
Quoted 7 lines
Quoted 2 lines
Quoted 2 lines
count will eventually make
PAGE_SIZE- count reach 0 at which point,scnprintf()won't be able to write into the buffer.I also think so.
I think, maybe it will be better to break the loop when we already
know that "count >=PAGE_SIZE" (it can save waste looping, although it seems unlikly happen, for example, using unlikly(...) ).
it doesn't seem like optimizing for this corner case is a huge concern. the list cannot be infinitely long.
Quoted 5 lines
Quoted 5 lines
Quoted 4 lines
By the way:
will it be better that always let "\n" at the end ?
(if count ==PAGE_SIZEin a loop, we can not let "\n" at the end).
Quoted 8 lines
Quoted 7 lines
oh, sorry ! count will never >=
PAGE_SIZE.I think let "
PAGE_SIZE- 2" instead of "PAGE_SIZE" in the loop, so we
can make the room for the end of "\n".sorry, "
PAGE_SIZE- 1" is enough, not need "PAGE_SIZE- 2".
did you mean '\0' instead of '\n'? scnprintf() considers the trailing '\0' when formatting.
Quoted 5 lines · chas williams - CONTRACTOR
On Wed, 05 Dec 2012 13:59:26 +0800
it doesn't seem like optimizing for this corner case is a huge concern. the list cannot be infinitely long.
ok.
Quoted 12 lines
Quoted 12 lines
Quoted 12 lines
Quoted 4 lines
By the way:
will it be better that always let "\n" at the end ?
(if count ==PAGE_SIZEin a loop, we can not let "\n" at the end).oh, sorry ! count will never >=
PAGE_SIZE.I think let "
PAGE_SIZE- 2" instead of "PAGE_SIZE" in the loop, so we
can make the room for the end of "\n".
Quoted 4 lines
Quoted 1 line
sorry, "
PAGE_SIZE- 1" is enough, not need "PAGE_SIZE- 2".did you mean '\0' instead of '\n'?
scnprintf()considers the trailing '\0' when formatting.
no, originally, the end is "\n\0".
I prefer we still compatible "\n" when the contents are very large. if count already == (PAGE_SIZE - 1), we have no chance to append "\n" to the end.
- pos += sprintf(pos, "\n");
+ count += scnprintf(buf + count, PAGE_SIZE - count, "\n");
Quoted 6 lines
--
To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Signature · 3 lines
Chen Gang Asianux Corporation
Hi Chas Williams:
all of my original reply are my idea (or suggestions), not for issues.
if you do not need them, please help to send patch.
I have tried to check it. at least, I did not find issues (and I also
also learned from it)
hope the patch can pass reviewers checking !
Good Luck !
:-)
gchen.
Quoted 44 lines · Chen Gang
Quoted 5 lines · chas williams - CONTRACTOR
On Wed, 05 Dec 2012 13:59:26 +0800
it doesn't seem like optimizing for this corner case is a huge concern. the list cannot be infinitely long.
ok.
Quoted 12 lines
Quoted 12 lines
Quoted 12 lines
Quoted 4 lines
By the way:
will it be better that always let "\n" at the end ?
(if count ==PAGE_SIZEin a loop, we can not let "\n" at the end).oh, sorry ! count will never >=
PAGE_SIZE.I think let "
PAGE_SIZE- 2" instead of "PAGE_SIZE" in the loop, so we
can make the room for the end of "\n".Quoted 4 lines
Quoted 1 line
sorry, "
PAGE_SIZE- 1" is enough, not need "PAGE_SIZE- 2".did you mean '\0' instead of '\n'?
scnprintf()considers the trailing '\0' when formatting.no, originally, the end is "\n\0".
I prefer we still compatible "\n" when the contents are very large. if count already == (
PAGE_SIZE- 1), we have no chance to append "\n" to the end.(quoted diff) +1
-1- pos += sprintf(pos, "\n"); + count += scnprintf(buf + count, PAGE_SIZE - count, "\n");Quoted 8 lines
--
To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Signature · 3 lines
Chen Gang Asianux Corporation
Review
Quoted 12 lines · On Thu, 06 Dec 2012 09:15:10 +0800 Chen Gang <[email protected]> wrote:
Quoted 2 lines · chas williams - CONTRACTOR
did you mean '\0' instead of '\n'?
scnprintf()considers the trailing '\0' when formatting.no, originally, the end is "\n\0".
I prefer we still compatible "\n" when the contents are very large. if count already == (
PAGE_SIZE- 1), we have no chance to append "\n" to the end.(quoted diff) +1
-1- pos += sprintf(pos, "\n"); + count += scnprintf(buf + count, PAGE_SIZE - count, "\n");
i would make the code a bit messy to do this for not much gain. again, it isnt likely you would run into this in a normal situation.
Quoted 10 lines · chas williams - CONTRACTOR
Quoted 7 lines · On Thu, 06 Dec 2012 09:15:10 +0800 Chen Gang <[email protected]> wrote:
Quoted 2 lines · chas williams - CONTRACTOR
did you mean '\0' instead of '\n'?
scnprintf()considers the trailing '\0' when formatting.no, originally, the end is "\n\0".
Quoted 8 lines
Review
Quoted 5 lines
I prefer we still compatible "\n" when the contents are very large. if count already == (
PAGE_SIZE- 1), we have no chance to append "\n" to the end.(quoted diff) +1
-1- pos += sprintf(pos, "\n"); + count += scnprintf(buf + count, PAGE_SIZE - count, "\n");i would make the code a bit messy to do this for not much gain. again, it isnt likely you would run into this in a normal situation.
surely.
thanks.
Quoted 6 lines
--
To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Signature · 3 lines
Chen Gang Asianux Corporation
Hello Chas Williams:
Excuse me:
I am a reporter (not a reviewer), and not suitable to review another's patch.
so:
I suggest you to send your patch (as your willing) to the reviewers, directly. and need not cc to me (I am not a reviewer).
thanks.
gchen.
Quoted 32 lines · Chen Gang
Quoted 10 lines · chas williams - CONTRACTOR
Quoted 7 lines · On Thu, 06 Dec 2012 09:15:10 +0800 Chen Gang <[email protected]> wrote:
Quoted 2 lines · chas williams - CONTRACTOR
did you mean '\0' instead of '\n'?
scnprintf()considers the trailing '\0' when formatting.no, originally, the end is "\n\0".
Quoted 8 lines
Review
Quoted 5 lines
I prefer we still compatible "\n" when the contents are very large. if count already == (
PAGE_SIZE- 1), we have no chance to append "\n" to the end.(quoted diff) +1
-1- pos += sprintf(pos, "\n"); + count += scnprintf(buf + count, PAGE_SIZE - count, "\n");i would make the code a bit messy to do this for not much gain. again, it isnt likely you would run into this in a normal situation.
surely.
thanks.
Quoted 6 lines
--
To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Signature · 3 lines
Chen Gang Asianux Corporation