public inbox for libc-alpha@sourceware.org
 help / color / mirror / Atom feed
* [RFC PATCH 0/3] Improved ALSR
@ 2020-10-04 13:09 Topi Miettinen
  2020-10-04 13:09 ` [RFC PATCH 1/3] csu: randomize location of TCB Topi Miettinen
                   ` (4 more replies)
  0 siblings, 5 replies; 18+ messages in thread
From: Topi Miettinen @ 2020-10-04 13:09 UTC (permalink / raw)
  To: libc-alpha; +Cc: 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 malloc() and TCB use mmap() instead.

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

 csu/libc-tls.c                          | 20 ++++++++++++++------
 elf/dl-sysdep.c                         |  2 ++
 malloc/arena.c                          |  5 ++++-
 malloc/malloc.c                         | 16 +++++++++++++---
 malloc/morecore.c                       |  2 ++
 sysdeps/unix/sysv/linux/dl-sysdep.c     |  2 ++
 sysdeps/unix/sysv/linux/mmap64.c        | 19 +++++++++++++++++++
 sysdeps/unix/sysv/linux/mmap_internal.h |  3 +++
 8 files changed, 59 insertions(+), 10 deletions(-)

-- 
2.28.0


^ permalink raw reply	[flat|nested] 18+ messages in thread

* [RFC PATCH 1/3] csu: randomize location of TCB
  2020-10-04 13:09 [RFC PATCH 0/3] Improved ALSR Topi Miettinen
@ 2020-10-04 13:09 ` Topi Miettinen
  2020-11-24 18:44   ` Adhemerval Zanella
  2020-10-04 13:09 ` [RFC PATCH 2/3] malloc: always use mmap() to improve ASLR Topi Miettinen
                   ` (3 subsequent siblings)
  4 siblings, 1 reply; 18+ messages in thread
From: Topi Miettinen @ 2020-10-04 13:09 UTC (permalink / raw)
  To: libc-alpha; +Cc: Topi Miettinen

Let's use mmap() for TCB, which makes the location of TCB random
instead of always staying predictably next to data segment. Also
improve the logic so that allocation of TCB can be assumed to fail
insted of segfaulting.

RFC: make independent of Linux.

Signed-off-by: Topi Miettinen <toiwoton@gmail.com>
---
 csu/libc-tls.c                          | 20 ++++++++++++++------
 sysdeps/unix/sysv/linux/mmap64.c        | 19 +++++++++++++++++++
 sysdeps/unix/sysv/linux/mmap_internal.h |  3 +++
 3 files changed, 36 insertions(+), 6 deletions(-)

diff --git a/csu/libc-tls.c b/csu/libc-tls.c
index 06e76bd395..59700c3a95 100644
--- a/csu/libc-tls.c
+++ b/csu/libc-tls.c
@@ -24,6 +24,9 @@
 #include <stdio.h>
 #include <sys/param.h>
 #include <array_length.h>
+#include <sys/mman.h>
+#include <sysdep.h>
+#include <mmap_internal.h>
 
 #ifdef SHARED
  #error makefile bug, this file is for static only
@@ -134,25 +137,30 @@ __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 'internal_mmap' which does not use 'errno'. */
+
+  int error = 0;
+
 #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 = __mmap_internal (NULL, tcb_offset + TLS_INIT_TCB_SIZE + max_align,
+			      PROT_READ | PROT_WRITE, MAP_ANONYMOUS | MAP_PRIVATE, -1, 0, &error);
 #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 = __mmap_internal (NULL, tcb_offset + memsz + max_align
+			      + TLS_PRE_TCB_SIZE + GLRO(dl_tls_static_surplus),
+			      PROT_READ | PROT_WRITE, MAP_ANONYMOUS | MAP_PRIVATE, -1, 0, &error);
   tlsblock += TLS_PRE_TCB_SIZE;
 #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 (error)
+    _startup_fatal ("Cannot allocate TCB");
 
   /* Align the TLS block.  */
   tlsblock = (void *) (((uintptr_t) tlsblock + max_align - 1)
diff --git a/sysdeps/unix/sysv/linux/mmap64.c b/sysdeps/unix/sysv/linux/mmap64.c
index 8074deb466..11f7c3f99b 100644
--- a/sysdeps/unix/sysv/linux/mmap64.c
+++ b/sysdeps/unix/sysv/linux/mmap64.c
@@ -67,3 +67,22 @@ weak_alias (__mmap64, mmap)
 weak_alias (__mmap64, __mmap)
 libc_hidden_def (__mmap)
 #endif
+
+void *
+__mmap_internal (void *addr, size_t len, int prot, int flags, int fd, off64_t offset, int *error)
+{
+  unsigned long int ret;
+#ifdef __NR_mmap2
+  ret = INTERNAL_SYSCALL_CALL (mmap2, addr, len, prot, flags, fd,
+			     (off_t) (offset / MMAP2_PAGE_UNIT));
+#else
+  ret = INTERNAL_SYSCALL_CALL (mmap, addr, len, prot, flags, fd, offset);
+#endif
+  if (INTERNAL_SYSCALL_ERROR_P(ret))
+    {
+      *error = ret;
+      return MAP_FAILED;
+    }
+
+  return (void *) ret;
+}
diff --git a/sysdeps/unix/sysv/linux/mmap_internal.h b/sysdeps/unix/sysv/linux/mmap_internal.h
index d53f0c642a..00fc14902e 100644
--- a/sysdeps/unix/sysv/linux/mmap_internal.h
+++ b/sysdeps/unix/sysv/linux/mmap_internal.h
@@ -46,4 +46,7 @@ static uint64_t page_unit;
   INLINE_SYSCALL_CALL (__nr, __addr, __len, __prot, __flags, __fd, __offset)
 #endif
 
+/* Internal version of mmap() which doesn't attempt to access errno */
+void *__mmap_internal (void *addr, size_t len, int prot, int flags, int fd, off64_t offset, int *error);
+
 #endif /* MMAP_INTERNAL_LINUX_H  */
-- 
2.28.0


^ permalink raw reply	[flat|nested] 18+ messages in thread

* [RFC PATCH 2/3] malloc: always use mmap() to improve ASLR
  2020-10-04 13:09 [RFC PATCH 0/3] Improved ALSR Topi Miettinen
  2020-10-04 13:09 ` [RFC PATCH 1/3] csu: randomize location of TCB Topi Miettinen
@ 2020-10-04 13:09 ` Topi Miettinen
  2020-10-04 13:09 ` [RFC PATCH 3/3] dl-sysdep: disable remaining calls to sbrk() Topi Miettinen
                   ` (2 subsequent siblings)
  4 siblings, 0 replies; 18+ messages in thread
From: Topi Miettinen @ 2020-10-04 13:09 UTC (permalink / raw)
  To: libc-alpha; +Cc: Topi Miettinen

sbrk() returns rather predictable allocations because they are located
close to the data segment. Let's always use mmap() instead.

RFC: How to do this properly?

Signed-off-by: Topi Miettinen <toiwoton@gmail.com>
---
 malloc/arena.c    |  5 ++++-
 malloc/malloc.c   | 16 +++++++++++++---
 malloc/morecore.c |  2 ++
 3 files changed, 19 insertions(+), 4 deletions(-)

diff --git a/malloc/arena.c b/malloc/arena.c
index cecdb7f4c4..f88db5f248 100644
--- a/malloc/arena.c
+++ b/malloc/arena.c
@@ -273,7 +273,7 @@ next_env_entry (char ***position)
 }
 #endif
 
