linux-kernel - Re: [PATCH v2] list: test: potential dereference of null pointer

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] [day] [month] [year] [list]

Date:   Fri, 22 Sep 2023 16:39:23 -0700
From:   Daniel Latypov <dlatypov@...gle.com>
To:     Ma Ke <make_ruc2021@....com>
Cc:     davidgow@...gle.com, linux-kselftest@...r.kernel.org,
        kunit-dev@...glegroups.com, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2] list: test: potential dereference of null pointer

On Fri, Sep 22, 2023 at 4:40 AM Ma Ke <make_ruc2021@....com> wrote:
>
> To avoid the failure of alloc, we could check the return value of
> kmalloc() and kzalloc().

Thanks, that's a good point, some suggestions below.
And also a question for David Gow whenever he sees this.

>
> Signed-off-by: Ma Ke <make_ruc2021@....com>
> ---
>  lib/list-test.c | 9 +++++++++
>  1 file changed, 9 insertions(+)
>
> diff --git a/lib/list-test.c b/lib/list-test.c
> index 0cc27de9cec8..70e898976dbf 100644
> --- a/lib/list-test.c
> +++ b/lib/list-test.c
> @@ -27,9 +27,18 @@ static void list_test_list_init(struct kunit *test)
>         INIT_LIST_HEAD(&list2);
>
>         list4 = kzalloc(sizeof(*list4), GFP_KERNEL | __GFP_NOFAIL);
> +       if (!list4) {
> +               KUNIT_FAIL(test, "Initialising list4 failed.\n");
> +               return;
> +       }

Note: we can replace this with a one-liner
  KUNIT_ASSERT_NOT_NULL(test, list4);

>         INIT_LIST_HEAD(list4);
>
>         list5 = kmalloc(sizeof(*list5), GFP_KERNEL | __GFP_NOFAIL);
> +       if (!list5) {
> +               kfree(list4);

We can also replace this check with the one-liner
  KUNIT_ASSERT_NOT_NULL(test, list5);

But we'd need to migrate the kzalloc() call to use kunit_kzalloc():
  kunit_kzalloc(test, sizeof(*list4), GFP_KERNEL | __GFP_NOFAIL);
that way we don't have to manually free list4.

I'm not sure why the original version didn't use the kunit helpers to
begin with.
Perhaps David would remember.

A quick lazy ^F over the original patch didn't find anything afaict,
https://lore.kernel.org/linux-kselftest/a127aeaa-e5ba-2d8d-0894-936e05637508@kernel.org/T/#u


> +               KUNIT_FAIL(test, "Initialising list5 failed.\n");
> +               return;
> +       }

Daniel