Re: [lustre-devel] [bug report] staging: add Lustre file system client support
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 ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [lustre-devel] [bug report] staging: add Lustre file system client support
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 ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [lustre-devel] [bug report] staging: add Lustre file system client support
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? ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [lustre-devel] [bug report] staging: add Lustre file system client support
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 ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [lustre-devel] [bug report] staging: add Lustre file system client support
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.
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [lustre-devel] [bug report] staging: add Lustre file system client support
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
Re: [lustre-devel] [bug report] staging: add Lustre file system client support
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(>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(>tes_param[0], param, paramlen);
> 1337 }
> 1338
>
> regards,
> dan carpenter
> ___
> lustre-devel mailing list
> lustre-de...@lists.lustre.org
> http://lists.lustre.org/listinfo.cgi/lustre-devel-lustre.org
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
