Date: Mon, 4 Mar 2024 09:39:45 +0800
From: Jiangfeng Xiao <xiaojiangfeng@...wei.com>
To: <gustavoars@...nel.org>, <akpm@...ux-foundation.org>,
	<jpoimboe@...nel.org>, <peterz@...radead.org>, <dave.hansen@...ux.intel.com>,
	<kirill.shutemov@...ux.intel.com>, <keescook@...omium.org>,
	<xiaojiangfeng@...wei.com>
CC: <linux-kernel@...r.kernel.org>, <linux-hardening@...r.kernel.org>,
	<linux-mm@...ck.org>, <nixiaoming@...wei.com>, <kepler.chenxin@...wei.com>,
	<wangbing6@...wei.com>, <wangfangpeng1@...wei.com>, <douzhaolei@...wei.com>
Subject: [PATCH] usercopy: delete __noreturn from usercopy_abort

When the last instruction of a noreturn function is a call
to another function, the return address falls outside
of the function boundary. This seems to cause kernel
to interrupt the backtrace.

My testcase is as follow:
```

static volatile size_t unconst = 0;
/*
check_object_size
    __check_object_size
        check_kernel_text_object
            usercopy_abort("kernel text", ...)
*/
void test_usercopy_kernel(void)
{
	check_object_size(schedule, unconst + PAGE_SIZE, 1);
}

static int __init test_usercopy_init(void)
{
        test_usercopy_kernel();
	return 0;
}

static void __exit test_usercopy_exit(void)
{
}

module_init(test_usercopy_init);
module_exit(test_usercopy_exit);
MODULE_LICENSE("GPL");
```

Running the testcase cause kernel oops,
and then the call stack is incorrect:
```
usercopy: Kernel memory exposure attempt detected from kernel text
Kernel BUG at usercopy_abort+0x98/0x9c
Internal error: Oops - undefined instruction: 0 [#1] SMP ARM
Modules linked in: test_usercopy(O+) usbcore usb_common
CPU: 0 PID: 609 Comm: insmod Tainted: G           O      5.10.0 #11
Hardware name: Hisilicon A9
PC is at usercopy_abort+0x98/0x9c
LR is at usercopy_abort+0x98/0x9c
[...]
 (usercopy_abort) from (memfd_fcntl+0x0/0x654)
 (memfd_fcntl) from (0xef7368f8)
Code: e1a01004 e58dc004 e58de000 eb108378 (e7f001f2)
---[ end trace e5fdc684259b0883 ]---
Kernel panic - not syncing: Fatal exception
```

Why Does the kernel backtrace cause errors?
Because usercopy_abort is marked as __noreturn.

You can see the related disassembling:
```
c02f24ac <__check_object_size>:
static_key_count():
[...]
check_kernel_text_object():
linux/mm/usercopy.c:125
        usercopy_abort("kernel text", NULL, to_user, ptr - textlow, n);
c02f26c4:       e3040110        movw    r0, #16656      ; 0x4110
c02f26c8:       e58d5000        str     r5, [sp]
c02f26cc:       e0443003        sub     r3, r4, r3
c02f26d0:       e1a02007        mov     r2, r7
c02f26d4:       e34c0096        movt    r0, #49302      ; 0xc096
c02f26d8:       ebffff4c        bl      c02f2410 <usercopy_abort>

c02f26dc <memfd_fcntl>:
memfd_fcntl():
[...]
```

The last instruction of __check_object_size is a call to usercopy_abort,
which is a noreturn function, the return address falls outside of the
__check_object_size function boundary, the return address falls into
the next function memfd_fcntl, therefore,
an error occurs when the kernel backtrace.

Delete __noreturn from usercopy_abort,
the correct call stack is as follow:
```
 (usercopy_abort) from (__check_object_size+0x170/0x234)
 (__check_object_size) from (test_usercopy_init+0x8/0xc0 [test_usercopy])
 (test_usercopy_init [test_usercopy]) from (do_one_initcall+0xac/0x204)
 (do_one_initcall) from (do_init_module+0x44/0x1c8)
 (do_init_module) from (load_module+0x1d48/0x2434)
 (load_module) from (sys_finit_module+0xc0/0xf4)
 (sys_finit_module) from (ret_fast_syscall+0x0/0x50)
```

Fixes: b394d468e7d7 ("usercopy: Enhance and rename report_usercopy()")

Signed-off-by: Jiangfeng Xiao <xiaojiangfeng@...wei.com>
---
 include/linux/uaccess.h   | 2 +-
 mm/usercopy.c             | 2 +-
 tools/objtool/noreturns.h | 1 -
 3 files changed, 2 insertions(+), 3 deletions(-)

diff --git a/include/linux/uaccess.h b/include/linux/uaccess.h
index 3064314..c37af70 100644
--- a/include/linux/uaccess.h
+++ b/include/linux/uaccess.h
@@ -437,7 +437,7 @@ static inline void user_access_restore(unsigned long flags) { }
 #endif
 
 #ifdef CONFIG_HARDENED_USERCOPY
-void __noreturn usercopy_abort(const char *name, const char *detail,
+void usercopy_abort(const char *name, const char *detail,
 			       bool to_user, unsigned long offset,
 			       unsigned long len);
 #endif
diff --git a/mm/usercopy.c b/mm/usercopy.c
index 83c164a..ca1b22e 100644
--- a/mm/usercopy.c
+++ b/mm/usercopy.c
@@ -83,7 +83,7 @@ static noinline int check_stack_object(const void *obj, unsigned long len)
  * kmem_cache_create_usercopy() function to create the cache (and
  * carefully audit the whitelist range).
  */
-void __noreturn usercopy_abort(const char *name, const char *detail,
+void usercopy_abort(const char *name, const char *detail,
 			       bool to_user, unsigned long offset,
 			       unsigned long len)
 {
diff --git a/tools/objtool/noreturns.h b/tools/objtool/noreturns.h
index 1685d7e..c7e341c 100644
--- a/tools/objtool/noreturns.h
+++ b/tools/objtool/noreturns.h
@@ -40,7 +40,6 @@
 NORETURN(snp_abort)
 NORETURN(start_kernel)
 NORETURN(stop_this_cpu)
-NORETURN(usercopy_abort)
 NORETURN(x86_64_start_kernel)
 NORETURN(x86_64_start_reservations)
 NORETURN(xen_cpu_bringup_again)
-- 
1.8.5.6

Powered by blists - more mailing lists