[PATCH v2 0/4] Improved ASLR

[PATCH v2 0/4] Improved ASLR

[Topi Miettinen]

Problem with using sbrk() for allocations is that the location of the
memory is relatively predicatable since it's always located next to
data segment. This series makes the tunables system, malloc() and TCB
use mmap() instead, except when instructed by tunable
glibc.malloc.use_sbrk.

In this version, mmap() is also used for temporary storage for
tunables environment variable. Since the tunable to select using
sbrk() is unavailable at that point of time, mmap() is always
used. mmap() and mmap_noerrno() (other functions use this suffix) have
been refactored (Adhemerval Zanella), there's also a version for Hurd.

Topi Miettinen (4):
  csu: randomize location of TCB
  malloc: use mmap() to improve ASLR
  dl-sysdep: disable remaining calls to sbrk()
  tunables: use mmap() instead of sbrk()

 csu/libc-tls.c                               | 40 ++++++++++++++++----
 elf/dl-sysdep.c                              | 11 +++++-
 elf/dl-tunables.c                            |  9 +++--
 elf/dl-tunables.list                         |  7 ++++
 include/sys/mman.h                           |  5 +++
 malloc/arena.c                               | 11 +++++-
 malloc/morecore.c                            | 10 +++++
 manual/tunables.texi                         |  5 +++
 sysdeps/mach/hurd/dl-sysdep.c                | 18 +++++++--
 sysdeps/unix/sysv/linux/dl-sysdep.c          | 10 +++++
 sysdeps/unix/sysv/linux/mmap.c               | 30 ++++++++++++---
 sysdeps/unix/sysv/linux/mmap64.c             | 23 ++++++++---
 sysdeps/unix/sysv/linux/mmap_internal.h      |  2 +-
 sysdeps/unix/sysv/linux/s390/mmap_internal.h |  2 +-
 14 files changed, 154 insertions(+), 29 deletions(-)


base-commit: aa69f19a937b679816ef10e8620ea1141bb1734b
-- 
2.29.2

[PATCH 1/4] csu: randomize location of TCB

[Topi Miettinen]

Use mmap() for allocating TCB except if instructed by tunable
glibc.malloc.use_sbrk. This makes the location of TCB random instead
of always staying predictably next to data segment. When using mmap(),
improve the logic so that allocation of TCB can be assumed to fail
insted of segfaulting.

--
v2: introduce a tunable to use sbrk()
v3:
- refactor mmap() (Adhemerval Zanella)
- rename mmap_internal to mmap_noerrno
---
 csu/libc-tls.c                               | 40 ++++++++++++++++----
 elf/dl-tunables.list                         |  7 ++++
 include/sys/mman.h                           |  5 +++
 manual/tunables.texi                         |  5 +++
 sysdeps/mach/hurd/dl-sysdep.c                | 18 +++++++--
 sysdeps/unix/sysv/linux/mmap.c               | 30 ++++++++++++---
 sysdeps/unix/sysv/linux/mmap64.c             | 23 ++++++++---
 sysdeps/unix/sysv/linux/mmap_internal.h      |  2 +-
 sysdeps/unix/sysv/linux/s390/mmap_internal.h |  2 +-
 9 files changed, 109 insertions(+), 23 deletions(-)

diff --git a/csu/libc-tls.c b/csu/libc-tls.c
index c3589f0a7d..0cb6cb2e42 100644
--- a/csu/libc-tls.c
+++ b/csu/libc-tls.c
@@ -25,11 +25,18 @@
 #include <sys/param.h>
 #include <array_length.h>
 #include <list.h>
+#include <sys/mman.h>
+#include <sysdep.h>
 
 #ifdef SHARED
  #error makefile bug, this file is for static only
 #endif
 
+#if HAVE_TUNABLES
+# define TUNABLE_NAMESPACE malloc
+#endif
+#include <elf/dl-tunables.h>
+
 dtv_t _dl_static_dtv[2 + TLS_SLOTINFO_SURPLUS];
 
 
@@ -135,26 +142,45 @@ __libc_setup_tls (void)
 
   /* We have to set up the TCB block which also (possibly) contains
      'errno'.  Therefore we avoid 'malloc' which might touch 'errno'.
-     Instead we use 'sbrk' which would only uses 'errno' if it fails.
-     In this case we are right away out of memory and the user gets
-     what she/he deserves.  */
+     Instead we use '__mmap_noerrno' (when available) which does not
+     use 'errno', except if instructed by tunable
+     glibc.malloc.use_sbrk to use 'sbrk()' instead. If 'sbrk()' fails,
+     it will access 'errno' with catastrophic results. */
+
+  size_t tlsblock_size;
 #if TLS_TCB_AT_TP
   /* Align the TCB offset to the maximum alignment, as
      _dl_allocate_tls_storage (in elf/dl-tls.c) does using __libc_memalign
      and dl_tls_static_align.  */
   tcb_offset = roundup (memsz + GLRO(dl_tls_static_surplus), max_align);
-  tlsblock = __sbrk (tcb_offset + TLS_INIT_TCB_SIZE + max_align);
+  tlsblock_size = tcb_offset + TLS_INIT_TCB_SIZE + max_align;
 #elif TLS_DTV_AT_TP
   tcb_offset = roundup (TLS_INIT_TCB_SIZE, align ?: 1);
-  tlsblock = __sbrk (tcb_offset + memsz + max_align
-		     + TLS_PRE_TCB_SIZE + GLRO(dl_tls_static_surplus));
-  tlsblock += TLS_PRE_TCB_SIZE;
+  tlsblock_size = tcb_offset + memsz + max_align
+	  + TLS_PRE_TCB_SIZE + GLRO(dl_tls_static_surplus);
 #else
   /* In case a model with a different layout for the TCB and DTV
      is defined add another #elif here and in the following #ifs.  */
 # error "Either TLS_TCB_AT_TP or TLS_DTV_AT_TP must be defined"
 #endif
 
+#if HAVE_TUNABLES
+  if (!TUNABLE_GET (use_sbrk, int32_t, NULL))
+    {
+      int error = 0;
+      tlsblock = __mmap_noerrno (NULL, tlsblock_size, PROT_READ | PROT_WRITE,
+				 MAP_ANONYMOUS | MAP_PRIVATE, -1, 0, &error);
+      if (error || tlsblock == MAP_FAILED)
+        _startup_fatal ("Cannot allocate TCB");
+    }
+  else
+#endif
+    tlsblock = __sbrk (tlsblock_size);
+
+#if TLS_DTV_AT_TP
+  tlsblock += TLS_PRE_TCB_SIZE;
+#endif
+
   /* Align the TLS block.  */
   tlsblock = (void *) (((uintptr_t) tlsblock + max_align - 1)
 		       & ~(max_align - 1));
diff --git a/elf/dl-tunables.list b/elf/dl-tunables.list
index e1d8225128..777ebee788 100644
--- a/elf/dl-tunables.list
+++ b/elf/dl-tunables.list
@@ -91,6 +91,13 @@ glibc {
       minval: 0
       security_level: SXID_IGNORE
     }
