diff mbox

malloc: additional unlink hardening for non-small bins [BZ #17344]

Message ID 541014A3.3020806@redhat.com
State New
Headers show

Commit Message

Florian Weimer Sept. 10, 2014, 9:06 a.m. UTC 
Chris Evans has suggested this as a way to stop certain exploits abusing 
malloc's metadata processing.  My patch is slightly different, the patch 
in the bug report has some whitespace changes and locking code which is 
not present in master.

Apparently, some downstreams compile malloc with asserts enabled, and 
they do not fire, so the validity of the additional checks has in effect 
already been tested in the field.

Tested on Fedora 20 x86_64, with no regressions.

Comments

Roland McGrath Sept. 10, 2014, 6:10 p.m. UTC | #1 
I think it's best practice to put each condition inside a __builtin_expect
separately, rather than to have a complex expression inside it (despite the
contrary example a few lines up).  The change seems fine aside from that.
Florian Weimer Sept. 10, 2014, 6:39 p.m. UTC | #2 
On 09/10/2014 08:10 PM, Roland McGrath wrote:
> I think it's best practice to put each condition inside a __builtin_expect
> separately, rather than to have a complex expression inside it (despite the
> contrary example a few lines up).  The change seems fine aside from that.

Thanks.  The generated code is the same, so I'm going to commit the 
version with two __builtin_expect expressions.

(Eventually, we have to convert malloc/ to __glibc_{,un}likely.)
diff mbox

Patch

From 43eb4f76b7736f673c46b1b908e2c5b51004498d Mon Sep 17 00:00:00 2001
From: Florian Weimer <fweimer@redhat.com>
Date: Wed, 10 Sep 2014 11:00:55 +0200
Subject: [PATCH] malloc: additional unlink hardening for non-small bins [BZ
 #17344]

Turn two asserts into a conditional call to malloc_printerr.  The
memory locations are accessed later anyway, so the performance
impact is minor.

2014-09-10  Florian Weimer  <fweimer@redhat.com>

	[BZ #17344]
	* malloc/malloc.c (unlink): Turn asserts into a call to
	malloc_printerr.


diff --git a/NEWS b/NEWS
index 721b457..30df941 100644
--- a/NEWS
+++ b/NEWS
@@ -23,7 +23,7 @@  Version 2.20
   16966, 16967, 16977, 16978, 16984, 16990, 16996, 17009, 17022, 17031,
   17042, 17048, 17050, 17058, 17061, 17062, 17069, 17075, 17078, 17079,
   17084, 17086, 17088, 17092, 17097, 17125, 17135, 17137, 17150, 17153,
-  17187, 17213, 17259, 17261, 17262, 17263, 17319, 17325, 17354.
+  17187, 17213, 17259, 17261, 17262, 17263, 17319, 17325, 17344, 17354.
 
 * Reverted change of ABI data structures for s390 and s390x:
   On s390 and s390x the size of struct ucontext and jmp_buf was increased in
diff --git a/malloc/malloc.c b/malloc/malloc.c
index 6ee3840..47859e1 100644
--- a/malloc/malloc.c
+++ b/malloc/malloc.c
@@ -1418,8 +1418,10 @@  typedef struct malloc_chunk *mbinptr;
         BK->fd = FD;							      \
         if (!in_smallbin_range (P->size)				      \
             && __builtin_expect (P->fd_nextsize != NULL, 0)) {		      \
-            assert (P->fd_nextsize->bk_nextsize == P);			      \
-            assert (P->bk_nextsize->fd_nextsize == P);			      \
+	    if (__builtin_expect (P->fd_nextsize->bk_nextsize != P	      \
+				  || P->bk_nextsize->fd_nextsize != P, 0))    \
+	      malloc_printerr (check_action,				      \
+			       "corrupted double-linked list (not small)", P);\
             if (FD->fd_nextsize == NULL) {				      \
                 if (P->fd_nextsize == P)				      \
                   FD->fd_nextsize = FD->bk_nextsize = FD;		      \
-- 
1.9.3