linux-kernel - Re: [PATCH 2/9] fortify: Allow KUnit test to build without FORTIFY

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: <CAGS_qxpk8WsPjN702nhQcEsK4yzzXZCc5n5cTVMSpwnVhCHSvA@mail.gmail.com>
Date:   Wed, 5 Apr 2023 18:22:25 -0700
From:   Daniel Latypov <dlatypov@...gle.com>
To:     Kees Cook <keescook@...omium.org>
Cc:     linux-hardening@...r.kernel.org, Kees Cook <kees@...flux.net>,
        Andy Shevchenko <andy@...nel.org>,
        Cezary Rojewski <cezary.rojewski@...el.com>,
        Puyou Lu <puyou.lu@...il.com>, Mark Brown <broonie@...nel.org>,
        Josh Poimboeuf <jpoimboe@...nel.org>,
        Peter Zijlstra <peterz@...radead.org>,
        Brendan Higgins <brendan.higgins@...ux.dev>,
        David Gow <davidgow@...gle.com>,
        Andrew Morton <akpm@...ux-foundation.org>,
        Nathan Chancellor <nathan@...nel.org>,
        Alexander Potapenko <glider@...gle.com>,
        Zhaoyang Huang <zhaoyang.huang@...soc.com>,
        Randy Dunlap <rdunlap@...radead.org>,
        Geert Uytterhoeven <geert+renesas@...der.be>,
        Miguel Ojeda <ojeda@...nel.org>,
        Nick Desaulniers <ndesaulniers@...gle.com>,
        Liam Howlett <liam.howlett@...cle.com>,
        Vlastimil Babka <vbabka@...e.cz>,
        Dan Williams <dan.j.williams@...el.com>,
        Rasmus Villemoes <linux@...musvillemoes.dk>,
        Yury Norov <yury.norov@...il.com>,
        "Jason A. Donenfeld" <Jason@...c4.com>,
        Sander Vanheule <sander@...nheule.net>,
        Eric Biggers <ebiggers@...gle.com>,
        "Masami Hiramatsu (Google)" <mhiramat@...nel.org>,
        Andrey Konovalov <andreyknvl@...il.com>,
        Linus Walleij <linus.walleij@...aro.org>,
        José Expósito <jose.exposito89@...il.com>,
        linux-kernel@...r.kernel.org, kunit-dev@...glegroups.com
Subject: Re: [PATCH 2/9] fortify: Allow KUnit test to build without FORTIFY

On Wed, Apr 5, 2023 at 5:08 PM Kees Cook <keescook@...omium.org> wrote:
>
> From: Kees Cook <kees@...flux.net>
>
> In order for CI systems to notice all the skipped tests related to
> CONFIG_FORTIFY_SOURCE, allow the FORTIFY_SOURCE KUnit tests to build
> with or without CONFIG_FORTIFY_SOURCE.

Hmm, I wonder if this warrants a deeper discussion.
It's a lot easier to have tests get disabled by kconfig if their deps
aren't met.

If there's pressure to have them compiled and just get marked skipped,
that sounds like that could be annoying.
Esp. in the cases where more code needs to be put behind

#ifdef CONFIG_MY_DEP
<test helpers, etc>
#endif

But I have a suggestion below to simplify this a bit

>
> Signed-off-by: Kees Cook <kees@...flux.net>
> ---
>  lib/Kconfig.debug   |  2 +-
>  lib/fortify_kunit.c | 15 +++++++++++++++
>  2 files changed, 16 insertions(+), 1 deletion(-)
>
> diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug
> index c8b379e2e9ad..d48a5f4b471e 100644
> --- a/lib/Kconfig.debug
> +++ b/lib/Kconfig.debug
> @@ -2614,7 +2614,7 @@ config STACKINIT_KUNIT_TEST
>
>  config FORTIFY_KUNIT_TEST
>         tristate "Test fortified str*() and mem*() function internals at runtime" if !KUNIT_ALL_TESTS
> -       depends on KUNIT && FORTIFY_SOURCE
> +       depends on KUNIT
>         default KUNIT_ALL_TESTS
>         help
>           Builds unit tests for checking internals of FORTIFY_SOURCE as used
> diff --git a/lib/fortify_kunit.c b/lib/fortify_kunit.c
> index c8c33cbaae9e..d054fc20a7d5 100644
> --- a/lib/fortify_kunit.c
> +++ b/lib/fortify_kunit.c
> @@ -25,8 +25,21 @@ static const char array_of_10[] = "this is 10";
>  static const char *ptr_of_11 = "this is 11!";
>  static char array_unknown[] = "compiler thinks I might change";
>
> +/* Handle being built without CONFIG_FORTIFY_SOURCE */
> +#ifndef __compiletime_strlen
> +# define __compiletime_strlen __builtin_strlen
> +#endif
> +
> +#define skip_without_fortify() \
> +do {                           \
> +       if (!IS_ENABLED(CONFIG_FORTIFY_SOURCE)) \
> +               kunit_skip(test, "Not built with CONFIG_FORTIFY_SOURCE=y"); \
> +} while (0)

Note: you can add an `init` function to the suite and skip the tests there.

static void fortify_init(struct kunit *test) {
       if (!IS_ENABLED(CONFIG_FORTIFY_SOURCE))
               kunit_skip(test, "Not built with CONFIG_FORTIFY_SOURCE=y");
}

...
  static struct kunit_suite fortify_test_suite = {
          .name = "fortify",
+         .init = fortify_init,
          .test_cases = fortify_test_cases,
  };

That way we don't have to add it to each test case.