* [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.