[PATCH 0/4] LKDTM: Various CI improvements

List overview All Threads
Download

newer

older

[PATCH] kunit: tool: allow...

[PATCH v5 00/11] Add futex2...

Kees Cook

18 Aug 2021 18 Aug '21

5:48 p.m.

Hi Greg,

Can you please pull these LKDTM changes for drivers/misc? I forgot to flush this queue of enhancements earlier. :) Here's what I've got built up, mostly tweaks for kernelCI, configs, consolidation. This also includes the __alloc_size hint adjustment I'd sent earlier, now fixed with a better comment.

Thanks!

-Kees

Kees Cook (4): lkdtm/bugs: Add ARRAY_BOUNDS to selftests lkdtm/fortify: Consolidate FORTIFY_SOURCE tests lkdtm: Add kernel version to failure hints lkdtm/heap: Avoid __alloc_size hint warning for VMALLOC_LINEAR_OVERFLOW

drivers/misc/lkdtm/bugs.c | 51 +----------------------- drivers/misc/lkdtm/core.c | 4 +- drivers/misc/lkdtm/fortify.c | 53 +++++++++++++++++++++++++ drivers/misc/lkdtm/heap.c | 9 ++++- drivers/misc/lkdtm/lkdtm.h | 24 ++++++----- tools/testing/selftests/lkdtm/config | 2 + tools/testing/selftests/lkdtm/tests.txt | 3 ++ 7 files changed, 83 insertions(+), 63 deletions(-)

-- 2.30.2

Show replies by date

Kees Cook

18 Aug 18 Aug

5:48 p.m.

New subject: [PATCH 1/4] lkdtm/bugs: Add ARRAY_BOUNDS to selftests

Add CONFIG hints about why the ARRAY_BOUNDS test might fail, and similarly include the CONFIGs needed to pass the ARRAY_BOUNDS test via the selftests, and add to selftests.

Cc: kernelci@groups.io Suggested-by: Guillaume Tucker guillaume.tucker@collabora.com Signed-off-by: Kees Cook keescook@chromium.org --- drivers/misc/lkdtm/bugs.c | 1 + tools/testing/selftests/lkdtm/config | 2 ++ tools/testing/selftests/lkdtm/tests.txt | 1 + 3 files changed, 4 insertions(+)

diff --git a/drivers/misc/lkdtm/bugs.c b/drivers/misc/lkdtm/bugs.c index 88c218a9f8b3..03171e412356 100644 --- a/drivers/misc/lkdtm/bugs.c +++ b/drivers/misc/lkdtm/bugs.c @@ -267,6 +267,7 @@ void lkdtm_ARRAY_BOUNDS(void) kfree(not_checked); kfree(checked); pr_err("FAIL: survived array bounds overflow!\n"); + pr_expected_config(CONFIG_UBSAN_BOUNDS); }

void lkdtm_CORRUPT_LIST_ADD(void) diff --git a/tools/testing/selftests/lkdtm/config b/tools/testing/selftests/lkdtm/config index 013446e87f1f..38edea25631b 100644 --- a/tools/testing/selftests/lkdtm/config +++ b/tools/testing/selftests/lkdtm/config @@ -6,3 +6,5 @@ CONFIG_HARDENED_USERCOPY=y # CONFIG_HARDENED_USERCOPY_FALLBACK is not set CONFIG_RANDOMIZE_KSTACK_OFFSET_DEFAULT=y CONFIG_INIT_ON_ALLOC_DEFAULT_ON=y +CONFIG_UBSAN_BOUNDS=y +CONFIG_UBSAN_TRAP=y diff --git a/tools/testing/selftests/lkdtm/tests.txt b/tools/testing/selftests/lkdtm/tests.txt index 846cfd508d3c..6a33dbea8491 100644 --- a/tools/testing/selftests/lkdtm/tests.txt +++ b/tools/testing/selftests/lkdtm/tests.txt @@ -7,6 +7,7 @@ EXCEPTION #EXHAUST_STACK Corrupts memory on failure #CORRUPT_STACK Crashes entire system on success #CORRUPT_STACK_STRONG Crashes entire system on success +ARRAY_BOUNDS CORRUPT_LIST_ADD list_add corruption CORRUPT_LIST_DEL list_del corruption STACK_GUARD_PAGE_LEADING

-- 2.30.2

Kees Cook

5:48 p.m.

New subject: [PATCH 2/4] lkdtm/fortify: Consolidate FORTIFY_SOURCE tests