+    use_sbrk {
+      type: INT_32
+      minval: 0
+      maxval: 1
+      default: 0
+      security_level: SXID_IGNORE
+    }
   }
   cpu {
     hwcap_mask {
diff --git a/include/sys/mman.h b/include/sys/mman.h
index 503edaae88..d2fc5608c3 100644
--- a/include/sys/mman.h
+++ b/include/sys/mman.h
@@ -22,6 +22,11 @@ extern void *__mremap (void *__addr, size_t __old_len,
 		       size_t __new_len, int __flags, ...);
 libc_hidden_proto (__mremap)
 
+/* Internal version of mmap() which doesn't attempt to access errno */
+extern void *__mmap_noerrno (void *addr, size_t len, int prot, int flags,
+			     int fd, off_t offset, int *err);
+libc_hidden_proto (__mmap_noerrno)
+
 # if IS_IN (rtld)
 #  include <dl-mman.h>
 # endif
diff --git a/manual/tunables.texi b/manual/tunables.texi
index d72d7a5ec0..46132900e3 100644
--- a/manual/tunables.texi
+++ b/manual/tunables.texi
@@ -227,6 +227,11 @@ pointer, so add 4 on 32-bit systems or 8 on 64-bit systems to the size
 passed to @code{malloc} for the largest bin size to enable.
 @end deftp
 
+@deftp Tunable glibc.malloc.use_sbrk
+A value of 1 instructs @theglibc{} to use @code{sbrk()} for memory
+allocation instead of @code{mmap()}.
+@end deftp
+
 @node Dynamic Linking Tunables
 @section Dynamic Linking Tunables
 @cindex dynamic linking tunables
diff --git a/sysdeps/mach/hurd/dl-sysdep.c b/sysdeps/mach/hurd/dl-sysdep.c
index 370495710e..40e2919b9d 100644
--- a/sysdeps/mach/hurd/dl-sysdep.c
+++ b/sysdeps/mach/hurd/dl-sysdep.c
@@ -482,9 +482,9 @@ __libc_lseek64 (int fd, off64_t offset, int whence)
   return offset;
 }
 
-check_no_hidden(__mmap);
+check_no_hidden(__mmap_noerrno);
 void *weak_function
-__mmap (void *addr, size_t len, int prot, int flags, int fd, off_t offset)
+__mmap_noerrno (void *addr, size_t len, int prot, int flags, int fd, off_t offset, int *error)
 {
   error_t err;
   vm_prot_t vmprot;
@@ -541,10 +541,22 @@ __mmap (void *addr, size_t len, int prot, int flags, int fd, off_t offset)
     __mach_port_deallocate (__mach_task_self (), memobj_rd);
 
   if (err)
-    return __hurd_fail (err), MAP_FAILED;
+    *error = err;
   return (void *) mapaddr;
 }
 
+check_no_hidden(__mmap);
+void *weak_function
+__mmap (void *addr, size_t len, int prot, int flags, int fd, off_t offset)
+{
+  int err = 0;
+
+  void *r = __mmap_noerrno (addr, len, prot, flags, fd, offset, &err);
+  if (err)
+    return __hurd_fail (err), MAP_FAILED;
+  return r;
+}
+
 check_no_hidden(__fstat64);
 int weak_function
 __fstat64 (int fd, struct stat64 *buf)
diff --git a/sysdeps/unix/sysv/linux/mmap.c b/sysdeps/unix/sysv/linux/mmap.c
index 22f276bb14..19eca3fe18 100644
--- a/sysdeps/unix/sysv/linux/mmap.c
+++ b/sysdeps/unix/sysv/linux/mmap.c
@@ -31,20 +31,38 @@
 # endif
 
 void *
-__mmap (void *addr, size_t len, int prot, int flags, int fd, off_t offset)
+__mmap_noerrno (void *addr, size_t len, int prot, int flags, int fd, off_t offset, int *err)
 {
   MMAP_CHECK_PAGE_UNIT ();
 
   if (offset & MMAP_OFF_LOW_MASK)
-    return (void *) INLINE_SYSCALL_ERROR_RETURN_VALUE (EINVAL);
+    return (void *) -EINVAL;
 
 #ifdef __NR_mmap2
-  return (void *) MMAP_CALL (mmap2, addr, len, prot, flags, fd,
-			     offset / (uint32_t) MMAP2_PAGE_UNIT);
+  long int r = MMAP_CALL (mmap2, addr, len, prot, flags, fd,
+			  offset / (uint32_t) MMAP2_PAGE_UNIT);
 #else
-  return (void *) MMAP_CALL (mmap, addr, len, prot, flags, fd,
-			     MMAP_ADJUST_OFFSET (offset));
+  long int r = MMAP_CALL (mmap, addr, len, prot, flags, fd,
+			  MMAP_ADJUST_OFFSET (offset));
 #endif
+  if (INTERNAL_SYSCALL_ERROR_P (r))
+    {
+      *err = (INTERNAL_SYSCALL_ERRNO (r));
+      return MAP_FAILED;
+    }
+  return (void*) r;
+}
+libc_hidden_def (__mmap_noerrno)
+
+void *
+__mmap (void *addr, size_t len, int prot, int flags, int fd, off_t offset)
+{
+  int error = 0;
+
+  void *r = __mmap_noerrno (addr, len, prot, flags, fd, offset, &error);
+  if (error)
+    __set_errno(error);
+  return r;
 }
 weak_alias (__mmap, mmap)
 libc_hidden_def (__mmap)
diff --git a/sysdeps/unix/sysv/linux/mmap64.c b/sysdeps/unix/sysv/linux/mmap64.c
index 8074deb466..3d6557734b 100644
--- a/sysdeps/unix/sysv/linux/mmap64.c
+++ b/sysdeps/unix/sysv/linux/mmap64.c
@@ -44,25 +44,38 @@
 #endif
 
 void *
-__mmap64 (void *addr, size_t len, int prot, int flags, int fd, off64_t offset)
+__mmap64_noerrno (void *addr, size_t len, int prot, int flags, int fd, off64_t offset, int *err)
 {
   MMAP_CHECK_PAGE_UNIT ();
 
   if (offset & MMAP_OFF_MASK)
-    return (void *) INLINE_SYSCALL_ERROR_RETURN_VALUE (EINVAL);
+    return (void *) -EINVAL;
 
   MMAP_PREPARE (addr, len, prot, flags, fd, offset);
 #ifdef __NR_mmap2
-  return (void *) MMAP_CALL (mmap2, addr, len, prot, flags, fd,
-			     (off_t) (offset / MMAP2_PAGE_UNIT));
+  long int r = MMAP_CALL (mmap2, addr, len, prot, flags, fd,
+			  (off_t) (offset / MMAP2_PAGE_UNIT));
 #else
-  return (void *) MMAP_CALL (mmap, addr, len, prot, flags, fd, offset);
+  long int r = MMAP_CALL (mmap, addr, len, prot, flags, fd, offset);
 #endif
+  if (INTERNAL_SYSCALL_ERROR_P (r))
+    {
+      *err = INTERNAL_SYSCALL_ERRNO (r);
+      return MAP_FAILED;
+    }
+  return (void *) r;
+}
+
+void *
+__mmap64 (void *addr, size_t len, int prot, int flags, int fd, off64_t offset)
+{
+  return __mmap64_noerrno (addr, len, prot, flags, fd, offset, &errno);
 }
 weak_alias (__mmap64, mmap64)
 libc_hidden_def (__mmap64)
 
 #ifdef __OFF_T_MATCHES_OFF64_T
+weak_alias (__mmap64_noerrno, __mmap_noerrno)
 weak_alias (__mmap64, mmap)
 weak_alias (__mmap64, __mmap)
 libc_hidden_def (__mmap)
diff --git a/sysdeps/unix/sysv/linux/mmap_internal.h b/sysdeps/unix/sysv/linux/mmap_internal.h
index d53f0c642a..5386b5eb63 100644
--- a/sysdeps/unix/sysv/linux/mmap_internal.h
+++ b/sysdeps/unix/sysv/linux/mmap_internal.h
@@ -43,7 +43,7 @@ static uint64_t page_unit;
 /* An architecture may override this.  */
 #ifndef MMAP_CALL
 # define MMAP_CALL(__nr, __addr, __len, __prot, __flags, __fd, __offset) \
-  INLINE_SYSCALL_CALL (__nr, __addr, __len, __prot, __flags, __fd, __offset)
+  INTERNAL_SYSCALL_CALL (__nr, __addr, __len, __prot, __flags, __fd, __offset)
 #endif
 
 #endif /* MMAP_INTERNAL_LINUX_H  */
diff --git a/sysdeps/unix/sysv/linux/s390/mmap_internal.h b/sysdeps/unix/sysv/linux/s390/mmap_internal.h
index 2884f2844b..d2289f311c 100644
--- a/sysdeps/unix/sysv/linux/s390/mmap_internal.h
+++ b/sysdeps/unix/sysv/linux/s390/mmap_internal.h
@@ -24,7 +24,7 @@
     long int __args[6] = { (long int) (__addr), (long int) (__len),	\
 			   (long int) (__prot), (long int) (__flags),	\
 			   (long int) (__fd), (long int) (__offset) };	\
-    INLINE_SYSCALL_CALL (__nr, __args);					\
+    INTERNAL_SYSCALL_CALL (__nr, __args);					\
   })
 
 #include_next <mmap_internal.h>
