diff mbox series

usercopy: delete __noreturn from usercopy_abort

Message ID 1709516385-7778-1-git-send-email-xiaojiangfeng@huawei.com
State New
Headers
Series usercopy: delete __noreturn from usercopy_abort |

Commit Message

Jiangfeng Xiao March 4, 2024, 1:39 a.m. UTC 
  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@huawei.com>
---
 include/linux/uaccess.h   | 2 +-
 mm/usercopy.c             | 2 +-
 tools/objtool/noreturns.h | 1 -
 3 files changed, 2 insertions(+), 3 deletions(-)

Comments

Kees Cook March 4, 2024, 5:40 p.m. UTC | #1 
On Mon, Mar 04, 2024 at 04:15:07PM +0100, Jann Horn wrote:
> On Mon, Mar 4, 2024 at 3:02 AM Jiangfeng Xiao <xiaojiangfeng@huawei.com> wrote:
> > 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.

FWIW, all email from huawei.com continues to get eaten by anti-spam
checking. I've reported this a few times -- it'd be really nice if the
domain configuration could get fixed.

> [...]
> > Delete __noreturn from usercopy_abort,
> 
> This sounds like the actual bug is in the backtracing logic? I don't
> think removing __noreturn annotations from an individual function is a
> good fix, since the same thing can happen with other __noreturn
> functions depending on what choices the compiler makes.

Yeah, NAK. usercopy_abort() doesn't return. It ends with BUG().
diff mbox series

Patch

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)