[RFC PATCH v2 2/4] support SafeStack in init and threading

[RFC PATCH v2 2/4] support SafeStack in init and threading

[LeMay, Michael]

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@intel.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
--- 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
+	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];
+
+__attribute__((no_sanitize("safe-stack"), __visibility__("hidden")))
+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);
+
+	if (pthread_getattr_np(self, &attr) != 0)
+		a_crash();
+
+	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)
 {
 	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

Re: [RFC PATCH v2 2/4] support SafeStack in init and threading

[Szabolcs Nagy]

* LeMay, Michael <michael.lemay@intel.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@intel.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

Re: [RFC PATCH v2 2/4] support SafeStack in init and threading

[LeMay, Michael]

On 11/1/2016 16:52, Szabolcs Nagy wrote:
> * LeMay, Michael <michael.lemay@intel.com> [2016-10-28 20:00:24 +0000]:
...
>> +#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?
Yes, I can do that.

...
>> +/* 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).
I chose to use PAGE_SIZE simply to provide ample space.  It does not 
actually need to match the machine's page size.  This file is 
architecture-independent, so I'll replace PAGE_SIZE with a numeric constant.
> and i'd use char type.
Sure, I'll update that.  For my future reference, can you please tell me 
why char is preferred?
>> +
>> +__attribute__((no_sanitize("safe-stack"), __visibility__("hidden")))
> can this file be whitelisted to remain uninstrumented?
Yes, I'll do that.
>
>> +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?
No.
>
>> +
>> +	if (pthread_getattr_np(self, &attr) != 0)
>> +		a_crash();
> this may have significant startup overhead because determining
> the main stack size is not optimized.
I could use a constant stack size here instead.  However, this will be 
needed for supporting a separate stack segment, since it is used to 
determine the proper limit for the DS and ES segments.
>
>> +
>> +	if (pthread_attr_getstack(&attr, &stack_base, &stack_size) != 0)
>> +		a_crash();
>> +
>> +	stack_size *= 2;
>> +
...
>>   
>> -_Noreturn void __pthread_exit(void *result)
>> +_Noreturn void __pthread_exit_generic(void *result)
> i dont see why this is needed.
Oops, I had implemented an alternative __pthread_exit routine to 
deallocate the safe stack before invoking __pthread_exit_generic, but it 
must have gotten lost during a rebase.

>
...

Re: [RFC PATCH v2 2/4] support SafeStack in init and threading

[Szabolcs Nagy]

* LeMay, Michael <michael.lemay@intel.com> [2016-11-02 09:56:14 -0700]:
> On 11/1/2016 16:52, Szabolcs Nagy wrote:
> > * LeMay, Michael <michael.lemay@intel.com> [2016-10-28 20:00:24 +0000]:
> ...
> > > +#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?
> Yes, I can do that.

i'm actually not sure about this, wait for others to comment

musl uses the directory tree for target specific behaviour
(target specific version of foo.c is arch/foo.*), but
in case of SAFE_STACK that's probably not reasonable.

> > > +/* 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).
> I chose to use PAGE_SIZE simply to provide ample space.  It does not
> actually need to match the machine's page size.  This file is
> architecture-independent, so I'll replace PAGE_SIZE with a numeric constant.
> > and i'd use char type.
> Sure, I'll update that.  For my future reference, can you please tell me why
> char is preferred?

this is just style, others might disagree, but (unsigned) char has
special properties in c (it can alias any object, its pointer has
the same representation as void*, it has size 1) that is not
guaranteed for uint8_t in principle and it is always available
without header includes.

(here only the size 1 property is used which holds for uint8_t too
in posix so both are fine.)

> > > +	if (pthread_getattr_np(self, &attr) != 0)
> > > +		a_crash();
> > this may have significant startup overhead because determining
> > the main stack size is not optimized.
> I could use a constant stack size here instead.  However, this will be
> needed for supporting a separate stack segment, since it is used to
> determine the proper limit for the DS and ES segments.

ok

the pthread_* symbol should be moved to the reserved
namespace (__pthread_*) since this code will get
static linked into iso c conforming code which is
allowed to redefine pthread_*.

Re: [RFC PATCH v2 2/4] support SafeStack in init and threading

[LeMay, Michael]

On 11/2/2016 10:40, Szabolcs Nagy wrote:
> * LeMay, Michael <michael.lemay@intel.com> [2016-11-02 09:56:14 -0700]:
>> On 11/1/2016 16:52, Szabolcs Nagy wrote:
>>> * LeMay, Michael <michael.lemay@intel.com> [2016-10-28 20:00:24 +0000]:
>> ...
>>>> +	if (pthread_getattr_np(self, &attr) != 0)
>>>> +		a_crash();
>>> this may have significant startup overhead because determining
>>> the main stack size is not optimized.
>> I could use a constant stack size here instead.  However, this will be
>> needed for supporting a separate stack segment, since it is used to
>> determine the proper limit for the DS and ES segments.
> ok
>
> the pthread_* symbol should be moved to the reserved
> namespace (__pthread_*) since this code will get
> static linked into iso c conforming code which is
> allowed to redefine pthread_*.
pthread_getattr_np and pthread_attr_getstack currently have strong 
definitions in musl.  Are you proposing that I rename the existing 
definitions into the __pthread_* namespace and add weak aliases?

By the way, I need to update my patch of __pthread_create to pass 
new->stack as the second parameter to __clone, since 
__safestack_init_thread updates that field.