-
+#if 0
 #ifdef SHARED
 static void *
 __failing_morecore (ptrdiff_t d)
@@ -284,6 +284,7 @@ __failing_morecore (ptrdiff_t d)
 extern struct dl_open_hook *_dl_open_hook;
 libc_hidden_proto (_dl_open_hook);
 #endif
+#endif
 
 static void
 ptmalloc_init (void)
@@ -293,6 +294,7 @@ ptmalloc_init (void)
 
   __malloc_initialized = 0;
 
+#if 0
 #ifdef SHARED
   /* In case this libc copy is in a non-default namespace, never use brk.
      Likewise if dlopened from statically linked program.  */
@@ -303,6 +305,7 @@ ptmalloc_init (void)
       || (_dl_addr (ptmalloc_init, &di, &l, NULL) != 0
           && l->l_ns != LM_ID_BASE))
     __morecore = __failing_morecore;
+#endif
 #endif
 
   thread_arena = &main_arena;
diff --git a/malloc/malloc.c b/malloc/malloc.c
index cd9933b4e5..2f894b9c60 100644
--- a/malloc/malloc.c
+++ b/malloc/malloc.c
@@ -371,13 +371,23 @@ __malloc_assert (const char *assertion, const char *file, unsigned int line,
 #define TRIM_FASTBINS  0
 #endif
 
-
+#if 0
 /* Definition for getting more memory from the OS.  */
 #define MORECORE         (*__morecore)
 #define MORECORE_FAILURE 0
 void * __default_morecore (ptrdiff_t);
 void *(*__morecore)(ptrdiff_t) = __default_morecore;
-
+#else
+#define MORECORE_FAILURE (-1)
+#define MORECORE(x)         (MORECORE_FAILURE)
+static void *
+__failing_morecore2 (ptrdiff_t d)
+{
+  return (void *) MORECORE_FAILURE;
+}
+void *(*__morecore)(ptrdiff_t) = __failing_morecore2;
+#define MORECORE_CONTIGUOUS 0
+#endif
 
 #include <string.h>
 
@@ -2796,7 +2806,7 @@ systrim (size_t pad, mstate av)
          some downstream failure.)
        */
 
-      MORECORE (-extra);
+      (void) MORECORE (-extra);
       /* Call the `morecore' hook if necessary.  */
       void (*hook) (void) = atomic_forced_read (__after_morecore_hook);
       if (__builtin_expect (hook != NULL, 0))
diff --git a/malloc/morecore.c b/malloc/morecore.c
index 72e655f84f..931b37e41f 100644
--- a/malloc/morecore.c
+++ b/malloc/morecore.c
@@ -15,6 +15,7 @@
    License along with the GNU C Library; if not, see
    <https://www.gnu.org/licenses/>.  */
 
+#if 0
 #ifndef _MALLOC_INTERNAL
 # define _MALLOC_INTERNAL
 # include <malloc.h>
@@ -51,3 +52,4 @@ __default_morecore (ptrdiff_t increment)
   return result;
 }
 libc_hidden_def (__default_morecore)
+#endif
-- 
2.28.0


^ permalink raw reply	[flat|nested] 18+ messages in thread

* [RFC PATCH 3/3] dl-sysdep: disable remaining calls to sbrk()
  2020-10-04 13:09 [RFC PATCH 0/3] Improved ALSR Topi Miettinen
  2020-10-04 13:09 ` [RFC PATCH 1/3] csu: randomize location of TCB Topi Miettinen
  2020-10-04 13:09 ` [RFC PATCH 2/3] malloc: always use mmap() to improve ASLR Topi Miettinen
@ 2020-10-04 13:09 ` Topi Miettinen
  2020-11-23 16:06 ` [RFC PATCH 0/3] Improved ALSR Topi Miettinen
  2020-11-23 16:44 ` Florian Weimer
  4 siblings, 0 replies; 18+ messages in thread
From: Topi Miettinen @ 2020-10-04 13:09 UTC (permalink / raw)
  To: libc-alpha; +Cc: Topi Miettinen

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

RFC: how to do this properly? Removing frob_brk() entirely causes
build to break with strange link problems.

Signed-off-by: Topi Miettinen <toiwoton@gmail.com>
---
 elf/dl-sysdep.c                     | 2 ++
 sysdeps/unix/sysv/linux/dl-sysdep.c | 2 ++
 2 files changed, 4 insertions(+)

diff --git a/elf/dl-sysdep.c b/elf/dl-sysdep.c
index 854570821c..200cea3ca7 100644
--- a/elf/dl-sysdep.c
+++ b/elf/dl-sysdep.c
@@ -234,6 +234,7 @@ _dl_sysdep_start (void **start_argptr,
   if (GLRO(dl_platform) != NULL)
     GLRO(dl_platformlen) = strlen (GLRO(dl_platform));
 
+#if 0
   if (__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
@@ -242,6 +243,7 @@ _dl_sysdep_start (void **start_argptr,
        will see this new value and not clobber our data.  */
     __sbrk (GLRO(dl_pagesize)
 	    - ((_end - (char *) 0) & (GLRO(dl_pagesize) - 1)));
+#endif
 
   /* If this is a SUID program we make sure that FDs 0, 1, and 2 are
      allocated.  If necessary we are doing it ourself.  If it is not
diff --git a/sysdeps/unix/sysv/linux/dl-sysdep.c b/sysdeps/unix/sysv/linux/dl-sysdep.c
index 90c9b1db2d..d2f64b1137 100644
--- a/sysdeps/unix/sysv/linux/dl-sysdep.c
+++ b/sysdeps/unix/sysv/linux/dl-sysdep.c
@@ -33,7 +33,9 @@
 static inline void
 frob_brk (void)
 {
+#if 0
   __brk (0);			/* Initialize the break.  */
+#endif
 }
 
 # include <elf/dl-sysdep.c>
-- 
2.28.0


^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [RFC PATCH 0/3] Improved ALSR
  2020-10-04 13:09 [RFC PATCH 0/3] Improved ALSR Topi Miettinen
                   ` (2 preceding siblings ...)
  2020-10-04 13:09 ` [RFC PATCH 3/3] dl-sysdep: disable remaining calls to sbrk() Topi Miettinen
@ 2020-11-23 16:06 ` Topi Miettinen
  2020-11-23 16:41   ` Szabolcs Nagy
  2020-11-23 16:44 ` Florian Weimer
  4 siblings, 1 reply; 18+ messages in thread
