From mboxrd@z Thu Jan 1 00:00:00 1970 X-Msuck: nntp://news.gmane.org/gmane.linux.lib.musl.general/10694 Path: news.gmane.org!.POSTED!not-for-mail From: "LeMay, Michael" Newsgroups: gmane.linux.lib.musl.general Subject: Re: [RFC PATCH v2 2/4] support SafeStack in init and threading Date: Wed, 2 Nov 2016 09:56:14 -0700 Message-ID: <456a88bf-50bb-479d-11c5-bd82ce116e67@intel.com> References: <390CE752059EB848A71F4F676EBAB76D3AC2636A@ORSMSX114.amr.corp.intel.com> <20161101235219.GH5749@port70.net> Reply-To: musl@lists.openwall.com NNTP-Posting-Host: blaine.gmane.org Mime-Version: 1.0 Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: 7bit X-Trace: blaine.gmane.org 1478105826 7147 195.159.176.226 (2 Nov 2016 16:57:06 GMT) X-Complaints-To: usenet@blaine.gmane.org NNTP-Posting-Date: Wed, 2 Nov 2016 16:57:06 +0000 (UTC) User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:45.0) Gecko/20100101 Thunderbird/45.3.0 Cc: "musl@lists.openwall.com" To: Szabolcs Nagy Original-X-From: musl-return-10707-gllmg-musl=m.gmane.org@lists.openwall.com Wed Nov 02 17:57:02 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 1c1ypw-0005os-Go for gllmg-musl@m.gmane.org; Wed, 02 Nov 2016 17:56:28 +0100 Original-Received: (qmail 23711 invoked by uid 550); 2 Nov 2016 16:56:28 -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 23686 invoked from network); 2 Nov 2016 16:56:27 -0000 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.31,583,1473145200"; d="scan'208";a="26609025" In-Reply-To: <20161101235219.GH5749@port70.net> Xref: news.gmane.org gmane.linux.lib.musl.general:10694 Archived-At: On 11/1/2016 16:52, Szabolcs Nagy wrote: > * LeMay, Michael [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. > ...