linux-kernel - [PATCH] lkdtm: usercopy: Make USERCOPY_STACK_FRAME

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [day] [month] [year] [list]

Message-Id: <20220425062835.3945462-1-zhe.he@windriver.com>
Date:   Mon, 25 Apr 2022 14:28:35 +0800
From:   He Zhe <zhe.he@...driver.com>
To:     keescook@...omium.org, linux-kernel@...r.kernel.org,
        zhe.he@...driver.com
Subject: [PATCH] lkdtm: usercopy: Make USERCOPY_STACK_FRAME_x able to work for all archs

Currently the way to generate bad frame 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 land 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 immediate _copy_from_user writing 0 to
the returned 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 <zhe.he@...driver.com>
---
 drivers/misc/lkdtm/usercopy.c | 24 +-----------------------
 1 file changed, 1 insertion(+), 23 deletions(-)

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;
-- 
2.25.1