From: Topi Miettinen @ 2020-11-23 16:06 UTC (permalink / raw)
  To: libc-alpha

On 4.10.2020 16.09, Topi Miettinen 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 malloc() and TCB use mmap() instead.

No comments at all? I see several implementation options here:

1. Always use mmap() instead of sbrk(), delete any uses of sbrk()

I have hard time thinking why sbrk() would ever be the preferred choice 
over mmap(), especially considering security. There may be some bytes 
wasted, so embedded systems could want to save those and also MMU-less 
systems can't map pages anywhere, but then those probably won't use glibc.

2. Conditionally use mmap() instead of sbrk()

Something like `#define USE_SBRK`, enabled by `configure` or a header 
file. Sub-options:

2.1. Default to sbrk(), use mmap() only for Linux

This is of course safer if some obscure system needs sbrk(). It would be 
even safer to limit mmap() only to Linux/x86_64 (which is all I care).

2.2. Default to mmap() but don't enable sbrk() anywhere

This is pretty much like #1 but after breakage is noticed for some 
obscure systems, it's easy to `#define USE_SBRK` somewhere.


I've been using a patched glibc for a month without seeing problems. I 
enabled audit logging for the brk() system call and installed a global 
seccomp filter (in initrd) which returns ENOSYS to catch any uses. So 
far I've only noticed that cpp (used by X11 startup in addition to 
compiling) calls sbrk() to check memory usage. Perhaps it should use 
official malloc statistics interface instead, since malloc() may use 
mmap() for other reasons and then sbrk() won't return true data.

