[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20161101235219.GH5749@port70.net>
Date: Wed, 2 Nov 2016 00:52:19 +0100
From: Szabolcs Nagy <nsz@...t70.net>
To: "LeMay, Michael" <michael.lemay@...el.com>
Cc: "musl@...ts.openwall.com" <musl@...ts.openwall.com>
Subject: Re: [RFC PATCH v2 2/4] support SafeStack in init and threading
* LeMay, Michael <michael.lemay@...el.com> [2016-10-28 20:00:24 +0000]:
> This patch adds storage in the thread control block for the unsafe
> stack pointer and safe stack metadata for that thread when SafeStack
> is enabled. It modifies the libc initialization routines and thread
> creation routines to allocate and initialize both safe and unsafe
> stacks. Likewise, it modifies the thread destruction routine to unmap
> the safe stack.
>
> Signed-off-by: Michael LeMay <michael.lemay@...el.com>
> ---
> src/env/__libc_start_main.c | 16 +++++++
> src/internal/pthread_impl.h | 13 ++++++
> src/internal/safe_stack.c | 109 ++++++++++++++++++++++++++++++++++++++++++++
> src/thread/pthread_create.c | 16 ++++++-
> 4 files changed, 153 insertions(+), 1 deletion(-)
> create mode 100644 src/internal/safe_stack.c
>
> diff --git a/src/env/__libc_start_main.c b/src/env/__libc_start_main.c
> index 5c79be2..b3c2b0d 100644
> --- a/src/env/__libc_start_main.c
> +++ b/src/env/__libc_start_main.c
> @@ -19,6 +19,11 @@ weak_alias(dummy1, __init_ssp);
>
> #define AUX_CNT 38
>
> +#if SAFE_STACK
> +void __init_unsafe_stack(void);
> +
> +__attribute__((no_sanitize("safe-stack")))
> +#endif
> void __init_libc(char **envp, char *pn)
> {
> size_t i, *auxv, aux[AUX_CNT] = { 0 };
> @@ -36,6 +41,9 @@ void __init_libc(char **envp, char *pn)
> }
>
> __init_tls(aux);
> +#if SAFE_STACK
> + __init_unsafe_stack();
> +#endif
> __init_ssp((void *)aux[AT_RANDOM]);
>
> if (aux[AT_UID]==aux[AT_EUID] && aux[AT_GID]==aux[AT_EGID]
> @@ -63,10 +71,18 @@ static void libc_start_init(void)
>
> weak_alias(libc_start_init, __libc_start_init);
>
> +#if SAFE_STACK
> +void __preinit_unsafe_stack(void);
> +
> +__attribute__((no_sanitize("safe-stack")))
> +#endif
> int __libc_start_main(int (*main)(int,char **,char **), int argc, char **argv)
> {
> char **envp = argv+argc+1;
>
> +#if SAFE_STACK
> + __preinit_unsafe_stack();
> +#endif
> __init_libc(envp, argv[0]);
> __libc_start_init();
>
> diff --git a/src/internal/pthread_impl.h b/src/internal/pthread_impl.h
> index 3890bb5..2b4fc30 100644
in general i'd avoid using ifdefs if possible (e.g. have
the function calls there unconditionally just make them
dummy when there is no safe stack), but i don't know how
to deal with the attribute: can't you whitelist this file
in the makefile so it's not instrumented?
> --- a/src/internal/pthread_impl.h
> +++ b/src/internal/pthread_impl.h
> @@ -18,6 +18,19 @@ struct pthread {
> uintptr_t sysinfo;
> uintptr_t canary, canary2;
> pid_t tid, pid;
> +#if SAFE_STACK
> + /* offset of unsafe_stack_ptr must be 0x24 for 32-bit targets
> + * and 0x48 for 64-bit targets */
> +#if __SIZEOF_POINTER__ == 8
some musl controlled macro should be used here
(LONG_MAX or whatever).
> + void *safe_stack_base;
> + void *unsafe_stack_ptr;
> +#else
> + void *unsafe_stack_ptr;
> + void *safe_stack_base;
> +#endif
> + /* size of safe stack allocation, including the guard */
> + size_t safe_stack_size;
> +#endif
> int tsd_used, errno_val;
> volatile int cancel, canceldisable, cancelasync;
> int detached;
> diff --git a/src/internal/safe_stack.c b/src/internal/safe_stack.c
> new file mode 100644
> index 0000000..7c873e3
> --- /dev/null
> +++ b/src/internal/safe_stack.c
> @@ -0,0 +1,109 @@
> +#if SAFE_STACK
> +
> +#define _GNU_SOURCE
> +#include "pthread_impl.h"
> +#include <stdbool.h>
> +#include <stdint.h>
> +#include <sys/mman.h>
> +
> +static bool unsafe_stack_ptr_inited = false;
> +
> +void *__mmap(void *, size_t, int, int, int, off_t);
> +int __munmap(void *, size_t);
> +int __mprotect(void *, size_t, int);
> +
> +/* There are no checks for overflows past the end of this stack buffer. It must
> + * be allocated with adequate space to meet the requirements of all of the code
> + * that runs prior to __init_unsafe_stack allocating a new unsafe stack. This
> + * buffer is not used after that. */
> +static uint8_t preinit_us[PAGE_SIZE];
note that PAGE_SIZE is not compile time constant expression
on some targets see internal/libc.h (but on x86 this should
work).
and i'd use char type.
> +
> +__attribute__((no_sanitize("safe-stack"), __visibility__("hidden")))
can this file be whitelisted to remain uninstrumented?
> +void __init_unsafe_stack(void)
> +{
> + void *stack_base;
> + size_t stack_size;
> + pthread_attr_t attr;
> + struct pthread *self;
> +
> + if (unsafe_stack_ptr_inited)
> + return;
> +
> + self = __pthread_self();
> +
> + /* Set the unsafe stack pointer in the current TCB to the statically-allocated
> + * unsafe stack, since some of the subroutines invoked below may use the
> + * unsafe stack. */
> + self->unsafe_stack_ptr = preinit_us + sizeof(preinit_us);
is there an alignment requirement for this pointer?
> +
> + if (pthread_getattr_np(self, &attr) != 0)
> + a_crash();
this may have significant startup overhead because determining
the main stack size is not optimized.
> +
> + if (pthread_attr_getstack(&attr, &stack_base, &stack_size) != 0)
> + a_crash();
> +
> + stack_size *= 2;
> +
> + /* This mapping is not reclaimed until the process exits. */
> + uint8_t *unsafe_stack = __mmap(0, stack_size, PROT_NONE, MAP_PRIVATE|MAP_ANON, -1, 0);
> + if (unsafe_stack == MAP_FAILED)
> + a_crash();
> +
> + unsafe_stack += DEFAULT_GUARD_SIZE;
> + stack_size -= DEFAULT_GUARD_SIZE;
> +
> + if (__mprotect(unsafe_stack, stack_size, PROT_READ|PROT_WRITE)
> + && errno != ENOSYS)
> + a_crash();
> +
> + self->unsafe_stack_ptr = unsafe_stack + stack_size;
> +
> + unsafe_stack_ptr_inited = true;
> +}
> +
> +static struct pthread preinit_tcb = {
> + .unsafe_stack_ptr = preinit_us + sizeof(preinit_us)
> +};
> +
> +/* Install a TCB with just the unsafe stack pointer initialized. */
> +__attribute__((no_sanitize("safe-stack"), __visibility__("hidden")))
> +void __preinit_unsafe_stack(void)
> +{
> + if (unsafe_stack_ptr_inited)
> + return;
> +
> + __set_thread_area(&preinit_tcb);
> +}
> +
> +#define ROUND(x) (((x)+PAGE_SIZE-1)&-PAGE_SIZE)
> +
> +__attribute__((__visibility__("hidden")))
> +int __safestack_init_thread(struct pthread *restrict new, const pthread_attr_t *restrict attr)
> +{
> + size_t size, guard;
> + unsigned char *map = 0;
> +
> + new->unsafe_stack_ptr = new->stack;
> +
> + guard = ROUND(DEFAULT_GUARD_SIZE + attr->_a_guardsize);
> + size = ROUND(new->stack_size + guard);
> +
> + map = __mmap(0, size, PROT_NONE, MAP_PRIVATE|MAP_ANON, -1, 0);
> + if (map == MAP_FAILED)
> + goto fail;
> + if (__mprotect(map+guard, size-guard, PROT_READ|PROT_WRITE)
> + && errno != ENOSYS) {
> + __munmap(map, size);
> + goto fail;
> + }
> +
> + new->safe_stack_base = map;
> + new->safe_stack_size = size;
> + new->stack = map + size;
> +
> + return 0;
> +fail:
> + return EAGAIN;
> +}
> +
> +#endif /*SAFE_STACK*/
> diff --git a/src/thread/pthread_create.c b/src/thread/pthread_create.c
> index 9f6b98e..1a8304b 100644
> --- a/src/thread/pthread_create.c
> +++ b/src/thread/pthread_create.c
> @@ -19,7 +19,7 @@ weak_alias(dummy_0, __pthread_tsd_run_dtors);
> weak_alias(dummy_0, __do_orphaned_stdio_locks);
> weak_alias(dummy_0, __dl_thread_cleanup);
>
> -_Noreturn void __pthread_exit(void *result)
> +_Noreturn void __pthread_exit_generic(void *result)
i dont see why this is needed.
> {
> pthread_t self = __pthread_self();
> sigset_t set;
> @@ -116,6 +116,8 @@ _Noreturn void __pthread_exit(void *result)
> for (;;) __syscall(SYS_exit, 0);
> }
>
> +weak_alias(__pthread_exit_generic, __pthread_exit);
> +
> void __do_cleanup_push(struct __ptcb *cb)
> {
> struct pthread *self = __pthread_self();
> @@ -176,6 +178,10 @@ static void init_file_lock(FILE *f)
>
> void *__copy_tls(unsigned char *);
>
> +#if SAFE_STACK
> +int __safestack_init_thread(struct pthread *restrict, const pthread_attr_t *restrict);
> +#endif
> +
> int __pthread_create(pthread_t *restrict res, const pthread_attr_t *restrict attrp, void *(*entry)(void *), void *restrict arg)
> {
> int ret, c11 = (attrp == __ATTRP_C11_THREAD);
> @@ -269,6 +275,14 @@ int __pthread_create(pthread_t *restrict res, const pthread_attr_t *restrict att
> new->robust_list.head = &new->robust_list.head;
> new->unblock_cancel = self->cancel;
> new->CANARY = self->CANARY;
> +#if SAFE_STACK
> + ret = __safestack_init_thread(new, &attr);
> + if (ret != 0) {
> + if (map) __munmap(map, size);
> + __release_ptc();
> + return ret;
> + }
> +#endif
>
> a_inc(&libc.threads_minus_1);
> ret = __clone((c11 ? start_c11 : start), stack, flags, new, &new->tid, TP_ADJ(new), &new->tid);
> --
> 2.7.4
Powered by blists - more mailing lists
Confused about mailing lists and their use?
Read about mailing lists on Wikipedia
and check out these
guidelines on proper formatting of your messages.
