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

[Daniel Latypov]

On Wed, Apr 5, 2023 at 5:08 PM Kees Cook <keescook@xxxxxxxxxxxx> wrote:
>
> From: Kees Cook <kees@xxxxxxxxxxx>
>
> 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@xxxxxxxxxxx>
> ---
>  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.