It's easy to check that sbrk() has not been used with the command `grep 
'\[heap\]' /proc/*/maps` (as root), which should print nothing since 
there are no heaps (as in "extended data segment") anymore.

-Topi

> Topi Miettinen (3):
>    csu: randomize location of TCB
>    malloc: always use mmap() to improve ASLR
>    dl-sysdep: disable remaining calls to sbrk()
> 
>   csu/libc-tls.c                          | 20 ++++++++++++++------
>   elf/dl-sysdep.c                         |  2 ++
>   malloc/arena.c                          |  5 ++++-
>   malloc/malloc.c                         | 16 +++++++++++++---
>   malloc/morecore.c                       |  2 ++
>   sysdeps/unix/sysv/linux/dl-sysdep.c     |  2 ++
>   sysdeps/unix/sysv/linux/mmap64.c        | 19 +++++++++++++++++++
>   sysdeps/unix/sysv/linux/mmap_internal.h |  3 +++
>   8 files changed, 59 insertions(+), 10 deletions(-)
> 


^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [RFC PATCH 0/3] Improved ALSR
  2020-11-23 16:06 ` [RFC PATCH 0/3] Improved ALSR Topi Miettinen
@ 2020-11-23 16:41   ` Szabolcs Nagy
  2020-11-23 17:55     ` Topi Miettinen
  0 siblings, 1 reply; 18+ messages in thread
From: Szabolcs Nagy @ 2020-11-23 16:41 UTC (permalink / raw)
  To: Topi Miettinen; +Cc: libc-alpha

The 11/23/2020 18:06, Topi Miettinen via Libc-alpha wrote:
> No comments at all? I see several implementation options here:
> 
> 1. Always use mmap() instead of sbrk(), delete any uses of sbrk()
> 
> I have hard time thinking why sbrk() would ever be the preferred choice over
> mmap(), especially considering security. There may be some bytes wasted, so

i'm not against using mmap instead brk in malloc
but the latter has more overhead so such change
should be measured.

> 2. Conditionally use mmap() instead of sbrk()
> 
> Something like `#define USE_SBRK`, enabled by `configure` or a header file.

i think configure time option is not a good idea,
but e.g. it can be a runtime tunable.

> I've been using a patched glibc for a month without seeing problems. I
> enabled audit logging for the brk() system call and installed a global
> seccomp filter (in initrd) which returns ENOSYS to catch any uses. So far
> I've only noticed that cpp (used by X11 startup in addition to compiling)
> calls sbrk() to check memory usage. Perhaps it should use official malloc
> statistics interface instead, since malloc() may use mmap() for other
> reasons and then sbrk() won't return true data.

sbrk should continue to work even if glibc itself
does not use it internally, that's public api/abi.

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [RFC PATCH 0/3] Improved ALSR
  2020-10-04 13:09 [RFC PATCH 0/3] Improved ALSR Topi Miettinen
                   ` (3 preceding siblings ...)
  2020-11-23 16:06 ` [RFC PATCH 0/3] Improved ALSR Topi Miettinen
@ 2020-11-23 16:44 ` Florian Weimer
  2020-11-23 18:16   ` Topi Miettinen
  4 siblings, 1 reply; 18+ messages in thread
From: Florian Weimer @ 2020-11-23 16:44 UTC (permalink / raw)
  To: Topi Miettinen via Libc-alpha; +Cc: Topi Miettinen

* Topi Miettinen via Libc-alpha:

> 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 malloc() and TCB use mmap() instead.

Doesn't switching to mmap trade one (relatively) fixed offset for
another?  I think anonymous mmap is not randomized independently from
the file mappings used for loading DSOs.

And the series only changes the TCB allocation for the main thread.
Fixing thread TCB/stack collocation is a completely different matter
(and probably the more significant issue than lack of ASLR).

Thanks,
Florian
-- 
Red Hat GmbH, https://de.redhat.com/ , Registered seat: Grasbrunn,
Commercial register: Amtsgericht Muenchen, HRB 153243,
Managing Directors: Charles Cachera, Brian Klemm, Laurie Krebs, Michael O'Neill


^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [RFC PATCH 0/3] Improved ALSR
  2020-11-23 16:41   ` Szabolcs Nagy
@ 2020-11-23 17:55     ` Topi Miettinen
  2020-11-23 21:45       ` Florian Weimer
  0 siblings, 1 reply; 18+ messages in thread
From: Topi Miettinen @ 2020-11-23 17:55 UTC (permalink / raw)
  To: Szabolcs Nagy; +Cc: libc-alpha

On 23.11.2020 18.41, Szabolcs Nagy wrote:
> The 11/23/2020 18:06, Topi Miettinen via Libc-alpha wrote:
>> No comments at all? I see several implementation options here:
>>
>> 1. Always use mmap() instead of sbrk(), delete any uses of sbrk()
>>
>> I have hard time thinking why sbrk() would ever be the preferred choice over
>> mmap(), especially considering security. There may be some bytes wasted, so
> 
> i'm not against using mmap instead brk in malloc
> but the latter has more overhead so such change
> should be measured.

This test shows 48% increase when using mmap() vs. sbrk():

$ cat malloc-vs-sbrk.c
#include <sys/mman.h>
#include <stdio.h>
#include <stdlib.h>
#include <unistd.h>

#define ROUNDS 1000000
#define SIZES 4
#define SIZE_FACTOR 4

int main(int argc, char **argv) {
         if (argc == 2) {
                 for (int i = 0; i < ROUNDS; i++) {
                         for (int j = 0; j < SIZES; j++) {
                                 size_t s = 4096 * (1 << (j * SIZE_FACTOR));

                                 void *ptr = mmap(NULL, s, PROT_READ | 
PROT_WRITE, MAP_PRIVATE | MAP_ANONYMOUS, -1, 0);
                                 if (ptr == MAP_FAILED) {
                                         fprintf(stderr, "mmap() failed, 
size %zu iter %d\n", s, i);
                                         return 1;
                                 }
                                 munmap(ptr, s);
                         }
                 }
         } else {
                 for (int i = 0; i < ROUNDS; i++) {
                         for (int j = 0; j < SIZES; j++) {
                                 size_t s = 4096 * (1 << (j * SIZE_FACTOR));

                                 void *ptr = sbrk(s);
                                 if (ptr == (void *) -1) {
                                         fprintf(stderr, "sbrk() failed, 
size %zu iter %d\n", s, i);
                                         return 1;
                                 }
                                 sbrk(-s);
                         }
                 }
         }

         return 0;
}
$ time ./malloc-vs-sbrk

real    0m1.923s
user    0m0.160s
sys     0m1.762s
$ time ./malloc-vs-sbrk 1

real    0m2.847s
user    0m0.176s
sys     0m2.669s

>> 2. Conditionally use mmap() instead of sbrk()
>>
>> Something like `#define USE_SBRK`, enabled by `configure` or a header file.
> 
> i think configure time option is not a good idea,
> but e.g. it can be a runtime tunable.

The runtime option needs to be available very early in the dynamic 
loader, before errno and malloc() are available. Would getenv() work?

>> I've been using a patched glibc for a month without seeing problems. I
>> enabled audit logging for the brk() system call and installed a global
>> seccomp filter (in initrd) which returns ENOSYS to catch any uses. So far
>> I've only noticed that cpp (used by X11 startup in addition to compiling)
>> calls sbrk() to check memory usage. Perhaps it should use official malloc
>> statistics interface instead, since malloc() may use mmap() for other
>> reasons and then sbrk() won't return true data.
> 
> sbrk should continue to work even if glibc itself
> does not use it internally, that's public api/abi.

Yes, the patches don't remove the API/ABI.

-Topi


^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [RFC PATCH 0/3] Improved ALSR
  2020-11-23 16:44 ` Florian Weimer
@ 2020-11-23 18:16   ` Topi Miettinen
  2020-11-24  9:47     ` Szabolcs Nagy
  0 siblings, 1 reply; 18+ messages in thread
From: Topi Miettinen @ 2020-11-23 18:16 UTC (permalink / raw)
  To: Florian Weimer, Topi Miettinen via Libc-alpha

On 23.11.2020 18.44, Florian Weimer wrote:
> * Topi Miettinen via Libc-alpha:
> 
>> 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 malloc() and TCB use mmap() instead.
> 
> Doesn't switching to mmap trade one (relatively) fixed offset for
> another?  I think anonymous mmap is not randomized independently from
> the file mappings used for loading DSOs.

The mappings are indeed rather predictable relative to each other, even 
with /proc/sys/kernel/randomize_va_space=2. The base address is somewhat 
  randomized. I've sent a patch to fully randomize the mappings:

https://patchwork.kernel.org/project/linux-mm/patch/20201026160518.9212-1-toiwoton@gmail.com/

Glibc could do similar randomization by itself, by calling mmap() with 
an address hint generated with a random numbers from getrandom(), but I 
think hardening the kernel is better choice.

> And the series only changes the TCB allocation for the main thread.
> Fixing thread TCB/stack collocation is a completely different matter
> (and probably the more significant issue than lack of ASLR).

I thought I covered all uses of sbrk(), perhaps I missed that one. Does 
the thread TCB/stack allocation use sbrk()?

-Topi

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [RFC PATCH 0/3] Improved ALSR
  2020-11-23 17:55     ` Topi Miettinen
@ 2020-11-23 21:45       ` Florian Weimer
  2020-11-23 22:28         ` Topi Miettinen
  0 siblings, 1 reply; 18+ messages in thread
From: Florian Weimer @ 2020-11-23 21:45 UTC (permalink / raw)
  To: Topi Miettinen via Libc-alpha; +Cc: Szabolcs Nagy, Topi Miettinen

* Topi Miettinen via Libc-alpha:

> $ time ./malloc-vs-sbrk
>
> real    0m1.923s
> user    0m0.160s
> sys     0m1.762s
> $ time ./malloc-vs-sbrk 1
>
> real    0m2.847s
> user    0m0.176s
> sys     0m2.669s

Does the difference go away if you change the mmap granularity to
128 KiB?  I think this happens under the covers (on the kernel side)
with sbrk.

>>> 2. Conditionally use mmap() instead of sbrk()
>>>
>>> Something like `#define USE_SBRK`, enabled by `configure` or a header file.
>> i think configure time option is not a good idea,
>> but e.g. it can be a runtime tunable.
>
> The runtime option needs to be available very early in the dynamic
> loader, before errno and malloc() are available. Would getenv() work?

getenv wouldn't work, but there is a parser for the GLIBC_TUNABLES
environment variable.

Thanks,
Florian
-- 
Red Hat GmbH, https://de.redhat.com/ , Registered seat: Grasbrunn,
Commercial register: Amtsgericht Muenchen, HRB 153243,
Managing Directors: Charles Cachera, Brian Klemm, Laurie Krebs, Michael O'Neill


^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [RFC PATCH 0/3] Improved ALSR
  2020-11-23 21:45       ` Florian Weimer
@ 2020-11-23 22:28         ` Topi Miettinen
  2020-11-24 11:24           ` Florian Weimer
  0 siblings, 1 reply; 18+ messages in thread
From: Topi Miettinen @ 2020-11-23 22:28 UTC (permalink / raw)
  To: Florian Weimer, Topi Miettinen via Libc-alpha

On 23.11.2020 23.45, Florian Weimer wrote:
> * Topi Miettinen via Libc-alpha:
> 
>> $ time ./malloc-vs-sbrk
>>
>> real    0m1.923s
>> user    0m0.160s
>> sys     0m1.762s
>> $ time ./malloc-vs-sbrk 1
>>
>> real    0m2.847s
>> user    0m0.176s
>> sys     0m2.669s
> 
> Does the difference go away if you change the mmap granularity to
> 128 KiB?  I think this happens under the covers (on the kernel side)
> with sbrk.

Does not seem so, 56% increase:

$ time ./malloc-vs-sbrk

real    0m2.911s
user    0m0.232s
sys     0m2.677s
$ time ./malloc-vs-sbrk 1

real    0m4.545s
user    0m0.196s
sys     0m4.347s

$ cat malloc-vs-sbrk.c
#include <sys/mman.h>
#include <stdio.h>
#include <stdlib.h>
#include <unistd.h>

#define ROUNDS 1000000
#define SIZES 4
#define SIZE_FACTOR 4
#define SIZE_BASE (128 * 1024)

int main(int argc, char **argv) {
         if (argc == 2) {
                 for (int i = 0; i < ROUNDS; i++) {
                         for (int j = 0; j < SIZES; j++) {
                                 size_t s = SIZE_BASE * (1 << (j * 
SIZE_FACTOR));

                                 void *ptr = mmap(NULL, s, PROT_READ | 
PROT_WRITE, MAP_PRIVATE | MAP_ANONYMOUS, -1, 0);
                                 if (ptr == MAP_FAILED) {
                                         fprintf(stderr, "mmap() failed, 
size %zu iter %d\n", s, i);
                                         return 1;
                                 }
                                 munmap(ptr, s);
                         }
                 }
         } else {
                 for (int i = 0; i < ROUNDS; i++) {
                         for (int j = 0; j < SIZES; j++) {
                                 size_t s = SIZE_BASE * (1 << (j * 
SIZE_FACTOR));

                                 void *ptr = sbrk(s);
                                 if (ptr == (void *) -1) {
                                         fprintf(stderr, "sbrk() failed, 
size %zu iter %d\n", s, i);
                                         return 1;
                                 }
                                 sbrk(-s);
                         }
                 }
         }

         return 0;
}

>>>> 2. Conditionally use mmap() instead of sbrk()
>>>>
>>>> Something like `#define USE_SBRK`, enabled by `configure` or a header file.
>>> i think configure time option is not a good idea,
>>> but e.g. it can be a runtime tunable.
>>
>> The runtime option needs to be available very early in the dynamic
>> loader, before errno and malloc() are available. Would getenv() work?
> 
> getenv wouldn't work, but there is a parser for the GLIBC_TUNABLES
> environment variable.

OK, I'll try to use that in the next version.

-Topi

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [RFC PATCH 0/3] Improved ALSR
  2020-11-23 18:16   ` Topi Miettinen
@ 2020-11-24  9:47     ` Szabolcs Nagy
  0 siblings, 0 replies; 18+ messages in thread
From: Szabolcs Nagy @ 2020-11-24  9:47 UTC (permalink / raw)
  To: Topi Miettinen; +Cc: Florian Weimer, Topi Miettinen via Libc-alpha

The 11/23/2020 20:16, Topi Miettinen via Libc-alpha wrote:
> On 23.11.2020 18.44, Florian Weimer wrote:
> > And the series only changes the TCB allocation for the main thread.
> > Fixing thread TCB/stack collocation is a completely different matter
> > (and probably the more significant issue than lack of ASLR).
> 
> I thought I covered all uses of sbrk(), perhaps I missed that one. Does the
> thread TCB/stack allocation use sbrk()?

the point is that thread stack and tls is allocated
as one block (with mmap, but that does not matter).

you want at least a guard page in between, so stack
corruption does not clobber tcb/tls data.

but this has costs (kernel side vma) and significant
work in glibc (separate tls and stack size accounting).

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [RFC PATCH 0/3] Improved ALSR
  2020-11-23 22:28         ` Topi Miettinen
@ 2020-11-24 11:24           ` Florian Weimer
  2020-11-24 11:59             ` Topi Miettinen
  0 siblings, 1 reply; 18+ messages in thread
From: Florian Weimer @ 2020-11-24 11:24 UTC (permalink / raw)
  To: Topi Miettinen; +Cc: Topi Miettinen via Libc-alpha, Szabolcs Nagy

* Topi Miettinen:

> On 23.11.2020 23.45, Florian Weimer wrote:
>> * Topi Miettinen via Libc-alpha:
>> 
>>> $ time ./malloc-vs-sbrk
>>>
>>> real    0m1.923s
>>> user    0m0.160s
>>> sys     0m1.762s
>>> $ time ./malloc-vs-sbrk 1
>>>
>>> real    0m2.847s
>>> user    0m0.176s
>>> sys     0m2.669s
>> Does the difference go away if you change the mmap granularity to
>> 128 KiB?  I think this happens under the covers (on the kernel side)
>> with sbrk.
>
> Does not seem so, 56% increase:

But the test does not seem very realistic because the pages are never
faulted in.  Sorry, I didn't check that before.

Thanks,
Florian
-- 
Red Hat GmbH, https://de.redhat.com/ , Registered seat: Grasbrunn,
Commercial register: Amtsgericht Muenchen, HRB 153243,
Managing Directors: Charles Cachera, Brian Klemm, Laurie Krebs, Michael O'Neill


^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [RFC PATCH 0/3] Improved ALSR
  2020-11-24 11:24           ` Florian Weimer
@ 2020-11-24 11:59             ` Topi Miettinen
  2020-11-24 14:29               ` Adhemerval Zanella
  2020-11-30 10:28               ` Florian Weimer
  0 siblings, 2 replies; 18+ messages in thread
From: Topi Miettinen @ 2020-11-24 11:59 UTC (permalink / raw)
  To: Florian Weimer; +Cc: Topi Miettinen via Libc-alpha, Szabolcs Nagy

On 24.11.2020 13.24, Florian Weimer wrote:
> * Topi Miettinen:
> 
>> On 23.11.2020 23.45, Florian Weimer wrote:
>>> * Topi Miettinen via Libc-alpha:
>>>
>>>> $ time ./malloc-vs-sbrk
>>>>
>>>> real    0m1.923s
>>>> user    0m0.160s
>>>> sys     0m1.762s
>>>> $ time ./malloc-vs-sbrk 1
>>>>
>>>> real    0m2.847s
>>>> user    0m0.176s
>>>> sys     0m2.669s
>>> Does the difference go away if you change the mmap granularity to
>>> 128 KiB?  I think this happens under the covers (on the kernel side)
>>> with sbrk.
>>
>> Does not seem so, 56% increase:
> 
> But the test does not seem very realistic because the pages are never
> faulted in.  Sorry, I didn't check that before.

Right, this changes the equation dramatically:

# time ./malloc-vs-sbrk

real    0m19.498s
user    0m1.192s
sys     0m18.302s
# time ./malloc-vs-sbrk 1

real    0m19.428s
user    0m1.276s
sys     0m18.146s

FYI, the effect of full ASLR of mmap() by kernel also seems small:

# echo 3 >/proc/sys/kernel/randomize_va_space
# time ./malloc-vs-sbrk

real    0m19.489s
user    0m1.263s
sys     0m18.211s
# time ./malloc-vs-sbrk 1

real    0m19.532s
user    0m1.148s
sys     0m18.366s

# cat malloc-vs-sbrk.c
#include <sys/mman.h>
#include <stdio.h>
#include <stdlib.h>
#include <string.h>
#include <unistd.h>

#define ROUNDS 1000
#define SIZES 4
#define SIZE_FACTOR 3
#define SIZE_BASE (128 * 1024)

int main(int argc, char **argv) {
         if (argc == 2) {
                 for (int i = 0; i < ROUNDS; i++) {
                         for (int j = 0; j < SIZES; j++) {
                                 size_t s = SIZE_BASE * (1 << (j * 
SIZE_FACTOR));

                                 void *ptr = mmap(NULL, s, PROT_READ | 
PROT_WRITE, MAP_PRIVATE | MAP_ANONYMOUS, -1, 0);
                                 if (ptr == MAP_FAILED) {
                                         fprintf(stderr, "mmap() failed, 
size %zu iter %d\n", s, i);
                                         return 1;
                                 }
                                 memset(ptr, 0, s);
                                 munmap(ptr, s);
                         }
                 }
         } else {
                 for (int i = 0; i < ROUNDS; i++) {
                         for (int j = 0; j < SIZES; j++) {
                                 size_t s = SIZE_BASE * (1 << (j * 
SIZE_FACTOR));

                                 void *ptr = sbrk(s);
                                 if (ptr == (void *) -1) {
                                         fprintf(stderr, "sbrk() failed, 
size %zu iter %d\n", s, i);
                                         return 1;
                                 }
                                 memset(ptr, 0, s);
                                 sbrk(-s);
                         }
                 }
         }

         return 0;
}

-Topi

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [RFC PATCH 0/3] Improved ALSR
  2020-11-24 11:59             ` Topi Miettinen
@ 2020-11-24 14:29               ` Adhemerval Zanella
  2020-11-30 10:28               ` Florian Weimer
  1 sibling, 0 replies; 18+ messages in thread
From: Adhemerval Zanella @ 2020-11-24 14:29 UTC (permalink / raw)
  To: libc-alpha



On 24/11/2020 08:59, Topi Miettinen via Libc-alpha wrote:
> On 24.11.2020 13.24, Florian Weimer wrote:
>> * Topi Miettinen:
>>
>>> On 23.11.2020 23.45, Florian Weimer wrote:
>>>> * Topi Miettinen via Libc-alpha:
>>>>
>>>>> $ time ./malloc-vs-sbrk
>>>>>
>>>>> real    0m1.923s
>>>>> user    0m0.160s
>>>>> sys     0m1.762s
>>>>> $ time ./malloc-vs-sbrk 1
>>>>>
>>>>> real    0m2.847s
>>>>> user    0m0.176s
>>>>> sys     0m2.669s
>>>> Does the difference go away if you change the mmap granularity to
>>>> 128 KiB?  I think this happens under the covers (on the kernel side)
>>>> with sbrk.
>>>
>>> Does not seem so, 56% increase:
>>
>> But the test does not seem very realistic because the pages are never
>> faulted in.  Sorry, I didn't check that before.
> 
> Right, this changes the equation dramatically:
> 
> # time ./malloc-vs-sbrk
> 
> real    0m19.498s
> user    0m1.192s
> sys     0m18.302s
> # time ./malloc-vs-sbrk 1
> 
> real    0m19.428s
> user    0m1.276s
> sys     0m18.146s
> 
> FYI, the effect of full ASLR of mmap() by kernel also seems small:
> 
> # echo 3 >/proc/sys/kernel/randomize_va_space
> # time ./malloc-vs-sbrk
> 
> real    0m19.489s
> user    0m1.263s
> sys     0m18.211s
> # time ./malloc-vs-sbrk 1
> 
> real    0m19.532s
> user    0m1.148s
> sys     0m18.366s

I saw similar results showing little performance difference on different
architectures as well (aarch64, s390x, sparc64, and armhf), so I don't 
think performance should an impending reason for this change.

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [RFC PATCH 1/3] csu: randomize location of TCB
  2020-10-04 13:09 ` [RFC PATCH 1/3] csu: randomize location of TCB Topi Miettinen
@ 2020-11-24 18:44   ` Adhemerval Zanella
  0 siblings, 0 replies; 18+ messages in thread
From: Adhemerval Zanella @ 2020-11-24 18:44 UTC (permalink / raw)
  To: libc-alpha



On 04/10/2020 10:09, Topi Miettinen via Libc-alpha wrote:
> Let's use mmap() for TCB, which makes the location of TCB random
> instead of always staying predictably next to data segment. Also
> improve the logic so that allocation of TCB can be assumed to fail
> insted of segfaulting.
> 
> RFC: make independent of Linux.

The interface does provide the required independence, the issue is
adapt Hurd code to add a mmap interface that does no set errno.

> 
> Signed-off-by: Topi Miettinen <toiwoton@gmail.com>

We do not use SCO, but rather Copyright assignment.

> ---
>  csu/libc-tls.c                          | 20 ++++++++++++++------
>  sysdeps/unix/sysv/linux/mmap64.c        | 19 +++++++++++++++++++
>  sysdeps/unix/sysv/linux/mmap_internal.h |  3 +++
>  3 files changed, 36 insertions(+), 6 deletions(-)
> 
> diff --git a/csu/libc-tls.c b/csu/libc-tls.c
> index 06e76bd395..59700c3a95 100644
> --- a/csu/libc-tls.c
> +++ b/csu/libc-tls.c
> @@ -24,6 +24,9 @@
>  #include <stdio.h>
>  #include <sys/param.h>
>  #include <array_length.h>
> +#include <sys/mman.h>
> +#include <sysdep.h>
> +#include <mmap_internal.h>
>  
>  #ifdef SHARED
>   #error makefile bug, this file is for static only
> @@ -134,25 +137,30 @@ __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 'internal_mmap' which does not use 'errno'. */