-- 
2.29.2

[PATCH 2/4] malloc: use mmap() to improve ASLR

[Topi Miettinen]

sbrk() returns rather predictable allocations because they are located
close to the data segment. Let's use mmap() instead, except if
instructed by a tunable.

--
v2: use tunable
---
 malloc/arena.c    | 11 +++++++++--
 malloc/morecore.c | 10 ++++++++++
 2 files changed, 19 insertions(+), 2 deletions(-)

diff --git a/malloc/arena.c b/malloc/arena.c
index 202daf15b0..129e231bae 100644
--- a/malloc/arena.c
+++ b/malloc/arena.c
@@ -295,14 +295,21 @@ ptmalloc_init (void)
 
 #ifdef SHARED
   /* In case this libc copy is in a non-default namespace, never use brk.
-     Likewise if dlopened from statically linked program.  */
+     Likewise if dlopened from statically linked program.
+     Otherwise the use of brk is controlled by a tunable
+     glibc.malloc.use_sbrk. */
   Dl_info di;
   struct link_map *l;
 
   if (_dl_open_hook != NULL
       || (_dl_addr (ptmalloc_init, &di, &l, NULL) != 0
-          && l->l_ns != LM_ID_BASE))
+          && l->l_ns != LM_ID_BASE)
+#if HAVE_TUNABLES
+      || !TUNABLE_GET (use_sbrk, int32_t, NULL)
+#endif
+      )
     __morecore = __failing_morecore;
+
 #endif
 
   thread_arena = &main_arena;
diff --git a/malloc/morecore.c b/malloc/morecore.c
index 72e655f84f..d5da5ffc45 100644
--- a/malloc/morecore.c
+++ b/malloc/morecore.c
@@ -38,12 +38,22 @@ libc_hidden_proto (__sbrk)
 # define NULL 0
 #endif
 
