Hi Greg,
Can you please apply these patches to your drivers/misc tree for LKDTM? It's mostly a collection of fixes and improvements and tweaks to the selftest integration.
Thanks!
-Kees
Kees Cook (4): lkdtm: Avoid more compiler optimizations for bad writes lkdtm/heap: Avoid edge and middle of slabs selftests/lkdtm: Reset WARN_ONCE to avoid false negatives lkdtm: Make arch-specific tests always available
drivers/misc/lkdtm/bugs.c | 45 +++++++++++++------------ drivers/misc/lkdtm/heap.c | 9 ++--- drivers/misc/lkdtm/lkdtm.h | 2 -- drivers/misc/lkdtm/perms.c | 22 ++++++++---- drivers/misc/lkdtm/usercopy.c | 7 ++-- tools/testing/selftests/lkdtm/run.sh | 6 ++++ tools/testing/selftests/lkdtm/tests.txt | 1 + 7 files changed, 56 insertions(+), 36 deletions(-)
It seems at least Clang is able to throw away writes it knows are destined for read-only memory, which makes things like the WRITE_RO test fail, as the write gets elided. Instead, force the variable to be volatile, and make similar changes through-out other tests in an effort to avoid needing to repeat fixing these kinds of problems. Also includes pr_err() calls in failure paths so that kernel logs are more clear in the failure case.
Reported-by: Prasad Sodagudi psodagud@codeaurora.org Suggested-by: Sami Tolvanen samitolvanen@google.com Signed-off-by: Kees Cook keescook@chromium.org --- drivers/misc/lkdtm/bugs.c | 11 +++++------ drivers/misc/lkdtm/perms.c | 22 +++++++++++++++------- drivers/misc/lkdtm/usercopy.c | 7 +++++-- 3 files changed, 25 insertions(+), 15 deletions(-)
diff --git a/drivers/misc/lkdtm/bugs.c b/drivers/misc/lkdtm/bugs.c index 886459e0ddd9..e1b43f615549 100644 --- a/drivers/misc/lkdtm/bugs.c +++ b/drivers/misc/lkdtm/bugs.c @@ -118,9 +118,8 @@ noinline void lkdtm_CORRUPT_STACK(void) /* Use default char array length that triggers stack protection. */ char data[8] __aligned(sizeof(void *));
- __lkdtm_CORRUPT_STACK(&data); - - pr_info("Corrupted stack containing char array ...\n"); + pr_info("Corrupting stack containing char array ...\n"); + __lkdtm_CORRUPT_STACK((void *)&data); }
/* Same as above but will only get a canary with -fstack-protector-strong */ @@ -131,9 +130,8 @@ noinline void lkdtm_CORRUPT_STACK_STRONG(void) unsigned long *ptr; } data __aligned(sizeof(void *));
- __lkdtm_CORRUPT_STACK(&data); - - pr_info("Corrupted stack containing union ...\n"); + pr_info("Corrupting stack containing union ...\n"); + __lkdtm_CORRUPT_STACK((void *)&data); }
void lkdtm_UNALIGNED_LOAD_STORE_WRITE(void) @@ -248,6 +246,7 @@ void lkdtm_ARRAY_BOUNDS(void)
kfree(not_checked); kfree(checked); + pr_err("FAIL: survived array bounds overflow!\n"); }
void lkdtm_CORRUPT_LIST_ADD(void) diff --git a/drivers/misc/lkdtm/perms.c b/drivers/misc/lkdtm/perms.c index 62f76d506f04..2dede2ef658f 100644 --- a/drivers/misc/lkdtm/perms.c +++ b/drivers/misc/lkdtm/perms.c @@ -57,6 +57,7 @@ static noinline void execute_location(void *dst, bool write) } pr_info("attempting bad execution at %px\n", func); func(); + pr_err("FAIL: func returned\n"); }
static void execute_user_location(void *dst) @@ -75,20 +76,22 @@ static void execute_user_location(void *dst) return; pr_info("attempting bad execution at %px\n", func); func(); + pr_err("FAIL: func returned\n"); }
void lkdtm_WRITE_RO(void) { - /* Explicitly cast away "const" for the test. */ - unsigned long *ptr = (unsigned long *)&rodata; + /* Explicitly cast away "const" for the test and make volatile. */ + volatile unsigned long *ptr = (unsigned long *)&rodata;
pr_info("attempting bad rodata write at %px\n", ptr); *ptr ^= 0xabcd1234; + pr_err("FAIL: survived bad write\n"); }
void lkdtm_WRITE_RO_AFTER_INIT(void) { - unsigned long *ptr = &ro_after_init; + volatile unsigned long *ptr = &ro_after_init;
/* * Verify we were written to during init. Since an Oops @@ -102,19 +105,21 @@ void lkdtm_WRITE_RO_AFTER_INIT(void)
pr_info("attempting bad ro_after_init write at %px\n", ptr); *ptr ^= 0xabcd1234; + pr_err("FAIL: survived bad write\n"); }
void lkdtm_WRITE_KERN(void) { size_t size; - unsigned char *ptr; + volatile unsigned char *ptr;
size = (unsigned long)do_overwritten - (unsigned long)do_nothing; ptr = (unsigned char *)do_overwritten;
pr_info("attempting bad %zu byte write at %px\n", size, ptr); - memcpy(ptr, (unsigned char *)do_nothing, size); + memcpy((void *)ptr, (unsigned char *)do_nothing, size); flush_icache_range((unsigned long)ptr, (unsigned long)(ptr + size)); + pr_err("FAIL: survived bad write\n");
do_overwritten(); } @@ -193,9 +198,11 @@ void lkdtm_ACCESS_USERSPACE(void) pr_info("attempting bad read at %px\n", ptr); tmp = *ptr; tmp += 0xc0dec0de; + pr_err("FAIL: survived bad read\n");
pr_info("attempting bad write at %px\n", ptr); *ptr = tmp; + pr_err("FAIL: survived bad write\n");
vm_munmap(user_addr, PAGE_SIZE); } @@ -203,19 +210,20 @@ void lkdtm_ACCESS_USERSPACE(void) void lkdtm_ACCESS_NULL(void) { unsigned long tmp; - unsigned long *ptr = (unsigned long *)NULL; + volatile unsigned long *ptr = (unsigned long *)NULL;
pr_info("attempting bad read at %px\n", ptr); tmp = *ptr; tmp += 0xc0dec0de; + pr_err("FAIL: survived bad read\n");
pr_info("attempting bad write at %px\n", ptr); *ptr = tmp; + pr_err("FAIL: survived bad write\n"); }
void __init lkdtm_perms_init(void) { /* Make sure we can write to __ro_after_init values during __init */ ro_after_init |= 0xAA; - } diff --git a/drivers/misc/lkdtm/usercopy.c b/drivers/misc/lkdtm/usercopy.c index e172719dd86d..b833367a45d0 100644 --- a/drivers/misc/lkdtm/usercopy.c +++ b/drivers/misc/lkdtm/usercopy.c @@ -304,19 +304,22 @@ void lkdtm_USERCOPY_KERNEL(void) return; }
- pr_info("attempting good copy_to_user from kernel rodata\n"); + pr_info("attempting good copy_to_user from kernel rodata: %px\n", + test_text); if (copy_to_user((void __user *)user_addr, test_text, unconst + sizeof(test_text))) { pr_warn("copy_to_user failed unexpectedly?!\n"); goto free_user; }
- pr_info("attempting bad copy_to_user from kernel text\n"); + pr_info("attempting bad copy_to_user from kernel text: %px\n", + vm_mmap); if (copy_to_user((void __user *)user_addr, vm_mmap, unconst + PAGE_SIZE)) { pr_warn("copy_to_user failed, but lacked Oops\n"); goto free_user; } + pr_err("FAIL: survived bad copy_to_user()\n");
free_user: vm_munmap(user_addr, PAGE_SIZE);
On Fri, May 29, 2020 at 1:03 PM Kees Cook keescook@chromium.org wrote:
It seems at least Clang is able to throw away writes it knows are destined for read-only memory, which makes things like the WRITE_RO test fail, as the write gets elided. Instead, force the variable to be
Heh, yep. I recall the exact patch in LLVM causing build breakages for kernels and various parts of Android userspace within the past year, for code that tried to write to variables declared const through casts that removed the const. (Was the last patch for us to build MIPS IIRC). Doing so is explicitly UB. I did feel that that particular "optimization" was very specific to C/C++, and should not have been performed in LLVM (which should be more agnostic to the front end language's wacky rules, IMO) but rather Clang (which doesn't do much C/C++ language specific optimizations currently, though there are rough plans forming to change that).
volatile, and make similar changes through-out other tests in an effort to avoid needing to repeat fixing these kinds of problems. Also includes pr_err() calls in failure paths so that kernel logs are more clear in the failure case.
Reported-by: Prasad Sodagudi psodagud@codeaurora.org Suggested-by: Sami Tolvanen samitolvanen@google.com Signed-off-by: Kees Cook keescook@chromium.org
drivers/misc/lkdtm/bugs.c | 11 +++++------ drivers/misc/lkdtm/perms.c | 22 +++++++++++++++------- drivers/misc/lkdtm/usercopy.c | 7 +++++-- 3 files changed, 25 insertions(+), 15 deletions(-)
diff --git a/drivers/misc/lkdtm/bugs.c b/drivers/misc/lkdtm/bugs.c index 886459e0ddd9..e1b43f615549 100644 --- a/drivers/misc/lkdtm/bugs.c +++ b/drivers/misc/lkdtm/bugs.c @@ -118,9 +118,8 @@ noinline void lkdtm_CORRUPT_STACK(void) /* Use default char array length that triggers stack protection. */ char data[8] __aligned(sizeof(void *));
__lkdtm_CORRUPT_STACK(&data);pr_info("Corrupted stack containing char array ...\n");
pr_info("Corrupting stack containing char array ...\n");__lkdtm_CORRUPT_STACK((void *)&data);}
/* Same as above but will only get a canary with -fstack-protector-strong */ @@ -131,9 +130,8 @@ noinline void lkdtm_CORRUPT_STACK_STRONG(void) unsigned long *ptr; } data __aligned(sizeof(void *));
__lkdtm_CORRUPT_STACK(&data);pr_info("Corrupted stack containing union ...\n");
pr_info("Corrupting stack containing union ...\n");__lkdtm_CORRUPT_STACK((void *)&data);}
void lkdtm_UNALIGNED_LOAD_STORE_WRITE(void) @@ -248,6 +246,7 @@ void lkdtm_ARRAY_BOUNDS(void)
kfree(not_checked); kfree(checked);
pr_err("FAIL: survived array bounds overflow!\n");}
void lkdtm_CORRUPT_LIST_ADD(void) diff --git a/drivers/misc/lkdtm/perms.c b/drivers/misc/lkdtm/perms.c index 62f76d506f04..2dede2ef658f 100644 --- a/drivers/misc/lkdtm/perms.c +++ b/drivers/misc/lkdtm/perms.c @@ -57,6 +57,7 @@ static noinline void execute_location(void *dst, bool write) } pr_info("attempting bad execution at %px\n", func); func();
pr_err("FAIL: func returned\n");}
static void execute_user_location(void *dst) @@ -75,20 +76,22 @@ static void execute_user_location(void *dst) return; pr_info("attempting bad execution at %px\n", func); func();
pr_err("FAIL: func returned\n");}
void lkdtm_WRITE_RO(void) {
/* Explicitly cast away "const" for the test. */unsigned long *ptr = (unsigned long *)&rodata;
/* Explicitly cast away "const" for the test and make volatile. */volatile unsigned long *ptr = (unsigned long *)&rodata; pr_info("attempting bad rodata write at %px\n", ptr); *ptr ^= 0xabcd1234;pr_err("FAIL: survived bad write\n");}
void lkdtm_WRITE_RO_AFTER_INIT(void) {
unsigned long *ptr = &ro_after_init;
volatile unsigned long *ptr = &ro_after_init; /* * Verify we were written to during init. Since an Oops@@ -102,19 +105,21 @@ void lkdtm_WRITE_RO_AFTER_INIT(void)
pr_info("attempting bad ro_after_init write at %px\n", ptr); *ptr ^= 0xabcd1234;
pr_err("FAIL: survived bad write\n");}
void lkdtm_WRITE_KERN(void) { size_t size;
unsigned char *ptr;
volatile unsigned char *ptr; size = (unsigned long)do_overwritten - (unsigned long)do_nothing; ptr = (unsigned char *)do_overwritten; pr_info("attempting bad %zu byte write at %px\n", size, ptr);
memcpy(ptr, (unsigned char *)do_nothing, size);
memcpy((void *)ptr, (unsigned char *)do_nothing, size); flush_icache_range((unsigned long)ptr, (unsigned long)(ptr + size));pr_err("FAIL: survived bad write\n"); do_overwritten();} @@ -193,9 +198,11 @@ void lkdtm_ACCESS_USERSPACE(void) pr_info("attempting bad read at %px\n", ptr); tmp = *ptr; tmp += 0xc0dec0de;
pr_err("FAIL: survived bad read\n"); pr_info("attempting bad write at %px\n", ptr); *ptr = tmp;pr_err("FAIL: survived bad write\n"); vm_munmap(user_addr, PAGE_SIZE);} @@ -203,19 +210,20 @@ void lkdtm_ACCESS_USERSPACE(void) void lkdtm_ACCESS_NULL(void) { unsigned long tmp;
unsigned long *ptr = (unsigned long *)NULL;
volatile unsigned long *ptr = (unsigned long *)NULL; pr_info("attempting bad read at %px\n", ptr); tmp = *ptr; tmp += 0xc0dec0de;pr_err("FAIL: survived bad read\n"); pr_info("attempting bad write at %px\n", ptr); *ptr = tmp;pr_err("FAIL: survived bad write\n");}
void __init lkdtm_perms_init(void) { /* Make sure we can write to __ro_after_init values during __init */ ro_after_init |= 0xAA;
} diff --git a/drivers/misc/lkdtm/usercopy.c b/drivers/misc/lkdtm/usercopy.c index e172719dd86d..b833367a45d0 100644 --- a/drivers/misc/lkdtm/usercopy.c +++ b/drivers/misc/lkdtm/usercopy.c @@ -304,19 +304,22 @@ void lkdtm_USERCOPY_KERNEL(void) return; }
pr_info("attempting good copy_to_user from kernel rodata\n");
pr_info("attempting good copy_to_user from kernel rodata: %px\n",test_text); if (copy_to_user((void __user *)user_addr, test_text, unconst + sizeof(test_text))) { pr_warn("copy_to_user failed unexpectedly?!\n"); goto free_user; }
pr_info("attempting bad copy_to_user from kernel text\n");
pr_info("attempting bad copy_to_user from kernel text: %px\n",vm_mmap); if (copy_to_user((void __user *)user_addr, vm_mmap, unconst + PAGE_SIZE)) { pr_warn("copy_to_user failed, but lacked Oops\n"); goto free_user; }pr_err("FAIL: survived bad copy_to_user()\n");free_user: vm_munmap(user_addr, PAGE_SIZE); -- 2.25.1
-- You received this message because you are subscribed to the Google Groups "Clang Built Linux" group. To unsubscribe from this group and stop receiving emails from it, send an email to clang-built-linux+unsubscribe@googlegroups.com. To view this discussion on the web visit https://groups.google.com/d/msgid/clang-built-linux/20200529200347.2464284-2....
Har har, after I moved the slab freelist pointer into the middle of the slab, now it looks like the contents are getting poisoned. Adjust the test to avoid the freelist pointer again.
Fixes: 3202fa62fb43 ("slub: relocate freelist pointer to middle of object") Cc: stable@vger.kernel.org Signed-off-by: Kees Cook keescook@chromium.org --- drivers/misc/lkdtm/heap.c | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-)
diff --git a/drivers/misc/lkdtm/heap.c b/drivers/misc/lkdtm/heap.c index 3c5cec85edce..1323bc16f113 100644 --- a/drivers/misc/lkdtm/heap.c +++ b/drivers/misc/lkdtm/heap.c @@ -58,11 +58,12 @@ void lkdtm_READ_AFTER_FREE(void) int *base, *val, saw; size_t len = 1024; /* - * The slub allocator uses the first word to store the free - * pointer in some configurations. Use the middle of the - * allocation to avoid running into the freelist + * The slub allocator will use the either the first word or + * the middle of the allocation to store the free pointer, + * depending on configurations. Store in the second word to + * avoid running into the freelist. */ - size_t offset = (len / sizeof(*base)) / 2; + size_t offset = sizeof(*base);
base = kmalloc(len, GFP_KERNEL); if (!base) {
Since we expect to see warnings every time for many tests, just reset the WARN_ONCE flags each time the script runs.
Signed-off-by: Kees Cook keescook@chromium.org --- tools/testing/selftests/lkdtm/run.sh | 6 ++++++ 1 file changed, 6 insertions(+)
diff --git a/tools/testing/selftests/lkdtm/run.sh b/tools/testing/selftests/lkdtm/run.sh index ee64ff8df8f4..8383eb89d88a 100755 --- a/tools/testing/selftests/lkdtm/run.sh +++ b/tools/testing/selftests/lkdtm/run.sh @@ -8,6 +8,7 @@ # set -e TRIGGER=/sys/kernel/debug/provoke-crash/DIRECT +CLEAR_ONCE=/sys/kernel/debug/clear_warn_once KSELFTEST_SKIP_TEST=4
# Verify we have LKDTM available in the kernel. @@ -67,6 +68,11 @@ cleanup() { } trap cleanup EXIT
+# Reset WARN_ONCE counters so we trip it each time this runs. +if [ -w $CLEAR_ONCE ] ; then + echo 1 > $CLEAR_ONCE +fi + # Save existing dmesg so we can detect new content below dmesg > "$DMESG"
On 5/29/20 2:03 PM, Kees Cook wrote:
Since we expect to see warnings every time for many tests, just reset the WARN_ONCE flags each time the script runs.
Signed-off-by: Kees Cook keescook@chromium.org
tools/testing/selftests/lkdtm/run.sh | 6 ++++++ 1 file changed, 6 insertions(+)
diff --git a/tools/testing/selftests/lkdtm/run.sh b/tools/testing/selftests/lkdtm/run.sh index ee64ff8df8f4..8383eb89d88a 100755 --- a/tools/testing/selftests/lkdtm/run.sh +++ b/tools/testing/selftests/lkdtm/run.sh @@ -8,6 +8,7 @@ # set -e TRIGGER=/sys/kernel/debug/provoke-crash/DIRECT +CLEAR_ONCE=/sys/kernel/debug/clear_warn_once KSELFTEST_SKIP_TEST=4 # Verify we have LKDTM available in the kernel. @@ -67,6 +68,11 @@ cleanup() { } trap cleanup EXIT +# Reset WARN_ONCE counters so we trip it each time this runs. +if [ -w $CLEAR_ONCE ] ; then
- echo 1 > $CLEAR_ONCE
+fi
- # Save existing dmesg so we can detect new content below dmesg > "$DMESG"
Acked-by: Shuah Khan skhan@linuxfoundation.org
thanks, -- Shuah
I'd like arch-specific tests to XFAIL when on a mismatched architecture so that we can more easily compare test coverage across all systems. Lacking kernel configs or CPU features count as a FAIL, not an XFAIL.
Signed-off-by: Kees Cook keescook@chromium.org --- drivers/misc/lkdtm/bugs.c | 34 ++++++++++++++----------- drivers/misc/lkdtm/lkdtm.h | 2 -- tools/testing/selftests/lkdtm/tests.txt | 1 + 3 files changed, 20 insertions(+), 17 deletions(-)
diff --git a/drivers/misc/lkdtm/bugs.c b/drivers/misc/lkdtm/bugs.c index e1b43f615549..f3fde0759f2c 100644 --- a/drivers/misc/lkdtm/bugs.c +++ b/drivers/misc/lkdtm/bugs.c @@ -453,38 +453,42 @@ void lkdtm_DOUBLE_FAULT(void) #endif }
-#ifdef CONFIG_ARM64_PTR_AUTH +#ifdef CONFIG_ARM64 static noinline void change_pac_parameters(void) { - /* Reset the keys of current task */ - ptrauth_thread_init_kernel(current); - ptrauth_thread_switch_kernel(current); + if (IS_ENABLED(CONFIG_ARM64_PTR_AUTH)) { + /* Reset the keys of current task */ + ptrauth_thread_init_kernel(current); + ptrauth_thread_switch_kernel(current); + } } +#endif
-#define CORRUPT_PAC_ITERATE 10 noinline void lkdtm_CORRUPT_PAC(void) { +#ifdef CONFIG_ARM64 +#define CORRUPT_PAC_ITERATE 10 int i;
+ if (!IS_ENABLED(CONFIG_ARM64_PTR_AUTH)) + pr_err("FAIL: kernel not built with CONFIG_ARM64_PTR_AUTH\n"); + if (!system_supports_address_auth()) { - pr_err("FAIL: arm64 pointer authentication feature not present\n"); + pr_err("FAIL: CPU lacks pointer authentication feature\n"); return; }
- pr_info("Change the PAC parameters to force function return failure\n"); + pr_info("changing PAC parameters to force function return failure...\n"); /* - * Pac is a hash value computed from input keys, return address and + * PAC is a hash value computed from input keys, return address and * stack pointer. As pac has fewer bits so there is a chance of * collision, so iterate few times to reduce the collision probability. */ for (i = 0; i < CORRUPT_PAC_ITERATE; i++) change_pac_parameters();
- pr_err("FAIL: %s test failed. Kernel may be unstable from here\n", __func__); -} -#else /* !CONFIG_ARM64_PTR_AUTH */ -noinline void lkdtm_CORRUPT_PAC(void) -{ - pr_err("FAIL: arm64 pointer authentication config disabled\n"); -} + pr_err("FAIL: survived PAC changes! Kernel may be unstable from here\n"); +#else + pr_err("XFAIL: this test is arm64-only\n"); #endif +} diff --git a/drivers/misc/lkdtm/lkdtm.h b/drivers/misc/lkdtm/lkdtm.h index 601a2156a0d4..8878538b2c13 100644 --- a/drivers/misc/lkdtm/lkdtm.h +++ b/drivers/misc/lkdtm/lkdtm.h @@ -31,9 +31,7 @@ void lkdtm_CORRUPT_USER_DS(void); void lkdtm_STACK_GUARD_PAGE_LEADING(void); void lkdtm_STACK_GUARD_PAGE_TRAILING(void); void lkdtm_UNSET_SMEP(void); -#ifdef CONFIG_X86_32 void lkdtm_DOUBLE_FAULT(void); -#endif void lkdtm_CORRUPT_PAC(void);
/* lkdtm_heap.c */ diff --git a/tools/testing/selftests/lkdtm/tests.txt b/tools/testing/selftests/lkdtm/tests.txt index 92ca32143ae5..9d266e79c6a2 100644 --- a/tools/testing/selftests/lkdtm/tests.txt +++ b/tools/testing/selftests/lkdtm/tests.txt @@ -14,6 +14,7 @@ STACK_GUARD_PAGE_LEADING STACK_GUARD_PAGE_TRAILING UNSET_SMEP CR4 bits went missing DOUBLE_FAULT +CORRUPT_PAC UNALIGNED_LOAD_STORE_WRITE #OVERWRITE_ALLOCATION Corrupts memory on failure #WRITE_AFTER_FREE Corrupts memory on failure
On Fri, May 29, 2020 at 01:03:43PM -0700, Kees Cook wrote:
Hi Greg,
Can you please apply these patches to your drivers/misc tree for LKDTM? It's mostly a collection of fixes and improvements and tweaks to the selftest integration.
Friendly ping -- we're past -rc2 now. :)
Thanks!
-Kees
Thanks!
-Kees
Kees Cook (4): lkdtm: Avoid more compiler optimizations for bad writes lkdtm/heap: Avoid edge and middle of slabs selftests/lkdtm: Reset WARN_ONCE to avoid false negatives lkdtm: Make arch-specific tests always available
drivers/misc/lkdtm/bugs.c | 45 +++++++++++++------------ drivers/misc/lkdtm/heap.c | 9 ++--- drivers/misc/lkdtm/lkdtm.h | 2 -- drivers/misc/lkdtm/perms.c | 22 ++++++++---- drivers/misc/lkdtm/usercopy.c | 7 ++-- tools/testing/selftests/lkdtm/run.sh | 6 ++++ tools/testing/selftests/lkdtm/tests.txt | 1 + 7 files changed, 56 insertions(+), 36 deletions(-)
-- 2.25.1
On 6/23/20 4:10 PM, Kees Cook wrote:
On Fri, May 29, 2020 at 01:03:43PM -0700, Kees Cook wrote:
Hi Greg,
Can you please apply these patches to your drivers/misc tree for LKDTM? It's mostly a collection of fixes and improvements and tweaks to the selftest integration.
Friendly ping -- we're past -rc2 now. :)
Thanks!
-Kees
Thanks!
-Kees
Kees Cook (4): lkdtm: Avoid more compiler optimizations for bad writes lkdtm/heap: Avoid edge and middle of slabs selftests/lkdtm: Reset WARN_ONCE to avoid false negatives lkdtm: Make arch-specific tests always available
drivers/misc/lkdtm/bugs.c | 45 +++++++++++++------------ drivers/misc/lkdtm/heap.c | 9 ++--- drivers/misc/lkdtm/lkdtm.h | 2 -- drivers/misc/lkdtm/perms.c | 22 ++++++++---- drivers/misc/lkdtm/usercopy.c | 7 ++-- tools/testing/selftests/lkdtm/run.sh | 6 ++++ tools/testing/selftests/lkdtm/tests.txt | 1 + 7 files changed, 56 insertions(+), 36 deletions(-)
-- 2.25.1
Regardless, it seems arch/x86/um/asm/desc.h is not needed any more?
True that, we can rip the file.
Has anyone fixed the uml build errors?
thanks.
On Tue, Jun 23, 2020 at 04:32:12PM -0700, Randy Dunlap wrote:
On 6/23/20 4:10 PM, Kees Cook wrote:
On Fri, May 29, 2020 at 01:03:43PM -0700, Kees Cook wrote:
Hi Greg,
Can you please apply these patches to your drivers/misc tree for LKDTM? It's mostly a collection of fixes and improvements and tweaks to the selftest integration.
Friendly ping -- we're past -rc2 now. :)
Thanks!
-Kees
Thanks!
-Kees
Kees Cook (4): lkdtm: Avoid more compiler optimizations for bad writes lkdtm/heap: Avoid edge and middle of slabs selftests/lkdtm: Reset WARN_ONCE to avoid false negatives lkdtm: Make arch-specific tests always available
drivers/misc/lkdtm/bugs.c | 45 +++++++++++++------------ drivers/misc/lkdtm/heap.c | 9 ++--- drivers/misc/lkdtm/lkdtm.h | 2 -- drivers/misc/lkdtm/perms.c | 22 ++++++++---- drivers/misc/lkdtm/usercopy.c | 7 ++-- tools/testing/selftests/lkdtm/run.sh | 6 ++++ tools/testing/selftests/lkdtm/tests.txt | 1 + 7 files changed, 56 insertions(+), 36 deletions(-)
-- 2.25.1
Regardless, it seems arch/x86/um/asm/desc.h is not needed any more?
True that, we can rip the file.
Has anyone fixed the uml build errors?
Oh, I thought that had nothing to do with the lkdtm changes? Should I rip out that file and resend?
----- Ursprüngliche Mail -----
Regardless, it seems arch/x86/um/asm/desc.h is not needed any more?
True that, we can rip the file.
Has anyone fixed the uml build errors?
I didn't realize that this is a super urgent issue. ;-)
Kees, if you want you can carry a patch in your series, I'll ack it. Otherwise I can also do a patch and bring it via the uml tree upstream as soon more fixes queued up.
Thanks, //richard
On Wed, Jun 24, 2020 at 09:23:25AM +0200, Richard Weinberger wrote:
----- Ursprüngliche Mail -----
Regardless, it seems arch/x86/um/asm/desc.h is not needed any more?
True that, we can rip the file.
Has anyone fixed the uml build errors?
I didn't realize that this is a super urgent issue. ;-)
Kees, if you want you can carry a patch in your series, I'll ack it. Otherwise I can also do a patch and bring it via the uml tree upstream as soon more fixes queued up.
I think the lkdtm change will tweak this bug, so I'm happy to carry the patch (I just haven't had time to create and test one). Is it really just as simple as removing arch/x86/um/asm/desc.h?
On 6/24/20 1:36 PM, Kees Cook wrote:
On Wed, Jun 24, 2020 at 09:23:25AM +0200, Richard Weinberger wrote:
----- Ursprüngliche Mail -----
Regardless, it seems arch/x86/um/asm/desc.h is not needed any more?
True that, we can rip the file.
Has anyone fixed the uml build errors?
I didn't realize that this is a super urgent issue. ;-)
Kees, if you want you can carry a patch in your series, I'll ack it. Otherwise I can also do a patch and bring it via the uml tree upstream as soon more fixes queued up.
I think the lkdtm change will tweak this bug, so I'm happy to carry the patch (I just haven't had time to create and test one). Is it really just as simple as removing arch/x86/um/asm/desc.h?
I just tried that and the build is still failing, so No, it's not that simple.
But thanks for offering.
I'll just ignore the UML build errors for now.
On Wed, Jun 24, 2020 at 11:29 PM Randy Dunlap rdunlap@infradead.org wrote:
On 6/24/20 1:36 PM, Kees Cook wrote:
On Wed, Jun 24, 2020 at 09:23:25AM +0200, Richard Weinberger wrote:
----- Ursprüngliche Mail -----
Regardless, it seems arch/x86/um/asm/desc.h is not needed any more?
True that, we can rip the file.
Has anyone fixed the uml build errors?
I didn't realize that this is a super urgent issue. ;-)
Kees, if you want you can carry a patch in your series, I'll ack it. Otherwise I can also do a patch and bring it via the uml tree upstream as soon more fixes queued up.
I think the lkdtm change will tweak this bug, so I'm happy to carry the patch (I just haven't had time to create and test one). Is it really just as simple as removing arch/x86/um/asm/desc.h?
I just tried that and the build is still failing, so No, it's not that simple.
But thanks for offering.
I'll just ignore the UML build errors for now.
This is a allyesconfig? I just gave CONFIG_LKDTM=y a try, builds fine here.
But the desc.h in uml is still in vain and can be deleted AFAICT.
On 6/24/20 3:01 PM, Richard Weinberger wrote:
On Wed, Jun 24, 2020 at 11:29 PM Randy Dunlap rdunlap@infradead.org wrote:
On 6/24/20 1:36 PM, Kees Cook wrote:
On Wed, Jun 24, 2020 at 09:23:25AM +0200, Richard Weinberger wrote:
----- Ursprüngliche Mail -----
> Regardless, it seems arch/x86/um/asm/desc.h is not needed any more?
True that, we can rip the file.
Has anyone fixed the uml build errors?
I didn't realize that this is a super urgent issue. ;-)
Kees, if you want you can carry a patch in your series, I'll ack it. Otherwise I can also do a patch and bring it via the uml tree upstream as soon more fixes queued up.
I think the lkdtm change will tweak this bug, so I'm happy to carry the patch (I just haven't had time to create and test one). Is it really just as simple as removing arch/x86/um/asm/desc.h?
I just tried that and the build is still failing, so No, it's not that simple.
But thanks for offering.
I'll just ignore the UML build errors for now.
This is a allyesconfig? I just gave CONFIG_LKDTM=y a try, builds fine here.
I'm building linux-next and it fails.
But the desc.h in uml is still in vain and can be deleted AFAICT.
On 6/24/20 3:23 PM, Randy Dunlap wrote:
On 6/24/20 3:01 PM, Richard Weinberger wrote:
On Wed, Jun 24, 2020 at 11:29 PM Randy Dunlap rdunlap@infradead.org wrote:
On 6/24/20 1:36 PM, Kees Cook wrote:
On Wed, Jun 24, 2020 at 09:23:25AM +0200, Richard Weinberger wrote:
----- Ursprüngliche Mail -----
>> Regardless, it seems arch/x86/um/asm/desc.h is not needed any more?
> True that, we can rip the file.
Has anyone fixed the uml build errors?
I didn't realize that this is a super urgent issue. ;-)
Kees, if you want you can carry a patch in your series, I'll ack it. Otherwise I can also do a patch and bring it via the uml tree upstream as soon more fixes queued up.
I think the lkdtm change will tweak this bug, so I'm happy to carry the patch (I just haven't had time to create and test one). Is it really just as simple as removing arch/x86/um/asm/desc.h?
I just tried that and the build is still failing, so No, it's not that simple.
But thanks for offering.
I'll just ignore the UML build errors for now.
This is a allyesconfig? I just gave CONFIG_LKDTM=y a try, builds fine here.
I'm building linux-next and it fails.
More specifically, uml for i386 fails. x86_64 is OK. The problem is with the <asm/desc.h> file. I'm tampering with it...
But the desc.h in uml is still in vain and can be deleted AFAICT.
On 6/24/20 3:35 PM, Randy Dunlap wrote:
On 6/24/20 3:23 PM, Randy Dunlap wrote:
On 6/24/20 3:01 PM, Richard Weinberger wrote:
On Wed, Jun 24, 2020 at 11:29 PM Randy Dunlap rdunlap@infradead.org wrote:
On 6/24/20 1:36 PM, Kees Cook wrote:
On Wed, Jun 24, 2020 at 09:23:25AM +0200, Richard Weinberger wrote:
----- Ursprüngliche Mail ----- >>> Regardless, it seems arch/x86/um/asm/desc.h is not needed any more? > >> True that, we can rip the file. > > Has anyone fixed the uml build errors?
I didn't realize that this is a super urgent issue. ;-)
Kees, if you want you can carry a patch in your series, I'll ack it. Otherwise I can also do a patch and bring it via the uml tree upstream as soon more fixes queued up.
I think the lkdtm change will tweak this bug, so I'm happy to carry the patch (I just haven't had time to create and test one). Is it really just as simple as removing arch/x86/um/asm/desc.h?
I just tried that and the build is still failing, so No, it's not that simple.
But thanks for offering.
I'll just ignore the UML build errors for now.
This is a allyesconfig? I just gave CONFIG_LKDTM=y a try, builds fine here.
I'm building linux-next and it fails.
More specifically, uml for i386 fails. x86_64 is OK. The problem is with the <asm/desc.h> file. I'm tampering with it...
I'm not getting anywhere with this. Too many mazes of tiny twisty passages.
But the desc.h in uml is still in vain and can be deleted AFAICT.
Looks like lkdtm/bugs.c needs to get/use arch/x86/include/asm/processor.h but it actually uses arch/x86/um/asm/processor*.h, which does not have the needed structs etc.
Here are the build errors and warnings that I am seeing with allmodconfig:
CC [M] drivers/misc/lkdtm/bugs.o In file included from ../arch/x86/include/asm/desc.h:11:0, from ../drivers/misc/lkdtm/bugs.c:17: ../arch/x86/include/asm/cpu_entry_area.h:65:42: error: invalid application of ‘sizeof’ to incomplete type ‘struct x86_hw_tss’ unsigned long stack[(PAGE_SIZE - sizeof(struct x86_hw_tss)) / sizeof(unsigned long)]; ^~~~~~ ../arch/x86/include/asm/cpu_entry_area.h:66:20: error: field ‘tss’ has incomplete type struct x86_hw_tss tss; ^~~ ../arch/x86/include/asm/cpu_entry_area.h:89:26: error: field ‘entry_stack_page’ has incomplete type struct entry_stack_page entry_stack_page; ^~~~~~~~~~~~~~~~ ../arch/x86/include/asm/cpu_entry_area.h:100:20: error: field ‘tss’ has incomplete type struct tss_struct tss; ^~~ In file included from ../drivers/misc/lkdtm/bugs.c:17:0: ../arch/x86/include/asm/desc.h:45:25: error: ‘GDT_ENTRIES’ undeclared here (not in a function); did you mean ‘LDT_ENTRIES’? struct desc_struct gdt[GDT_ENTRIES]; ^~~~~~~~~~~ LDT_ENTRIES ../arch/x86/include/asm/desc.h: In function ‘__set_tss_desc’: ../arch/x86/include/asm/desc.h:186:10: error: ‘__KERNEL_TSS_LIMIT’ undeclared (first use in this function); did you mean ‘__KERNEL__’? __KERNEL_TSS_LIMIT); ^~~~~~~~~~~~~~~~~~ __KERNEL__ ../arch/x86/include/asm/desc.h:186:10: note: each undeclared identifier is reported only once for each function it appears in ../arch/x86/include/asm/desc.h: In function ‘native_set_ldt’: ../arch/x86/include/asm/desc.h:202:40: error: ‘GDT_ENTRY_LDT’ undeclared (first use in this function); did you mean ‘GDT_ENTRY_INIT’? write_gdt_entry(get_cpu_gdt_rw(cpu), GDT_ENTRY_LDT, ^ ../arch/x86/include/asm/desc.h:123:75: note: in definition of macro ‘write_gdt_entry’ #define write_gdt_entry(dt, entry, desc, type) native_write_gdt_entry(dt, entry, desc, type) ^~~~~ ../arch/x86/include/asm/desc.h: In function ‘native_load_tr_desc’: ../arch/x86/include/asm/desc.h:259:31: error: ‘GDT_ENTRY_TSS’ undeclared (first use in this function); did you mean ‘GDT_ENTRIES’? asm volatile("ltr %w0"::"q" (GDT_ENTRY_TSS*8)); ^~~~~~~~~~~~~ GDT_ENTRIES ../arch/x86/include/asm/desc.h: In function ‘native_load_tls’: ../arch/x86/include/asm/desc.h:278:33: error: ‘struct thread_struct’ has no member named ‘tls_array’ gdt[GDT_ENTRY_TLS_MIN + i] = t->tls_array[i]; ^~ In file included from ../arch/x86/include/asm/string.h:3:0, from ../include/linux/string.h:20, from ../arch/x86/um/asm/processor_32.h:9, from ../arch/x86/um/asm/processor.h:10, from ../include/linux/rcupdate.h:30, from ../include/linux/rculist.h:11, from ../include/linux/pid.h:5, from ../include/linux/sched.h:14, from ../drivers/misc/lkdtm/bugs.c:10: ../arch/x86/include/asm/desc.h: In function ‘force_reload_TR’: ../arch/x86/include/asm/desc.h:288:18: error: ‘GDT_ENTRY_TSS’ undeclared (first use in this function); did you mean ‘GDT_ENTRIES’? memcpy(&tss, &d[GDT_ENTRY_TSS], sizeof(tss_desc)); ^ ../arch/x86/include/asm/string_32.h:182:45: note: in definition of macro ‘memcpy’ #define memcpy(t, f, n) __builtin_memcpy(t, f, n) ^ In file included from ../include/linux/kernel.h:11:0, from ../drivers/misc/lkdtm/lkdtm.h:7, from ../drivers/misc/lkdtm/bugs.c:8: ../arch/x86/include/asm/desc.h: In function ‘invalidate_tss_limit’: ../arch/x86/include/asm/desc.h:327:32: error: ‘TIF_IO_BITMAP’ undeclared (first use in this function); did you mean ‘CONFIG_SBITMAP’? if (unlikely(test_thread_flag(TIF_IO_BITMAP))) ^ ../include/linux/compiler.h:78:42: note: in definition of macro ‘unlikely’ # define unlikely(x) __builtin_expect(!!(x), 0) ^ ../arch/x86/include/asm/desc.h:327:15: note: in expansion of macro ‘test_thread_flag’ if (unlikely(test_thread_flag(TIF_IO_BITMAP))) ^~~~~~~~~~~~~~~~ In file included from ../drivers/misc/lkdtm/bugs.c:17:0: ../arch/x86/include/asm/desc.h: At top level: ../arch/x86/include/asm/desc.h:334:0: warning: "LDT_empty" redefined #define LDT_empty(info) \
In file included from ../arch/um/include/asm/mmu.h:10:0, from ../include/linux/mm_types.h:18, from ../include/linux/sched/signal.h:13, from ../drivers/misc/lkdtm/bugs.c:11: ../arch/x86/um/asm/mm_context.h:65:0: note: this is the location of the previous definition #define LDT_empty(info) (_LDT_empty(info))
In file included from ../drivers/misc/lkdtm/bugs.c:17:0: ../arch/x86/include/asm/desc.h: In function ‘get_cpu_gdt_rw’: ../arch/x86/include/asm/desc.h:54:1: warning: control reaches end of non-void function [-Wreturn-type] } ^
On Wed, Jun 24, 2020 at 06:45:47PM -0700, Randy Dunlap wrote:
Looks like lkdtm/bugs.c needs to get/use arch/x86/include/asm/processor.h but it actually uses arch/x86/um/asm/processor*.h, which does not have the needed structs etc.
Should I just test for !UML in bugs.c? (This is all for the lkdtm_DOUBLE_FAULT() test.) I already do those kinds of checks for the lkdtm_UNSET_SMEP() test. e.g.:
diff --git a/drivers/misc/lkdtm/bugs.c b/drivers/misc/lkdtm/bugs.c index 736675f0a246..f3e7040a7739 100644 --- a/drivers/misc/lkdtm/bugs.c +++ b/drivers/misc/lkdtm/bugs.c @@ -13,7 +13,7 @@ #include <linux/uaccess.h> #include <linux/slab.h>
-#ifdef CONFIG_X86_32 +#if IS_ENABLED(CONFIG_X86_32) && !IS_ENABLED(CONFIG_UML) #include <asm/desc.h> #endif
@@ -419,7 +419,7 @@ void lkdtm_UNSET_SMEP(void)
void lkdtm_DOUBLE_FAULT(void) { -#ifdef CONFIG_X86_32 +#if IS_ENABLED(CONFIG_X86_32) && !IS_ENABLED(CONFIG_UML) /* * Trigger #DF by setting the stack limit to zero. This clobbers * a GDT TLS slot, which is okay because the current task will die
----- Ursprüngliche Mail -----
Von: "Kees Cook" keescook@chromium.org An: "Randy Dunlap" rdunlap@infradead.org CC: "Richard Weinberger" richard.weinberger@gmail.com, "richard" richard@nod.at, "Greg Kroah-Hartman" gregkh@linuxfoundation.org, "Prasad Sodagudi" psodagud@codeaurora.org, "Sami Tolvanen" samitolvanen@google.com, "Amit Daniel Kachhap" amit.kachhap@arm.com, "linux-kselftest" linux-kselftest@vger.kernel.org, "clang-built-linux" clang-built-linux@googlegroups.com, "linux-kernel" linux-kernel@vger.kernel.org Gesendet: Donnerstag, 25. Juni 2020 08:06:18 Betreff: Re: [PATCH drivers/misc 0/4] lkdtm: Various clean ups
On Wed, Jun 24, 2020 at 06:45:47PM -0700, Randy Dunlap wrote:
Looks like lkdtm/bugs.c needs to get/use arch/x86/include/asm/processor.h but it actually uses arch/x86/um/asm/processor*.h, which does not have the needed structs etc.
Should I just test for !UML in bugs.c? (This is all for the lkdtm_DOUBLE_FAULT() test.) I already do those kinds of checks for the lkdtm_UNSET_SMEP() test. e.g.:
Just had a look. Yes, this sounds good to me. UML has CONFIG_X86_32=y but no GDT. :-)
Thanks, //richard
On 6/24/20 11:24 PM, Richard Weinberger wrote:
----- Ursprüngliche Mail -----
Von: "Kees Cook" keescook@chromium.org An: "Randy Dunlap" rdunlap@infradead.org CC: "Richard Weinberger" richard.weinberger@gmail.com, "richard" richard@nod.at, "Greg Kroah-Hartman" gregkh@linuxfoundation.org, "Prasad Sodagudi" psodagud@codeaurora.org, "Sami Tolvanen" samitolvanen@google.com, "Amit Daniel Kachhap" amit.kachhap@arm.com, "linux-kselftest" linux-kselftest@vger.kernel.org, "clang-built-linux" clang-built-linux@googlegroups.com, "linux-kernel" linux-kernel@vger.kernel.org Gesendet: Donnerstag, 25. Juni 2020 08:06:18 Betreff: Re: [PATCH drivers/misc 0/4] lkdtm: Various clean ups
On Wed, Jun 24, 2020 at 06:45:47PM -0700, Randy Dunlap wrote:
Looks like lkdtm/bugs.c needs to get/use arch/x86/include/asm/processor.h but it actually uses arch/x86/um/asm/processor*.h, which does not have the needed structs etc.
Should I just test for !UML in bugs.c? (This is all for the lkdtm_DOUBLE_FAULT() test.) I already do those kinds of checks for the lkdtm_UNSET_SMEP() test. e.g.:
Just had a look. Yes, this sounds good to me. UML has CONFIG_X86_32=y but no GDT. :-)
Sounds good to me also. Thanks.
On Tue, Jun 23, 2020 at 04:10:23PM -0700, Kees Cook wrote:
On Fri, May 29, 2020 at 01:03:43PM -0700, Kees Cook wrote:
Hi Greg,
Can you please apply these patches to your drivers/misc tree for LKDTM? It's mostly a collection of fixes and improvements and tweaks to the selftest integration.
Friendly ping -- we're past -rc2 now. :)
$ mdfrm -c ~/mail/todo/ 1970 messages in /home/gregkh/mail/todo/
You are in good company, give me some time to catch up please...
thanks,
greg k-h
linux-kselftest-mirror@lists.linaro.org
-
Greg Kroah-Hartman -
Kees Cook -
Nick Desaulniers -
Randy Dunlap -
Richard Weinberger -
Richard Weinberger -
Shuah Khan