| Message ID | 1466172679-10156-1-git-send-email-peter.maydell@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 06/17/2016 07:11 AM, Peter Maydell wrote: > Some architectures require the stack to be executable; notably > this includes MIPS, because the kernel's floating point emulator > may try to put trampoline code on the stack to handle some cases. > (See https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=815409 > for an example of this causing QEMU to crash.) > > Create a utility function qemu_alloc_stack() which allocates a > block of memory for use as a stack with the correct permissions. > Since we would prefer to make the stack non-executable if we can > as a defence against code execution exploits, we detect whether > the existing stack is mapped executable. Unfortunately this > requires us to grovel through /proc/self/maps to determine the > permissions on it. > > Signed-off-by: Peter Maydell <peter.maydell@linaro.org> > --- > This method of figuring out the correct perms for the stack is > not exactly pretty; better suggestions welcome. > > NB that this utility function also gives us a handy place to put > code for allocating a guard page at the bottom of the stack, or > mapping it as MAP_GROWSDOWN, or whatever. ... > + /* Some architectures (notably MIPS) require an executable stack, but > + * we would prefer to avoid making the stack executable unnecessarily, > + * to defend against code execution exploits. > + * Check whether the current stack is executable, and follow its lead. > + * Unfortunately to do this we have to wade through /proc/self/maps > + * looking for the stack memory. We default to assuming we need an > + * executable stack and remove the permission only if we can successfully > + * confirm that non-executable is OK. > + */ > + > + prot = PROT_READ | PROT_WRITE | PROT_EXEC; ... > +#else > +static int stack_prot(void) > +{ > + /* Assume an executable stack is needed, since we can't detect it. */ > + return PROT_READ | PROT_WRITE | PROT_EXEC; > +} > +#endif What about using dl_iterate_phdr, looking for PT_GNU_STACK? That interface is present on a few other hosts besides Linux. But really this is a place that I'd much rather fall back to an ifdef ladder than assume executable permission is required. r~
On 17 June 2016 at 17:12, Richard Henderson <rth@twiddle.net> wrote: > What about using dl_iterate_phdr, looking for PT_GNU_STACK? > That interface is present on a few other hosts besides Linux. We could do that. I note that the MIPS kernel is buggy in that it will assume the stack is executable even if the binary has PT_GNU_STACK saying "please don't be executable". And most architectures except x86-64 won't honour PT_GNU_STACK=non-exec unless the parent process also had nonexec stack (because they let the READ_IMPLIES_EXEC personality flag be inherited; see https://insights.sei.cmu.edu/cert/2014/02/feeling-insecure-blame-your-parent.html ). So the PT_GNU_STACK flag doesn't necessarily match up with either the actual executability of the standard stack or with what the kernel actually requires. > But really this is a place that I'd much rather fall back to an ifdef ladder > than assume executable permission is required. The trouble with this is that it means that as and when the MIPS folks fix their kernel and libc and compiler to support non-exec stacks we won't automatically pick this up, and our stacks will remain executable. Also it requires us to audit every architecture to find out which ones require exec-stack. But maybe it is just MIPS? (Maybe we could just say "this is a MIPS kernel bug" ? :-)) thanks -- PMM
On 06/17/2016 09:36 AM, Peter Maydell wrote: > On 17 June 2016 at 17:12, Richard Henderson <rth@twiddle.net> wrote: >> What about using dl_iterate_phdr, looking for PT_GNU_STACK? >> That interface is present on a few other hosts besides Linux. > > We could do that. I note that the MIPS kernel is buggy in that > it will assume the stack is executable even if the binary > has PT_GNU_STACK saying "please don't be executable". And > most architectures except x86-64 won't honour PT_GNU_STACK=non-exec > unless the parent process also had nonexec stack (because they > let the READ_IMPLIES_EXEC personality flag be inherited; see > https://insights.sei.cmu.edu/cert/2014/02/feeling-insecure-blame-your-parent.html > ). So the PT_GNU_STACK flag doesn't necessarily match up with > either the actual executability of the standard stack or with > what the kernel actually requires. How bizarre. Glibc will most definitely honour PT_GNU_STACK when allocating thread stacks, so it's a weird thing for the kernel to want to inherit for the initial thread stack. >> But really this is a place that I'd much rather fall back to an ifdef ladder >> than assume executable permission is required. > > The trouble with this is that it means that as and when the MIPS > folks fix their kernel and libc and compiler to support non-exec > stacks we won't automatically pick this up, and our stacks will > remain executable. Also it requires us to audit every architecture > to find out which ones require exec-stack. But maybe it is just > MIPS? (Maybe we could just say "this is a MIPS kernel bug" ? :-)) I am inclined to hope that this is just a mips thing. It's a pretty strange situation. But I did really mean fall back. Yes, try the other methods, but if we don't detect anything about the stack, only enable it via ifdef ladder. r~
On 17 June 2016 at 18:27, Richard Henderson <rth@twiddle.net> wrote: > On 06/17/2016 09:36 AM, Peter Maydell wrote: >> And >> most architectures except x86-64 won't honour PT_GNU_STACK=non-exec >> unless the parent process also had nonexec stack (because they >> let the READ_IMPLIES_EXEC personality flag be inherited; see >> https://insights.sei.cmu.edu/cert/2014/02/feeling-insecure-blame-your-parent.html >> ). So the PT_GNU_STACK flag doesn't necessarily match up with >> either the actual executability of the standard stack or with >> what the kernel actually requires. > > How bizarre. > > Glibc will most definitely honour PT_GNU_STACK when allocating thread stacks, > so it's a weird thing for the kernel to want to inherit for the initial thread > stack. I agree. I think it's a bug in the non-x86-64 architectures (and particularly annoying if /sbin/init happens to not be marked as being ok for non-exec stack). >>> But really this is a place that I'd much rather fall back to an ifdef ladder >>> than assume executable permission is required. >> >> The trouble with this is that it means that as and when the MIPS >> folks fix their kernel and libc and compiler to support non-exec >> stacks we won't automatically pick this up, and our stacks will >> remain executable. Also it requires us to audit every architecture >> to find out which ones require exec-stack. But maybe it is just >> MIPS? (Maybe we could just say "this is a MIPS kernel bug" ? :-)) > > I am inclined to hope that this is just a mips thing. > It's a pretty strange situation. > > But I did really mean fall back. Yes, try the other methods, but > if we don't detect anything about the stack, only enable it via > ifdef ladder. Oh, I see. thanks -- PMM
On 06/17/16 16:11, Peter Maydell wrote: > Some architectures require the stack to be executable; notably > this includes MIPS, because the kernel's floating point emulator > may try to put trampoline code on the stack to handle some cases. > (See https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=815409 > for an example of this causing QEMU to crash.) > > Create a utility function qemu_alloc_stack() which allocates a > block of memory for use as a stack with the correct permissions. > Since we would prefer to make the stack non-executable if we can > as a defence against code execution exploits, we detect whether > the existing stack is mapped executable. Unfortunately this > requires us to grovel through /proc/self/maps to determine the > permissions on it. > > Signed-off-by: Peter Maydell <peter.maydell@linaro.org> > --- > This method of figuring out the correct perms for the stack is > not exactly pretty; better suggestions welcome. > > NB that this utility function also gives us a handy place to put > code for allocating a guard page at the bottom of the stack, or > mapping it as MAP_GROWSDOWN, or whatever. > --- > include/sysemu/os-posix.h | 12 ++++++++ > util/coroutine-sigaltstack.c | 2 +- > util/coroutine-ucontext.c | 2 +- > util/oslib-posix.c | 70 ++++++++++++++++++++++++++++++++++++++++++++ > 4 files changed, 84 insertions(+), 2 deletions(-) > > diff --git a/include/sysemu/os-posix.h b/include/sysemu/os-posix.h > index 9c7dfdf..1dc9d3c 100644 > --- a/include/sysemu/os-posix.h > +++ b/include/sysemu/os-posix.h > @@ -60,4 +60,16 @@ int qemu_utimens(const char *path, const qemu_timespec *times); > > bool is_daemonized(void); > > +/** > + * qemu_alloc_stack: > + * @sz: size of required stack in bytes > + * > + * Allocate memory that can be used as a stack, for instance for > + * coroutines. If the memory cannot be allocated, this function > + * will abort (like g_malloc()). > + * > + * Returns: pointer to (the lowest address of) the stack memory. > + */ > +void *qemu_alloc_stack(size_t sz); > + > #endif > diff --git a/util/coroutine-sigaltstack.c b/util/coroutine-sigaltstack.c > index a7c3366..209f6e5 100644 > --- a/util/coroutine-sigaltstack.c > +++ b/util/coroutine-sigaltstack.c > @@ -164,7 +164,7 @@ Coroutine *qemu_coroutine_new(void) > */ > > co = g_malloc0(sizeof(*co)); > - co->stack = g_malloc(stack_size); > + co->stack = qemu_alloc_stack(stack_size); > co->base.entry_arg = &old_env; /* stash away our jmp_buf */ > > coTS = coroutine_get_thread_state(); > diff --git a/util/coroutine-ucontext.c b/util/coroutine-ucontext.c > index 2bb7e10..a455519 100644 > --- a/util/coroutine-ucontext.c > +++ b/util/coroutine-ucontext.c > @@ -101,7 +101,7 @@ Coroutine *qemu_coroutine_new(void) > } > > co = g_malloc0(sizeof(*co)); > - co->stack = g_malloc(stack_size); > + co->stack = qemu_alloc_stack(stack_size); > co->base.entry_arg = &old_env; /* stash away our jmp_buf */ > > uc.uc_link = &old_uc; > diff --git a/util/oslib-posix.c b/util/oslib-posix.c > index e2e1d4d..311093e 100644 > --- a/util/oslib-posix.c > +++ b/util/oslib-posix.c > @@ -497,3 +497,73 @@ pid_t qemu_fork(Error **errp) > } > return pid; > } > + > +#if defined(__linux__) > +static int stack_prot(void) > +{ > + static int prot; > + gchar *maps, *start, *end; > + > + if (prot) { > + return prot; > + } > + > + /* Some architectures (notably MIPS) require an executable stack, but > + * we would prefer to avoid making the stack executable unnecessarily, > + * to defend against code execution exploits. > + * Check whether the current stack is executable, and follow its lead. > + * Unfortunately to do this we have to wade through /proc/self/maps > + * looking for the stack memory. We default to assuming we need an > + * executable stack and remove the permission only if we can successfully > + * confirm that non-executable is OK. > + */ > + > + prot = PROT_READ | PROT_WRITE | PROT_EXEC; > + > + if (!g_file_get_contents("/proc/self/maps", &maps, NULL, NULL)) { > + return prot; > + } > + > + /* We are looking for a line like this: > + * 7fffbe217000-7fffbe238000 rw-p 00000000 00:00 0 [stack] > + * and checking whether it says 'rw-' or 'rwx'. > + * We look forwards for [stack], then back to the preceding newline, > + * then forwards for the rw- between the two. > + */ > + end = g_strstr_len(maps, -1, "[stack]"); > + if (!end) { > + return prot; > + } > + start = g_strrstr_len(maps, end - maps, "\n"); > + if (!start) { > + start = maps; > + } > + if (g_strstr_len(start, end - start, "rw-")) { > + prot &= ~PROT_EXEC; > + } > + > + return prot; > +} > + > +#else > +static int stack_prot(void) > +{ > + /* Assume an executable stack is needed, since we can't detect it. */ > + return PROT_READ | PROT_WRITE | PROT_EXEC; > +} > +#endif > + > +void *qemu_alloc_stack(size_t sz) > +{ > + /* Allocate memory for the coroutine's stack. > + * Note that some architectures require this to be executable. > + */ > + int prot = stack_prot(); > + void *stack = mmap(NULL, sz, prot, MAP_ANONYMOUS | MAP_PRIVATE, -1, 0); > + > + if (stack == MAP_FAILED) { > + g_error("%s: failed to allocate %zu bytes", __func__, sz); > + } > + > + return stack; > +} > Shouldn't you adapt the qemu_coroutine_delete() functions in "util/coroutine-sigaltstack.c" and "util/coroutine-ucontext.c"? Both of those call g_free(co->stack). I think a qemu_free_stack() function, calling munmap(), is necessary. Thanks, Laszlo
diff --git a/include/sysemu/os-posix.h b/include/sysemu/os-posix.h index 9c7dfdf..1dc9d3c 100644 --- a/include/sysemu/os-posix.h +++ b/include/sysemu/os-posix.h @@ -60,4 +60,16 @@ int qemu_utimens(const char *path, const qemu_timespec *times); bool is_daemonized(void); +/** + * qemu_alloc_stack: + * @sz: size of required stack in bytes + * + * Allocate memory that can be used as a stack, for instance for + * coroutines. If the memory cannot be allocated, this function + * will abort (like g_malloc()). + * + * Returns: pointer to (the lowest address of) the stack memory. + */ +void *qemu_alloc_stack(size_t sz); + #endif diff --git a/util/coroutine-sigaltstack.c b/util/coroutine-sigaltstack.c index a7c3366..209f6e5 100644 --- a/util/coroutine-sigaltstack.c +++ b/util/coroutine-sigaltstack.c @@ -164,7 +164,7 @@ Coroutine *qemu_coroutine_new(void) */ co = g_malloc0(sizeof(*co)); - co->stack = g_malloc(stack_size); + co->stack = qemu_alloc_stack(stack_size); co->base.entry_arg = &old_env; /* stash away our jmp_buf */ coTS = coroutine_get_thread_state(); diff --git a/util/coroutine-ucontext.c b/util/coroutine-ucontext.c index 2bb7e10..a455519 100644 --- a/util/coroutine-ucontext.c +++ b/util/coroutine-ucontext.c @@ -101,7 +101,7 @@ Coroutine *qemu_coroutine_new(void) } co = g_malloc0(sizeof(*co)); - co->stack = g_malloc(stack_size); + co->stack = qemu_alloc_stack(stack_size); co->base.entry_arg = &old_env; /* stash away our jmp_buf */ uc.uc_link = &old_uc; diff --git a/util/oslib-posix.c b/util/oslib-posix.c index e2e1d4d..311093e 100644 --- a/util/oslib-posix.c +++ b/util/oslib-posix.c @@ -497,3 +497,73 @@ pid_t qemu_fork(Error **errp) } return pid; } + +#if defined(__linux__) +static int stack_prot(void) +{ + static int prot; + gchar *maps, *start, *end; + + if (prot) { + return prot; + } + + /* Some architectures (notably MIPS) require an executable stack, but + * we would prefer to avoid making the stack executable unnecessarily, + * to defend against code execution exploits. + * Check whether the current stack is executable, and follow its lead. + * Unfortunately to do this we have to wade through /proc/self/maps + * looking for the stack memory. We default to assuming we need an + * executable stack and remove the permission only if we can successfully + * confirm that non-executable is OK. + */ + + prot = PROT_READ | PROT_WRITE | PROT_EXEC; + + if (!g_file_get_contents("/proc/self/maps", &maps, NULL, NULL)) { + return prot; + } + + /* We are looking for a line like this: + * 7fffbe217000-7fffbe238000 rw-p 00000000 00:00 0 [stack] + * and checking whether it says 'rw-' or 'rwx'. + * We look forwards for [stack], then back to the preceding newline, + * then forwards for the rw- between the two. + */ + end = g_strstr_len(maps, -1, "[stack]"); + if (!end) { + return prot; + } + start = g_strrstr_len(maps, end - maps, "\n"); + if (!start) { + start = maps; + } + if (g_strstr_len(start, end - start, "rw-")) { + prot &= ~PROT_EXEC; + } + + return prot; +} + +#else +static int stack_prot(void) +{ + /* Assume an executable stack is needed, since we can't detect it. */ + return PROT_READ | PROT_WRITE | PROT_EXEC; +} +#endif + +void *qemu_alloc_stack(size_t sz) +{ + /* Allocate memory for the coroutine's stack. + * Note that some architectures require this to be executable. + */ + int prot = stack_prot(); + void *stack = mmap(NULL, sz, prot, MAP_ANONYMOUS | MAP_PRIVATE, -1, 0); + + if (stack == MAP_FAILED) { + g_error("%s: failed to allocate %zu bytes", __func__, sz); + } + + return stack; +}
Some architectures require the stack to be executable; notably this includes MIPS, because the kernel's floating point emulator may try to put trampoline code on the stack to handle some cases. (See https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=815409 for an example of this causing QEMU to crash.) Create a utility function qemu_alloc_stack() which allocates a block of memory for use as a stack with the correct permissions. Since we would prefer to make the stack non-executable if we can as a defence against code execution exploits, we detect whether the existing stack is mapped executable. Unfortunately this requires us to grovel through /proc/self/maps to determine the permissions on it. Signed-off-by: Peter Maydell <peter.maydell@linaro.org> --- This method of figuring out the correct perms for the stack is not exactly pretty; better suggestions welcome. NB that this utility function also gives us a handy place to put code for allocating a guard page at the bottom of the stack, or mapping it as MAP_GROWSDOWN, or whatever. --- include/sysemu/os-posix.h | 12 ++++++++ util/coroutine-sigaltstack.c | 2 +- util/coroutine-ucontext.c | 2 +- util/oslib-posix.c | 70 ++++++++++++++++++++++++++++++++++++++++++++ 4 files changed, 84 insertions(+), 2 deletions(-)