+#if HAVE_TUNABLES
+# define TUNABLE_NAMESPACE malloc
+#endif
+#include <elf/dl-tunables.h>
+
 /* Allocate INCREMENT more bytes of data space,
    and return the start of data space, or NULL on errors.
    If INCREMENT is negative, shrink data space.  */
 void *
 __default_morecore (ptrdiff_t increment)
 {
+  /* Tunable glibc.malloc.use_sbrk controls use of 'sbrk()'. */
+#if HAVE_TUNABLES
+  if (!TUNABLE_GET (use_sbrk, int32_t, NULL))
+    return NULL;
+#endif
   void *result = (void *) __sbrk (increment);
   if (result == (void *) -1)
     return NULL;
-- 
2.29.2

[PATCH 3/4] dl-sysdep: disable remaining calls to sbrk()

[Topi Miettinen]

When sbrk() is not used for memory allocations, there's no need to
initialize the break anymore.

--
v2: use tunable
---
 elf/dl-sysdep.c                     | 11 ++++++++++-
 sysdeps/unix/sysv/linux/dl-sysdep.c | 10 ++++++++++
 2 files changed, 20 insertions(+), 1 deletion(-)

diff --git a/elf/dl-sysdep.c b/elf/dl-sysdep.c
index 854570821c..406b3a3809 100644
--- a/elf/dl-sysdep.c
+++ b/elf/dl-sysdep.c
@@ -44,6 +44,10 @@
 #include <libc-internal.h>
 #include <tls.h>
 
+#if HAVE_TUNABLES
+# undef TUNABLE_NAMESPACE
+# define TUNABLE_NAMESPACE malloc
+#endif
 #include <dl-tunables.h>
 #include <dl-auxv.h>
 
@@ -234,7 +238,12 @@ _dl_sysdep_start (void **start_argptr,
   if (GLRO(dl_platform) != NULL)
     GLRO(dl_platformlen) = strlen (GLRO(dl_platform));
 
-  if (__sbrk (0) == _end)
+  /* Tunable glibc.malloc.use_sbrk controls use of 'sbrk()'. */
+  if (
+#if HAVE_TUNABLES
+      TUNABLE_GET (use_sbrk, int32_t, NULL) &&
+#endif
+      __sbrk (0) == _end)
     /* The dynamic linker was run as a program, and so the initial break
        starts just after our bss, at &_end.  The malloc in dl-minimal.c
        will consume the rest of this page, so tell the kernel to move the
diff --git a/sysdeps/unix/sysv/linux/dl-sysdep.c b/sysdeps/unix/sysv/linux/dl-sysdep.c
index 90c9b1db2d..625e6cb759 100644
--- a/sysdeps/unix/sysv/linux/dl-sysdep.c
+++ b/sysdeps/unix/sysv/linux/dl-sysdep.c
@@ -27,14 +27,24 @@
 #include <ldsodefs.h>
 #include <not-cancel.h>
 
+#if HAVE_TUNABLES
+# undef TUNABLE_NAMESPACE
+# define TUNABLE_NAMESPACE malloc
+#endif
+
 #ifdef SHARED
 # define DL_SYSDEP_INIT frob_brk ()
 
 static inline void
 frob_brk (void)
 {
+  /* Tunable glibc.malloc.use_sbrk controls use of 'sbrk()'. */
+#if HAVE_TUNABLES
+  if (TUNABLE_GET (use_sbrk, int32_t, NULL))
+#endif
   __brk (0);			/* Initialize the break.  */
 }
+# undef TUNABLE_NAMESPACE
 
 # include <elf/dl-sysdep.c>
 #endif
-- 
2.29.2

[PATCH 4/4] tunables: use mmap() instead of sbrk()

[Topi Miettinen]

Randomize the location of copy of tunable environment variable, at the
cost of losing up to 4095 bytes.
---
 elf/dl-tunables.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/elf/dl-tunables.c b/elf/dl-tunables.c
index 2ba2844075..4ca15eb30f 100644
--- a/elf/dl-tunables.c
+++ b/elf/dl-tunables.c
@@ -43,13 +43,16 @@ tunables_strdup (const char *in)
   size_t i = 0;
 
   while (in[i++] != '\0');
-  char *out = __sbrk (i);
+
+  int error = 0;
+  char *out = __mmap_noerrno (NULL, ALIGN_UP (i, GLRO(dl_pagesize)), PROT_READ | PROT_WRITE,
+			      MAP_ANONYMOUS | MAP_PRIVATE, -1, 0, &error);
 
   /* For most of the tunables code, we ignore user errors.  However,
      this is a system error - and running out of memory at program
      startup should be reported, so we do.  */
-  if (out == (void *)-1)
-    _dl_fatal_printf ("sbrk() failure while processing tunables\n");
+  if (error || out == MAP_FAILED)
+    _dl_fatal_printf ("mmap() failure while processing tunables\n");
 
   i--;
 
-- 
2.29.2

Re: [PATCH 1/4] csu: randomize location of TCB

[Topi Miettinen]

On 28.11.2020 13.59, Topi Miettinen wrote:
> Use mmap() for allocating TCB except if instructed by tunable
> glibc.malloc.use_sbrk. This makes the location of TCB random instead
> of always staying predictably next to data segment. When using mmap(),
> improve the logic so that allocation of TCB can be assumed to fail
> insted of segfaulting.

insted -> instead

> 
> --
> v2: introduce a tunable to use sbrk()
> v3:
> - refactor mmap() (Adhemerval Zanella)
> - rename mmap_internal to mmap_noerrno
> ---
>   csu/libc-tls.c                               | 40 ++++++++++++++++----
>   elf/dl-tunables.list                         |  7 ++++
>   include/sys/mman.h                           |  5 +++
>   manual/tunables.texi                         |  5 +++
>   sysdeps/mach/hurd/dl-sysdep.c                | 18 +++++++--
>   sysdeps/unix/sysv/linux/mmap.c               | 30 ++++++++++++---
>   sysdeps/unix/sysv/linux/mmap64.c             | 23 ++++++++---
>   sysdeps/unix/sysv/linux/mmap_internal.h      |  2 +-
>   sysdeps/unix/sysv/linux/s390/mmap_internal.h |  2 +-
>   9 files changed, 109 insertions(+), 23 deletions(-)
> 
> diff --git a/csu/libc-tls.c b/csu/libc-tls.c
> index c3589f0a7d..0cb6cb2e42 100644
> --- a/csu/libc-tls.c
> +++ b/csu/libc-tls.c
> @@ -25,11 +25,18 @@
>   #include <sys/param.h>
>   #include <array_length.h>
>   #include <list.h>
> +#include <sys/mman.h>
> +#include <sysdep.h>
>   
>   #ifdef SHARED
>    #error makefile bug, this file is for static only
>   #endif
>   
> +#if HAVE_TUNABLES
> +# define TUNABLE_NAMESPACE malloc
> +#endif
> +#include <elf/dl-tunables.h>
> +
>   dtv_t _dl_static_dtv[2 + TLS_SLOTINFO_SURPLUS];
>   
>   
> @@ -135,26 +142,45 @@ __libc_setup_tls (void)
>   
>     /* We have to set up the TCB block which also (possibly) contains
>        'errno'.  Therefore we avoid 'malloc' which might touch 'errno'.
> -     Instead we use 'sbrk' which would only uses 'errno' if it fails.
> -     In this case we are right away out of memory and the user gets
> -     what she/he deserves.  */
> +     Instead we use '__mmap_noerrno' (when available) which does not
> +     use 'errno', except if instructed by tunable
> +     glibc.malloc.use_sbrk to use 'sbrk()' instead. If 'sbrk()' fails,
> +     it will access 'errno' with catastrophic results. */
> +
> +  size_t tlsblock_size;
>   #if TLS_TCB_AT_TP
>     /* Align the TCB offset to the maximum alignment, as
>        _dl_allocate_tls_storage (in elf/dl-tls.c) does using __libc_memalign
>        and dl_tls_static_align.  */
>     tcb_offset = roundup (memsz + GLRO(dl_tls_static_surplus), max_align);
> -  tlsblock = __sbrk (tcb_offset + TLS_INIT_TCB_SIZE + max_align);
> +  tlsblock_size = tcb_offset + TLS_INIT_TCB_SIZE + max_align;
>   #elif TLS_DTV_AT_TP
>     tcb_offset = roundup (TLS_INIT_TCB_SIZE, align ?: 1);
> -  tlsblock = __sbrk (tcb_offset + memsz + max_align
> -		     + TLS_PRE_TCB_SIZE + GLRO(dl_tls_static_surplus));
> -  tlsblock += TLS_PRE_TCB_SIZE;
> +  tlsblock_size = tcb_offset + memsz + max_align
> +	  + TLS_PRE_TCB_SIZE + GLRO(dl_tls_static_surplus);
>   #else
>     /* In case a model with a different layout for the TCB and DTV
>        is defined add another #elif here and in the following #ifs.  */
>   # error "Either TLS_TCB_AT_TP or TLS_DTV_AT_TP must be defined"
>   #endif
>   
> +#if HAVE_TUNABLES
> +  if (!TUNABLE_GET (use_sbrk, int32_t, NULL))
> +    {
> +      int error = 0;
> +      tlsblock = __mmap_noerrno (NULL, tlsblock_size, PROT_READ | PROT_WRITE,

Forgot to add ALIGN_UP (tlsblock_size, GLRO(dl_pagesize)).

> +				 MAP_ANONYMOUS | MAP_PRIVATE, -1, 0, &error);
> +      if (error || tlsblock == MAP_FAILED)
> +        _startup_fatal ("Cannot allocate TCB");
> +    }
> +  else
> +#endif
> +    tlsblock = __sbrk (tlsblock_size);
> +
> +#if TLS_DTV_AT_TP
> +  tlsblock += TLS_PRE_TCB_SIZE;
> +#endif
> +
>     /* Align the TLS block.  */
>     tlsblock = (void *) (((uintptr_t) tlsblock + max_align - 1)
>   		       & ~(max_align - 1));

This probably is not necessary for the mmap() case since it returns page 
aligned pointers, except if the addition above tlsblock += 
TLS_PRE_TCB_SIZE makes the alignment worse than needed.

> diff --git a/elf/dl-tunables.list b/elf/dl-tunables.list
> index e1d8225128..777ebee788 100644
> --- a/elf/dl-tunables.list
> +++ b/elf/dl-tunables.list
> @@ -91,6 +91,13 @@ glibc {
>         minval: 0
>         security_level: SXID_IGNORE
>       }
> +    use_sbrk {
> +      type: INT_32
> +      minval: 0
> +      maxval: 1
> +      default: 0
> +      security_level: SXID_IGNORE
> +    }
>     }
>     cpu {
>       hwcap_mask {
> diff --git a/include/sys/mman.h b/include/sys/mman.h
> index 503edaae88..d2fc5608c3 100644
> --- a/include/sys/mman.h
> +++ b/include/sys/mman.h
> @@ -22,6 +22,11 @@ extern void *__mremap (void *__addr, size_t __old_len,
>   		       size_t __new_len, int __flags, ...);
>   libc_hidden_proto (__mremap)
>   
> +/* Internal version of mmap() which doesn't attempt to access errno */
> +extern void *__mmap_noerrno (void *addr, size_t len, int prot, int flags,
> +			     int fd, off_t offset, int *err);
> +libc_hidden_proto (__mmap_noerrno)
> +
>   # if IS_IN (rtld)
>   #  include <dl-mman.h>
>   # endif
> diff --git a/manual/tunables.texi b/manual/tunables.texi
> index d72d7a5ec0..46132900e3 100644
> --- a/manual/tunables.texi
> +++ b/manual/tunables.texi
> @@ -227,6 +227,11 @@ pointer, so add 4 on 32-bit systems or 8 on 64-bit systems to the size
>   passed to @code{malloc} for the largest bin size to enable.
>   @end deftp
>   
> +@deftp Tunable glibc.malloc.use_sbrk
> +A value of 1 instructs @theglibc{} to use @code{sbrk()} for memory
> +allocation instead of @code{mmap()}.
> +@end deftp
> +
>   @node Dynamic Linking Tunables
>   @section Dynamic Linking Tunables
>   @cindex dynamic linking tunables
> diff --git a/sysdeps/mach/hurd/dl-sysdep.c b/sysdeps/mach/hurd/dl-sysdep.c
> index 370495710e..40e2919b9d 100644
> --- a/sysdeps/mach/hurd/dl-sysdep.c
> +++ b/sysdeps/mach/hurd/dl-sysdep.c
> @@ -482,9 +482,9 @@ __libc_lseek64 (int fd, off64_t offset, int whence)
>     return offset;
>   }
>   
> -check_no_hidden(__mmap);
> +check_no_hidden(__mmap_noerrno);
>   void *weak_function
> -__mmap (void *addr, size_t len, int prot, int flags, int fd, off_t offset)
> +__mmap_noerrno (void *addr, size_t len, int prot, int flags, int fd, off_t offset, int *error)
>   {
>     error_t err;
>     vm_prot_t vmprot;
> @@ -541,10 +541,22 @@ __mmap (void *addr, size_t len, int prot, int flags, int fd, off_t offset)
>       __mach_port_deallocate (__mach_task_self (), memobj_rd);
>   
>     if (err)
> -    return __hurd_fail (err), MAP_FAILED;
> +    *error = err;
>     return (void *) mapaddr;
>   }
>   
> +check_no_hidden(__mmap);
> +void *weak_function
> +__mmap (void *addr, size_t len, int prot, int flags, int fd, off_t offset)
> +{
> +  int err = 0;
> +
> +  void *r = __mmap_noerrno (addr, len, prot, flags, fd, offset, &err);
> +  if (err)
> +    return __hurd_fail (err), MAP_FAILED;
> +  return r;
> +}
> +
>   check_no_hidden(__fstat64);
>   int weak_function
>   __fstat64 (int fd, struct stat64 *buf)
> diff --git a/sysdeps/unix/sysv/linux/mmap.c b/sysdeps/unix/sysv/linux/mmap.c
> index 22f276bb14..19eca3fe18 100644
> --- a/sysdeps/unix/sysv/linux/mmap.c
> +++ b/sysdeps/unix/sysv/linux/mmap.c
> @@ -31,20 +31,38 @@
>   # endif
>   
>   void *
> -__mmap (void *addr, size_t len, int prot, int flags, int fd, off_t offset)
> +__mmap_noerrno (void *addr, size_t len, int prot, int flags, int fd, off_t offset, int *err)
>   {
>     MMAP_CHECK_PAGE_UNIT ();
>   
>     if (offset & MMAP_OFF_LOW_MASK)
> -    return (void *) INLINE_SYSCALL_ERROR_RETURN_VALUE (EINVAL);
> +    return (void *) -EINVAL;
>   
>   #ifdef __NR_mmap2
> -  return (void *) MMAP_CALL (mmap2, addr, len, prot, flags, fd,
> -			     offset / (uint32_t) MMAP2_PAGE_UNIT);
> +  long int r = MMAP_CALL (mmap2, addr, len, prot, flags, fd,
> +			  offset / (uint32_t) MMAP2_PAGE_UNIT);
>   #else
> -  return (void *) MMAP_CALL (mmap, addr, len, prot, flags, fd,
> -			     MMAP_ADJUST_OFFSET (offset));
> +  long int r = MMAP_CALL (mmap, addr, len, prot, flags, fd,
> +			  MMAP_ADJUST_OFFSET (offset));
>   #endif
> +  if (INTERNAL_SYSCALL_ERROR_P (r))
> +    {
> +      *err = (INTERNAL_SYSCALL_ERRNO (r));
> +      return MAP_FAILED;
> +    }
> +  return (void*) r;
> +}
> +libc_hidden_def (__mmap_noerrno)
> +
> +void *
> +__mmap (void *addr, size_t len, int prot, int flags, int fd, off_t offset)
> +{
> +  int error = 0;
> +
> +  void *r = __mmap_noerrno (addr, len, prot, flags, fd, offset, &error);
> +  if (error)
> +    __set_errno(error);

I suppose errno should be cleared if there's no error, so remove the check.

-Topi

> +  return r;
>   }
>   weak_alias (__mmap, mmap)
>   libc_hidden_def (__mmap)
> diff --git a/sysdeps/unix/sysv/linux/mmap64.c b/sysdeps/unix/sysv/linux/mmap64.c
> index 8074deb466..3d6557734b 100644
> --- a/sysdeps/unix/sysv/linux/mmap64.c
> +++ b/sysdeps/unix/sysv/linux/mmap64.c
> @@ -44,25 +44,38 @@
>   #endif
>   
>   void *
> -__mmap64 (void *addr, size_t len, int prot, int flags, int fd, off64_t offset)
> +__mmap64_noerrno (void *addr, size_t len, int prot, int flags, int fd, off64_t offset, int *err)
>   {
>     MMAP_CHECK_PAGE_UNIT ();
>   
>     if (offset & MMAP_OFF_MASK)
> -    return (void *) INLINE_SYSCALL_ERROR_RETURN_VALUE (EINVAL);
> +    return (void *) -EINVAL;
>   
>     MMAP_PREPARE (addr, len, prot, flags, fd, offset);
>   #ifdef __NR_mmap2
> -  return (void *) MMAP_CALL (mmap2, addr, len, prot, flags, fd,
> -			     (off_t) (offset / MMAP2_PAGE_UNIT));
> +  long int r = MMAP_CALL (mmap2, addr, len, prot, flags, fd,
> +			  (off_t) (offset / MMAP2_PAGE_UNIT));
>   #else
> -  return (void *) MMAP_CALL (mmap, addr, len, prot, flags, fd, offset);
> +  long int r = MMAP_CALL (mmap, addr, len, prot, flags, fd, offset);
>   #endif
> +  if (INTERNAL_SYSCALL_ERROR_P (r))
> +    {
> +      *err = INTERNAL_SYSCALL_ERRNO (r);
> +      return MAP_FAILED;
> +    }
> +  return (void *) r;
> +}
> +
> +void *
> +__mmap64 (void *addr, size_t len, int prot, int flags, int fd, off64_t offset)
> +{
> +  return __mmap64_noerrno (addr, len, prot, flags, fd, offset, &errno);
>   }
>   weak_alias (__mmap64, mmap64)
>   libc_hidden_def (__mmap64)
>   
>   #ifdef __OFF_T_MATCHES_OFF64_T
> +weak_alias (__mmap64_noerrno, __mmap_noerrno)
>   weak_alias (__mmap64, mmap)
>   weak_alias (__mmap64, __mmap)
>   libc_hidden_def (__mmap)
> diff --git a/sysdeps/unix/sysv/linux/mmap_internal.h b/sysdeps/unix/sysv/linux/mmap_internal.h
> index d53f0c642a..5386b5eb63 100644
> --- a/sysdeps/unix/sysv/linux/mmap_internal.h
> +++ b/sysdeps/unix/sysv/linux/mmap_internal.h
> @@ -43,7 +43,7 @@ static uint64_t page_unit;
>   /* An architecture may override this.  */
>   #ifndef MMAP_CALL
>   # define MMAP_CALL(__nr, __addr, __len, __prot, __flags, __fd, __offset) \
> -  INLINE_SYSCALL_CALL (__nr, __addr, __len, __prot, __flags, __fd, __offset)
> +  INTERNAL_SYSCALL_CALL (__nr, __addr, __len, __prot, __flags, __fd, __offset)
>   #endif
>   
>   #endif /* MMAP_INTERNAL_LINUX_H  */
> diff --git a/sysdeps/unix/sysv/linux/s390/mmap_internal.h b/sysdeps/unix/sysv/linux/s390/mmap_internal.h
> index 2884f2844b..d2289f311c 100644
> --- a/sysdeps/unix/sysv/linux/s390/mmap_internal.h
> +++ b/sysdeps/unix/sysv/linux/s390/mmap_internal.h
> @@ -24,7 +24,7 @@
>       long int __args[6] = { (long int) (__addr), (long int) (__len),	\
>   			   (long int) (__prot), (long int) (__flags),	\
>   			   (long int) (__fd), (long int) (__offset) };	\
> -    INLINE_SYSCALL_CALL (__nr, __args);					\
> +    INTERNAL_SYSCALL_CALL (__nr, __args);					\
>     })
>   
>   #include_next <mmap_internal.h>
> 

