Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <20230802212834.761327-1-rmoar@google.com> <CABVgOSkGL2QhcQ7067NddajXGOJvjN88O7OjdTs3YY2xgMSZPQ@mail.gmail.com>
In-Reply-To: <CABVgOSkGL2QhcQ7067NddajXGOJvjN88O7OjdTs3YY2xgMSZPQ@mail.gmail.com>
From:   Rae Moar <rmoar@google.com>
Date:   Thu, 3 Aug 2023 15:39:25 -0400
Message-ID: <CA+GJov6MGXUsDjGa0TyoMVz7jrc+zT05KkHar63jj0gOppTKqA@mail.gmail.com>
Subject: Re: [PATCH -next] kunit: fix uninitialized variables bug in
 attributes filtering
To:     David Gow <davidgow@google.com>
Cc:     shuah@kernel.org, brendan.higgins@linux.dev, ruanjinjie@huawei.com,
        linux-kselftest@vger.kernel.org, kunit-dev@googlegroups.com,
        linux-kernel@vger.kernel.org, kernel test robot <lkp@intel.com>,
        Dan Carpenter <dan.carpenter@linaro.org>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

On Thu, Aug 3, 2023 at 4:15=E2=80=AFAM David Gow <davidgow@google.com> wrot=
e:
>
> On Thu, 3 Aug 2023 at 05:28, Rae Moar <rmoar@google.com> wrote:
> >
> > Fix smatch warnings regarding uninitialized variables in the filtering
> > patch of the new KUnit Attributes feature.
> >
> > Fixes: 529534e8cba3 ("kunit: Add ability to filter attributes")
> >
> > Reported-by: kernel test robot <lkp@intel.com>
> > Reported-by: Dan Carpenter <dan.carpenter@linaro.org>
> > Closes: https://lore.kernel.org/r/202307270610.s0w4NKEn-lkp@intel.com/
> >
> > Signed-off-by: Rae Moar <rmoar@google.com>
> > ---
>
> These fixes look good, especially the ones in attributes.c.
>
> There's still a possibility of returning uninitialised or freed
> pointers in executor.c. If we can keep 'filtered' valid at all times,
> this should be easier to deal with, e.g.:
>
> - Initialise 'filtered' to {NULL, NULL}, which is a valid "empty" value.
> - Only ever set start and end at the same time, so don't set 'start'
> immediately after allocation.
> - Wait until the filtering is complete and successful (i.e., where
> 'end' is set now), and set 'start' there as well.
> - Then return filtered will definitely either return the completely
> filtered value, or a valid empty suite_set.
>

Hi!

Great point. I will definitely change this. This is definitely a flaw
in the code. I have sent out a v2 of this patch with your suggested
changes above. Let me know what you think.

Thanks!
-Rae

> Otherwise, this looks good.
>
> -- David
>
> >
> > Note that this is rebased on top of the recent fix:
> > ("kunit: fix possible memory leak in kunit_filter_suites()").
> >
> >  lib/kunit/attributes.c | 40 +++++++++++++++++-----------------------
> >  lib/kunit/executor.c   | 10 +++++++---
> >  2 files changed, 24 insertions(+), 26 deletions(-)
> >
> > diff --git a/lib/kunit/attributes.c b/lib/kunit/attributes.c
> > index d37c40c0ce4f..5e3034b6be99 100644
> > --- a/lib/kunit/attributes.c
> > +++ b/lib/kunit/attributes.c
> > @@ -102,7 +102,7 @@ static int int_filter(long val, const char *op, int=
 input, int *err)
