From mboxrd@z Thu Jan 1 00:00:00 1970 X-Msuck: nntp://news.gmane.org/gmane.linux.lib.musl.general/10692 Path: news.gmane.org!.POSTED!not-for-mail From: Szabolcs Nagy Newsgroups: gmane.linux.lib.musl.general Subject: Re: [RFC PATCH v2 2/4] support SafeStack in init and threading Date: Wed, 2 Nov 2016 00:52:19 +0100 Message-ID: <20161101235219.GH5749@port70.net> References: <390CE752059EB848A71F4F676EBAB76D3AC2636A@ORSMSX114.amr.corp.intel.com> Reply-To: musl@lists.openwall.com NNTP-Posting-Host: blaine.gmane.org Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii X-Trace: blaine.gmane.org 1478044380 28578 195.159.176.226 (1 Nov 2016 23:53:00 GMT) X-Complaints-To: usenet@blaine.gmane.org NNTP-Posting-Date: Tue, 1 Nov 2016 23:53:00 +0000 (UTC) User-Agent: Mutt/1.6.0 (2016-04-01) Cc: "musl@lists.openwall.com" To: "LeMay, Michael" Original-X-From: musl-return-10705-gllmg-musl=m.gmane.org@lists.openwall.com Wed Nov 02 00:52:56 2016 Return-path: Envelope-to: gllmg-musl@m.gmane.org Original-Received: from mother.openwall.net ([195.42.179.200]) by blaine.gmane.org with smtp (Exim 4.84_2) (envelope-from ) id 1c1ir0-0003ev-6C for gllmg-musl@m.gmane.org; Wed, 02 Nov 2016 00:52:30 +0100 Original-Received: (qmail 11399 invoked by uid 550); 1 Nov 2016 23:52:31 -0000 Mailing-List: contact musl-help@lists.openwall.com; run by ezmlm Precedence: bulk List-Post: List-Help: List-Unsubscribe: List-Subscribe: List-ID: Original-Received: (qmail 11377 invoked from network); 1 Nov 2016 23:52:30 -0000 Mail-Followup-To: "LeMay, Michael" , "musl@lists.openwall.com" Content-Disposition: inline In-Reply-To: <390CE752059EB848A71F4F676EBAB76D3AC2636A@ORSMSX114.amr.corp.intel.com> Xref: news.gmane.org gmane.linux.lib.musl.general:10692 Archived-At: * LeMay, Michael [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 > --- > 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 > +#include > +#include > + > +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