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

[Szabolcs Nagy <nsz@port70.net>]

* 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_*.