| Message ID | 20220224055145.1853657-1-keescook@chromium.org (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | lib: stackinit: Convert to KUnit | expand |
On Wed, Feb 23, 2022 at 9:51 PM Kees Cook <keescook@chromium.org> wrote: > <snip> > /* Userspace headers. */ > +#define _GNU_SOURCE > #include <stdio.h> > #include <stdint.h> > +#include <stdlib.h> > #include <string.h> > #include <stdbool.h> > #include <errno.h> > #include <sys/types.h> > > /* Linux kernel-ism stubs for stand-alone userspace build. */ This is neat and esp. so that it works. But may I ask, what's the value of using this vs UML? Given this has changed into mainly just a KUnit-compatibility layer, it feels like it can maybe live as a standalone file, if there's ever interest in doing this for other tests. It feels like something that will never quite be "supported", but I find it neat enough I'd have fun sending some patches to make it more realistic. > -#define KBUILD_MODNAME "stackinit" > -#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt > -#define pr_err(fmt, ...) fprintf(stderr, pr_fmt(fmt), ##__VA_ARGS__) > -#define pr_warn(fmt, ...) fprintf(stderr, pr_fmt(fmt), ##__VA_ARGS__) > -#define pr_info(fmt, ...) fprintf(stdout, pr_fmt(fmt), ##__VA_ARGS__) > -#define __init /**/ > -#define __exit /**/ > +#define TEST_PASS 0 > +#define TEST_SKIP 1 > +#define TEST_FAIL 2 > +struct kunit { > + int status; > + char *msg; > +}; > +struct kunit_case { > + void (*run_case)(struct kunit *test); > + const char *name; > +}; > +struct kunit_suite { > + const char *name; > + const struct kunit_case *test_cases; > +}; > +#define KUNIT_CASE(test_name) { .run_case = test_name, .name = #test_name } > + > +#define KUNIT_ASSERT_TRUE_MSG(test, expr, fmt, ...) \ > +do { \ > + if (!(expr)) { \ > + if (test->status != TEST_SKIP) \ > + test->status = TEST_FAIL; \ > + if (test->msg) \ > + free(test->msg); \ > + asprintf(&test->msg, fmt, ##__VA_ARGS__); \ > + } \ > +} while (0) This looks more like KUNIT_EXPECT_TRUE_MSG(), since this macro won't abort the test if the expectation fails. Looking at the code, it looks like we do want the ability to abort. Perhaps we can do what Googletest does in C++ and just add in a `return;`? It has some annoying implications, like using them from helper functions doesn't work as one would expect. But people seem to be doing fine with that tradeoff in C++ land. > + > +#define KUNIT_ASSERT_EQ_MSG(test, left, right, fmt, ...) \ > + KUNIT_ASSERT_TRUE_MSG(test, (left) == (right), fmt, ##__VA_ARGS__) Very optional: It might be nice to show the expressions automatically on failure. We could implement that via something like KUNIT_ASSERT_TRUE_MSG(test, (left) == (right), #left " != " #right ": " fmt, ##__VA_ARGS__); E.g. KUNIT_ASSERT_EQ_MSG(test, 2+2, 5, "math is broken") => 2+2 != 5: math is broken But I can see that being a bit too complicated to want to do here. And the failure messages we had before are already decent at giving context. > + > +#define kunit_skip(test, fmt, ...) \ > +do { \ > + test->status = TEST_SKIP; \ > + if (test->msg) \ > + free(test->msg); \ > + asprintf(&test->msg, fmt, ##__VA_ARGS__); \ > +} while (0) Similarly, this has no control flow implications, so the current semantics match kunit_mark_skipped(). But looking at the code, I think we want to abort early here too. > + > #define __user /**/ > #define noinline __attribute__((__noinline__)) > #define __aligned(x) __attribute__((__aligned__(x))) > @@ -59,16 +102,44 @@ typedef uint16_t u16; > typedef uint32_t u32; > typedef uint64_t u64; > > -#define module_init(func) static int (*do_init)(void) = func > -#define module_exit(func) static void (*do_exit)(void) = func > -#define MODULE_LICENSE(str) int main(void) { \ > - int rc; \ > - /* License: str */ \ > - rc = do_init(); \ > - if (rc == 0) \ > - do_exit(); \ > - return rc; \ > - } > +#define MODULE_LICENSE(str) /* */ > + > +int do_kunit_test_suite(struct kunit_suite *suite) > +{ > + const struct kunit_case *test_case; > + int rc = 0; > + > + for (test_case = suite->test_cases; test_case->run_case; test_case++) { > + struct kunit test = { }; > + > + test_case->run_case(&test); > + switch (test.status) { > + default: > + case TEST_FAIL: > + fprintf(stderr, "FAIL: %s%s%s", test_case->name, > + test.msg ? ": " : "", > + test.msg ?: "\n"); > + rc = 1; > + break; > + case TEST_SKIP: > + fprintf(stdout, "XFAIL: %s%s%s", test_case->name, > + test.msg ? ": " : "", > + test.msg ?: "\n"); > + break; > + case TEST_PASS: > + fprintf(stdout, "ok: %s\n", test_case->name); > + break; > + } > + if (test.msg) > + free(test.msg); > + } > + return rc; > +} > + > +#define kunit_test_suite(suite) \ > +int main(void) { \ > + return do_kunit_test_suite(&(suite)); \ > +} very optional: emulating kunit_test_suites() might be more future-proof here, if we ever want to setup more suites. There's little reason to do so right now given the lack of init and exit support. Like the stuff above, it wouldn't be hard to do, but I can see it not being worth the extra code, i.e. #define kunit_test_suites(suites...) \ int main(void) { static struct kunit_suite *suites =[] = { __VA_ARGS__ }; int i, ret = 0; for (i = 0; i < ARRAY_SIZE(suites); ++i) ret += do_kunit_test_suite(suites[i]); return ret; }
On Thu, Feb 24, 2022 at 11:43:40AM -0800, Daniel Latypov wrote: > On Wed, Feb 23, 2022 at 9:51 PM Kees Cook <keescook@chromium.org> wrote: > > > > <snip> > > > > /* Userspace headers. */ > > +#define _GNU_SOURCE > > #include <stdio.h> > > #include <stdint.h> > > +#include <stdlib.h> > > #include <string.h> > > #include <stdbool.h> > > #include <errno.h> > > #include <sys/types.h> > > > > /* Linux kernel-ism stubs for stand-alone userspace build. */ > > This is neat and esp. so that it works. > But may I ask, what's the value of using this vs UML? Mainly it's been for giving a single stand-alone file for testing to compiler devs, packagers, and distro maintainers instead of asking them to pull down the entire kernel, etc, etc. :) > Given this has changed into mainly just a KUnit-compatibility layer, > it feels like it can maybe live as a standalone file, if there's ever > interest in doing this for other tests. That's a terrifying and lovely idea! > It feels like something that will never quite be "supported", but I > find it neat enough I'd have fun sending some patches to make it more > realistic. Right, and as you found, I took some short-cuts that were specific to how this code used KUnit. :P I'll ponder this and go through your other suggestions. Thanks! -Kees
On Thu, Feb 24, 2022 at 1:51 PM Kees Cook <keescook@chromium.org> wrote: > > Convert to running under Kunit (and retain being able to run stand-alone > too). Building under Clang (or GCC 12) with CONFIG_INIT_STACK_ALL_ZERO=y, > this now passes as expected: > > $ ./tools/testing/kunit/kunit.py config --make_option LLVM=1 > $ ./tools/testing/kunit/kunit.py run overflow --make_option LLVM=1 \ > --kconfig_add CONFIG_INIT_STACK_ALL_ZERO=y Should this be "...kunit.py run 'stackinit'...", not overflow? > ... > [09:47:28] ================= stackinit (65 subtests) ================== > [09:47:28] [PASSED] test_u8_zero > [09:47:28] [PASSED] test_u16_zero > [09:47:28] [PASSED] test_u32_zero > [09:47:28] [PASSED] test_u64_zero > [09:47:28] [PASSED] test_char_array_zero > [09:47:28] [PASSED] test_small_hole_zero > [09:47:28] [PASSED] test_big_hole_zero > [09:47:28] [PASSED] test_trailing_hole_zero > [09:47:28] [PASSED] test_packed_zero > [09:47:28] [PASSED] test_small_hole_dynamic_partial > [09:47:28] [PASSED] test_big_hole_dynamic_partial > [09:47:28] [PASSED] test_trailing_hole_dynamic_partial > [09:47:28] [PASSED] test_packed_dynamic_partial > [09:47:28] [PASSED] test_small_hole_assigned_dynamic_partial > [09:47:28] [PASSED] test_big_hole_assigned_dynamic_partial > [09:47:28] [PASSED] test_trailing_hole_assigned_dynamic_partial > [09:47:28] [PASSED] test_packed_assigned_dynamic_partial > [09:47:28] [PASSED] test_small_hole_static_partial > [09:47:28] [PASSED] test_big_hole_static_partial > [09:47:28] [PASSED] test_trailing_hole_static_partial > [09:47:28] [PASSED] test_packed_static_partial > [09:47:28] [PASSED] test_small_hole_static_all > [09:47:28] [PASSED] test_big_hole_static_all > [09:47:28] [PASSED] test_trailing_hole_static_all > [09:47:28] [PASSED] test_packed_static_all > [09:47:28] [PASSED] test_small_hole_dynamic_all > [09:47:28] [PASSED] test_big_hole_dynamic_all > [09:47:28] [PASSED] test_trailing_hole_dynamic_all > [09:47:28] [PASSED] test_packed_dynamic_all > [09:47:28] [PASSED] test_small_hole_runtime_partial > [09:47:28] [PASSED] test_big_hole_runtime_partial > [09:47:28] [PASSED] test_trailing_hole_runtime_partial > [09:47:28] [PASSED] test_packed_runtime_partial > [09:47:28] [PASSED] test_small_hole_runtime_all > [09:47:28] [PASSED] test_big_hole_runtime_all > [09:47:28] [PASSED] test_trailing_hole_runtime_all > [09:47:28] [PASSED] test_packed_runtime_all > [09:47:28] [PASSED] test_small_hole_assigned_static_partial > [09:47:28] [PASSED] test_big_hole_assigned_static_partial > [09:47:28] [PASSED] test_trailing_hole_assigned_static_partial > [09:47:28] [PASSED] test_packed_assigned_static_partial > [09:47:28] [PASSED] test_small_hole_assigned_static_all > [09:47:28] [PASSED] test_big_hole_assigned_static_all > [09:47:28] [PASSED] test_trailing_hole_assigned_static_all > [09:47:28] [PASSED] test_packed_assigned_static_all > [09:47:28] [PASSED] test_small_hole_assigned_dynamic_all > [09:47:28] [PASSED] test_big_hole_assigned_dynamic_all > [09:47:28] [PASSED] test_trailing_hole_assigned_dynamic_all > [09:47:28] [PASSED] test_packed_assigned_dynamic_all > [09:47:28] [SKIPPED] test_small_hole_assigned_copy > [09:47:28] [SKIPPED] test_big_hole_assigned_copy > [09:47:28] [SKIPPED] test_trailing_hole_assigned_copy > [09:47:28] [PASSED] test_packed_assigned_copy > [09:47:28] [PASSED] test_u8_none > [09:47:28] [PASSED] test_u16_none > [09:47:28] [PASSED] test_u32_none > [09:47:28] [PASSED] test_u64_none > [09:47:28] [PASSED] test_char_array_none > [09:47:28] [SKIPPED] test_switch_1_none > [09:47:28] [SKIPPED] test_switch_2_none > [09:47:28] [PASSED] test_small_hole_none > [09:47:28] [PASSED] test_big_hole_none > [09:47:28] [PASSED] test_trailing_hole_none > [09:47:28] [PASSED] test_packed_none > [09:47:28] [PASSED] test_user > [09:47:28] ==================== [PASSED] stackinit ==================== > [09:47:28] ============================================================ > [09:47:28] Testing complete. Passed: 60, Failed: 0, Crashed: 0, Skipped: 5, Errors: 0 > [09:47:28] Elapsed time: 4.192s total, 0.001s configuring, 4.070s building, 0.102s running > > Cc: David Gow <davidgow@google.com> > Cc: Daniel Latypov <dlatypov@google.com> > Cc: Arnd Bergmann <arnd@arndb.de> > Signed-off-by: Kees Cook <keescook@chromium.org> > --- This looks pretty good as a KUnit test overall, even if it does some unusual (but cool!) things with the userland version. A few comments inline. Cheers, -- David > lib/Kconfig.debug | 26 +- > lib/Makefile | 4 +- > lib/{test_stackinit.c => stackinit_kunit.c} | 249 ++++++++++++-------- > 3 files changed, 168 insertions(+), 111 deletions(-) > rename lib/{test_stackinit.c => stackinit_kunit.c} (73%) > > diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug > index 14d90d03bc8d..ea4415275563 100644 > --- a/lib/Kconfig.debug > +++ b/lib/Kconfig.debug > @@ -2511,6 +2511,21 @@ config OVERFLOW_KUNIT_TEST > > If unsure, say N. > > +config STACKINIT_KUNIT_TEST > + tristate "Test level of stack variable initialization" if !KUNIT_ALL_TESTS > + depends on KUNIT > + default KUNIT_ALL_TESTS We probably don't want to enable this test by default (with KUNIT_ALL_TESTS) if we don't have CONFIG_INIT_STACK_ALL_ZERO=y, as that'll result in a number of failures for any "all tests" configuration. Does it make sense to either: 1. Make this test depend on INIT_STACK_ALL_ZERO? 2. Just change the default, so that KUNIT_ALL_TESTS doesn't trigger it (or doesn't trigger it unless INIT_STACK_ALL_ZERO is also enabled), so people can force it to build/run and see the failures? 3. Make it run, but mark some or all tests as SKIPPED if INIT_STACK_ALL_ZERO is not set (as a failure here is expected)? > + help > + Test if the kernel is zero-initializing stack variables and > + padding. Coverage is controlled by compiler flags, > + CONFIG_GCC_PLUGIN_STRUCTLEAK, CONFIG_GCC_PLUGIN_STRUCTLEAK_BYREF, > + or CONFIG_GCC_PLUGIN_STRUCTLEAK_BYREF_ALL. > + > + For more information on KUnit and unit tests in general please refer > + to the KUnit documentation in Documentation/dev-tools/kunit/. > + > + If unsure, say N. > + > config TEST_UDELAY > tristate "udelay test driver" > help > @@ -2602,17 +2617,6 @@ config TEST_OBJAGG > Enable this option to test object aggregation manager on boot > (or module load). > > - > -config TEST_STACKINIT > - tristate "Test level of stack variable initialization" > - help > - Test if the kernel is zero-initializing stack variables and > - padding. Coverage is controlled by compiler flags, > - CONFIG_GCC_PLUGIN_STRUCTLEAK, CONFIG_GCC_PLUGIN_STRUCTLEAK_BYREF, > - or CONFIG_GCC_PLUGIN_STRUCTLEAK_BYREF_ALL. > - > - If unsure, say N. > - > config TEST_MEMINIT > tristate "Test heap/page initialization" > help > diff --git a/lib/Makefile b/lib/Makefile > index fdfcbfaff32f..353bc09ce38d 100644 > --- a/lib/Makefile > +++ b/lib/Makefile > @@ -93,8 +93,6 @@ obj-$(CONFIG_TEST_KMOD) += test_kmod.o > obj-$(CONFIG_TEST_DEBUG_VIRTUAL) += test_debug_virtual.o > obj-$(CONFIG_TEST_MEMCAT_P) += test_memcat_p.o > obj-$(CONFIG_TEST_OBJAGG) += test_objagg.o > -CFLAGS_test_stackinit.o += $(call cc-disable-warning, switch-unreachable) > -obj-$(CONFIG_TEST_STACKINIT) += test_stackinit.o > obj-$(CONFIG_TEST_BLACKHOLE_DEV) += test_blackhole_dev.o > obj-$(CONFIG_TEST_MEMINIT) += test_meminit.o > obj-$(CONFIG_TEST_LOCKUP) += test_lockup.o > @@ -363,6 +361,8 @@ obj-$(CONFIG_CMDLINE_KUNIT_TEST) += cmdline_kunit.o > obj-$(CONFIG_SLUB_KUNIT_TEST) += slub_kunit.o > obj-$(CONFIG_MEMCPY_KUNIT_TEST) += memcpy_kunit.o > obj-$(CONFIG_OVERFLOW_KUNIT_TEST) += overflow_kunit.o FYI: Due to this context line, this patch only applies cleanly if the overflow test port patch is applied first. > +CFLAGS_stackinit_kunit.o += $(call cc-disable-warning, switch-unreachable) > +obj-$(CONFIG_STACKINIT_KUNIT_TEST) += stackinit_kunit.o > > obj-$(CONFIG_GENERIC_LIB_DEVMEM_IS_ALLOWED) += devmem_is_allowed.o > > diff --git a/lib/test_stackinit.c b/lib/stackinit_kunit.c > similarity index 73% > rename from lib/test_stackinit.c > rename to lib/stackinit_kunit.c > index a3c74e6a21ff..72c7d4cb8ed2 100644 > --- a/lib/test_stackinit.c > +++ b/lib/stackinit_kunit.c > @@ -2,14 +2,23 @@ > /* > * Test cases for compiler-based stack variable zeroing via > * -ftrivial-auto-var-init={zero,pattern} or CONFIG_GCC_PLUGIN_STRUCTLEAK*. > + * For example, see: > + * https://www.kernel.org/doc/html/latest/dev-tools/kunit/kunit-tool.html#configuring-building-and-running-tests > + * ./tools/testing/kunit/kunit.py config --make_option LLVM=1 > + * ./tools/testing/kunit/kunit.py run overflow [--raw_output] \ As above, this should probably be 'stackinit', not 'overflow'. > + * --make_option LLVM=1 \ > + * --kconfig_add CONFIG_INIT_STACK_ALL_ZERO=y > * > * External build example: > * clang -O2 -Wall -ftrivial-auto-var-init=pattern \ > - * -o test_stackinit test_stackinit.c > + * -o stackinit_kunit stackinit_kunit.c > + * ./stackinit_kunit > + * > */ > #ifdef __KERNEL__ > #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt > > +#include <kunit/test.h> > #include <linux/init.h> > #include <linux/kernel.h> > #include <linux/module.h> > @@ -18,21 +27,55 @@ > #else > > /* Userspace headers. */ > +#define _GNU_SOURCE > #include <stdio.h> > #include <stdint.h> > +#include <stdlib.h> > #include <string.h> > #include <stdbool.h> > #include <errno.h> > #include <sys/types.h> > > /* Linux kernel-ism stubs for stand-alone userspace build. */ > -#define KBUILD_MODNAME "stackinit" > -#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt > -#define pr_err(fmt, ...) fprintf(stderr, pr_fmt(fmt), ##__VA_ARGS__) > -#define pr_warn(fmt, ...) fprintf(stderr, pr_fmt(fmt), ##__VA_ARGS__) > -#define pr_info(fmt, ...) fprintf(stdout, pr_fmt(fmt), ##__VA_ARGS__) > -#define __init /**/ > -#define __exit /**/ > +#define TEST_PASS 0 > +#define TEST_SKIP 1 > +#define TEST_FAIL 2 > +struct kunit { > + int status; > + char *msg; > +}; > +struct kunit_case { > + void (*run_case)(struct kunit *test); > + const char *name; > +}; > +struct kunit_suite { > + const char *name; > + const struct kunit_case *test_cases; > +}; > +#define KUNIT_CASE(test_name) { .run_case = test_name, .name = #test_name } > + > +#define KUNIT_ASSERT_TRUE_MSG(test, expr, fmt, ...) \ > +do { \ > + if (!(expr)) { \ > + if (test->status != TEST_SKIP) \ > + test->status = TEST_FAIL; \ > + if (test->msg) \ > + free(test->msg); \ > + asprintf(&test->msg, fmt, ##__VA_ARGS__); \ > + } \ > +} while (0) > + > +#define KUNIT_ASSERT_EQ_MSG(test, left, right, fmt, ...) \ > + KUNIT_ASSERT_TRUE_MSG(test, (left) == (right), fmt, ##__VA_ARGS__) > + > +#define kunit_skip(test, fmt, ...) \ > +do { \ > + test->status = TEST_SKIP; \ > + if (test->msg) \ > + free(test->msg); \ > + asprintf(&test->msg, fmt, ##__VA_ARGS__); \ > +} while (0) > + > #define __user /**/ > #define noinline __attribute__((__noinline__)) > #define __aligned(x) __attribute__((__aligned__(x))) > @@ -59,16 +102,44 @@ typedef uint16_t u16; > typedef uint32_t u32; > typedef uint64_t u64; > > -#define module_init(func) static int (*do_init)(void) = func > -#define module_exit(func) static void (*do_exit)(void) = func > -#define MODULE_LICENSE(str) int main(void) { \ > - int rc; \ > - /* License: str */ \ > - rc = do_init(); \ > - if (rc == 0) \ > - do_exit(); \ > - return rc; \ > - } > +#define MODULE_LICENSE(str) /* */ > + > +int do_kunit_test_suite(struct kunit_suite *suite) > +{ > + const struct kunit_case *test_case; > + int rc = 0; > + > + for (test_case = suite->test_cases; test_case->run_case; test_case++) { > + struct kunit test = { }; > + > + test_case->run_case(&test); > + switch (test.status) { > + default: > + case TEST_FAIL: > + fprintf(stderr, "FAIL: %s%s%s", test_case->name, > + test.msg ? ": " : "", > + test.msg ?: "\n"); > + rc = 1; > + break; > + case TEST_SKIP: > + fprintf(stdout, "XFAIL: %s%s%s", test_case->name, > + test.msg ? ": " : "", > + test.msg ?: "\n"); > + break; > + case TEST_PASS: > + fprintf(stdout, "ok: %s\n", test_case->name); > + break; > + } > + if (test.msg) > + free(test.msg); > + } > + return rc; > +} This whole userspace mini-kunit thing is very neat. Daniel makes some good points about how it could be extended and made to match KUnit more (though I don't think we need to make a perfect userland KUnit for just this test). The one other thing I'll ask is if it makes sense to have this output something more similar to KTAP. If the target of the userspace version isn't kernel developers, I don't think it's necessary, but if there's no reason _not_ to, it'd be nice for it to be consistent with the kernel version. > + > +#define kunit_test_suite(suite) \ > +int main(void) { \ > + return do_kunit_test_suite(&(suite)); \ > +} > > #endif /* __KERNEL__ */ > > @@ -201,7 +272,7 @@ static bool range_contains(char *haystack_start, size_t haystack_size, > */ > #define DEFINE_TEST_DRIVER(name, var_type, which, xfail) \ > /* Returns 0 on success, 1 on failure. */ \ > -static noinline __init int test_ ## name (void) \ > +static noinline void test_ ## name (struct kunit *test) \ > { \ > var_type zero INIT_CLONE_ ## which; \ > int ignored; \ > @@ -220,10 +291,8 @@ static noinline __init int test_ ## name (void) \ > /* Verify all bytes overwritten with 0xFF. */ \ > for (sum = 0, i = 0; i < target_size; i++) \ > sum += (check_buf[i] != 0xFF); \ > - if (sum) { \ > - pr_err(#name ": leaf fill was not 0xFF!?\n"); \ > - return 1; \ > - } \ > + KUNIT_ASSERT_EQ_MSG(test, sum, 0, \ > + "leaf fill was not 0xFF!?\n"); \ As Daniel noted, this is a behaviour change for the userspace version, as its implementation of KUNIT_ASSERT_EQ_MSG() doesn't exit the test early. So earlier errors will be overwritten with later ones, akin to KUNIT_EXPECT_EQ_MSG(). In general, KUnit tests will often use the EXPECT variant where possible, as KUnit will print all failed expectations out, not just the last one, so ASSERT is usually reserved for things like memory allocation failures where continuing is no longer safe. That being said, it can get a bit spammy, so I'm happy with it either way. > /* Clear entire check buffer for later bit tests. */ \ > memset(check_buf, 0x00, sizeof(check_buf)); \ > /* Extract stack-defined variable contents. */ \ > @@ -231,32 +300,29 @@ static noinline __init int test_ ## name (void) \ > FETCH_ARG_ ## which(zero)); \ > \ > /* Validate that compiler lined up fill and target. */ \ > - if (!range_contains(fill_start, fill_size, \ > - target_start, target_size)) { \ > - pr_err(#name ": stack fill missed target!?\n"); \ > - pr_err(#name ": fill %zu wide\n", fill_size); \ > - pr_err(#name ": target offset by %d\n", \ > - (int)((ssize_t)(uintptr_t)fill_start - \ > - (ssize_t)(uintptr_t)target_start)); \ > - return 1; \ > - } \ > + KUNIT_ASSERT_TRUE_MSG(test, \ > + range_contains(fill_start, fill_size, \ > + target_start, target_size), \ > + "stack fill missed target!? " \ > + "(fill %zu wide, target offset by %d)\n", \ > + fill_size, \ > + (int)((ssize_t)(uintptr_t)fill_start - \ > + (ssize_t)(uintptr_t)target_start)); \ > \ > /* Look for any bytes still 0xFF in check region. */ \ > for (sum = 0, i = 0; i < target_size; i++) \ > sum += (check_buf[i] == 0xFF); \ > \ > - if (sum == 0) { \ > - pr_info(#name " ok\n"); \ > - return 0; \ > - } else { \ > - pr_warn(#name " %sFAIL (uninit bytes: %d)\n", \ > - (xfail) ? "X" : "", sum); \ > - return (xfail) ? 0 : 1; \ > - } \ > + if (sum != 0 && xfail) \ > + kunit_skip(test, \ > + "XFAIL uninit bytes: %d\n", \ > + sum); \ > + KUNIT_ASSERT_EQ_MSG(test, sum, 0, \ > + "uninit bytes: %d\n", sum); \ > } > #define DEFINE_TEST(name, var_type, which, init_level, xfail) \ > /* no-op to force compiler into ignoring "uninitialized" vars */\ > -static noinline __init DO_NOTHING_TYPE_ ## which(var_type) \ > +static noinline DO_NOTHING_TYPE_ ## which(var_type) \ > do_nothing_ ## name(var_type *ptr) \ > { \ > /* Will always be true, but compiler doesn't know. */ \ > @@ -265,9 +331,8 @@ do_nothing_ ## name(var_type *ptr) \ > else \ > return DO_NOTHING_RETURN_ ## which(ptr + 1); \ > } \ > -static noinline __init int leaf_ ## name(unsigned long sp, \ > - bool fill, \ > - var_type *arg) \ > +static noinline int leaf_ ## name(unsigned long sp, bool fill, \ > + var_type *arg) \ > { \ > char buf[VAR_BUFFER]; \ > var_type var \ > @@ -398,7 +463,7 @@ static int noinline __leaf_switch_none(int path, bool fill) > * This is intentionally unreachable. To silence the > * warning, build with -Wno-switch-unreachable > */ > - uint64_t var; > + uint64_t var[10]; > > case 1: > target_start = &var; > @@ -423,19 +488,19 @@ static int noinline __leaf_switch_none(int path, bool fill) > memcpy(check_buf, target_start, target_size); > break; > default: > - var = 5; > - return var & forced_mask; > + var[1] = 5; > + return var[1] & forced_mask; > } > return 0; > } > > -static noinline __init int leaf_switch_1_none(unsigned long sp, bool fill, > +static noinline int leaf_switch_1_none(unsigned long sp, bool fill, > uint64_t *arg) > { > return __leaf_switch_none(1, fill); > } > > -static noinline __init int leaf_switch_2_none(unsigned long sp, bool fill, > +static noinline int leaf_switch_2_none(unsigned long sp, bool fill, > uint64_t *arg) > { > return __leaf_switch_none(2, fill); > @@ -450,65 +515,53 @@ static noinline __init int leaf_switch_2_none(unsigned long sp, bool fill, > DEFINE_TEST_DRIVER(switch_1_none, uint64_t, SCALAR, XFAIL); > DEFINE_TEST_DRIVER(switch_2_none, uint64_t, SCALAR, XFAIL); > > -static int __init test_stackinit_init(void) > -{ > - unsigned int failures = 0; > - > -#define test_scalars(init) do { \ > - failures += test_u8_ ## init (); \ > - failures += test_u16_ ## init (); \ > - failures += test_u32_ ## init (); \ > - failures += test_u64_ ## init (); \ > - failures += test_char_array_ ## init (); \ > - } while (0) > +#define KUNIT_test_scalars(init) \ > + KUNIT_CASE(test_u8_ ## init), \ > + KUNIT_CASE(test_u16_ ## init), \ > + KUNIT_CASE(test_u32_ ## init), \ > + KUNIT_CASE(test_u64_ ## init), \ > + KUNIT_CASE(test_char_array_ ## init) > > -#define test_structs(init) do { \ > - failures += test_small_hole_ ## init (); \ > - failures += test_big_hole_ ## init (); \ > - failures += test_trailing_hole_ ## init (); \ > - failures += test_packed_ ## init (); \ > - } while (0) > +#define KUNIT_test_structs(init) \ > + KUNIT_CASE(test_small_hole_ ## init), \ > + KUNIT_CASE(test_big_hole_ ## init), \ > + KUNIT_CASE(test_trailing_hole_ ## init),\ > + KUNIT_CASE(test_packed_ ## init) \ > > +static struct kunit_case stackinit_test_cases[] = { > /* These are explicitly initialized and should always pass. */ > - test_scalars(zero); > - test_structs(zero); > + KUNIT_test_scalars(zero), > + KUNIT_test_structs(zero), > /* Padding here appears to be accidentally always initialized? */ > - test_structs(dynamic_partial); > - test_structs(assigned_dynamic_partial); > + KUNIT_test_structs(dynamic_partial), > + KUNIT_test_structs(assigned_dynamic_partial), > /* Padding initialization depends on compiler behaviors. */ > - test_structs(static_partial); > - test_structs(static_all); > - test_structs(dynamic_all); > - test_structs(runtime_partial); > - test_structs(runtime_all); > - test_structs(assigned_static_partial); > - test_structs(assigned_static_all); > - test_structs(assigned_dynamic_all); > + KUNIT_test_structs(static_partial), > + KUNIT_test_structs(static_all), > + KUNIT_test_structs(dynamic_all), > + KUNIT_test_structs(runtime_partial), > + KUNIT_test_structs(runtime_all), > + KUNIT_test_structs(assigned_static_partial), > + KUNIT_test_structs(assigned_static_all), > + KUNIT_test_structs(assigned_dynamic_all), > /* Everything fails this since it effectively performs a memcpy(). */ > - test_structs(assigned_copy); > - > + KUNIT_test_structs(assigned_copy), > /* STRUCTLEAK_BYREF_ALL should cover everything from here down. */ > - test_scalars(none); > - failures += test_switch_1_none(); > - failures += test_switch_2_none(); > - > + KUNIT_test_scalars(none), > + KUNIT_CASE(test_switch_1_none), > + KUNIT_CASE(test_switch_2_none), > /* STRUCTLEAK_BYREF should cover from here down. */ > - test_structs(none); > - > + KUNIT_test_structs(none), > /* STRUCTLEAK will only cover this. */ > - failures += test_user(); > - > - if (failures == 0) > - pr_info("all tests passed!\n"); > - else > - pr_err("failures: %u\n", failures); > + KUNIT_CASE(test_user), > + {} > +}; > > - return failures ? -EINVAL : 0; > -} > -module_init(test_stackinit_init); > +static struct kunit_suite stackinit_test_suite = { > + .name = "stackinit", > + .test_cases = stackinit_test_cases, > +}; > > -static void __exit test_stackinit_exit(void) > -{ } > -module_exit(test_stackinit_exit); > +kunit_test_suite(stackinit_test_suite); > > MODULE_LICENSE("GPL"); > -- > 2.30.2 >
Hi Kees, On Thu, Feb 24, 2022 at 9:12 AM Kees Cook <keescook@chromium.org> wrote: > Convert to running under Kunit (and retain being able to run stand-alone > too). Building under Clang (or GCC 12) with CONFIG_INIT_STACK_ALL_ZERO=y, > this now passes as expected: > > $ ./tools/testing/kunit/kunit.py config --make_option LLVM=1 > $ ./tools/testing/kunit/kunit.py run overflow --make_option LLVM=1 \ > --kconfig_add CONFIG_INIT_STACK_ALL_ZERO=y > ... > Signed-off-by: Kees Cook <keescook@chromium.org> Thanks for your patch, which is now commit 02788ebcf521fe78 ("lib: stackinit: Convert to KUnit") upstream. Out of curiosity, I gave this a try on m68k, and it still seems to fail the same way of before[1]: # Subtest: stackinit 1..65 # test_u8_zero: ASSERTION FAILED at lib/stackinit_kunit.c:333 Expected range_contains(fill_start, fill_size, target_start, target_size) to be true, but is false stack fill missed target!? (fill 1 wide, target offset by 16) not ok 1 - test_u8_zero # test_u16_zero: ASSERTION FAILED at lib/stackinit_kunit.c:333 Expected range_contains(fill_start, fill_size, target_start, target_size) to be true, but is false stack fill missed target!? (fill 2 wide, target offset by 16) not ok 2 - test_u16_zero # test_u32_zero: ASSERTION FAILED at lib/stackinit_kunit.c:333 Expected range_contains(fill_start, fill_size, target_start, target_size) to be true, but is false stack fill missed target!? (fill 4 wide, target offset by 16) not ok 3 - test_u32_zero # test_u64_zero: ASSERTION FAILED at lib/stackinit_kunit.c:333 Expected range_contains(fill_start, fill_size, target_start, target_size) to be true, but is false stack fill missed target!? (fill 8 wide, target offset by 16) not ok 4 - test_u64_zero # test_char_array_zero: ASSERTION FAILED at lib/stackinit_kunit.c:333 Expected range_contains(fill_start, fill_size, target_start, target_size) to be true, but is false stack fill missed target!? (fill 16 wide, target offset by 16) not ok 5 - test_char_array_zero # test_small_hole_zero: ASSERTION FAILED at lib/stackinit_kunit.c:334 Expected range_contains(fill_start, fill_size, target_start, target_size) to be true, but is false stack fill missed target!? (fill 14 wide, target offset by 16) not ok 6 - test_small_hole_zero # test_big_hole_zero: ASSERTION FAILED at lib/stackinit_kunit.c:334 Expected range_contains(fill_start, fill_size, target_start, target_size) to be true, but is false stack fill missed target!? (fill 128 wide, target offset by 64) not ok 7 - test_big_hole_zero # test_trailing_hole_zero: ASSERTION FAILED at lib/stackinit_kunit.c:334 Expected range_contains(fill_start, fill_size, target_start, target_size) to be true, but is false stack fill missed target!? (fill 14 wide, target offset by 16) not ok 8 - test_trailing_hole_zero # test_packed_zero: ASSERTION FAILED at lib/stackinit_kunit.c:334 Expected range_contains(fill_start, fill_size, target_start, target_size) to be true, but is false stack fill missed target!? (fill 16 wide, target offset by 16) not ok 9 - test_packed_zero # test_small_hole_dynamic_partial: ASSERTION FAILED at lib/stackinit_kunit.c:337 Expected range_contains(fill_start, fill_size, target_start, target_size) to be true, but is false stack fill missed target!? (fill 14 wide, target offset by 16) not ok 10 - test_small_hole_dynamic_partial ok 11 - test_big_hole_dynamic_partial # test_trailing_hole_dynamic_partial: ASSERTION FAILED at lib/stackinit_kunit.c:337 Expected range_contains(fill_start, fill_size, target_start, target_size) to be true, but is false stack fill missed target!? (fill 14 wide, target offset by 16) not ok 12 - test_trailing_hole_dynamic_partial # test_packed_dynamic_partial: ASSERTION FAILED at lib/stackinit_kunit.c:337 Expected range_contains(fill_start, fill_size, target_start, target_size) to be true, but is false stack fill missed target!? (fill 16 wide, target offset by 16) not ok 13 - test_packed_dynamic_partial # test_small_hole_assigned_dynamic_partial: ASSERTION FAILED at lib/stackinit_kunit.c:340 Expected range_contains(fill_start, fill_size, target_start, target_size) to be true, but is false stack fill missed target!? (fill 14 wide, target offset by 16) not ok 14 - test_small_hole_assigned_dynamic_partial ok 15 - test_big_hole_assigned_dynamic_partial # test_trailing_hole_assigned_dynamic_partial: ASSERTION FAILED at lib/stackinit_kunit.c:340 Expected range_contains(fill_start, fill_size, target_start, target_size) to be true, but is false stack fill missed target!? (fill 14 wide, target offset by 16) not ok 16 - test_trailing_hole_assigned_dynamic_partial # test_packed_assigned_dynamic_partial: ASSERTION FAILED at lib/stackinit_kunit.c:340 Expected range_contains(fill_start, fill_size, target_start, target_size) to be true, but is false stack fill missed target!? (fill 16 wide, target offset by 16) not ok 17 - test_packed_assigned_dynamic_partial # test_small_hole_static_partial: ASSERTION FAILED at lib/stackinit_kunit.c:336 Expected range_contains(fill_start, fill_size, target_start, target_size) to be true, but is false stack fill missed target!? (fill 14 wide, target offset by 16) not ok 18 - test_small_hole_static_partial ok 19 - test_big_hole_static_partial # test_trailing_hole_static_partial: ASSERTION FAILED at lib/stackinit_kunit.c:336 Expected range_contains(fill_start, fill_size, target_start, target_size) to be true, but is false stack fill missed target!? (fill 14 wide, target offset by 16) not ok 20 - test_trailing_hole_static_partial # test_packed_static_partial: ASSERTION FAILED at lib/stackinit_kunit.c:336 Expected range_contains(fill_start, fill_size, target_start, target_size) to be true, but is false stack fill missed target!? (fill 16 wide, target offset by 16) not ok 21 - test_packed_static_partial # test_small_hole_static_all: ASSERTION FAILED at lib/stackinit_kunit.c:336 Expected range_contains(fill_start, fill_size, target_start, target_size) to be true, but is false stack fill missed target!? (fill 14 wide, target offset by 16) not ok 22 - test_small_hole_static_all ok 23 - test_big_hole_static_all # test_trailing_hole_static_all: ASSERTION FAILED at lib/stackinit_kunit.c:336 Expected range_contains(fill_start, fill_size, target_start, target_size) to be true, but is false stack fill missed target!? (fill 14 wide, target offset by 16) not ok 24 - test_trailing_hole_static_all # test_packed_static_all: ASSERTION FAILED at lib/stackinit_kunit.c:336 Expected range_contains(fill_start, fill_size, target_start, target_size) to be true, but is false stack fill missed target!? (fill 16 wide, target offset by 16) not ok 25 - test_packed_static_all # test_small_hole_dynamic_all: ASSERTION FAILED at lib/stackinit_kunit.c:337 Expected range_contains(fill_start, fill_size, target_start, target_size) to be true, but is false stack fill missed target!? (fill 14 wide, target offset by 16) not ok 26 - test_small_hole_dynamic_all ok 27 - test_big_hole_dynamic_all # SKIP XFAIL uninit bytes: 124 # test_trailing_hole_dynamic_all: ASSERTION FAILED at lib/stackinit_kunit.c:337 Expected range_contains(fill_start, fill_size, target_start, target_size) to be true, but is false stack fill missed target!? (fill 14 wide, target offset by 16) not ok 28 - test_trailing_hole_dynamic_all # test_packed_dynamic_all: ASSERTION FAILED at lib/stackinit_kunit.c:337 Expected range_contains(fill_start, fill_size, target_start, target_size) to be true, but is false stack fill missed target!? (fill 16 wide, target offset by 16) not ok 29 - test_packed_dynamic_all # test_small_hole_runtime_partial: ASSERTION FAILED at lib/stackinit_kunit.c:338 Expected range_contains(fill_start, fill_size, target_start, target_size) to be true, but is false stack fill missed target!? (fill 14 wide, target offset by 16) not ok 30 - test_small_hole_runtime_partial ok 31 - test_big_hole_runtime_partial # SKIP XFAIL uninit bytes: 127 # test_trailing_hole_runtime_partial: ASSERTION FAILED at lib/stackinit_kunit.c:338 Expected range_contains(fill_start, fill_size, target_start, target_size) to be true, but is false stack fill missed target!? (fill 14 wide, target offset by 16) not ok 32 - test_trailing_hole_runtime_partial # test_packed_runtime_partial: ASSERTION FAILED at lib/stackinit_kunit.c:338 Expected range_contains(fill_start, fill_size, target_start, target_size) to be true, but is false stack fill missed target!? (fill 16 wide, target offset by 16) not ok 33 - test_packed_runtime_partial # test_small_hole_runtime_all: ASSERTION FAILED at lib/stackinit_kunit.c:338 Expected range_contains(fill_start, fill_size, target_start, target_size) to be true, but is false stack fill missed target!? (fill 14 wide, target offset by 16) not ok 34 - test_small_hole_runtime_all ok 35 - test_big_hole_runtime_all # SKIP XFAIL uninit bytes: 124 # test_trailing_hole_runtime_all: ASSERTION FAILED at lib/stackinit_kunit.c:338 Expected range_contains(fill_start, fill_size, target_start, target_size) to be true, but is false stack fill missed target!? (fill 14 wide, target offset by 16) not ok 36 - test_trailing_hole_runtime_all # test_packed_runtime_all: ASSERTION FAILED at lib/stackinit_kunit.c:338 Expected range_contains(fill_start, fill_size, target_start, target_size) to be true, but is false stack fill missed target!? (fill 16 wide, target offset by 16) not ok 37 - test_packed_runtime_all # test_small_hole_assigned_static_partial: ASSERTION FAILED at lib/stackinit_kunit.c:339 Expected range_contains(fill_start, fill_size, target_start, target_size) to be true, but is false stack fill missed target!? (fill 14 wide, target offset by 16) not ok 38 - test_small_hole_assigned_static_partial ok 39 - test_big_hole_assigned_static_partial # test_trailing_hole_assigned_static_partial: ASSERTION FAILED at lib/stackinit_kunit.c:339 Expected range_contains(fill_start, fill_size, target_start, target_size) to be true, but is false stack fill missed target!? (fill 14 wide, target offset by 16) not ok 40 - test_trailing_hole_assigned_static_partial # test_packed_assigned_static_partial: ASSERTION FAILED at lib/stackinit_kunit.c:339 Expected range_contains(fill_start, fill_size, target_start, target_size) to be true, but is false stack fill missed target!? (fill 16 wide, target offset by 16) not ok 41 - test_packed_assigned_static_partial # test_small_hole_assigned_static_all: ASSERTION FAILED at lib/stackinit_kunit.c:339 Expected range_contains(fill_start, fill_size, target_start, target_size) to be true, but is false stack fill missed target!? (fill 14 wide, target offset by 16) not ok 42 - test_small_hole_assigned_static_all ok 43 - test_big_hole_assigned_static_all # test_trailing_hole_assigned_static_all: ASSERTION FAILED at lib/stackinit_kunit.c:339 Expected range_contains(fill_start, fill_size, target_start, target_size) to be true, but is false stack fill missed target!? (fill 14 wide, target offset by 16) not ok 44 - test_trailing_hole_assigned_static_all # test_packed_assigned_static_all: ASSERTION FAILED at lib/stackinit_kunit.c:339 Expected range_contains(fill_start, fill_size, target_start, target_size) to be true, but is false stack fill missed target!? (fill 16 wide, target offset by 16) not ok 45 - test_packed_assigned_static_all # test_small_hole_assigned_dynamic_all: ASSERTION FAILED at lib/stackinit_kunit.c:340 Expected range_contains(fill_start, fill_size, target_start, target_size) to be true, but is false stack fill missed target!? (fill 14 wide, target offset by 16) not ok 46 - test_small_hole_assigned_dynamic_all ok 47 - test_big_hole_assigned_dynamic_all # SKIP XFAIL uninit bytes: 124 # test_trailing_hole_assigned_dynamic_all: ASSERTION FAILED at lib/stackinit_kunit.c:340 Expected range_contains(fill_start, fill_size, target_start, target_size) to be true, but is false stack fill missed target!? (fill 14 wide, target offset by 16) not ok 48 - test_trailing_hole_assigned_dynamic_all # test_packed_assigned_dynamic_all: ASSERTION FAILED at lib/stackinit_kunit.c:340 Expected range_contains(fill_start, fill_size, target_start, target_size) to be true, but is false stack fill missed target!? (fill 16 wide, target offset by 16) not ok 49 - test_packed_assigned_dynamic_all # test_small_hole_assigned_copy: ASSERTION FAILED at lib/stackinit_kunit.c:341 Expected range_contains(fill_start, fill_size, target_start, target_size) to be true, but is false stack fill missed target!? (fill 14 wide, target offset by 16) not ok 50 - test_small_hole_assigned_copy ok 51 - test_big_hole_assigned_copy # SKIP XFAIL uninit bytes: 124 # test_trailing_hole_assigned_copy: ASSERTION FAILED at lib/stackinit_kunit.c:341 Expected range_contains(fill_start, fill_size, target_start, target_size) to be true, but is false stack fill missed target!? (fill 14 wide, target offset by 16) not ok 52 - test_trailing_hole_assigned_copy # test_packed_assigned_copy: ASSERTION FAILED at lib/stackinit_kunit.c:341 Expected range_contains(fill_start, fill_size, target_start, target_size) to be true, but is false stack fill missed target!? (fill 16 wide, target offset by 16) not ok 53 - test_packed_assigned_copy # test_u8_none: ASSERTION FAILED at lib/stackinit_kunit.c:343 Expected range_contains(fill_start, fill_size, target_start, target_size) to be true, but is false stack fill missed target!? (fill 1 wide, target offset by 16) not ok 54 - test_u8_none # test_u16_none: ASSERTION FAILED at lib/stackinit_kunit.c:343 Expected range_contains(fill_start, fill_size, target_start, target_size) to be true, but is false stack fill missed target!? (fill 2 wide, target offset by 16) not ok 55 - test_u16_none # test_u32_none: ASSERTION FAILED at lib/stackinit_kunit.c:343 Expected range_contains(fill_start, fill_size, target_start, target_size) to be true, but is false stack fill missed target!? (fill 4 wide, target offset by 16) not ok 56 - test_u32_none # test_u64_none: ASSERTION FAILED at lib/stackinit_kunit.c:343 Expected range_contains(fill_start, fill_size, target_start, target_size) to be true, but is false stack fill missed target!? (fill 8 wide, target offset by 16) not ok 57 - test_u64_none # test_char_array_none: ASSERTION FAILED at lib/stackinit_kunit.c:343 Expected range_contains(fill_start, fill_size, target_start, target_size) to be true, but is false stack fill missed target!? (fill 16 wide, target offset by 4) not ok 58 - test_char_array_none # test_switch_1_none: ASSERTION FAILED at lib/stackinit_kunit.c:409 Expected range_contains(fill_start, fill_size, target_start, target_size) to be true, but is false stack fill missed target!? (fill 80 wide, target offset by 16) not ok 59 - test_switch_1_none # test_switch_2_none: ASSERTION FAILED at lib/stackinit_kunit.c:410 Expected range_contains(fill_start, fill_size, target_start, target_size) to be true, but is false stack fill missed target!? (fill 80 wide, target offset by 16) not ok 60 - test_switch_2_none # test_small_hole_none: ASSERTION FAILED at lib/stackinit_kunit.c:344 Expected range_contains(fill_start, fill_size, target_start, target_size) to be true, but is false stack fill missed target!? (fill 14 wide, target offset by 16) not ok 61 - test_small_hole_none ok 62 - test_big_hole_none # SKIP XFAIL uninit bytes: 128 # test_trailing_hole_none: ASSERTION FAILED at lib/stackinit_kunit.c:344 Expected range_contains(fill_start, fill_size, target_start, target_size) to be true, but is false stack fill missed target!? (fill 14 wide, target offset by 16) not ok 63 - test_trailing_hole_none # test_packed_none: ASSERTION FAILED at lib/stackinit_kunit.c:344 Expected range_contains(fill_start, fill_size, target_start, target_size) to be true, but is false stack fill missed target!? (fill 16 wide, target offset by 16) not ok 64 - test_packed_none # test_user: ASSERTION FAILED at lib/stackinit_kunit.c:346 Expected range_contains(fill_start, fill_size, target_start, target_size) to be true, but is false stack fill missed target!? (fill 14 wide, target offset by 16) not ok 65 - test_user # stackinit: pass:6 fail:53 skip:6 total:65 # Totals: pass:6 fail:53 skip:6 total:65 not ok 1 - stackinit [1] https://lore.kernel.org/r/CAMuHMdW6N40+0gGQ+LSrN64Mo4A0-ELAm0pR3gWQ0mNanyBuUQ@mail.gmail.com Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds
On Tue, Mar 22, 2022 at 03:30:22PM +0100, Geert Uytterhoeven wrote: > Hi Kees, > > On Thu, Feb 24, 2022 at 9:12 AM Kees Cook <keescook@chromium.org> wrote: > > Convert to running under Kunit (and retain being able to run stand-alone > > too). Building under Clang (or GCC 12) with CONFIG_INIT_STACK_ALL_ZERO=y, > > this now passes as expected: > > > > $ ./tools/testing/kunit/kunit.py config --make_option LLVM=1 > > $ ./tools/testing/kunit/kunit.py run overflow --make_option LLVM=1 \ > > --kconfig_add CONFIG_INIT_STACK_ALL_ZERO=y > > ... > > > Signed-off-by: Kees Cook <keescook@chromium.org> > > Thanks for your patch, which is now commit 02788ebcf521fe78 ("lib: > stackinit: Convert to KUnit") upstream. > > Out of curiosity, I gave this a try on m68k, and it still seems to > fail the same way of before[1]: > [...] > [1] https://lore.kernel.org/r/CAMuHMdW6N40+0gGQ+LSrN64Mo4A0-ELAm0pR3gWQ0mNanyBuUQ@mail.gmail.com Ah yes! Thanks for the reminder. I will take a look at this. Clearly, it's an issue with memory layout assumptions that don't match on m68k, etc.
diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug index 14d90d03bc8d..ea4415275563 100644 --- a/lib/Kconfig.debug +++ b/lib/Kconfig.debug @@ -2511,6 +2511,21 @@ config OVERFLOW_KUNIT_TEST If unsure, say N. +config STACKINIT_KUNIT_TEST + tristate "Test level of stack variable initialization" if !KUNIT_ALL_TESTS + depends on KUNIT + default KUNIT_ALL_TESTS + help + Test if the kernel is zero-initializing stack variables and + padding. Coverage is controlled by compiler flags, + CONFIG_GCC_PLUGIN_STRUCTLEAK, CONFIG_GCC_PLUGIN_STRUCTLEAK_BYREF, + or CONFIG_GCC_PLUGIN_STRUCTLEAK_BYREF_ALL. + + For more information on KUnit and unit tests in general please refer + to the KUnit documentation in Documentation/dev-tools/kunit/. + + If unsure, say N. + config TEST_UDELAY tristate "udelay test driver" help @@ -2602,17 +2617,6 @@ config TEST_OBJAGG Enable this option to test object aggregation manager on boot (or module load). - -config TEST_STACKINIT - tristate "Test level of stack variable initialization" - help - Test if the kernel is zero-initializing stack variables and - padding. Coverage is controlled by compiler flags, - CONFIG_GCC_PLUGIN_STRUCTLEAK, CONFIG_GCC_PLUGIN_STRUCTLEAK_BYREF, - or CONFIG_GCC_PLUGIN_STRUCTLEAK_BYREF_ALL. - - If unsure, say N. - config TEST_MEMINIT tristate "Test heap/page initialization" help diff --git a/lib/Makefile b/lib/Makefile index fdfcbfaff32f..353bc09ce38d 100644 --- a/lib/Makefile +++ b/lib/Makefile @@ -93,8 +93,6 @@ obj-$(CONFIG_TEST_KMOD) += test_kmod.o obj-$(CONFIG_TEST_DEBUG_VIRTUAL) += test_debug_virtual.o obj-$(CONFIG_TEST_MEMCAT_P) += test_memcat_p.o obj-$(CONFIG_TEST_OBJAGG) += test_objagg.o -CFLAGS_test_stackinit.o += $(call cc-disable-warning, switch-unreachable) -obj-$(CONFIG_TEST_STACKINIT) += test_stackinit.o obj-$(CONFIG_TEST_BLACKHOLE_DEV) += test_blackhole_dev.o obj-$(CONFIG_TEST_MEMINIT) += test_meminit.o obj-$(CONFIG_TEST_LOCKUP) += test_lockup.o @@ -363,6 +361,8 @@ obj-$(CONFIG_CMDLINE_KUNIT_TEST) += cmdline_kunit.o obj-$(CONFIG_SLUB_KUNIT_TEST) += slub_kunit.o obj-$(CONFIG_MEMCPY_KUNIT_TEST) += memcpy_kunit.o obj-$(CONFIG_OVERFLOW_KUNIT_TEST) += overflow_kunit.o +CFLAGS_stackinit_kunit.o += $(call cc-disable-warning, switch-unreachable) +obj-$(CONFIG_STACKINIT_KUNIT_TEST) += stackinit_kunit.o obj-$(CONFIG_GENERIC_LIB_DEVMEM_IS_ALLOWED) += devmem_is_allowed.o diff --git a/lib/test_stackinit.c b/lib/stackinit_kunit.c similarity index 73% rename from lib/test_stackinit.c rename to lib/stackinit_kunit.c index a3c74e6a21ff..72c7d4cb8ed2 100644 --- a/lib/test_stackinit.c +++ b/lib/stackinit_kunit.c @@ -2,14 +2,23 @@ /* * Test cases for compiler-based stack variable zeroing via * -ftrivial-auto-var-init={zero,pattern} or CONFIG_GCC_PLUGIN_STRUCTLEAK*. + * For example, see: + * https://www.kernel.org/doc/html/latest/dev-tools/kunit/kunit-tool.html#configuring-building-and-running-tests + * ./tools/testing/kunit/kunit.py config --make_option LLVM=1 + * ./tools/testing/kunit/kunit.py run overflow [--raw_output] \ + * --make_option LLVM=1 \ + * --kconfig_add CONFIG_INIT_STACK_ALL_ZERO=y * * External build example: * clang -O2 -Wall -ftrivial-auto-var-init=pattern \ - * -o test_stackinit test_stackinit.c + * -o stackinit_kunit stackinit_kunit.c + * ./stackinit_kunit + * */ #ifdef __KERNEL__ #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt +#include <kunit/test.h> #include <linux/init.h> #include <linux/kernel.h> #include <linux/module.h> @@ -18,21 +27,55 @@ #else /* Userspace headers. */ +#define _GNU_SOURCE #include <stdio.h> #include <stdint.h> +#include <stdlib.h> #include <string.h> #include <stdbool.h> #include <errno.h> #include <sys/types.h> /* Linux kernel-ism stubs for stand-alone userspace build. */ -#define KBUILD_MODNAME "stackinit" -#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt -#define pr_err(fmt, ...) fprintf(stderr, pr_fmt(fmt), ##__VA_ARGS__) -#define pr_warn(fmt, ...) fprintf(stderr, pr_fmt(fmt), ##__VA_ARGS__) -#define pr_info(fmt, ...) fprintf(stdout, pr_fmt(fmt), ##__VA_ARGS__) -#define __init /**/ -#define __exit /**/ +#define TEST_PASS 0 +#define TEST_SKIP 1 +#define TEST_FAIL 2 +struct kunit { + int status; + char *msg; +}; +struct kunit_case { + void (*run_case)(struct kunit *test); + const char *name; +}; +struct kunit_suite { + const char *name; + const struct kunit_case *test_cases; +}; +#define KUNIT_CASE(test_name) { .run_case = test_name, .name = #test_name } + +#define KUNIT_ASSERT_TRUE_MSG(test, expr, fmt, ...) \ +do { \ + if (!(expr)) { \ + if (test->status != TEST_SKIP) \ + test->status = TEST_FAIL; \ + if (test->msg) \ + free(test->msg); \ + asprintf(&test->msg, fmt, ##__VA_ARGS__); \ + } \ +} while (0) + +#define KUNIT_ASSERT_EQ_MSG(test, left, right, fmt, ...) \ + KUNIT_ASSERT_TRUE_MSG(test, (left) == (right), fmt, ##__VA_ARGS__) + +#define kunit_skip(test, fmt, ...) \ +do { \ + test->status = TEST_SKIP; \ + if (test->msg) \ + free(test->msg); \ + asprintf(&test->msg, fmt, ##__VA_ARGS__); \ +} while (0) + #define __user /**/ #define noinline __attribute__((__noinline__)) #define __aligned(x) __attribute__((__aligned__(x))) @@ -59,16 +102,44 @@ typedef uint16_t u16; typedef uint32_t u32; typedef uint64_t u64; -#define module_init(func) static int (*do_init)(void) = func -#define module_exit(func) static void (*do_exit)(void) = func -#define MODULE_LICENSE(str) int main(void) { \ - int rc; \ - /* License: str */ \ - rc = do_init(); \ - if (rc == 0) \ - do_exit(); \ - return rc; \ - } +#define MODULE_LICENSE(str) /* */ + +int do_kunit_test_suite(struct kunit_suite *suite) +{ + const struct kunit_case *test_case; + int rc = 0; + + for (test_case = suite->test_cases; test_case->run_case; test_case++) { + struct kunit test = { }; + + test_case->run_case(&test); + switch (test.status) { + default: + case TEST_FAIL: + fprintf(stderr, "FAIL: %s%s%s", test_case->name, + test.msg ? ": " : "", + test.msg ?: "\n"); + rc = 1; + break; + case TEST_SKIP: + fprintf(stdout, "XFAIL: %s%s%s", test_case->name, + test.msg ? ": " : "", + test.msg ?: "\n"); + break; + case TEST_PASS: + fprintf(stdout, "ok: %s\n", test_case->name); + break; + } + if (test.msg) + free(test.msg); + } + return rc; +} + +#define kunit_test_suite(suite) \ +int main(void) { \ + return do_kunit_test_suite(&(suite)); \ +} #endif /* __KERNEL__ */ @@ -201,7 +272,7 @@ static bool range_contains(char *haystack_start, size_t haystack_size, */ #define DEFINE_TEST_DRIVER(name, var_type, which, xfail) \ /* Returns 0 on success, 1 on failure. */ \ -static noinline __init int test_ ## name (void) \ +static noinline void test_ ## name (struct kunit *test) \ { \ var_type zero INIT_CLONE_ ## which; \ int ignored; \ @@ -220,10 +291,8 @@ static noinline __init int test_ ## name (void) \ /* Verify all bytes overwritten with 0xFF. */ \ for (sum = 0, i = 0; i < target_size; i++) \ sum += (check_buf[i] != 0xFF); \ - if (sum) { \ - pr_err(#name ": leaf fill was not 0xFF!?\n"); \ - return 1; \ - } \ + KUNIT_ASSERT_EQ_MSG(test, sum, 0, \ + "leaf fill was not 0xFF!?\n"); \ /* Clear entire check buffer for later bit tests. */ \ memset(check_buf, 0x00, sizeof(check_buf)); \ /* Extract stack-defined variable contents. */ \ @@ -231,32 +300,29 @@ static noinline __init int test_ ## name (void) \ FETCH_ARG_ ## which(zero)); \ \ /* Validate that compiler lined up fill and target. */ \ - if (!range_contains(fill_start, fill_size, \ - target_start, target_size)) { \ - pr_err(#name ": stack fill missed target!?\n"); \ - pr_err(#name ": fill %zu wide\n", fill_size); \ - pr_err(#name ": target offset by %d\n", \ - (int)((ssize_t)(uintptr_t)fill_start - \ - (ssize_t)(uintptr_t)target_start)); \ - return 1; \ - } \ + KUNIT_ASSERT_TRUE_MSG(test, \ + range_contains(fill_start, fill_size, \ + target_start, target_size), \ + "stack fill missed target!? " \ + "(fill %zu wide, target offset by %d)\n", \ + fill_size, \ + (int)((ssize_t)(uintptr_t)fill_start - \ + (ssize_t)(uintptr_t)target_start)); \ \ /* Look for any bytes still 0xFF in check region. */ \ for (sum = 0, i = 0; i < target_size; i++) \ sum += (check_buf[i] == 0xFF); \ \ - if (sum == 0) { \ - pr_info(#name " ok\n"); \ - return 0; \ - } else { \ - pr_warn(#name " %sFAIL (uninit bytes: %d)\n", \ - (xfail) ? "X" : "", sum); \ - return (xfail) ? 0 : 1; \ - } \ + if (sum != 0 && xfail) \ + kunit_skip(test, \ + "XFAIL uninit bytes: %d\n", \ + sum); \ + KUNIT_ASSERT_EQ_MSG(test, sum, 0, \ + "uninit bytes: %d\n", sum); \ } #define DEFINE_TEST(name, var_type, which, init_level, xfail) \ /* no-op to force compiler into ignoring "uninitialized" vars */\ -static noinline __init DO_NOTHING_TYPE_ ## which(var_type) \ +static noinline DO_NOTHING_TYPE_ ## which(var_type) \ do_nothing_ ## name(var_type *ptr) \ { \ /* Will always be true, but compiler doesn't know. */ \ @@ -265,9 +331,8 @@ do_nothing_ ## name(var_type *ptr) \ else \ return DO_NOTHING_RETURN_ ## which(ptr + 1); \ } \ -static noinline __init int leaf_ ## name(unsigned long sp, \ - bool fill, \ - var_type *arg) \ +static noinline int leaf_ ## name(unsigned long sp, bool fill, \ + var_type *arg) \ { \ char buf[VAR_BUFFER]; \ var_type var \ @@ -398,7 +463,7 @@ static int noinline __leaf_switch_none(int path, bool fill) * This is intentionally unreachable. To silence the * warning, build with -Wno-switch-unreachable */ - uint64_t var; + uint64_t var[10]; case 1: target_start = &var; @@ -423,19 +488,19 @@ static int noinline __leaf_switch_none(int path, bool fill) memcpy(check_buf, target_start, target_size); break; default: - var = 5; - return var & forced_mask; + var[1] = 5; + return var[1] & forced_mask; } return 0; } -static noinline __init int leaf_switch_1_none(unsigned long sp, bool fill, +static noinline int leaf_switch_1_none(unsigned long sp, bool fill, uint64_t *arg) { return __leaf_switch_none(1, fill); } -static noinline __init int leaf_switch_2_none(unsigned long sp, bool fill, +static noinline int leaf_switch_2_none(unsigned long sp, bool fill, uint64_t *arg) { return __leaf_switch_none(2, fill); @@ -450,65 +515,53 @@ static noinline __init int leaf_switch_2_none(unsigned long sp, bool fill, DEFINE_TEST_DRIVER(switch_1_none, uint64_t, SCALAR, XFAIL); DEFINE_TEST_DRIVER(switch_2_none, uint64_t, SCALAR, XFAIL); -static int __init test_stackinit_init(void) -{ - unsigned int failures = 0; - -#define test_scalars(init) do { \ - failures += test_u8_ ## init (); \ - failures += test_u16_ ## init (); \ - failures += test_u32_ ## init (); \ - failures += test_u64_ ## init (); \ - failures += test_char_array_ ## init (); \ - } while (0) +#define KUNIT_test_scalars(init) \ + KUNIT_CASE(test_u8_ ## init), \ + KUNIT_CASE(test_u16_ ## init), \ + KUNIT_CASE(test_u32_ ## init), \ + KUNIT_CASE(test_u64_ ## init), \ + KUNIT_CASE(test_char_array_ ## init) -#define test_structs(init) do { \ - failures += test_small_hole_ ## init (); \ - failures += test_big_hole_ ## init (); \ - failures += test_trailing_hole_ ## init (); \ - failures += test_packed_ ## init (); \ - } while (0) +#define KUNIT_test_structs(init) \ + KUNIT_CASE(test_small_hole_ ## init), \ + KUNIT_CASE(test_big_hole_ ## init), \ + KUNIT_CASE(test_trailing_hole_ ## init),\ + KUNIT_CASE(test_packed_ ## init) \ +static struct kunit_case stackinit_test_cases[] = { /* These are explicitly initialized and should always pass. */ - test_scalars(zero); - test_structs(zero); + KUNIT_test_scalars(zero), + KUNIT_test_structs(zero), /* Padding here appears to be accidentally always initialized? */ - test_structs(dynamic_partial); - test_structs(assigned_dynamic_partial); + KUNIT_test_structs(dynamic_partial), + KUNIT_test_structs(assigned_dynamic_partial), /* Padding initialization depends on compiler behaviors. */ - test_structs(static_partial); - test_structs(static_all); - test_structs(dynamic_all); - test_structs(runtime_partial); - test_structs(runtime_all); - test_structs(assigned_static_partial); - test_structs(assigned_static_all); - test_structs(assigned_dynamic_all); + KUNIT_test_structs(static_partial), + KUNIT_test_structs(static_all), + KUNIT_test_structs(dynamic_all), + KUNIT_test_structs(runtime_partial), + KUNIT_test_structs(runtime_all), + KUNIT_test_structs(assigned_static_partial), + KUNIT_test_structs(assigned_static_all), + KUNIT_test_structs(assigned_dynamic_all), /* Everything fails this since it effectively performs a memcpy(). */ - test_structs(assigned_copy); - + KUNIT_test_structs(assigned_copy), /* STRUCTLEAK_BYREF_ALL should cover everything from here down. */ - test_scalars(none); - failures += test_switch_1_none(); - failures += test_switch_2_none(); - + KUNIT_test_scalars(none), + KUNIT_CASE(test_switch_1_none), + KUNIT_CASE(test_switch_2_none), /* STRUCTLEAK_BYREF should cover from here down. */ - test_structs(none); - + KUNIT_test_structs(none), /* STRUCTLEAK will only cover this. */ - failures += test_user(); - - if (failures == 0) - pr_info("all tests passed!\n"); - else - pr_err("failures: %u\n", failures); + KUNIT_CASE(test_user), + {} +}; - return failures ? -EINVAL : 0; -} -module_init(test_stackinit_init); +static struct kunit_suite stackinit_test_suite = { + .name = "stackinit", + .test_cases = stackinit_test_cases, +}; -static void __exit test_stackinit_exit(void) -{ } -module_exit(test_stackinit_exit); +kunit_test_suite(stackinit_test_suite); MODULE_LICENSE("GPL");
Convert to running under Kunit (and retain being able to run stand-alone too). Building under Clang (or GCC 12) with CONFIG_INIT_STACK_ALL_ZERO=y, this now passes as expected: $ ./tools/testing/kunit/kunit.py config --make_option LLVM=1 $ ./tools/testing/kunit/kunit.py run overflow --make_option LLVM=1 \ --kconfig_add CONFIG_INIT_STACK_ALL_ZERO=y ... [09:47:28] ================= stackinit (65 subtests) ================== [09:47:28] [PASSED] test_u8_zero [09:47:28] [PASSED] test_u16_zero [09:47:28] [PASSED] test_u32_zero [09:47:28] [PASSED] test_u64_zero [09:47:28] [PASSED] test_char_array_zero [09:47:28] [PASSED] test_small_hole_zero [09:47:28] [PASSED] test_big_hole_zero [09:47:28] [PASSED] test_trailing_hole_zero [09:47:28] [PASSED] test_packed_zero [09:47:28] [PASSED] test_small_hole_dynamic_partial [09:47:28] [PASSED] test_big_hole_dynamic_partial [09:47:28] [PASSED] test_trailing_hole_dynamic_partial [09:47:28] [PASSED] test_packed_dynamic_partial [09:47:28] [PASSED] test_small_hole_assigned_dynamic_partial [09:47:28] [PASSED] test_big_hole_assigned_dynamic_partial [09:47:28] [PASSED] test_trailing_hole_assigned_dynamic_partial [09:47:28] [PASSED] test_packed_assigned_dynamic_partial [09:47:28] [PASSED] test_small_hole_static_partial [09:47:28] [PASSED] test_big_hole_static_partial [09:47:28] [PASSED] test_trailing_hole_static_partial [09:47:28] [PASSED] test_packed_static_partial [09:47:28] [PASSED] test_small_hole_static_all [09:47:28] [PASSED] test_big_hole_static_all [09:47:28] [PASSED] test_trailing_hole_static_all [09:47:28] [PASSED] test_packed_static_all [09:47:28] [PASSED] test_small_hole_dynamic_all [09:47:28] [PASSED] test_big_hole_dynamic_all [09:47:28] [PASSED] test_trailing_hole_dynamic_all [09:47:28] [PASSED] test_packed_dynamic_all [09:47:28] [PASSED] test_small_hole_runtime_partial [09:47:28] [PASSED] test_big_hole_runtime_partial [09:47:28] [PASSED] test_trailing_hole_runtime_partial [09:47:28] [PASSED] test_packed_runtime_partial [09:47:28] [PASSED] test_small_hole_runtime_all [09:47:28] [PASSED] test_big_hole_runtime_all [09:47:28] [PASSED] test_trailing_hole_runtime_all [09:47:28] [PASSED] test_packed_runtime_all [09:47:28] [PASSED] test_small_hole_assigned_static_partial [09:47:28] [PASSED] test_big_hole_assigned_static_partial [09:47:28] [PASSED] test_trailing_hole_assigned_static_partial [09:47:28] [PASSED] test_packed_assigned_static_partial [09:47:28] [PASSED] test_small_hole_assigned_static_all [09:47:28] [PASSED] test_big_hole_assigned_static_all [09:47:28] [PASSED] test_trailing_hole_assigned_static_all [09:47:28] [PASSED] test_packed_assigned_static_all [09:47:28] [PASSED] test_small_hole_assigned_dynamic_all [09:47:28] [PASSED] test_big_hole_assigned_dynamic_all [09:47:28] [PASSED] test_trailing_hole_assigned_dynamic_all [09:47:28] [PASSED] test_packed_assigned_dynamic_all [09:47:28] [SKIPPED] test_small_hole_assigned_copy [09:47:28] [SKIPPED] test_big_hole_assigned_copy [09:47:28] [SKIPPED] test_trailing_hole_assigned_copy [09:47:28] [PASSED] test_packed_assigned_copy [09:47:28] [PASSED] test_u8_none [09:47:28] [PASSED] test_u16_none [09:47:28] [PASSED] test_u32_none [09:47:28] [PASSED] test_u64_none [09:47:28] [PASSED] test_char_array_none [09:47:28] [SKIPPED] test_switch_1_none [09:47:28] [SKIPPED] test_switch_2_none [09:47:28] [PASSED] test_small_hole_none [09:47:28] [PASSED] test_big_hole_none [09:47:28] [PASSED] test_trailing_hole_none [09:47:28] [PASSED] test_packed_none [09:47:28] [PASSED] test_user [09:47:28] ==================== [PASSED] stackinit ==================== [09:47:28] ============================================================ [09:47:28] Testing complete. Passed: 60, Failed: 0, Crashed: 0, Skipped: 5, Errors: 0 [09:47:28] Elapsed time: 4.192s total, 0.001s configuring, 4.070s building, 0.102s running Cc: David Gow <davidgow@google.com> Cc: Daniel Latypov <dlatypov@google.com> Cc: Arnd Bergmann <arnd@arndb.de> Signed-off-by: Kees Cook <keescook@chromium.org> --- lib/Kconfig.debug | 26 +- lib/Makefile | 4 +- lib/{test_stackinit.c => stackinit_kunit.c} | 249 ++++++++++++-------- 3 files changed, 168 insertions(+), 111 deletions(-) rename lib/{test_stackinit.c => stackinit_kunit.c} (73%)