Re: [PATCH v2 0/4] Improved ASLR

[Rich Felker]

On Sat, Nov 28, 2020 at 01:59:41PM +0200, Topi Miettinen via Libc-alpha wrote:
> Problem with using sbrk() for allocations is that the location of the
> memory is relatively predicatable since it's always located next to
> data segment. This series makes the tunables system, malloc() and TCB
> use mmap() instead, except when instructed by tunable
> glibc.malloc.use_sbrk.

The above description is contrary to present reality on Linux. With
kernel.randomize_va_space=2 (default), the brk area starts at a
randomize gap above end of data/bss. This is *stronger* ASLR than
mmap, which aside from the initial gap, generally appears just below
the previous map and thereby at a predictable offset from an anchor in
a shared library.

Rich


> In this version, mmap() is also used for temporary storage for
> tunables environment variable. Since the tunable to select using
> sbrk() is unavailable at that point of time, mmap() is always
> used. mmap() and mmap_noerrno() (other functions use this suffix) have
> been refactored (Adhemerval Zanella), there's also a version for Hurd.
> 
> Topi Miettinen (4):
>   csu: randomize location of TCB
>   malloc: use mmap() to improve ASLR
>   dl-sysdep: disable remaining calls to sbrk()
>   tunables: use mmap() instead of sbrk()
> 
>  csu/libc-tls.c                               | 40 ++++++++++++++++----
>  elf/dl-sysdep.c                              | 11 +++++-
>  elf/dl-tunables.c                            |  9 +++--
>  elf/dl-tunables.list                         |  7 ++++
>  include/sys/mman.h                           |  5 +++
>  malloc/arena.c                               | 11 +++++-
>  malloc/morecore.c                            | 10 +++++
>  manual/tunables.texi                         |  5 +++
>  sysdeps/mach/hurd/dl-sysdep.c                | 18 +++++++--
>  sysdeps/unix/sysv/linux/dl-sysdep.c          | 10 +++++
>  sysdeps/unix/sysv/linux/mmap.c               | 30 ++++++++++++---
>  sysdeps/unix/sysv/linux/mmap64.c             | 23 ++++++++---
>  sysdeps/unix/sysv/linux/mmap_internal.h      |  2 +-
>  sysdeps/unix/sysv/linux/s390/mmap_internal.h |  2 +-
>  14 files changed, 154 insertions(+), 29 deletions(-)
> 
> 
> base-commit: aa69f19a937b679816ef10e8620ea1141bb1734b
> -- 
> 2.29.2