I think you mean '__mmap_internal' here, however I think it should be rename
__mmap64_internal since it does use off64_t (more below).

> +
> +  int error = 0;
> +
>  #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 = __mmap_internal (NULL, tcb_offset + TLS_INIT_TCB_SIZE + max_align,
> +			      PROT_READ | PROT_WRITE, MAP_ANONYMOUS | MAP_PRIVATE, -1, 0, &error);

Line too long.

>  #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 = __mmap_internal (NULL, tcb_offset + memsz + max_align
> +			      + TLS_PRE_TCB_SIZE + GLRO(dl_tls_static_surplus),
> +			      PROT_READ | PROT_WRITE, MAP_ANONYMOUS | MAP_PRIVATE, -1, 0, &error);
>    tlsblock += TLS_PRE_TCB_SIZE;
>  #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 (error)
> +    _startup_fatal ("Cannot allocate TCB");

No implicit checks.  Also, since you are not really using its value I think 
you just use a an automatic here to avoid its definition and check the
tlsblock against MAP_FAILED (it would need to move the tlsblock
adjustment for !TLS_DTV_AT_TP to after the check). Something like:

diff --git a/csu/libc-tls.c b/csu/libc-tls.c
index c3589f0a7d..69b5bd1301 100644
--- a/csu/libc-tls.c
+++ b/csu/libc-tls.c
@@ -135,24 +135,32 @@ __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 'mmap64_internal' which does not use 'errno'. */
 #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 = __mmap64_internal (NULL, tcb_offset + TLS_INIT_TCB_SIZE
