Skip to contentSkip to conversation

net/atm : for sprintf, need check the total write length whether larger than a page.

Suggestion Chen Gang 2 participants 16 messages Nov 21, 2012 – Dec 10, 2012

Subsystems (3)
lore
Share & tools
lore

Chen Gang

author
#0 lore raw

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         }
...         ...
...         ...
author
#1 lore raw

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_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         }
...         ...
...         ...
Signature · 3 lines
Chen Gang

Asianux Corporation
#2 lore raw

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_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         }
...         ...
...         ...
author
#3 lore raw
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)

#4 lore raw
In reply to Chen Gang
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_buffer at fs/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
index f49da58..350bf62 100644
@@ -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
author
#5 lore raw

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_t show_address(struct device *cdev,

net/atm/atm_sysfs.c +0 -1
 			    struct 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 -6
 	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);
 }

"%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_t show_atmaddress(struct device *cdev,

net/atm/atm_sysfs.c +5 -5
 			       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",

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
#6 lore raw
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.

author
#7 lore raw
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
author
#8 lore raw
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_SIZE in 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
author
#9 lore raw
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_SIZE in 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
#10 lore raw
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_SIZE in 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.

author
#11 lore raw
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_SIZE in 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
author
#12 lore raw

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_SIZE in 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
#13 lore raw
In reply to Chen Gang

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.

author
#14 lore raw
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
author
#15 lore raw

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