diff mbox series

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

Message ID 20230406000212.3442647-2-keescook@chromium.org
State New
Headers
Series fortify: Add KUnit tests for runtime overflows |

Commit Message

Kees Cook April 6, 2023, 12:02 a.m. UTC 
  From: Kees Cook <kees@outflux.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.

Signed-off-by: Kees Cook <kees@outflux.net>
---
 lib/Kconfig.debug   |  2 +-
 lib/fortify_kunit.c | 15 +++++++++++++++
 2 files changed, 16 insertions(+), 1 deletion(-)

Comments

Daniel Latypov April 6, 2023, 1:22 a.m. UTC | #1 
On Wed, Apr 5, 2023 at 5:08 PM Kees Cook <keescook@chromium.org> wrote:
>
> From: Kees Cook <kees@outflux.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@outflux.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.
Kees Cook April 6, 2023, 11:09 p.m. UTC | #2 
On Wed, Apr 05, 2023 at 06:22:25PM -0700, Daniel Latypov wrote:
> On Wed, Apr 5, 2023 at 5:08 PM Kees Cook <keescook@chromium.org> wrote:
> > 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.

Yeah, I wasn't sure where to put the "kunit defconfig" settings.
"default.config" didn't seem to actually work as I was expecting. The
real "problem" I'm solving is that FORTIFY_SOURCE isn't in the standard
defconfig.

> 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@outflux.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.

Ah! Excellent. I didn't realize it would have that effect. I will do
that. :)
diff mbox series

Patch

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)
+
 static void known_sizes_test(struct kunit *test)
 {
+	skip_without_fortify();
+
 	KUNIT_EXPECT_EQ(test, __compiletime_strlen("88888888"), 8);
 	KUNIT_EXPECT_EQ(test, __compiletime_strlen(array_of_10), 10);
 	KUNIT_EXPECT_EQ(test, __compiletime_strlen(ptr_of_11), 11);
@@ -60,6 +73,8 @@  static noinline size_t want_minus_one(int pick)
 
 static void control_flow_split_test(struct kunit *test)
 {
+	skip_without_fortify();
+
 	KUNIT_EXPECT_EQ(test, want_minus_one(pick), SIZE_MAX);
 }