[PATCH] malloc: always fail with ENOMEM

[PATCH] malloc: always fail with ENOMEM

[Julien Ramseier]

malloc may set errno to something else than ENOMEM indirectly
through mmap, though ENOMEM is the only error allowed by POSIX.

There are cases where mmap will return EPERM instead of ENOMEM,
as highlighted by libc-test[1]. This can happen when mmap tries to
map pages near `mmap_min_addr` [2][3], as a security measure.

[1] http://www.openwall.com/lists/musl/2016/03/30/9
[2] https://ghc.haskell.org/trac/ghc/ticket/7500
[3] https://github.com/torvalds/linux/blob/master/security/min_addr.c

diff --git a/src/malloc/expand_heap.c b/src/malloc/expand_heap.c
index d8c0be7..4051b1b 100644
--- a/src/malloc/expand_heap.c
+++ b/src/malloc/expand_heap.c
@@ -65,7 +65,10 @@ void *__expand_heap(size_t *pn)
 	if (n < min) n = min;
 	void *area = __mmap(0, n, PROT_READ|PROT_WRITE,
 		MAP_PRIVATE|MAP_ANONYMOUS, -1, 0);
-	if (area == MAP_FAILED) return 0;
+	if (area == MAP_FAILED) {
+		errno = ENOMEM;
+		return 0;
+	}
 	*pn = n;
 	mmap_step++;
 	return area;
diff --git a/src/malloc/malloc.c b/src/malloc/malloc.c
index c38c46f..593c4dd 100644
--- a/src/malloc/malloc.c
+++ b/src/malloc/malloc.c
@@ -328,7 +328,10 @@ void *malloc(size_t n)
 		size_t len = n + OVERHEAD + PAGE_SIZE - 1 & -PAGE_SIZE;
 		char *base = __mmap(0, len, PROT_READ|PROT_WRITE,
 			MAP_PRIVATE|MAP_ANONYMOUS, -1, 0);
-		if (base == (void *)-1) return 0;
+		if (base == MAP_FAILED) {
+			errno = ENOMEM;
+			return 0;
+		}
 		c = (void *)(base + SIZE_ALIGN - OVERHEAD);
 		c->csize = len - (SIZE_ALIGN - OVERHEAD);
 		c->psize = SIZE_ALIGN - OVERHEAD;

Re: [PATCH] malloc: always fail with ENOMEM

[Alexander Monakov]

On Mon, 9 Jan 2017, Julien Ramseier wrote:

> malloc may set errno to something else than ENOMEM indirectly
> through mmap, though ENOMEM is the only error allowed by POSIX.

Pedantically speaking, no: POSIX explicitly allows implementations
to report errors other than specified, in case errors are allowed;
from 2.3 "Error Numbers":

    Implementations [...] may generate additional errors unless
    explicitly disallowed for a particular function.

But this is just FYI, of course reporting EPERM from malloc is not useful
(and Rich indicated that he'd like to avoid that).

> There are cases where mmap will return EPERM instead of ENOMEM,
> as highlighted by libc-test[1]. This can happen when mmap tries to
> map pages near `mmap_min_addr` [2][3], as a security measure.
> 
> [1] http://www.openwall.com/lists/musl/2016/03/30/9
> [2] https://ghc.haskell.org/trac/ghc/ticket/7500
> [3] https://github.com/torvalds/linux/blob/master/security/min_addr.c

Thanks for digging out the cause.  I think you should wait for feedback
from Rich on the patch.

Alexander

Re: [PATCH] malloc: always fail with ENOMEM

[Szabolcs Nagy]

* Julien Ramseier <j.ramseier@gmail.com> [2017-01-09 09:47:35 +0100]:
> malloc may set errno to something else than ENOMEM indirectly
> through mmap, though ENOMEM is the only error allowed by POSIX.
> 
> There are cases where mmap will return EPERM instead of ENOMEM,
> as highlighted by libc-test[1]. This can happen when mmap tries to
> map pages near `mmap_min_addr` [2][3], as a security measure.

i've seen this too and i think this should be fixed in mmap
(i.e. never return EPERM for anonymous mmap(0,...), since
posix specifies ENOMEM for that failure)

> 
> [1] http://www.openwall.com/lists/musl/2016/03/30/9
> [2] https://ghc.haskell.org/trac/ghc/ticket/7500
> [3] https://github.com/torvalds/linux/blob/master/security/min_addr.c

Re: [PATCH] malloc: always fail with ENOMEM

[Rich Felker]

On Mon, Jan 09, 2017 at 12:13:52PM +0100, Szabolcs Nagy wrote:
> * Julien Ramseier <j.ramseier@gmail.com> [2017-01-09 09:47:35 +0100]:
> > malloc may set errno to something else than ENOMEM indirectly
> > through mmap, though ENOMEM is the only error allowed by POSIX.
> > 
> > There are cases where mmap will return EPERM instead of ENOMEM,
> > as highlighted by libc-test[1]. This can happen when mmap tries to
> > map pages near `mmap_min_addr` [2][3], as a security measure.
> 
> i've seen this too and i think this should be fixed in mmap
> (i.e. never return EPERM for anonymous mmap(0,...), since
> posix specifies ENOMEM for that failure)

Yes, both places in the patch call __mmap, so it could just be fixed
in __mmap. Rather than return syscall(...) we should probably have
something like"

	long ret = __syscall(...);
	if (ret == -EPERM && ...) ret = -ENOMEM;
	return (void *)__syscall_ret(ret);

where the ... checks for something like MAP_ANON and not MAP_FIXED or
similar. Does that look correct?

Rich