Re: [PATCH v2 0/4] Improved ASLR

[Topi Miettinen]

On 3.12.2020 1.09, Rich Felker wrote:
> On Sat, Nov 28, 2020 at 01:59:41PM +0200, Topi Miettinen via Libc-alpha wrote:
>> Problem with using sbrk() for allocations is that the location of the
>> memory is relatively predicatable since it's always located next to
>> data segment. This series makes the tunables system, malloc() and TCB
>> use mmap() instead, except when instructed by tunable
>> glibc.malloc.use_sbrk.
> 
> The above description is contrary to present reality on Linux. With
> kernel.randomize_va_space=2 (default), the brk area starts at a
> randomize gap above end of data/bss. This is *stronger* ASLR than
> mmap, which aside from the initial gap, generally appears just below
> the previous map and thereby at a predictable offset from an anchor in
> a shared library.

Thanks for the review. I'd note that the randomization for brk is only 
12 bits, so it's still relatively predictable. With 
randomize_va_space=3, the randomization provided by mmap() will be much 
better, the maximum allowed by the kernel design. The best in all cases 
would be if randomize_va_space=3 could be detected by libc and then it 
would change the default choice between brk() and mmap() automatically, 
or libc could generate a random address by itself and use 
mmap(random_addr,, MAP_FIXED_NOREPLACE) instead of brk(). For example, 
kernel could use aux vectors to pass the information.

-Topi

> 
> Rich
> 
> 
>> In this version, mmap() is also used for temporary storage for
>> tunables environment variable. Since the tunable to select using
>> sbrk() is unavailable at that point of time, mmap() is always
>> used. mmap() and mmap_noerrno() (other functions use this suffix) have
>> been refactored (Adhemerval Zanella), there's also a version for Hurd.
>>
>> Topi Miettinen (4):
>>    csu: randomize location of TCB
>>    malloc: use mmap() to improve ASLR
>>    dl-sysdep: disable remaining calls to sbrk()
>>    tunables: use mmap() instead of sbrk()
>>
>>   csu/libc-tls.c                               | 40 ++++++++++++++++----
>>   elf/dl-sysdep.c                              | 11 +++++-
>>   elf/dl-tunables.c                            |  9 +++--
>>   elf/dl-tunables.list                         |  7 ++++
>>   include/sys/mman.h                           |  5 +++
>>   malloc/arena.c                               | 11 +++++-
>>   malloc/morecore.c                            | 10 +++++
>>   manual/tunables.texi                         |  5 +++
>>   sysdeps/mach/hurd/dl-sysdep.c                | 18 +++++++--
>>   sysdeps/unix/sysv/linux/dl-sysdep.c          | 10 +++++
>>   sysdeps/unix/sysv/linux/mmap.c               | 30 ++++++++++++---
>>   sysdeps/unix/sysv/linux/mmap64.c             | 23 ++++++++---
>>   sysdeps/unix/sysv/linux/mmap_internal.h      |  2 +-
>>   sysdeps/unix/sysv/linux/s390/mmap_internal.h |  2 +-
>>   14 files changed, 154 insertions(+), 29 deletions(-)
>>
>>
>> base-commit: aa69f19a937b679816ef10e8620ea1141bb1734b
>> -- 
>> 2.29.2

Re: [PATCH v2 0/4] Improved ASLR

[Rich Felker]

On Thu, Dec 03, 2020 at 10:43:29AM +0200, Topi Miettinen wrote:
> On 3.12.2020 1.09, Rich Felker wrote:
> >On Sat, Nov 28, 2020 at 01:59:41PM +0200, Topi Miettinen via Libc-alpha wrote:
> >>Problem with using sbrk() for allocations is that the location of the
> >>memory is relatively predicatable since it's always located next to
> >>data segment. This series makes the tunables system, malloc() and TCB
> >>use mmap() instead, except when instructed by tunable
> >>glibc.malloc.use_sbrk.
> >
> >The above description is contrary to present reality on Linux. With
> >kernel.randomize_va_space=2 (default), the brk area starts at a
> >randomize gap above end of data/bss. This is *stronger* ASLR than
> >mmap, which aside from the initial gap, generally appears just below
> >the previous map and thereby at a predictable offset from an anchor in
> >a shared library.
> 
> Thanks for the review. I'd note that the randomization for brk is
> only 12 bits, so it's still relatively predictable. With
> randomize_va_space=3, the randomization provided by mmap() will be
> much better, the maximum allowed by the kernel design. The best in
> all cases would be if randomize_va_space=3 could be detected by libc
> and then it would change the default choice between brk() and mmap()
> automatically, or libc could generate a random address by itself and
> use mmap(random_addr,, MAP_FIXED_NOREPLACE) instead of brk(). For
> example, kernel could use aux vectors to pass the information.

