Date:   Thu, 3 Aug 2023 15:39:25 -0400
From:   Rae Moar <rmoar@...gle.com>
To:     David Gow <davidgow@...gle.com>
Cc:     shuah@...nel.org, brendan.higgins@...ux.dev, ruanjinjie@...wei.com,
        linux-kselftest@...r.kernel.org, kunit-dev@...glegroups.com,
        linux-kernel@...r.kernel.org, kernel test robot <lkp@...el.com>,
        Dan Carpenter <dan.carpenter@...aro.org>
Subject: Re: [PATCH -next] kunit: fix uninitialized variables bug in
 attributes filtering

On Thu, Aug 3, 2023 at 4:15 AM David Gow <davidgow@...gle.com> wrote:
>
> On Thu, 3 Aug 2023 at 05:28, Rae Moar <rmoar@...gle.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@...el.com>
> > Reported-by: Dan Carpenter <dan.carpenter@...aro.org>
> > Closes: https://lore.kernel.org/r/202307270610.s0w4NKEn-lkp@intel.com/
> >
> > Signed-off-by: Rae Moar <rmoar@...gle.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 = -1;
> >         long test_val = (long)attr;
> >         const char *input_val = NULL;
> >
> > @@ -124,7 +124,7 @@ static int attr_enum_filter(void *attr, const char *input, int *err,
> >                         input_int = j;
> >         }
> >
> > -       if (!input_int) {
> > +       if (input_int < 0) {
> >                 *err = -EINVAL;
> >                 pr_err("kunit executor: invalid filter input: %s\n", input);
> >                 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_attr_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 = false;
> >         void *attr;
> >         const char *attr_name, *attr_str;
> >         struct kunit_suite *suite = 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 = 0;
> > +       int i, comma_index = 0, count = 0;
> >
> >         for (i = 0; input[i]; i++) {
> >                 if (input[i] == ',') {
> > @@ -272,7 +274,7 @@ int kunit_get_filter_count(char *input)
> >  struct kunit_attr_filter kunit_next_attr_filter(char **filters, int *err)
> >  {
> >         struct kunit_attr_filter filter = {};
> > -       int i, j, comma_index, new_start_index;
> > +       int i, j, comma_index = 0, new_start_index = 0;
> >         int op_index = -1, attr_index = -1;
> >         char op;
> >         char *input = *filters;
> > @@ -316,7 +318,7 @@ struct kunit_attr_filter kunit_next_attr_filter(char **filters, int *err)
> >                 filter.attr = &kunit_attr_list[attr_index];
> >         }
> >
> > -       if (comma_index) {
> > +       if (comma_index > 0) {
> >                 input[comma_index] = '\0';
> >                 filter.input = input + op_index;
> >                 input = 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 = filter.attr->filter(filter.attr->attr_default, filter.input, err);
> > -       if (*err) {
> > -               kfree(copy);
> > -               kfree(filtered);
> > -               return NULL;
> > -       }
> > +       if (*err)
> > +               goto err;
> >
> >         /* Save suite attribute value and filtering result on that value */
> >         suite_val = filter.attr->get_attr((void *)suite, false);
> >         suite_result = filter.attr->filter(suite_val, filter.input, err);
> > -       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 = filter.attr->get_attr((void *) test_case, true);
> >                 test_result = filter.attr->filter(filter.attr->get_attr(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 that value.
> > @@ -406,7 +399,8 @@ struct kunit_suite *kunit_filter_attr_tests(const struct kunit_suite *const suit
> >                 }
> >         }
> >
> > -       if (n == 0) {
> > +err:
> > +       if (n == 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 struct 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 = NULL;
> >
> >         const size_t max = 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 = kunit_get_filter_count(filters);
> > -               parsed_filters = kcalloc(filter_count + 1, sizeof(*parsed_filters), GFP_KERNEL);
> > +               parsed_filters = 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 = 0; j < filter_count; j++)
> >                         parsed_filters[j] = kunit_next_attr_filter(&filters, err);
> >                 if (*err)
> > @@ -166,7 +170,7 @@ static struct suite_set kunit_filter_suites(const struct suite_set *suite_set,
> >                                 goto err;
> >                         }
> >                 }
> > -               if (filter_count) {
> > +               if (filter_count > 0 && parsed_filters != NULL) {
> >                         for (k = 0; k < filter_count; k++) {
> >                                 new_filtered_suite = kunit_filter_attr_tests(filtered_suite,
> >                                                 parsed_filters[k], filter_action, err);
> >
> > base-commit: 3bffe185ad11e408903d2782727877388d08d94e
> > --
> > 2.41.0.585.gd2178a4bd4-goog
> >

Powered by blists - more mailing lists