+				      + max_align,
+				PROT_READ | PROT_WRITE, MAP_ANONYMOUS
+				| MAP_PRIVATE, -1, 0, &(int){0});
 #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 = __mmap64_internal (NULL, tcb_offset + memsz + max_align
+				+ TLS_PRE_TCB_SIZE
+				+ GLRO(dl_tls_static_surplus),
+				PROT_READ | PROT_WRITE, MAP_ANONYMOUS
+				| MAP_PRIVATE, -1, 0, &(int){0});
 #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 (tlsblock == MAP_FAILED)
+    _startup_fatal ("Cannot allocate TCB");
+#if TLS_DTV_AT_TP
+  tlsblock += TLS_PRE_TCB_SIZE;
 #endif
 
   /* Align the TLS block.  */

>  
>    /* Align the TLS block.  */
>    tlsblock = (void *) (((uintptr_t) tlsblock + max_align - 1)
> diff --git a/sysdeps/unix/sysv/linux/mmap64.c b/sysdeps/unix/sysv/linux/mmap64.c
> index 8074deb466..11f7c3f99b 100644
> --- a/sysdeps/unix/sysv/linux/mmap64.c
> +++ b/sysdeps/unix/sysv/linux/mmap64.c
> @@ -67,3 +67,22 @@ weak_alias (__mmap64, mmap)
>  weak_alias (__mmap64, __mmap)
>  libc_hidden_def (__mmap)
>  #endif
> +
> +void *
> +__mmap_internal (void *addr, size_t len, int prot, int flags, int fd, off64_t offset, int *error)
> +{
> +  unsigned long int ret;
> +#ifdef __NR_mmap2
> +  ret = INTERNAL_SYSCALL_CALL (mmap2, addr, len, prot, flags, fd,
> +			     (off_t) (offset / MMAP2_PAGE_UNIT));
> +#else
> +  ret = INTERNAL_SYSCALL_CALL (mmap, addr, len, prot, flags, fd, offset);
> +#endif
> +  if (INTERNAL_SYSCALL_ERROR_P(ret))
> +    {
> +      *error = ret;
> +      return MAP_FAILED;
> +    }
> +
> +  return (void *) ret;
> +}

The MMAP_CALL macro is used to allow an architecture to redefine if
required, and s390 does. And I think it would be better to refactor the code 
to avoid a duplication of the syscall logic:

diff --git a/sysdeps/unix/sysv/linux/mmap.c b/sysdeps/unix/sysv/linux/mmap.c
index 22f276bb14..071c7462e0 100644
--- a/sysdeps/unix/sysv/linux/mmap.c
+++ b/sysdeps/unix/sysv/linux/mmap.c
@@ -39,12 +39,18 @@ __mmap (void *addr, size_t len, int prot, int flags, int fd, off_t offset)
     return (void *) INLINE_SYSCALL_ERROR_RETURN_VALUE (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))
