| From e21103b3ad69c33698ad6888893abfa57e068029 Mon Sep 17 00:00:00 2001 |
| From: Sasha Levin <sashal@kernel.org> |
| Date: Tue, 20 Apr 2021 00:56:10 -0500 |
| Subject: um: Fix stack pointer alignment |
| |
| From: YiFei Zhu <zhuyifei1999@gmail.com> |
| |
| [ Upstream commit 558f9b2f94dbd2d5c5c8292aa13e081cc11ea7d9 ] |
| |
| GCC assumes that stack is aligned to 16-byte on call sites [1]. |
| Since GCC 8, GCC began using 16-byte aligned SSE instructions to |
| implement assignments to structs on stack. When |
| CC_OPTIMIZE_FOR_PERFORMANCE is enabled, this affects |
| os-Linux/sigio.c, write_sigio_thread: |
| |
| struct pollfds *fds, tmp; |
| tmp = current_poll; |
| |
| Note that struct pollfds is exactly 16 bytes in size. |
| GCC 8+ generates assembly similar to: |
| |
| movdqa (%rdi),%xmm0 |
| movaps %xmm0,-0x50(%rbp) |
| |
| This is an issue, because movaps will #GP if -0x50(%rbp) is not |
| aligned to 16 bytes [2], and how rbp gets assigned to is via glibc |
| clone thread_start, then function prologue, going though execution |
| trace similar to (showing only relevant instructions): |
| |
| sub $0x10,%rsi |
| mov %rcx,0x8(%rsi) |
| mov %rdi,(%rsi) |
| syscall |
| pop %rax |
| pop %rdi |
| callq *%rax |
| push %rbp |
| mov %rsp,%rbp |
| |
| The stack pointer always points to the topmost element on stack, |
| rather then the space right above the topmost. On push, the |
| pointer decrements first before writing to the memory pointed to |
| by it. Therefore, there is no need to have the stack pointer |
| pointer always point to valid memory unless the stack is poped; |
| so the `- sizeof(void *)` in the code is unnecessary. |
| |
| On the other hand, glibc reserves the 16 bytes it needs on stack |
| and pops itself, so by the call instruction the stack pointer |
| is exactly the caller-supplied sp. It then push the 16 bytes of |
| the return address and the saved stack pointer, so the base |
| pointer will be 16-byte aligned if and only if the caller |
| supplied sp is 16-byte aligned. Therefore, the caller must supply |
| a 16-byte aligned pointer, which `stack + UM_KERN_PAGE_SIZE` |
| already satisfies. |
| |
| On a side note, musl is unaffected by this issue because it forces |
| 16 byte alignment via `and $-16,%rsi` in its clone wrapper. |
| Similarly, glibc i386 is also unaffected because it has |
| `andl $0xfffffff0, %ecx`. |
| |
| To reproduce this bug, enable CONFIG_UML_RTC and |
| CC_OPTIMIZE_FOR_PERFORMANCE. uml_rtc will call |
| add_sigio_fd which will then cause write_sigio_thread to either go |
| into segfault loop or panic with "Segfault with no mm". |
| |
| Similarly, signal stacks will be aligned by the host kernel upon |
| signal delivery. `- sizeof(void *)` to sigaltstack is |
| unconventional and extraneous. |
| |
| On a related note, initialization of longjmp buffers do require |
| `- sizeof(void *)`. This is to account for the return address |
| that would have been pushed to the stack at the call site. |
| |
| The reason for uml to respect 16-byte alignment, rather than |
| telling GCC to assume 8-byte alignment like the host kernel since |
| commit d9b0cde91c60 ("x86-64, gcc: Use |
| -mpreferred-stack-boundary=3 if supported"), is because uml links |
| against libc. There is no reason to assume libc is also compiled |
| with that flag and assumes 8-byte alignment rather than 16-byte. |
| |
| [1] https://gcc.gnu.org/bugzilla/show_bug.cgi?id=40838 |
| [2] https://c9x.me/x86/html/file_module_x86_id_180.html |
| |
| Signed-off-by: YiFei Zhu <zhuyifei1999@gmail.com> |
| Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2") |
| Reviewed-by: Johannes Berg <johannes@sipsolutions.net> |
| Signed-off-by: Richard Weinberger <richard@nod.at> |
| Signed-off-by: Sasha Levin <sashal@kernel.org> |
| --- |
| arch/um/drivers/ubd_kern.c | 3 +-- |
| arch/um/kernel/skas/clone.c | 2 +- |
| arch/um/os-Linux/helper.c | 4 ++-- |
| arch/um/os-Linux/signal.c | 2 +- |
| arch/um/os-Linux/skas/process.c | 2 +- |
| 5 files changed, 6 insertions(+), 7 deletions(-) |
| |
| diff --git a/arch/um/drivers/ubd_kern.c b/arch/um/drivers/ubd_kern.c |
| index 8e0b43cf089f..cbd4f00fe77e 100644 |
| --- a/arch/um/drivers/ubd_kern.c |
| +++ b/arch/um/drivers/ubd_kern.c |
| @@ -1242,8 +1242,7 @@ static int __init ubd_driver_init(void){ |
| * enough. So use anyway the io thread. */ |
| } |
| stack = alloc_stack(0, 0); |
| - io_pid = start_io_thread(stack + PAGE_SIZE - sizeof(void *), |
| - &thread_fd); |
| + io_pid = start_io_thread(stack + PAGE_SIZE, &thread_fd); |
| if(io_pid < 0){ |
| printk(KERN_ERR |
| "ubd : Failed to start I/O thread (errno = %d) - " |
| diff --git a/arch/um/kernel/skas/clone.c b/arch/um/kernel/skas/clone.c |
| index 592cdb138441..5afac0fef24e 100644 |
| --- a/arch/um/kernel/skas/clone.c |
| +++ b/arch/um/kernel/skas/clone.c |
| @@ -29,7 +29,7 @@ stub_clone_handler(void) |
| long err; |
| |
| err = stub_syscall2(__NR_clone, CLONE_PARENT | CLONE_FILES | SIGCHLD, |
| - (unsigned long)data + UM_KERN_PAGE_SIZE / 2 - sizeof(void *)); |
| + (unsigned long)data + UM_KERN_PAGE_SIZE / 2); |
| if (err) { |
| data->parent_err = err; |
| goto done; |
| diff --git a/arch/um/os-Linux/helper.c b/arch/um/os-Linux/helper.c |
| index 9fa6e4187d4f..32e88baf18dd 100644 |
| --- a/arch/um/os-Linux/helper.c |
| +++ b/arch/um/os-Linux/helper.c |
| @@ -64,7 +64,7 @@ int run_helper(void (*pre_exec)(void *), void *pre_data, char **argv) |
| goto out_close; |
| } |
| |
| - sp = stack + UM_KERN_PAGE_SIZE - sizeof(void *); |
| + sp = stack + UM_KERN_PAGE_SIZE; |
| data.pre_exec = pre_exec; |
| data.pre_data = pre_data; |
| data.argv = argv; |
| @@ -120,7 +120,7 @@ int run_helper_thread(int (*proc)(void *), void *arg, unsigned int flags, |
| if (stack == 0) |
| return -ENOMEM; |
| |
| - sp = stack + UM_KERN_PAGE_SIZE - sizeof(void *); |
| + sp = stack + UM_KERN_PAGE_SIZE; |
| pid = clone(proc, (void *) sp, flags, arg); |
| if (pid < 0) { |
| err = -errno; |
| diff --git a/arch/um/os-Linux/signal.c b/arch/um/os-Linux/signal.c |
| index 96f511d1aabe..e283f130aadc 100644 |
| --- a/arch/um/os-Linux/signal.c |
| +++ b/arch/um/os-Linux/signal.c |
| @@ -129,7 +129,7 @@ void set_sigstack(void *sig_stack, int size) |
| stack_t stack = { |
| .ss_flags = 0, |
| .ss_sp = sig_stack, |
| - .ss_size = size - sizeof(void *) |
| + .ss_size = size |
| }; |
| |
| if (sigaltstack(&stack, NULL) != 0) |
| diff --git a/arch/um/os-Linux/skas/process.c b/arch/um/os-Linux/skas/process.c |
| index fba674fac8b7..87d3129e7362 100644 |
| --- a/arch/um/os-Linux/skas/process.c |
| +++ b/arch/um/os-Linux/skas/process.c |
| @@ -327,7 +327,7 @@ int start_userspace(unsigned long stub_stack) |
| } |
| |
| /* set stack pointer to the end of the stack page, so it can grow downwards */ |
| - sp = (unsigned long) stack + UM_KERN_PAGE_SIZE - sizeof(void *); |
| + sp = (unsigned long)stack + UM_KERN_PAGE_SIZE; |
| |
| flags = CLONE_FILES | SIGCHLD; |
| |
| -- |
| 2.30.2 |
| |