Re: [RFC v2] aarch64: enforce >=64K guard size

[Carlos O'Donell <carlos at redhat dot com>]

On 12/14/2017 08:16 AM, Szabolcs Nagy wrote:
> v2:
> - only change guard size on aarch64
> - don't report the inflated guard size
> - this is on top of
> https://sourceware.org/ml/libc-alpha/2017-12/msg00368.html
> 
> There are several compiler implementations that allow large stack
> allocations to jump over the guard page at the end of the stack and
> corrupt memory beyond that. See CVE-2017-1000364.
> 
> Compilers can emit code to probe the stack such that the guard page
> cannot be skipped, but on aarch64 the probe interval is 64K instead
> of the minimum supported page size (4K).
> 
> This patch enforces at least 64K guard on aarch64 unless the guard
> is disabled by setting its size to 0.  For backward compatibility
> reasons the increased guard is not reported, so it is only observable
> by exhausting the address space or parsing /proc/self/maps on linux.
> 
> The patch does not affect threads with user allocated stacks.
> 
> 2017-12-14  Szabolcs Nagy  <szabolcs.nagy@arm.com>
> 
> 	* nptl/allocatestack.c (allocate_stack): Use ARCH_MIN_GUARD_SIZE.
> 	* nptl/descr.h (ARCH_MIN_GUARD_SIZE): Define.
> 	* sysdeps/aarch64/nptl/pthreaddef.h (ARCH_MIN_GUARD_SIZE): Define.

OK if you expand the comment and fix the macro api.

Reviewed-by: Carlos O'Donell <carlos@redhat.com>

> diff --git a/nptl/allocatestack.c b/nptl/allocatestack.c
> index 9525322b1f92bb34aa21dcab28566aecd7434e90..9d47b86cbfc45e40c06e0ee13889fcce48902261 100644
> --- a/nptl/allocatestack.c
> +++ b/nptl/allocatestack.c
> @@ -520,6 +520,7 @@ allocate_stack (const struct pthread_attr *attr, struct pthread **pdp,
>      {
>        /* Allocate some anonymous memory.  If possible use the cache.  */
>        size_t guardsize;
> +      size_t reported_guardsize;
>        size_t reqsize;
>        void *mem;
>        const int prot = (PROT_READ | PROT_WRITE
> @@ -530,8 +531,14 @@ allocate_stack (const struct pthread_attr *attr, struct pthread **pdp,
>        assert (size != 0);
>  
>        /* Make sure the size of the stack is enough for the guard and
> -	 eventually the thread descriptor.  */
> +	 eventually the thread descriptor.  On some targets there is
> +	 a minimum guard size requirement, ARCH_MIN_GUARD_SIZE, so
> +	 internally enforce it (unless the guard was disabled), but
> +	 report the original guard size for backward compatibility.  */

Please document, in the comment, the exact backwards compatibility case we
are solving here.

>        guardsize = (attr->guardsize + pagesize_m1) & ~pagesize_m1;
> +      reported_guardsize = guardsize;
> +      if (guardsize > 0 && guardsize < ARCH_MIN_GUARD_SIZE)
> +	guardsize = ARCH_MIN_GUARD_SIZE;
>        size += guardsize;
>        if (__builtin_expect (size < ((guardsize + __static_tls_size
>  				     + MINIMAL_REST_STACK + pagesize_m1)
> @@ -740,7 +747,7 @@ allocate_stack (const struct pthread_attr *attr, struct pthread **pdp,
>        /* The pthread_getattr_np() calls need to get passed the size
>  	 requested in the attribute, regardless of how large the
>  	 actually used guardsize is.  */
> -      pd->reported_guardsize = guardsize;
> +      pd->reported_guardsize = reported_guardsize;
>      }
>  
>    /* Initialize the lock.  We have to do this unconditionally since the
> diff --git a/nptl/descr.h b/nptl/descr.h
> index c83b17b674b07e5b4e2cbaecf984f6d1673187b5..b5f9412eec0a09e8af10eb9e0c5ff2a8b083559d 100644
> --- a/nptl/descr.h
> +++ b/nptl/descr.h
> @@ -39,6 +39,10 @@
>  # define TCB_ALIGNMENT	sizeof (double)
>  #endif
>  
> +#ifndef ARCH_MIN_GUARD_SIZE
> +# define ARCH_MIN_GUARD_SIZE 0
> +#endif

This is the "centralized defaults" pattern.

This is not a typo-proof macro API. All machines must define 
ARCH_MIN_GUARD_SIZE to zero in their pthreaddef.h instead of
in ntpl/descr.h.

See:
https://sourceware.org/glibc/wiki/Wundef

> +
>  
>  /* We keep thread specific data in a special data structure, a two-level
>     array.  The top-level array contains pointers to dynamically allocated
> diff --git a/sysdeps/aarch64/nptl/pthreaddef.h b/sysdeps/aarch64/nptl/pthreaddef.h
> index d0411a57a1f1356d7e3961f65b733a6b6eb96ae1..5d4c90f83f2b3f3759ab15ee2bc818078ec1f150 100644
> --- a/sysdeps/aarch64/nptl/pthreaddef.h
> +++ b/sysdeps/aarch64/nptl/pthreaddef.h
> @@ -19,6 +19,9 @@
>  /* Default stack size.  */
>  #define ARCH_STACK_DEFAULT_SIZE	(2 * 1024 * 1024)
>  
> +/* Minimum guard size.  */
> +#define ARCH_MIN_GUARD_SIZE (64 * 1024)
> +
>  /* Required stack pointer alignment at beginning.  */
>  #define STACK_ALIGN 16
>  


-- 
Cheers,
Carlos.