linux-hardening - Re: [PATCH v2 1/2] string: Convert selftest to KUnit

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]

Message-ID: <CAHp75VdhY6-OmNYtbpnb+dv1Du=LTnignsTRSU9+8yqpbFqPsQ@mail.gmail.com>
Date: Fri, 1 Mar 2024 13:09:27 +0200
From: Andy Shevchenko <andy.shevchenko@...il.com>
To: Kees Cook <keescook@...omium.org>
Cc: Andy Shevchenko <andy@...nel.org>, linux-hardening@...r.kernel.org, 
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 1/2] string: Convert selftest to KUnit

On Fri, Mar 1, 2024 at 2:26 AM Kees Cook <keescook@...omium.org> wrote:
>
> Convert test_string.c to KUnit so it can be easily run with everything
> else.

Have you run it?

...

>         if (i < 256)
> -               return (i << 24) | (j << 16) | k | 0x8000;
> -       return 0;
> +               KUNIT_EXPECT_EQ(test, 0, (i << 24) | (j << 16) | k | 0x8000);

First of all, this special value encodes the problematic patterns, so
you missed proper messaging.
Second, the returned value has a constant, how do you expect 0 to be
equal to something (guaranteed not to be 0)?

This needs a good rethink of what you should do in the KUnit approach.

...

> +               KUNIT_EXPECT_EQ(test, 0, (i << 24) | (j << 16) | k | 0x8000);

Ditto.

...

> +               KUNIT_EXPECT_EQ(test, 0, (i << 24) | (j << 16) | k | 0x8000);

Ditto.

...

>         for (i = 0; i < strlen(test_string) + 1; i++) {
>                 result = strchr(test_string, test_string[i]);
> -               if (result - test_string != i)
> -                       return i + 'a';
> +               KUNIT_ASSERT_EQ(test, result - test_string, i);

In a similar way, all returned values are *special*, you really need
to think about them before converting to a simple (and sometimes
wrong) checks)

...

I dunno if KUnit has a fault ejection simulation. It should, in order
to be sure that test cases are fine when they fail.

-- 
With Best Regards,
Andy Shevchenko