forked from Minki/linux
um: Fix stack pointer alignment
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 commitd9b0cde91c
("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:1da177e4c3
("Linux-2.6.12-rc2") Reviewed-by: Johannes Berg <johannes@sipsolutions.net> Signed-off-by: Richard Weinberger <richard@nod.at>
This commit is contained in:
parent
80f849bf54
commit
558f9b2f94
@ -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) - "
|
||||
|
@ -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;
|
||||
|
@ -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;
|
||||
|
@ -143,7 +143,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)
|
||||
|
@ -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;
|
||||
|
||||
|
Loading…
Reference in New Issue
Block a user