The FORTIFY_SOURCE tests were split between bugs.c and fortify.c. Move tests into fortify.c, standardize their naming, add CONFIG hints, and add them to the lkdtm selftests.

Cc: Arnd Bergmann arnd@arndb.de Signed-off-by: Kees Cook keescook@chromium.org --- drivers/misc/lkdtm/bugs.c | 50 ----------------------- drivers/misc/lkdtm/core.c | 4 +- drivers/misc/lkdtm/fortify.c | 53 +++++++++++++++++++++++++ drivers/misc/lkdtm/lkdtm.h | 4 +- tools/testing/selftests/lkdtm/tests.txt | 2 + 5 files changed, 59 insertions(+), 54 deletions(-)

diff --git a/drivers/misc/lkdtm/bugs.c b/drivers/misc/lkdtm/bugs.c index 03171e412356..4282b625200f 100644 --- a/drivers/misc/lkdtm/bugs.c +++ b/drivers/misc/lkdtm/bugs.c @@ -507,53 +507,3 @@ noinline void lkdtm_CORRUPT_PAC(void) pr_err("XFAIL: this test is arm64-only\n"); #endif } - -void lkdtm_FORTIFY_OBJECT(void) -{ - struct target { - char a[10]; - } target[2] = {}; - int result; - - /* - * Using volatile prevents the compiler from determining the value of - * 'size' at compile time. Without that, we would get a compile error - * rather than a runtime error. - */ - volatile int size = 11; - - pr_info("trying to read past the end of a struct\n"); - - result = memcmp(&target[0], &target[1], size); - - /* Print result to prevent the code from being eliminated */ - pr_err("FAIL: fortify did not catch an object overread!\n" - ""%d" was the memcmp result.\n", result); -} - -void lkdtm_FORTIFY_SUBOBJECT(void) -{ - struct target { - char a[10]; - char b[10]; - } target; - char *src; - - src = kmalloc(20, GFP_KERNEL); - strscpy(src, "over ten bytes", 20); - - pr_info("trying to strcpy past the end of a member of a struct\n"); - - /* - * strncpy(target.a, src, 20); will hit a compile error because the - * compiler knows at build time that target.a < 20 bytes. Use strcpy() - * to force a runtime error. - */ - strcpy(target.a, src); - - /* Use target.a to prevent the code from being eliminated */ - pr_err("FAIL: fortify did not catch an sub-object overrun!\n" - ""%s" was copied.\n", target.a); - - kfree(src); -} diff --git a/drivers/misc/lkdtm/core.c b/drivers/misc/lkdtm/core.c index 9dda87c6b54a..dbdb0af7f68a 100644 --- a/drivers/misc/lkdtm/core.c +++ b/drivers/misc/lkdtm/core.c @@ -119,8 +119,6 @@ static const struct crashtype crashtypes[] = { CRASHTYPE(UNSET_SMEP), CRASHTYPE(CORRUPT_PAC), CRASHTYPE(UNALIGNED_LOAD_STORE_WRITE), - CRASHTYPE(FORTIFY_OBJECT), - CRASHTYPE(FORTIFY_SUBOBJECT), CRASHTYPE(SLAB_LINEAR_OVERFLOW), CRASHTYPE(VMALLOC_LINEAR_OVERFLOW), CRASHTYPE(WRITE_AFTER_FREE), @@ -180,6 +178,8 @@ static const struct crashtype crashtypes[] = { CRASHTYPE(USERCOPY_KERNEL), CRASHTYPE(STACKLEAK_ERASING), CRASHTYPE(CFI_FORWARD_PROTO), + CRASHTYPE(FORTIFIED_OBJECT), + CRASHTYPE(FORTIFIED_SUBOBJECT), CRASHTYPE(FORTIFIED_STRSCPY), CRASHTYPE(DOUBLE_FAULT), #ifdef CONFIG_PPC_BOOK3S_64 diff --git a/drivers/misc/lkdtm/fortify.c b/drivers/misc/lkdtm/fortify.c index 0f51d31b57ca..d06458a4858e 100644 --- a/drivers/misc/lkdtm/fortify.c +++ b/drivers/misc/lkdtm/fortify.c @@ -8,6 +8,59 @@ #include <linux/string.h> #include <linux/slab.h>

