Re: [Qemu-devel] [PATCH] oslib-posix: New qemu_alloc_stack() to allocate stack with correct perms

[Laszlo Ersek]

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 <address@hidden>
> ---
> 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