| Message ID | 20230407192717.636137-2-keescook@chromium.org (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | fortify: Add KUnit tests for runtime overflows | expand |
Hi Kees, On Fri, Apr 7, 2023 at 9:27 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. > > Signed-off-by: Kees Cook <keescook@chromium.org> Thanks for your patch, which is now commit a9dc8d0442294b42 ("fortify: Allow KUnit test to build without FORTIFY") upstream. > --- 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 All other tests depend on the functionality they test. Which makes sense, as you only want to test the functionality that is available in the kernel you want to run. > 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..524132f33cf0 100644 > --- a/lib/fortify_kunit.c > +++ b/lib/fortify_kunit.c > @@ -307,6 +312,14 @@ DEFINE_ALLOC_SIZE_TEST_PAIR(kvmalloc) > } while (0) > DEFINE_ALLOC_SIZE_TEST_PAIR(devm_kmalloc) > > +static int fortify_test_init(struct kunit *test) > +{ > + if (!IS_ENABLED(CONFIG_FORTIFY_SOURCE)) > + kunit_skip(test, "Not built with CONFIG_FORTIFY_SOURCE=y"); I was greeted by this message, which wasn't that helpful, as CONFIG_FORTIFY_SOURCE depends on CONFIG_ARCH_HAS_FORTIFY_SOURCE, which is not available yet on all architectures. So I think the proper thing to do is to revert this patch. Thanks! > + > + return 0; > +} > + > static struct kunit_case fortify_test_cases[] = { > KUNIT_CASE(known_sizes_test), > KUNIT_CASE(control_flow_split_test), Gr{oetje,eeting}s, Geert
On Sun, Jul 02, 2023 at 05:07:05PM +0200, Geert Uytterhoeven wrote: > Hi Kees, > > On Fri, Apr 7, 2023 at 9:27 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. > > > > Signed-off-by: Kees Cook <keescook@chromium.org> > > Thanks for your patch, which is now commit a9dc8d0442294b42 > ("fortify: Allow KUnit test to build without FORTIFY") upstream. > > > --- 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 > > All other tests depend on the functionality they test. > Which makes sense, as you only want to test the functionality that is > available in the kernel you want to run. Yeah, that is true for 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..524132f33cf0 100644 > > --- a/lib/fortify_kunit.c > > +++ b/lib/fortify_kunit.c > > > @@ -307,6 +312,14 @@ DEFINE_ALLOC_SIZE_TEST_PAIR(kvmalloc) > > } while (0) > > DEFINE_ALLOC_SIZE_TEST_PAIR(devm_kmalloc) > > > > +static int fortify_test_init(struct kunit *test) > > +{ > > + if (!IS_ENABLED(CONFIG_FORTIFY_SOURCE)) > > + kunit_skip(test, "Not built with CONFIG_FORTIFY_SOURCE=y"); > > I was greeted by this message, which wasn't that helpful, as > CONFIG_FORTIFY_SOURCE depends on CONFIG_ARCH_HAS_FORTIFY_SOURCE, > which is not available yet on all architectures. > > So I think the proper thing to do is to revert this patch. > Thanks! I created this patch so that I could add CONFIG_FORTIFY_SOURCE support to UML, but you have a good point about other archs. I'll prepare a revert.
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..524132f33cf0 100644 --- a/lib/fortify_kunit.c +++ b/lib/fortify_kunit.c @@ -25,6 +25,11 @@ 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 + static void known_sizes_test(struct kunit *test) { KUNIT_EXPECT_EQ(test, __compiletime_strlen("88888888"), 8); @@ -307,6 +312,14 @@ DEFINE_ALLOC_SIZE_TEST_PAIR(kvmalloc) } while (0) DEFINE_ALLOC_SIZE_TEST_PAIR(devm_kmalloc) +static int fortify_test_init(struct kunit *test) +{ + if (!IS_ENABLED(CONFIG_FORTIFY_SOURCE)) + kunit_skip(test, "Not built with CONFIG_FORTIFY_SOURCE=y"); + + return 0; +} + static struct kunit_case fortify_test_cases[] = { KUNIT_CASE(known_sizes_test), KUNIT_CASE(control_flow_split_test), @@ -323,6 +336,7 @@ static struct kunit_case fortify_test_cases[] = { static struct kunit_suite fortify_test_suite = { .name = "fortify", + .init = fortify_test_init, .test_cases = fortify_test_cases, };
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 <keescook@chromium.org> --- lib/Kconfig.debug | 2 +- lib/fortify_kunit.c | 14 ++++++++++++++ 2 files changed, 15 insertions(+), 1 deletion(-)