Why isn't the proposed randomize_va_space=3 kernel patch just
using a completely random base for brk region (and reserving at least
a few GB for it to grow into)? Then all existing programs would get
the benefit.

Rich

Re: [PATCH v2 0/4] Improved ASLR

[Topi Miettinen]

On 3.12.2020 17.26, Rich Felker wrote:
> On Thu, Dec 03, 2020 at 10:43:29AM +0200, Topi Miettinen wrote:
>> On 3.12.2020 1.09, Rich Felker wrote:
>>> On Sat, Nov 28, 2020 at 01:59:41PM +0200, Topi Miettinen via Libc-alpha wrote:
>>>> Problem with using sbrk() for allocations is that the location of the
>>>> memory is relatively predicatable since it's always located next to
>>>> data segment. This series makes the tunables system, malloc() and TCB
>>>> use mmap() instead, except when instructed by tunable
>>>> glibc.malloc.use_sbrk.
>>>
>>> The above description is contrary to present reality on Linux. With
>>> kernel.randomize_va_space=2 (default), the brk area starts at a
>>> randomize gap above end of data/bss. This is *stronger* ASLR than
>>> mmap, which aside from the initial gap, generally appears just below
>>> the previous map and thereby at a predictable offset from an anchor in
>>> a shared library.
>>
>> Thanks for the review. I'd note that the randomization for brk is
>> only 12 bits, so it's still relatively predictable. With
>> randomize_va_space=3, the randomization provided by mmap() will be
>> much better, the maximum allowed by the kernel design. The best in
>> all cases would be if randomize_va_space=3 could be detected by libc
>> and then it would change the default choice between brk() and mmap()
>> automatically, or libc could generate a random address by itself and
>> use mmap(random_addr,, MAP_FIXED_NOREPLACE) instead of brk(). For
>> example, kernel could use aux vectors to pass the information.
> 
> Why isn't the proposed randomize_va_space=3 kernel patch just
> using a completely random base for brk region (and reserving at least
> a few GB for it to grow into)? Then all existing programs would get
> the benefit.

Great idea! I'll try to add that to the patch set.

-Topi

Re: [PATCH v2 0/4] Improved ASLR

[Rich Felker]

On Thu, Dec 03, 2020 at 06:05:56PM +0200, Topi Miettinen wrote:
> On 3.12.2020 17.26, Rich Felker wrote:
> >On Thu, Dec 03, 2020 at 10:43:29AM +0200, Topi Miettinen wrote:
> >>On 3.12.2020 1.09, Rich Felker wrote:
> >>>On Sat, Nov 28, 2020 at 01:59:41PM +0200, Topi Miettinen via Libc-alpha wrote:
> >>>>Problem with using sbrk() for allocations is that the location of the
> >>>>memory is relatively predicatable since it's always located next to
> >>>>data segment. This series makes the tunables system, malloc() and TCB
> >>>>use mmap() instead, except when instructed by tunable
> >>>>glibc.malloc.use_sbrk.
> >>>
> >>>The above description is contrary to present reality on Linux. With
> >>>kernel.randomize_va_space=2 (default), the brk area starts at a
> >>>randomize gap above end of data/bss. This is *stronger* ASLR than
> >>>mmap, which aside from the initial gap, generally appears just below
> >>>the previous map and thereby at a predictable offset from an anchor in
> >>>a shared library.
> >>
> >>Thanks for the review. I'd note that the randomization for brk is
> >>only 12 bits, so it's still relatively predictable. With
> >>randomize_va_space=3, the randomization provided by mmap() will be
> >>much better, the maximum allowed by the kernel design. The best in
> >>all cases would be if randomize_va_space=3 could be detected by libc
> >>and then it would change the default choice between brk() and mmap()
> >>automatically, or libc could generate a random address by itself and
> >>use mmap(random_addr,, MAP_FIXED_NOREPLACE) instead of brk(). For
> >>example, kernel could use aux vectors to pass the information.
> >
> >Why isn't the proposed randomize_va_space=3 kernel patch just
> >using a completely random base for brk region (and reserving at least
> >a few GB for it to grow into)? Then all existing programs would get
> >the benefit.
> 
> Great idea! I'll try to add that to the patch set.

Great! FYI we also use brk in musl libc's mallocng as a source of
memory for metadata areas that are expected not the be at predictable
offset from any module's text or data segment or from heap allocations
(which all come from mmap), so increased randomization for its
location will help a lot.

As a related aside, you probably need to recommend increasing
vm.max_map_count with kernel.randomize_va_space==3. Otherwise it
becomes pretty easy to hit the default VMA limit of 64k.

Rich

Re: [PATCH v2 0/4] Improved ASLR

[Adhemerval Zanella]

On 03/12/2020 13:05, Topi Miettinen via Libc-alpha wrote:
> On 3.12.2020 17.26, Rich Felker wrote:
>> On Thu, Dec 03, 2020 at 10:43:29AM +0200, Topi Miettinen wrote:
>>> On 3.12.2020 1.09, Rich Felker wrote:
>>>> On Sat, Nov 28, 2020 at 01:59:41PM +0200, Topi Miettinen via Libc-alpha wrote:
>>>>> Problem with using sbrk() for allocations is that the location of the
>>>>> memory is relatively predicatable since it's always located next to
>>>>> data segment. This series makes the tunables system, malloc() and TCB
>>>>> use mmap() instead, except when instructed by tunable
>>>>> glibc.malloc.use_sbrk.
>>>>
>>>> The above description is contrary to present reality on Linux. With
>>>> kernel.randomize_va_space=2 (default), the brk area starts at a
>>>> randomize gap above end of data/bss. This is *stronger* ASLR than
>>>> mmap, which aside from the initial gap, generally appears just below
>>>> the previous map and thereby at a predictable offset from an anchor in
>>>> a shared library.
>>>
>>> Thanks for the review. I'd note that the randomization for brk is
>>> only 12 bits, so it's still relatively predictable. With
>>> randomize_va_space=3, the randomization provided by mmap() will be
>>> much better, the maximum allowed by the kernel design. The best in
>>> all cases would be if randomize_va_space=3 could be detected by libc
>>> and then it would change the default choice between brk() and mmap()
>>> automatically, or libc could generate a random address by itself and
>>> use mmap(random_addr,, MAP_FIXED_NOREPLACE) instead of brk(). For
>>> example, kernel could use aux vectors to pass the information.
>>
>> Why isn't the proposed randomize_va_space=3 kernel patch just
>> using a completely random base for brk region (and reserving at least
>> a few GB for it to grow into)? Then all existing programs would get
>> the benefit.
> 
> Great idea! I'll try to add that to the patch set.

Which the sbrk entropy increase, will use mmap on TCB allocation add 
any security gain?

Re: [PATCH v2 0/4] Improved ASLR

[Adhemerval Zanella]