+static volatile int fortify_scratch_space; + +void lkdtm_FORTIFIED_OBJECT(void) +{ + struct target { + char a[10]; + } target[2] = {}; + /* + * Using volatile prevents the compiler from determining the value of + * 'size' at compile time. Without that, we would get a compile error + * rather than a runtime error. + */ + volatile int size = 11; + + pr_info("trying to read past the end of a struct\n"); + + /* Store result to global to prevent the code from being eliminated */ + fortify_scratch_space = memcmp(&target[0], &target[1], size); + + pr_err("FAIL: fortify did not block an object overread!\n"); + pr_expected_config(CONFIG_FORTIFY_SOURCE); +} + +void lkdtm_FORTIFIED_SUBOBJECT(void) +{ + struct target { + char a[10]; + char b[10]; + } target; + volatile int size = 20; + char *src; + + src = kmalloc(size, GFP_KERNEL); + strscpy(src, "over ten bytes", size); + size = strlen(src) + 1; + + pr_info("trying to strcpy past the end of a member of a struct\n"); + + /* + * memcpy(target.a, src, 20); will hit a compile error because the + * compiler knows at build time that target.a < 20 bytes. Use a + * volatile to force a runtime error. + */ + memcpy(target.a, src, size); + + /* Store result to global to prevent the code from being eliminated */ + fortify_scratch_space = target.a[3]; + + pr_err("FAIL: fortify did not block an sub-object overrun!\n"); + pr_expected_config(CONFIG_FORTIFY_SOURCE); + + kfree(src); +}

/* * Calls fortified strscpy to test that it returns the same result as vanilla diff --git a/drivers/misc/lkdtm/lkdtm.h b/drivers/misc/lkdtm/lkdtm.h index 6a30b60519f3..f2e61581c1ae 100644 --- a/drivers/misc/lkdtm/lkdtm.h +++ b/drivers/misc/lkdtm/lkdtm.h @@ -74,8 +74,6 @@ void lkdtm_STACK_GUARD_PAGE_TRAILING(void); void lkdtm_UNSET_SMEP(void); void lkdtm_DOUBLE_FAULT(void); void lkdtm_CORRUPT_PAC(void); -void lkdtm_FORTIFY_OBJECT(void); -void lkdtm_FORTIFY_SUBOBJECT(void);

/* heap.c */ void __init lkdtm_heap_init(void); @@ -150,6 +148,8 @@ void lkdtm_STACKLEAK_ERASING(void); void lkdtm_CFI_FORWARD_PROTO(void);

/* fortify.c */ +void lkdtm_FORTIFIED_OBJECT(void); +void lkdtm_FORTIFIED_SUBOBJECT(void); void lkdtm_FORTIFIED_STRSCPY(void);

/* powerpc.c */ diff --git a/tools/testing/selftests/lkdtm/tests.txt b/tools/testing/selftests/lkdtm/tests.txt index 6a33dbea8491..09f7bfa383cc 100644 --- a/tools/testing/selftests/lkdtm/tests.txt +++ b/tools/testing/selftests/lkdtm/tests.txt @@ -73,4 +73,6 @@ USERCOPY_KERNEL STACKLEAK_ERASING OK: the rest of the thread stack is properly erased CFI_FORWARD_PROTO FORTIFIED_STRSCPY +FORTIFIED_OBJECT +FORTIFIED_SUBOBJECT PPC_SLB_MULTIHIT Recovered

-- 2.30.2

Kees Cook

5:48 p.m.

New subject: [PATCH 3/4] lkdtm: Add kernel version to failure hints

In an effort to keep as much information in once place as possible in CI logs, report the kernel version and architecture in the failure hints.

Cc: Arnd Bergmann arnd@arndb.de Cc: kernelci@groups.io Signed-off-by: Kees Cook keescook@chromium.org --- drivers/misc/lkdtm/lkdtm.h | 20 ++++++++++++-------- 1 file changed, 12 insertions(+), 8 deletions(-)

diff --git a/drivers/misc/lkdtm/lkdtm.h b/drivers/misc/lkdtm/lkdtm.h index f2e61581c1ae..d7d64d9765eb 100644 --- a/drivers/misc/lkdtm/lkdtm.h +++ b/drivers/misc/lkdtm/lkdtm.h @@ -5,13 +5,17 @@ #define pr_fmt(fmt) "lkdtm: " fmt

#include <linux/kernel.h> +#include <generated/compile.h> +#include <generated/utsrelease.h> + +#define LKDTM_KERNEL "kernel (" UTS_RELEASE " " UTS_MACHINE ")"

