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

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

[Szabolcs Nagy]

[-- Attachment #1: Type: text/plain, Size: 1092 bytes --]

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.

[-- Attachment #2: guard.diff --]
[-- Type: text/x-patch, Size: 2760 bytes --]

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.  */
       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
+
 
 /* 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
 

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

[Szabolcs Nagy]

On 14/12/17 16:16, 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.
> 

ping.

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

[Carlos O'Donell]

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.