> >  static int attr_enum_filter(void *attr, const char *input, int *err,
> >                 const char * const str_list[], int max)
> >  {
> > -       int i, j, input_int;
> > +       int i, j, input_int =3D -1;
> >         long test_val =3D (long)attr;
> >         const char *input_val =3D NULL;
> >
> > @@ -124,7 +124,7 @@ static int attr_enum_filter(void *attr, const char =
*input, int *err,
> >                         input_int =3D j;
> >         }
> >
> > -       if (!input_int) {
> > +       if (input_int < 0) {
> >                 *err =3D -EINVAL;
> >                 pr_err("kunit executor: invalid filter input: %s\n", in=
put);
> >                 return false;
> > @@ -186,8 +186,10 @@ static void *attr_module_get(void *test_or_suite, =
bool is_test)
> >         // Suites get their module attribute from their first test_case
> >         if (test)
> >                 return ((void *) test->module_name);
> > -       else
> > +       else if (kunit_suite_num_test_cases(suite) > 0)
> >                 return ((void *) suite->test_cases[0].module_name);
> > +       else
> > +               return (void *) "";
> >  }
> >
> >  /* List of all Test Attributes */
> > @@ -221,7 +223,7 @@ const char *kunit_attr_filter_name(struct kunit_att=
r_filter filter)
> >  void kunit_print_attr(void *test_or_suite, bool is_test, unsigned int =
test_level)
> >  {
> >         int i;
> > -       bool to_free;
> > +       bool to_free =3D false;
> >         void *attr;
> >         const char *attr_name, *attr_str;
> >         struct kunit_suite *suite =3D is_test ? NULL : test_or_suite;
> > @@ -255,7 +257,7 @@ void kunit_print_attr(void *test_or_suite, bool is_=
test, unsigned int test_level
> >
> >  int kunit_get_filter_count(char *input)
> >  {
> > -       int i, comma_index, count =3D 0;
> > +       int i, comma_index =3D 0, count =3D 0;
> >
> >         for (i =3D 0; input[i]; i++) {
> >                 if (input[i] =3D=3D ',') {
> > @@ -272,7 +274,7 @@ int kunit_get_filter_count(char *input)
> >  struct kunit_attr_filter kunit_next_attr_filter(char **filters, int *e=
rr)
> >  {
> >         struct kunit_attr_filter filter =3D {};
> > -       int i, j, comma_index, new_start_index;
> > +       int i, j, comma_index =3D 0, new_start_index =3D 0;
> >         int op_index =3D -1, attr_index =3D -1;
> >         char op;
> >         char *input =3D *filters;
> > @@ -316,7 +318,7 @@ struct kunit_attr_filter kunit_next_attr_filter(cha=
r **filters, int *err)
> >                 filter.attr =3D &kunit_attr_list[attr_index];
> >         }
> >
> > -       if (comma_index) {
> > +       if (comma_index > 0) {
> >                 input[comma_index] =3D '\0';
> >                 filter.input =3D input + op_index;
> >                 input =3D input + new_start_index;
> > @@ -356,31 +358,22 @@ struct kunit_suite *kunit_filter_attr_tests(const=
 struct kunit_suite *const suit
> >
> >         /* Save filtering result on default value */
> >         default_result =3D filter.attr->filter(filter.attr->attr_defaul=
t, filter.input, err);
> > -       if (*err) {
> > -               kfree(copy);
> > -               kfree(filtered);
> > -               return NULL;
> > -       }
> > +       if (*err)
> > +               goto err;
> >
> >         /* Save suite attribute value and filtering result on that valu=
e */
> >         suite_val =3D filter.attr->get_attr((void *)suite, false);
> >         suite_result =3D filter.attr->filter(suite_val, filter.input, e=
rr);
> > -       if (*err) {
> > -               kfree(copy);
> > -               kfree(filtered);
> > -               return NULL;
> > -       }
> > +       if (*err)
> > +               goto err;
> >
> >         /* For each test case, save test case if passes filtering. */
> >         kunit_suite_for_each_test_case(suite, test_case) {
> >                 test_val =3D filter.attr->get_attr((void *) test_case, =
true);
> >                 test_result =3D filter.attr->filter(filter.attr->get_at=
tr(test_case, true),
> >                                 filter.input, err);
> > -               if (*err) {
> > -                       kfree(copy);
> > -                       kfree(filtered);
> > -                       return NULL;
> > -               }
> > +               if (*err)
> > +                       goto err;
> >
> >                 /*
> >                  * If attribute value of test case is set, filter on th=
at value.
> > @@ -406,7 +399,8 @@ struct kunit_suite *kunit_filter_attr_tests(const s=
truct kunit_suite *const suit
> >                 }
> >         }
> >
> > -       if (n =3D=3D 0) {
> > +err:
> > +       if (n =3D=3D 0 || *err) {
> >                 kfree(copy);
> >                 kfree(filtered);
> >                 return NULL;
> > diff --git a/lib/kunit/executor.c b/lib/kunit/executor.c
> > index 481901d245d0..b6e07de2876a 100644
> > --- a/lib/kunit/executor.c
> > +++ b/lib/kunit/executor.c
> > @@ -130,7 +130,7 @@ static struct suite_set kunit_filter_suites(const s=
truct suite_set *suite_set,
> >         struct kunit_suite **copy, *filtered_suite, *new_filtered_suite=
;
> >         struct suite_set filtered;
> >         struct kunit_glob_filter parsed_glob;
> > -       struct kunit_attr_filter *parsed_filters;
> > +       struct kunit_attr_filter *parsed_filters =3D NULL;
> >
> >         const size_t max =3D suite_set->end - suite_set->start;
> >
> > @@ -147,7 +147,11 @@ static struct suite_set kunit_filter_suites(const =
struct suite_set *suite_set,
> >         /* Parse attribute filters */
> >         if (filters) {
> >                 filter_count =3D kunit_get_filter_count(filters);
> > -               parsed_filters =3D kcalloc(filter_count + 1, sizeof(*pa=
rsed_filters), GFP_KERNEL);
> > +               parsed_filters =3D kcalloc(filter_count, sizeof(*parsed=
_filters), GFP_KERNEL);
> > +               if (!parsed_filters) {
> > +                       kfree(copy);
> > +                       return filtered;
>
> Is 'filtered' properly initialised here?
> filtered.start is already set to 'copy' by this point (so, having
> freed 'copy', this would now be an invalid pointer).
> filtered.end is uninitialised.
>
> Can we instead initialise filtered to {NULL, NULL} at the start, and
> only set start and end after the filtering has succeeded?
>
> > +               }
> >                 for (j =3D 0; j < filter_count; j++)
> >                         parsed_filters[j] =3D kunit_next_attr_filter(&f=
ilters, err);
> >                 if (*err)
> > @@ -166,7 +170,7 @@ static struct suite_set kunit_filter_suites(const s=
truct suite_set *suite_set,
> >                                 goto err;
> >                         }
> >                 }
> > -               if (filter_count) {
> > +               if (filter_count > 0 && parsed_filters !=3D NULL) {
> >                         for (k =3D 0; k < filter_count; k++) {
> >                                 new_filtered_suite =3D kunit_filter_att=
r_tests(filtered_suite,
> >                                                 parsed_filters[k], filt=
er_action, err);
> >
> > base-commit: 3bffe185ad11e408903d2782727877388d08d94e
> > --
> > 2.41.0.585.gd2178a4bd4-goog
> >