| Message ID | 20230406000212.3442647-2-keescook@chromium.org (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | fortify: Add KUnit tests for runtime overflows | expand |
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.
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 --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); }