+    {
+      __set_errno (INTERNAL_SYSCALL_ERRNO (r));
+      return MAP_FAILED;
+    }
+  return (void*) 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..8487096726 100644
--- a/sysdeps/unix/sysv/linux/mmap64.c
+++ b/sysdeps/unix/sysv/linux/mmap64.c
@@ -44,20 +44,33 @@
 #endif
 
 void *
-__mmap64 (void *addr, size_t len, int prot, int flags, int fd, off64_t offset)
+__mmap64_internal (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_internal (addr, len, prot, flags, fd, offset, &errno);
 }
 weak_alias (__mmap64, mmap64)
 libc_hidden_def (__mmap64)
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..4d5e0291c3 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>

> diff --git a/sysdeps/unix/sysv/linux/mmap_internal.h b/sysdeps/unix/sysv/linux/mmap_internal.h
> index d53f0c642a..00fc14902e 100644
> --- a/sysdeps/unix/sysv/linux/mmap_internal.h
> +++ b/sysdeps/unix/sysv/linux/mmap_internal.h
> @@ -46,4 +46,7 @@ static uint64_t page_unit;
>    INLINE_SYSCALL_CALL (__nr, __addr, __len, __prot, __flags, __fd, __offset)
>  #endif
>  
> +/* Internal version of mmap() which doesn't attempt to access errno */
> +void *__mmap_internal (void *addr, size_t len, int prot, int flags, int fd, off64_t offset, int *error);
> +
>  #endif /* MMAP_INTERNAL_LINUX_H  */
> 