#define pr_expected_config(kconfig) \ { \ if (IS_ENABLED(kconfig)) \ - pr_err("Unexpected! This kernel was built with " #kconfig "=y\n"); \ + pr_err("Unexpected! This " LKDTM_KERNEL " was built with " #kconfig "=y\n"); \ else \ - pr_warn("This is probably expected, since this kernel was built *without* " #kconfig "=y\n"); \ + pr_warn("This is probably expected, since this " LKDTM_KERNEL " was built *without* " #kconfig "=y\n"); \ }

#ifndef MODULE @@ -21,24 +25,24 @@ int lkdtm_check_bool_cmdline(const char *param); if (IS_ENABLED(kconfig)) { \ switch (lkdtm_check_bool_cmdline(param)) { \ case 0: \ - pr_warn("This is probably expected, since this kernel was built with " #kconfig "=y but booted with '" param "=N'\n"); \ + pr_warn("This is probably expected, since this " LKDTM_KERNEL " was built with " #kconfig "=y but booted with '" param "=N'\n"); \ break; \ case 1: \ - pr_err("Unexpected! This kernel was built with " #kconfig "=y and booted with '" param "=Y'\n"); \ + pr_err("Unexpected! This " LKDTM_KERNEL " was built with " #kconfig "=y and booted with '" param "=Y'\n"); \ break; \ default: \ - pr_err("Unexpected! This kernel was built with " #kconfig "=y (and booted without '" param "' specified)\n"); \ + pr_err("Unexpected! This " LKDTM_KERNEL " was built with " #kconfig "=y (and booted without '" param "' specified)\n"); \ } \ } else { \ switch (lkdtm_check_bool_cmdline(param)) { \ case 0: \ - pr_warn("This is probably expected, as kernel was built *without* " #kconfig "=y and booted with '" param "=N'\n"); \ + pr_warn("This is probably expected, as this " LKDTM_KERNEL " was built *without* " #kconfig "=y and booted with '" param "=N'\n"); \ break; \ case 1: \ - pr_err("Unexpected! This kernel was built *without* " #kconfig "=y but booted with '" param "=Y'\n"); \ + pr_err("Unexpected! This " LKDTM_KERNEL " was built *without* " #kconfig "=y but booted with '" param "=Y'\n"); \ break; \ default: \ - pr_err("This is probably expected, since this kernel was built *without* " #kconfig "=y (and booted without '" param "' specified)\n"); \ + pr_err("This is probably expected, since this " LKDTM_KERNEL " was built *without* " #kconfig "=y (and booted without '" param "' specified)\n"); \ break; \ } \ } \

-- 2.30.2

Kees Cook

5:48 p.m.

New subject: [PATCH 4/4] lkdtm/heap: Avoid __alloc_size hint warning for VMALLOC_LINEAR_OVERFLOW

Once __alloc_size hints have been added, the compiler will (correctly!) see this as an overflow. We are, however, trying to test for this condition at run-time (not compile-time), so work around it with a volatile int offset.

Cc: Arnd Bergmann arnd@arndb.de Signed-off-by: Kees Cook keescook@chromium.org --- drivers/misc/lkdtm/heap.c | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/drivers/misc/lkdtm/heap.c b/drivers/misc/lkdtm/heap.c index 3d9aae5821a0..8a92f5a800fa 100644 --- a/drivers/misc/lkdtm/heap.c +++ b/drivers/misc/lkdtm/heap.c @@ -12,6 +12,13 @@ static struct kmem_cache *double_free_cache; static struct kmem_cache *a_cache; static struct kmem_cache *b_cache;

+/* + * Using volatile here means the compiler cannot ever make assumptions + * about this value. This means compile-time length checks involving + * this variable cannot be performed; only run-time checks. + */ +static volatile int __offset = 1; + /* * If there aren't guard pages, it's likely that a consecutive allocation will * let us overflow into the second allocation without overwriting something real. @@ -24,7 +31,7 @@ void lkdtm_VMALLOC_LINEAR_OVERFLOW(void) two = vzalloc(PAGE_SIZE);

pr_info("Attempting vmalloc linear overflow ...\n"); - memset(one, 0xAA, PAGE_SIZE + 1); + memset(one, 0xAA, PAGE_SIZE + __offset);

vfree(two); vfree(one);

-- 2.30.2

990

days inactive

990

days old

linux-kselftest-mirror@lists.linaro.org

4 comments

participants

tags (0)

participants (1)

Kees Cook