On 04/12/2020 07:03, Topi Miettinen wrote:
> On 3.12.2020 22.33, Adhemerval Zanella wrote:
>>
>>
>> On 03/12/2020 13:05, Topi Miettinen via Libc-alpha wrote:
>>> On 3.12.2020 17.26, Rich Felker wrote:
>>>> On Thu, Dec 03, 2020 at 10:43:29AM +0200, Topi Miettinen wrote:
>>>>> On 3.12.2020 1.09, Rich Felker wrote:
>>>>>> On Sat, Nov 28, 2020 at 01:59:41PM +0200, Topi Miettinen via Libc-alpha wrote:
>>>>>>> Problem with using sbrk() for allocations is that the location of the
>>>>>>> memory is relatively predicatable since it's always located next to
>>>>>>> data segment. This series makes the tunables system, malloc() and TCB
>>>>>>> use mmap() instead, except when instructed by tunable
>>>>>>> glibc.malloc.use_sbrk.
>>>>>>
>>>>>> The above description is contrary to present reality on Linux. With
>>>>>> kernel.randomize_va_space=2 (default), the brk area starts at a
>>>>>> randomize gap above end of data/bss. This is *stronger* ASLR than
>>>>>> mmap, which aside from the initial gap, generally appears just below
>>>>>> the previous map and thereby at a predictable offset from an anchor in
>>>>>> a shared library.
>>>>>
>>>>> Thanks for the review. I'd note that the randomization for brk is
>>>>> only 12 bits, so it's still relatively predictable. With
>>>>> randomize_va_space=3, the randomization provided by mmap() will be
>>>>> much better, the maximum allowed by the kernel design. The best in
>>>>> all cases would be if randomize_va_space=3 could be detected by libc
>>>>> and then it would change the default choice between brk() and mmap()
>>>>> automatically, or libc could generate a random address by itself and
>>>>> use mmap(random_addr,, MAP_FIXED_NOREPLACE) instead of brk(). For
>>>>> example, kernel could use aux vectors to pass the information.
>>>>
>>>> Why isn't the proposed randomize_va_space=3 kernel patch just
>>>> using a completely random base for brk region (and reserving at least
>>>> a few GB for it to grow into)? Then all existing programs would get
>>>> the benefit.
>>>
>>> Great idea! I'll try to add that to the patch set.
>>
>> Which the sbrk entropy increase, will mmap use on TCB allocation add
>> any security gain?
> 
> I think the biggest improvement comes from randomizing the location either by improving sbrk() or mmap(). Using mmap() for TCB gives some additional improvement that the area will be more or less separate from the other allocations. Analysis of the effect is complicated by the actual usage of randomize_va_space, either by kernel default setting, more importantly what the distros will actually use and finally the end users may be enlightened enough to change the setting. Also maximum randomization may be too aggressive for 32 bit architectures because of fragmentation of the virtual memory.

If it could be mitigated by the kernel with a better sbrk entropy I 
would prefer than to add the tunable machinery on glibc.  Also, tunables
is not meant to be deployment mechanism on a security feature where
the idea is to set if globally (and tunables also does not have a 
future-proof ABI).

We might try to use a different strategy and replace sbrk call with
mmap without adding a tunable, but it would add underlying mmap overhead
so I am not sure if it should be a default option.  The initial 
assessment seems that the overhead should be ok, but synthetic benchmark
can be misleading. We would need more data of the expected extra overhead
it might imposes (increase in mmap segments, performance on different
workloads, multithread costs, etc).

One option might a configure switch, which I am not very found as well
(it increases maintainability costs).
   

Re: [PATCH v2 0/4] Improved ASLR

[Topi Miettinen]

On 4.12.2020 15.04, Adhemerval Zanella wrote:
> 
> 
> On 04/12/2020 07:03, Topi Miettinen wrote:
>> On 3.12.2020 22.33, Adhemerval Zanella wrote:
>>>
>>>
>>> On 03/12/2020 13:05, Topi Miettinen via Libc-alpha wrote:
>>>> On 3.12.2020 17.26, Rich Felker wrote:
>>>>> On Thu, Dec 03, 2020 at 10:43:29AM +0200, Topi Miettinen wrote:
>>>>>> On 3.12.2020 1.09, Rich Felker wrote:
>>>>>>> On Sat, Nov 28, 2020 at 01:59:41PM +0200, Topi Miettinen via Libc-alpha wrote:
>>>>>>>> Problem with using sbrk() for allocations is that the location of the
>>>>>>>> memory is relatively predicatable since it's always located next to
>>>>>>>> data segment. This series makes the tunables system, malloc() and TCB
>>>>>>>> use mmap() instead, except when instructed by tunable
>>>>>>>> glibc.malloc.use_sbrk.
>>>>>>>
>>>>>>> The above description is contrary to present reality on Linux. With
>>>>>>> kernel.randomize_va_space=2 (default), the brk area starts at a
>>>>>>> randomize gap above end of data/bss. This is *stronger* ASLR than
>>>>>>> mmap, which aside from the initial gap, generally appears just below
>>>>>>> the previous map and thereby at a predictable offset from an anchor in
>>>>>>> a shared library.
>>>>>>
>>>>>> Thanks for the review. I'd note that the randomization for brk is
>>>>>> only 12 bits, so it's still relatively predictable. With
>>>>>> randomize_va_space=3, the randomization provided by mmap() will be
>>>>>> much better, the maximum allowed by the kernel design. The best in
>>>>>> all cases would be if randomize_va_space=3 could be detected by libc
>>>>>> and then it would change the default choice between brk() and mmap()
>>>>>> automatically, or libc could generate a random address by itself and
>>>>>> use mmap(random_addr,, MAP_FIXED_NOREPLACE) instead of brk(). For
>>>>>> example, kernel could use aux vectors to pass the information.
>>>>>
>>>>> Why isn't the proposed randomize_va_space=3 kernel patch just
>>>>> using a completely random base for brk region (and reserving at least
>>>>> a few GB for it to grow into)? Then all existing programs would get
>>>>> the benefit.
>>>>
>>>> Great idea! I'll try to add that to the patch set.
>>>
>>> Which the sbrk entropy increase, will mmap use on TCB allocation add
>>> any security gain?
>>
>> I think the biggest improvement comes from randomizing the location either by improving sbrk() or mmap(). Using mmap() for TCB gives some additional improvement that the area will be more or less separate from the other allocations. Analysis of the effect is complicated by the actual usage of randomize_va_space, either by kernel default setting, more importantly what the distros will actually use and finally the end users may be enlightened enough to change the setting. Also maximum randomization may be too aggressive for 32 bit architectures because of fragmentation of the virtual memory.
> 
> If it could be mitigated by the kernel with a better sbrk entropy I
> would prefer than to add the tunable machinery on glibc.  Also, tunables
> is not meant to be deployment mechanism on a security feature where
> the idea is to set if globally (and tunables also does not have a
> future-proof ABI).
> 
> We might try to use a different strategy and replace sbrk call with
> mmap without adding a tunable, but it would add underlying mmap overhead
> so I am not sure if it should be a default option.  The initial
> assessment seems that the overhead should be ok, but synthetic benchmark
> can be misleading. We would need more data of the expected extra overhead
> it might imposes (increase in mmap segments, performance on different
> workloads, multithread costs, etc).
> 
> One option might a configure switch, which I am not very found as well
> (it increases maintainability costs).

I've sent a new version (v8) of the kernel patch:
https://lkml.org/lkml/2020/12/20/132

In that version, also heap (memory allocated with brk()) is randomized, 
including also lowest bits 11 to 4. Then malloc() on unmodified glibc 
and musl would be randomized on systems which enable 
randomize_va_space=3. For other systems, it would be good if the low 
bits of address of the heap would be randomized by the dynamic loader by 
calling sbrk() with a small (up to page size, aligned to 16 bytes) 
increment.

Assuming that randomize_va_space=3 becomes mainstream (and the patch is 
accepted...), I think the new tunable for glibc and other changes to 
avoid sbrk() for TCB, dynamic loader and tunables aren't so interesting. 
It would be still good to use mmap() for malloc() since then all arenas 
would be separate from heap. Perhaps this could be selectable with 
mallopt() flag instead of a tunable? This could be also complemented by 
adding a small (aligned) random offset to the address given by mmap() 
for malloc() arenas.

-Topi