* [lustre-devel] [bug report] staging: add Lustre file system client support
@ 2016-11-23 12:29 Dan Carpenter
2016-12-05 23:43 ` Oleg Drokin
0 siblings, 1 reply; 8+ messages in thread
From: Dan Carpenter @ 2016-11-23 12:29 UTC (permalink / raw)
To: lustre-devel
Hi Lustre Devs,
The patch d7e09d0397e8: "staging: add Lustre file system client
support" from May 2, 2013, leads to the following static checker
warning:
drivers/staging/lustre/lnet/selftest/console.c:1336 lstcon_test_add()
error: 'paramlen' from user is not capped properly
The story here, is that "paramlen" is capped but only if "param" is
non-NULL. This causes a problem.
drivers/staging/lustre/lnet/selftest/console.c
1311
1312 LIBCFS_ALLOC(test, offsetof(struct lstcon_test, tes_param[paramlen]));
We don't know that paramlen is non-NULL here. Because of integer
overflows we could end up allocating less than intended.
1313 if (!test) {
1314 CERROR("Can't allocate test descriptor\n");
1315 rc = -ENOMEM;
1316
1317 goto out;
1318 }
1319
1320 test->tes_hdr.tsb_id = batch->bat_hdr.tsb_id;
Which will lead to memory corruption when we use "test".
1321 test->tes_batch = batch;
1322 test->tes_type = type;
1323 test->tes_oneside = 0; /* TODO */
1324 test->tes_loop = loop;
1325 test->tes_concur = concur;
1326 test->tes_stop_onerr = 1; /* TODO */
1327 test->tes_span = span;
1328 test->tes_dist = dist;
1329 test->tes_cliidx = 0; /* just used for creating RPC */
1330 test->tes_src_grp = src_grp;
1331 test->tes_dst_grp = dst_grp;
1332 INIT_LIST_HEAD(&test->tes_trans_list);
1333
1334 if (param) {
Smatch is not smart enough to trace the implication that "'param' is
non-NULL, means that 'paramlen' has been verified" across a function
boundary. Storing that sort of information would really increase the
hardware requirements for running Smatch so it's not something I have
planned currently.
1335 test->tes_paramlen = paramlen;
1336 memcpy(&test->tes_param[0], param, paramlen);
1337 }
1338
regards,
dan carpenter
^ permalink raw reply [flat|nested] 8+ messages in thread* [lustre-devel] [bug report] staging: add Lustre file system client support 2016-11-23 12:29 [lustre-devel] [bug report] staging: add Lustre file system client support Dan Carpenter @ 2016-12-05 23:43 ` Oleg Drokin 2016-12-06 11:02 ` Dan Carpenter 0 siblings, 1 reply; 8+ messages in thread From: Oleg Drokin @ 2016-12-05 23:43 UTC (permalink / raw) To: lustre-devel On Nov 23, 2016, at 7:29 AM, Dan Carpenter wrote: > Hi Lustre Devs, > > The patch d7e09d0397e8: "staging: add Lustre file system client > support" from May 2, 2013, leads to the following static checker > warning: > > drivers/staging/lustre/lnet/selftest/console.c:1336 lstcon_test_add() > error: 'paramlen' from user is not capped properly > > The story here, is that "paramlen" is capped but only if "param" is > non-NULL. This causes a problem. > > drivers/staging/lustre/lnet/selftest/console.c > 1311 > 1312 LIBCFS_ALLOC(test, offsetof(struct lstcon_test, tes_param[paramlen])); > > We don't know that paramlen is non-NULL here. Because of integer > overflows we could end up allocating less than intended. I think this must be a false positive in this case? Before calling this function we do: LIBCFS_ALLOC(param, args->lstio_tes_param_len); in lst_test_add_ioctl(), so it's not any bigger than 128k (or kmalloc will fail). Even if kmalloc would allow more than 128k allocations, offsetof(struct lstcon_test, tes_param[0]) is bound to be a lot smaller than the baseline allocation address for kmalloc, and therefore integer overflow cannot happen at all. > > 1313 if (!test) { > 1314 CERROR("Can't allocate test descriptor\n"); > 1315 rc = -ENOMEM; > 1316 > 1317 goto out; > 1318 } > 1319 > 1320 test->tes_hdr.tsb_id = batch->bat_hdr.tsb_id; > > Which will lead to memory corruption when we use "test". > > 1321 test->tes_batch = batch; > 1322 test->tes_type = type; > 1323 test->tes_oneside = 0; /* TODO */ > 1324 test->tes_loop = loop; > 1325 test->tes_concur = concur; > 1326 test->tes_stop_onerr = 1; /* TODO */ > 1327 test->tes_span = span; > 1328 test->tes_dist = dist; > 1329 test->tes_cliidx = 0; /* just used for creating RPC */ > 1330 test->tes_src_grp = src_grp; > 1331 test->tes_dst_grp = dst_grp; > 1332 INIT_LIST_HEAD(&test->tes_trans_list); > 1333 > 1334 if (param) { > > Smatch is not smart enough to trace the implication that "'param' is > non-NULL, means that 'paramlen' has been verified" across a function > boundary. Storing that sort of information would really increase the > hardware requirements for running Smatch so it's not something I have > planned currently. > > 1335 test->tes_paramlen = paramlen; > 1336 memcpy(&test->tes_param[0], param, paramlen); > 1337 } > 1338 > > regards, > dan carpenter > _______________________________________________ > lustre-devel mailing list > lustre-devel at lists.lustre.org > http://lists.lustre.org/listinfo.cgi/lustre-devel-lustre.org ^ permalink raw reply [flat|nested] 8+ messages in thread
* [lustre-devel] [bug report] staging: add Lustre file system client support 2016-12-05 23:43 ` Oleg Drokin @ 2016-12-06 11:02 ` Dan Carpenter 2016-12-06 15:44 ` Oleg Drokin 0 siblings, 1 reply; 8+ messages in thread From: Dan Carpenter @ 2016-12-06 11:02 UTC (permalink / raw) To: lustre-devel On Mon, Dec 05, 2016 at 06:43:37PM -0500, Oleg Drokin wrote: > > On Nov 23, 2016, at 7:29 AM, Dan Carpenter wrote: > > > Hi Lustre Devs, > > > > The patch d7e09d0397e8: "staging: add Lustre file system client > > support" from May 2, 2013, leads to the following static checker > > warning: > > > > drivers/staging/lustre/lnet/selftest/console.c:1336 lstcon_test_add() > > error: 'paramlen' from user is not capped properly > > > > The story here, is that "paramlen" is capped but only if "param" is > > non-NULL. This causes a problem. > > > > drivers/staging/lustre/lnet/selftest/console.c > > 1311 > > 1312 LIBCFS_ALLOC(test, offsetof(struct lstcon_test, tes_param[paramlen])); > > > > We don't know that paramlen is non-NULL here. Because of integer > > overflows we could end up allocating less than intended. > > I think this must be a false positive in this case? > > Before calling this function we do: > LIBCFS_ALLOC(param, args->lstio_tes_param_len); > > in lst_test_add_ioctl(), so it's not any bigger than 128k (or kmalloc will fail). > Even if kmalloc would allow more than 128k allocations, > offsetof(struct lstcon_test, tes_param[0]) is bound to be a lot smaller than > the baseline allocation address for kmalloc, and therefore integer overflow > cannot happen at all. > I explained badly, and I typed the wrong variable names by mistake... Here is the relevant code from the caller: drivers/staging/lustre/lnet/selftest/conctl.c 710 static int lst_test_add_ioctl(lstio_test_args_t *args) 711 { 712 char *batch_name; 713 char *src_name = NULL; 714 char *dst_name = NULL; 715 void *param = NULL; 716 int ret = 0; 717 int rc = -ENOMEM; 718 719 if (!args->lstio_tes_resultp || 720 !args->lstio_tes_retp || 721 !args->lstio_tes_bat_name || /* no specified batch */ 722 args->lstio_tes_bat_nmlen <= 0 || 723 args->lstio_tes_bat_nmlen > LST_NAME_SIZE || 724 !args->lstio_tes_sgrp_name || /* no source group */ 725 args->lstio_tes_sgrp_nmlen <= 0 || 726 args->lstio_tes_sgrp_nmlen > LST_NAME_SIZE || 727 !args->lstio_tes_dgrp_name || /* no target group */ 728 args->lstio_tes_dgrp_nmlen <= 0 || 729 args->lstio_tes_dgrp_nmlen > LST_NAME_SIZE) 730 return -EINVAL; 731 732 if (!args->lstio_tes_loop || /* negative is infinite */ 733 args->lstio_tes_concur <= 0 || 734 args->lstio_tes_dist <= 0 || 735 args->lstio_tes_span <= 0) 736 return -EINVAL; 737 738 /* have parameter, check if parameter length is valid */ 739 if (args->lstio_tes_param && 740 (args->lstio_tes_param_len <= 0 || 741 args->lstio_tes_param_len > 742 PAGE_SIZE - sizeof(struct lstcon_test))) 743 return -EINVAL; If we don't have a parameter then we don't check ->lstio_tes_param_len. 744 745 LIBCFS_ALLOC(batch_name, args->lstio_tes_bat_nmlen + 1); 746 if (!batch_name) 747 return rc; 748 749 LIBCFS_ALLOC(src_name, args->lstio_tes_sgrp_nmlen + 1); 750 if (!src_name) 751 goto out; 752 753 LIBCFS_ALLOC(dst_name, args->lstio_tes_dgrp_nmlen + 1); 754 if (!dst_name) 755 goto out; 756 757 if (args->lstio_tes_param) { 758 LIBCFS_ALLOC(param, args->lstio_tes_param_len); 759 if (!param) 760 goto out; 761 if (copy_from_user(param, args->lstio_tes_param, 762 args->lstio_tes_param_len)) { 763 rc = -EFAULT; 764 goto out; 765 } 766 } This is the code that you mentioned. But we don't have a parameter so it's not invoked. 767 768 rc = -EFAULT; 769 if (copy_from_user(batch_name, args->lstio_tes_bat_name, 770 args->lstio_tes_bat_nmlen) || 771 copy_from_user(src_name, args->lstio_tes_sgrp_name, 772 args->lstio_tes_sgrp_nmlen) || 773 copy_from_user(dst_name, args->lstio_tes_dgrp_name, 774 args->lstio_tes_dgrp_nmlen)) 775 goto out; 776 777 rc = lstcon_test_add(batch_name, args->lstio_tes_type, 778 args->lstio_tes_loop, args->lstio_tes_concur, 779 args->lstio_tes_dist, args->lstio_tes_span, 780 src_name, dst_name, param, 781 args->lstio_tes_param_len, 782 &ret, args->lstio_tes_resultp); Here we are using "args->lstio_tes_param_len" which could be any integer value. "param" is NULL. 783 784 if (ret) 785 rc = (copy_to_user(args->lstio_tes_retp, &ret, 786 sizeof(ret))) ? -EFAULT : 0; Now here is the code again from lstcon_test_add(): drivers/staging/lustre/lnet/selftest/console.c 1279 int 1280 lstcon_test_add(char *batch_name, int type, int loop, 1281 int concur, int dist, int span, 1282 char *src_name, char *dst_name, 1283 void *param, int paramlen, int *retp, "param" is NULL and "paramlen" is any integer value. 1284 struct list_head __user *result_up) 1285 { 1286 struct lstcon_test *test = NULL; 1287 int rc; 1288 struct lstcon_group *src_grp = NULL; 1289 struct lstcon_group *dst_grp = NULL; 1290 struct lstcon_batch *batch = NULL; 1291 1292 /* 1293 * verify that a batch of the given name exists, and the groups 1294 * that will be part of the batch exist and have at least one 1295 * active node 1296 */ 1297 rc = lstcon_verify_batch(batch_name, &batch); 1298 if (rc) 1299 goto out; 1300 1301 rc = lstcon_verify_group(src_name, &src_grp); 1302 if (rc) 1303 goto out; 1304 1305 rc = lstcon_verify_group(dst_name, &dst_grp); 1306 if (rc) 1307 goto out; 1308 1309 if (dst_grp->grp_userland) 1310 *retp = 1; 1311 1312 LIBCFS_ALLOC(test, offsetof(struct lstcon_test, tes_param[paramlen])); If you pass "paramlen" as -4 then it will corrupt four bytes beyond the end of what we allocated. We could pass paramlen = -108 and it would return ZERO_SIZE_PTR (0x16) and oops. 1313 if (!test) { 1314 CERROR("Can't allocate test descriptor\n"); 1315 rc = -ENOMEM; 1316 1317 goto out; 1318 } 1319 1320 test->tes_hdr.tsb_id = batch->bat_hdr.tsb_id; 1321 test->tes_batch = batch; 1322 test->tes_type = type; 1323 test->tes_oneside = 0; /* TODO */ 1324 test->tes_loop = loop; 1325 test->tes_concur = concur; 1326 test->tes_stop_onerr = 1; /* TODO */ 1327 test->tes_span = span; 1328 test->tes_dist = dist; 1329 test->tes_cliidx = 0; /* just used for creating RPC */ 1330 test->tes_src_grp = src_grp; 1331 test->tes_dst_grp = dst_grp; 1332 INIT_LIST_HEAD(&test->tes_trans_list); 1333 regards, dan carpenter ^ permalink raw reply [flat|nested] 8+ messages in thread
* [lustre-devel] [bug report] staging: add Lustre file system client support 2016-12-06 11:02 ` Dan Carpenter @ 2016-12-06 15:44 ` Oleg Drokin 2016-12-06 18:37 ` Dan Carpenter 0 siblings, 1 reply; 8+ messages in thread From: Oleg Drokin @ 2016-12-06 15:44 UTC (permalink / raw) To: lustre-devel On Dec 6, 2016, at 6:02 AM, Dan Carpenter wrote: > On Mon, Dec 05, 2016 at 06:43:37PM -0500, Oleg Drokin wrote: >> >> On Nov 23, 2016, at 7:29 AM, Dan Carpenter wrote: >> >>> Hi Lustre Devs, >>> >>> The patch d7e09d0397e8: "staging: add Lustre file system client >>> support" from May 2, 2013, leads to the following static checker >>> warning: >>> >>> drivers/staging/lustre/lnet/selftest/console.c:1336 lstcon_test_add() >>> error: 'paramlen' from user is not capped properly >>> >>> The story here, is that "paramlen" is capped but only if "param" is >>> non-NULL. This causes a problem. >>> >>> drivers/staging/lustre/lnet/selftest/console.c >>> 1311 >>> 1312 LIBCFS_ALLOC(test, offsetof(struct lstcon_test, tes_param[paramlen])); >>> >>> We don't know that paramlen is non-NULL here. Because of integer >>> overflows we could end up allocating less than intended. >> >> I think this must be a false positive in this case? >> >> Before calling this function we do: >> LIBCFS_ALLOC(param, args->lstio_tes_param_len); >> >> in lst_test_add_ioctl(), so it's not any bigger than 128k (or kmalloc will fail). >> Even if kmalloc would allow more than 128k allocations, >> offsetof(struct lstcon_test, tes_param[0]) is bound to be a lot smaller than >> the baseline allocation address for kmalloc, and therefore integer overflow >> cannot happen at all. >> > > I explained badly, and I typed the wrong variable names by mistake... > Here is the relevant code from the caller: > > drivers/staging/lustre/lnet/selftest/conctl.c > 710 static int lst_test_add_ioctl(lstio_test_args_t *args) > 711 { > 712 char *batch_name; > 713 char *src_name = NULL; > 714 char *dst_name = NULL; > 715 void *param = NULL; > 716 int ret = 0; > 717 int rc = -ENOMEM; > 718 > 719 if (!args->lstio_tes_resultp || > 720 !args->lstio_tes_retp || > 721 !args->lstio_tes_bat_name || /* no specified batch */ > 722 args->lstio_tes_bat_nmlen <= 0 || > 723 args->lstio_tes_bat_nmlen > LST_NAME_SIZE || > 724 !args->lstio_tes_sgrp_name || /* no source group */ > 725 args->lstio_tes_sgrp_nmlen <= 0 || > 726 args->lstio_tes_sgrp_nmlen > LST_NAME_SIZE || > 727 !args->lstio_tes_dgrp_name || /* no target group */ > 728 args->lstio_tes_dgrp_nmlen <= 0 || > 729 args->lstio_tes_dgrp_nmlen > LST_NAME_SIZE) > 730 return -EINVAL; > 731 > 732 if (!args->lstio_tes_loop || /* negative is infinite */ > 733 args->lstio_tes_concur <= 0 || > 734 args->lstio_tes_dist <= 0 || > 735 args->lstio_tes_span <= 0) > 736 return -EINVAL; > 737 > 738 /* have parameter, check if parameter length is valid */ > 739 if (args->lstio_tes_param && > 740 (args->lstio_tes_param_len <= 0 || > 741 args->lstio_tes_param_len > > 742 PAGE_SIZE - sizeof(struct lstcon_test))) > 743 return -EINVAL; > > If we don't have a parameter then we don't check ->lstio_tes_param_len. I see, indeed, it all makes sense now. So basically if we unconditionally check for the size to be > 0, we should be fine then, I imagine. On the other hand there's probably no se for no param and nonzero param len, so it's probably even better to enforce size as zero when no param. Thank you. ^ permalink raw reply [flat|nested] 8+ messages in thread
* [lustre-devel] [bug report] staging: add Lustre file system client support 2016-12-06 15:44 ` Oleg Drokin @ 2016-12-06 18:37 ` Dan Carpenter 2016-12-06 19:10 ` Oleg Drokin 0 siblings, 1 reply; 8+ messages in thread From: Dan Carpenter @ 2016-12-06 18:37 UTC (permalink / raw) To: lustre-devel On Tue, Dec 06, 2016 at 10:44:54AM -0500, Oleg Drokin wrote: > I see, indeed, it all makes sense now. > So basically if we unconditionally check for the size to be > 0, we should be > fine then, I imagine. > On the other hand there's probably no se for no param and nonzero param len, > so it's probably even better to enforce size as zero when no param. Checking for > 0 is not enough, because it could also have an integer overflow on 32 bit systems. We need to cap the upper bound as well. regards, dan carpenter ^ permalink raw reply [flat|nested] 8+ messages in thread
* [lustre-devel] [bug report] staging: add Lustre file system client support 2016-12-06 18:37 ` Dan Carpenter @ 2016-12-06 19:10 ` Oleg Drokin 2016-12-06 19:19 ` Dan Carpenter 2016-12-06 20:14 ` Greg KH 0 siblings, 2 replies; 8+ messages in thread From: Oleg Drokin @ 2016-12-06 19:10 UTC (permalink / raw) To: lustre-devel On Dec 6, 2016, at 1:37 PM, Dan Carpenter wrote: > On Tue, Dec 06, 2016 at 10:44:54AM -0500, Oleg Drokin wrote: >> I see, indeed, it all makes sense now. >> So basically if we unconditionally check for the size to be > 0, we should be >> fine then, I imagine. >> On the other hand there's probably no se for no param and nonzero param len, >> so it's probably even better to enforce size as zero when no param. > > Checking for > 0 is not enough, because it could also have an integer > overflow on 32 bit systems. We need to cap the upper bound as well. How would it play out, though? offsetof(struct lstcon_test, tes_param[large_positive_int]) would result in some real "large" negative number. So trying to allocate this many negative bytes would fail, right? ^ permalink raw reply [flat|nested] 8+ messages in thread
* [lustre-devel] [bug report] staging: add Lustre file system client support 2016-12-06 19:10 ` Oleg Drokin @ 2016-12-06 19:19 ` Dan Carpenter 2016-12-06 20:14 ` Greg KH 1 sibling, 0 replies; 8+ messages in thread From: Dan Carpenter @ 2016-12-06 19:19 UTC (permalink / raw) To: lustre-devel On Tue, Dec 06, 2016 at 02:10:13PM -0500, Oleg Drokin wrote: > > On Dec 6, 2016, at 1:37 PM, Dan Carpenter wrote: > > > On Tue, Dec 06, 2016 at 10:44:54AM -0500, Oleg Drokin wrote: > >> I see, indeed, it all makes sense now. > >> So basically if we unconditionally check for the size to be > 0, we should be > >> fine then, I imagine. > >> On the other hand there's probably no se for no param and nonzero param len, > >> so it's probably even better to enforce size as zero when no param. > > > > Checking for > 0 is not enough, because it could also have an integer > > overflow on 32 bit systems. We need to cap the upper bound as well. > > How would it play out, though? > offsetof(struct lstcon_test, tes_param[large_positive_int]) would result in > some real "large" negative number. > So trying to allocate this many negative bytes would fail, right? Oh, yeah. You're right. Good point... regards, dan carpenter ^ permalink raw reply [flat|nested] 8+ messages in thread
* [lustre-devel] [bug report] staging: add Lustre file system client support 2016-12-06 19:10 ` Oleg Drokin 2016-12-06 19:19 ` Dan Carpenter @ 2016-12-06 20:14 ` Greg KH 1 sibling, 0 replies; 8+ messages in thread From: Greg KH @ 2016-12-06 20:14 UTC (permalink / raw) To: lustre-devel On Tue, Dec 06, 2016 at 02:10:13PM -0500, Oleg Drokin wrote: > > On Dec 6, 2016, at 1:37 PM, Dan Carpenter wrote: > > > On Tue, Dec 06, 2016 at 10:44:54AM -0500, Oleg Drokin wrote: > >> I see, indeed, it all makes sense now. > >> So basically if we unconditionally check for the size to be > 0, we should be > >> fine then, I imagine. > >> On the other hand there's probably no se for no param and nonzero param len, > >> so it's probably even better to enforce size as zero when no param. > > > > Checking for > 0 is not enough, because it could also have an integer > > overflow on 32 bit systems. We need to cap the upper bound as well. > > How would it play out, though? > offsetof(struct lstcon_test, tes_param[large_positive_int]) would result in > some real "large" negative number. > So trying to allocate this many negative bytes would fail, right? Not always. Please properly bound your allocations. thanks, greg k-h ^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2016-12-06 20:14 UTC | newest] Thread overview: 8+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2016-11-23 12:29 [lustre-devel] [bug report] staging: add Lustre file system client support Dan Carpenter 2016-12-05 23:43 ` Oleg Drokin 2016-12-06 11:02 ` Dan Carpenter 2016-12-06 15:44 ` Oleg Drokin 2016-12-06 18:37 ` Dan Carpenter 2016-12-06 19:10 ` Oleg Drokin 2016-12-06 19:19 ` Dan Carpenter 2016-12-06 20:14 ` Greg KH
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.