linux-kernel - Re: [PATCH] kunit: tool: add null pointer check

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Tue, 29 Mar 2022 17:32:17 -0400
From:   Brendan Higgins <brendanhiggins@...gle.com>
To:     Daniel Latypov <dlatypov@...gle.com>
Cc:     cgel.zte@...il.com, linux-kselftest@...r.kernel.org,
        kunit-dev@...glegroups.com, linux-kernel@...r.kernel.org,
        Lv Ruyi <lv.ruyi@....com.cn>, Zeal Robot <zealci@....com.cn>
Subject: Re: [PATCH] kunit: tool: add null pointer check

On Tue, Mar 29, 2022 at 3:29 PM Daniel Latypov <dlatypov@...gle.com> wrote:
>
> On Tue, Mar 29, 2022 at 5:39 AM <cgel.zte@...il.com> wrote:
> >
> > From: Lv Ruyi <lv.ruyi@....com.cn>
> >
> > kmalloc and kcalloc is a memory allocation function which can return NULL
> > when some internal memory errors happen. Add null pointer check to avoid
> > dereferencing null pointer.
> >
> > Reported-by: Zeal Robot <zealci@....com.cn>
> > Signed-off-by: Lv Ruyi <lv.ruyi@....com.cn>
> > ---
> >  lib/kunit/executor.c | 4 ++++
> >  1 file changed, 4 insertions(+)
> >
> > diff --git a/lib/kunit/executor.c b/lib/kunit/executor.c
> > index 22640c9ee819..be21d0451367 100644
> > --- a/lib/kunit/executor.c
> > +++ b/lib/kunit/executor.c
> > @@ -71,9 +71,13 @@ kunit_filter_tests(struct kunit_suite *const suite, const char *test_glob)
> >
> >         /* Use memcpy to workaround copy->name being const. */
> >         copy = kmalloc(sizeof(*copy), GFP_KERNEL);
> > +       if (!copy)
> > +               return NULL;
>
> While this is technically correct to check, in this context it's less clear.
> If we can't allocate this memory, we likely can't run any subsequent
> tests, either because the test cases will want to allocate some memory
> and/or KUnit will need to allocate some for internal bookkeeping.
>
> The existing code (and by extension this patch) "handles" OOM
> situations by silently dropping test suites/cases.
> So I sort of intentionally figured we should let it crash early in
> this case since that's probably more debuggable.
>
> This code does check for NULL returns earlier on in the call chain, i.e.
>
> first in kunit_filter_suites()
>    158          copy = kmalloc_array(max, sizeof(*filtered.start), GFP_KERNEL);
>    159          filtered.start = copy;
>    160          if (!copy) { /* won't be able to run anything, return
> an empty set */
>    161                  filtered.end = copy;
>    162                  return filtered;
>    163          }
>
> and second in kunit_filter_subsuite()
>    107          filtered = kmalloc_array(n + 1, sizeof(*filtered), GFP_KERNEL);
>    108          if (!filtered)
>    109                  return NULL;
>
> The first kmalloc_array() is our first allocation in this file.
> If we can't handle that, then things are really going wrong, and I
> assumed there'd be plenty of debug messages in dmesg, so silently
> returning is probably fine.
> The second one also felt similar.
>
> So I think that
> * it's highly unlikely that we pass those checks and fail on these new
> ones (we're not allocating much)
> * if we do fail, this is now harder to debug since it's partially
> running tests, partially not
>
> Should we instead rework the code to more clearly signal allocation
> errors instead of overloading NULL to mean "no matches or error?"
> Or maybe just adding some pr_err() calls is sufficient.

I think we should either return an err ptr, or log something (maybe both).

But yeah, I agree with you Daniel, I don't like overloading NULL.

> >         memcpy(copy, suite, sizeof(*copy));
> >
> >         filtered = kcalloc(n + 1, sizeof(*filtered), GFP_KERNEL);
> > +       if (!filtered)
> > +               return NULL;
> >
> >         n = 0;
> >         kunit_suite_for_each_test_case(suite, test_case) {
> > --
> > 2.25.1
> >