Move the internal definition to include/sys/mman.h instead, the
mmap_internal.h is a Linux one header meant to be used to
implement mmap itself.

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [RFC PATCH 0/3] Improved ALSR
  2020-11-24 11:59             ` Topi Miettinen
  2020-11-24 14:29               ` Adhemerval Zanella
@ 2020-11-30 10:28               ` Florian Weimer
  2020-11-30 11:39                 ` Topi Miettinen
  1 sibling, 1 reply; 18+ messages in thread
From: Florian Weimer @ 2020-11-30 10:28 UTC (permalink / raw)
  To: Topi Miettinen; +Cc: Topi Miettinen via Libc-alpha, Szabolcs Nagy

* Topi Miettinen:

> FYI, the effect of full ASLR of mmap() by kernel also seems small:
>
> # echo 3 >/proc/sys/kernel/randomize_va_space
> # time ./malloc-vs-sbrk
>
> real    0m19.489s
> user    0m1.263s
> sys     0m18.211s
> # time ./malloc-vs-sbrk 1
>
> real    0m19.532s
> user    0m1.148s
> sys     0m18.366s

The value 3 doesn't do anything in a mainline kernel.

I think you will see rather catastrophic effects if you have a workload
that triggers TLB misses.  I expect that the cost of walking page tables
increases dramatically with full ASLR.

Thanks,
Florian
-- 
Red Hat GmbH, https://de.redhat.com/ , Registered seat: Grasbrunn,
Commercial register: Amtsgericht Muenchen, HRB 153243,
Managing Directors: Charles Cachera, Brian Klemm, Laurie Krebs, Michael O'Neill


^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [RFC PATCH 0/3] Improved ALSR
  2020-11-30 10:28               ` Florian Weimer
@ 2020-11-30 11:39                 ` Topi Miettinen
  0 siblings, 0 replies; 18+ messages in thread
From: Topi Miettinen @ 2020-11-30 11:39 UTC (permalink / raw)
  To: Florian Weimer; +Cc: Topi Miettinen via Libc-alpha, Szabolcs Nagy

On 30.11.2020 12.28, Florian Weimer wrote:
> * Topi Miettinen:
> 
>> FYI, the effect of full ASLR of mmap() by kernel also seems small:
>>
>> # echo 3 >/proc/sys/kernel/randomize_va_space
>> # time ./malloc-vs-sbrk
>>
>> real    0m19.489s
>> user    0m1.263s
>> sys     0m18.211s
>> # time ./malloc-vs-sbrk 1
>>
>> real    0m19.532s
>> user    0m1.148s
>> sys     0m18.366s
> 
> The value 3 doesn't do anything in a mainline kernel.

No, you need this patch to randomize mmap(), mremap(..., 
MREMAP_MAYMOVE), stack and vdso:

https://lkml.org/lkml/2020/11/29/214

> I think you will see rather catastrophic effects if you have a workload
> that triggers TLB misses.  I expect that the cost of walking page tables
> increases dramatically with full ASLR.

Each random mapping may likely need new page tables, up to 4 pages. Thus 
for a workload with lots of small (page-size) mappings, the memory 
consumption will indeed increase a lot with 
sysctl.kernel.randomize_va_space=3. But as a real world example, main 
process of Firefox has roughly 1500 lines in /proc/$PID/maps in my 
system. Some are contiguous to other mappings, but for simplicity, 
pretending that they all were independent and each required a full set 
of page tables, it would mean 1500*4*4kB  = 24MB of page tables. RSS of 
the process is 340MB, so the effect is not that dramatic by itself, but 
page table entries may also compete for cache slots. So 
randomize_va_space=3 clearly isn't interesting for performance but 
hardening reasons. I'd also not use it on a 32-bit system because the 
virtual memory space could be fragmented too much. I don't think that 
could happen on a 64 bit system, the physical memory should run out first.

The effect of using mmap() instead of sbrk() in libc is independent of 
the kernel patch: one malloc arena and few other areas will use mmap() 
instead of sbrk(), so only a few mappings are affected. With 
randomize_va_space=2, the mappings made with mmap() will be allocated 
close to the other mappings and then a lot of page tables will be 
shared, so probably no additional memory is needed. With a patched 
kernel and randomize_va_space=3, each mapping will be placed randomly. 
This could take maybe tens of kBs more memory in page tables.

-Topi

^ permalink raw reply	[flat|nested] 18+ messages in thread

end of thread, other threads:[~2020-11-30 11:39 UTC | newest]

Thread overview: 18+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-10-04 13:09 [RFC PATCH 0/3] Improved ALSR Topi Miettinen
2020-10-04 13:09 ` [RFC PATCH 1/3] csu: randomize location of TCB Topi Miettinen
2020-11-24 18:44   ` Adhemerval Zanella
2020-10-04 13:09 ` [RFC PATCH 2/3] malloc: always use mmap() to improve ASLR Topi Miettinen
2020-10-04 13:09 ` [RFC PATCH 3/3] dl-sysdep: disable remaining calls to sbrk() Topi Miettinen
2020-11-23 16:06 ` [RFC PATCH 0/3] Improved ALSR Topi Miettinen
2020-11-23 16:41   ` Szabolcs Nagy
2020-11-23 17:55     ` Topi Miettinen
2020-11-23 21:45       ` Florian Weimer
2020-11-23 22:28         ` Topi Miettinen
2020-11-24 11:24           ` Florian Weimer
2020-11-24 11:59             ` Topi Miettinen
2020-11-24 14:29               ` Adhemerval Zanella
2020-11-30 10:28               ` Florian Weimer
2020-11-30 11:39                 ` Topi Miettinen
2020-11-23 16:44 ` Florian Weimer
2020-11-23 18:16   ` Topi Miettinen
2020-11-24  9:47     ` Szabolcs Nagy

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).