[RFC,8/8] lkdtm: usercopy: Make USERCOPY_STACK_FRAME_x able to work for all archs

Commit Message

He Zhe April 18, 2022, 1:22 p.m. UTC

Currently the way to generate bad frame use depends on stack layout of the
architecture and compiler. It happens to work with x86 but does not work
with arm64 since it ruins the stack.

On x86, the original do_usercopy_stack_callee returns the start address of
its local buffer which is located deeply enough in the stack, so that the
following arch_within_stack_frames finds that the address is out of the
frame under check and gives the warning.

While on arm64, the local buffer of do_usercopy_stack_callee is just not
deep enough and happens to lands in the range of a frame so that
arch_within_stack_frames cannot detect it with current algorithm. And worse
in USERCOPY_STACK_FRAME_FROM, the following writing 0 to the formed buffer
just overwrites the return address of the call chain.

$ echo USERCOPY_STACK_FRAME_FROM > /sys/kernel/debug/provoke-crash/DIRECT
lkdtm: Performing direct entry USERCOPY_STACK_FRAME_FROM
lkdtm: attempting good copy_from_user of local stack
lkdtm: attempting bad copy_from_user of distant stack
Unable to handle kernel NULL pointer dereference at virtual address 0000000000000000

The detection algorithm may be improved in the future, but before that
let's tune the test case to validate current one. We can simply use
__builtin_frame_address(0) as a bad start address that can be detected for
all architectures.

Signed-off-by: He Zhe <[e-mail]>
---
 drivers/misc/lkdtm/usercopy.c | 24 +-----------------------
 1 file changed, 1 insertion(+), 23 deletions(-)

Patch

diff --git a/drivers/misc/lkdtm/usercopy.c b/drivers/misc/lkdtm/usercopy.c
index 9161ce7ed47a..000a1be15200 100644
--- a/drivers/misc/lkdtm/usercopy.c
+++ b/drivers/misc/lkdtm/usercopy.c
@@ -24,28 +24,6 @@  static struct kmem_cache *whitelist_cache;
 
 static const unsigned char test_text[] = "This is a test.\n";
 
-/*
- * Instead of adding -Wno-return-local-addr, just pass the stack address
- * through a function to obfuscate it from the compiler.
- */
-static noinline unsigned char *trick_compiler(unsigned char *stack)
-{
-	return stack + 0;
-}
-
-static noinline unsigned char *do_usercopy_stack_callee(int value)
-{
-	unsigned char buf[32];
-	int i;
-
-	/* Exercise stack to avoid everything living in registers. */
-	for (i = 0; i < sizeof(buf); i++) {
-		buf[i] = value & 0xff;
-	}
-
-	return trick_compiler(buf);
-}
-
 static noinline void do_usercopy_stack(bool to_user, bool bad_frame)
 {
 	unsigned long user_addr;
@@ -59,7 +37,7 @@  static noinline void do_usercopy_stack(bool to_user, bool bad_frame)
 
 	/* This is a pointer to outside our current stack frame. */
 	if (bad_frame) {
-		bad_stack = do_usercopy_stack_callee((uintptr_t)&bad_stack);
+		bad_stack = __builtin_frame_address(0);
 	} else {
 		/* Put start address just inside stack. */
 		bad_stack = task_stack_page(current) + THREAD_SIZE;