[PATCH v3 0/3] Actually fix freelist pointer vs redzoning

[PATCH v3 0/3] Actually fix freelist pointer vs redzoning

[Kees Cook]

v3:
- fix commit messages to properly reflect the direction of the overwrite
- justify the less-than-word-size patch better
- add Acks
- move some Fixes up into the commit log as just references
v2: https://lore.kernel.org/lkml/20201009195411.4018141-1-keescook@chromium.org
v1: https://lore.kernel.org/lkml/20201008233443.3335464-1-keescook@chromium.org

This fixes redzoning vs the freelist pointer (both for middle-position
and very small caches). Both are "theoretical" fixes, in that I see no
evidence of such small-sized caches actually be used in the kernel, but
that's no reason to let the bugs continue to exist. :)

Note on patch 2: Christopher NAKed it, but I actually think this is a
reasonable thing to add -- the "too small" check is only made when built
with CONFIG_DEBUG_VM, so it *is* actually possible for someone to trip
over this directly, even if it would never make it into a released
kernel. I see no reason to just leave this foot-gun in place, though, so
we might as well just fix it too. (Which seems to be what Longman was
similarly supporting, IIUC.)

Anyway, if patch 2 stays NAKed, that's fine. It's entirely separable,
and the other 2 can land. :)

Thanks!

-Kees


Kees Cook (3):
  mm/slub: Clarify verification reporting
  mm/slub: Fix redzoning for small allocations
  mm/slub: Actually fix freelist pointer vs redzoning

 Documentation/vm/slub.rst | 10 +++++-----
 mm/slub.c                 | 36 +++++++++++++++---------------------
 2 files changed, 20 insertions(+), 26 deletions(-)

-- 
2.25.1

[PATCH v3 1/3] mm/slub: Clarify verification reporting

[Kees Cook]

Instead of repeating "Redzone" and "Poison", clarify which sides of
those zones got tripped. Additionally fix column alignment in the
trailer.

Before:

BUG test (Tainted: G    B            ): Redzone overwritten
...
Redzone (____ptrval____): bb bb bb bb bb bb bb bb      ........
Object (____ptrval____): f6 f4 a5 40 1d e8            ...@..
Redzone (____ptrval____): 1a aa                        ..
Padding (____ptrval____): 00 00 00 00 00 00 00 00      ........

After:

BUG test (Tainted: G    B            ): Right Redzone overwritten
...
Redzone  (____ptrval____): bb bb bb bb bb bb bb bb      ........
Object   (____ptrval____): f6 f4 a5 40 1d e8            ...@..
Redzone  (____ptrval____): 1a aa                        ..
Padding  (____ptrval____): 00 00 00 00 00 00 00 00      ........

The earlier commits that slowly resulted in the "Before" reporting were:

  d86bd1bece6f ("mm/slub: support left redzone")
  ffc79d288000 ("slub: use print_hex_dump")
  2492268472e7 ("SLUB: change error reporting format to follow lockdep loosely")

Signed-off-by: Kees Cook <keescook@chromium.org>
Acked-by: Vlastimil Babka <vbabka@suse.cz>
Link: https://lore.kernel.org/lkml/cfdb11d7-fb8e-e578-c939-f7f5fb69a6bd@suse.cz/
---
 Documentation/vm/slub.rst | 10 +++++-----
 mm/slub.c                 | 14 +++++++-------
 2 files changed, 12 insertions(+), 12 deletions(-)

diff --git a/Documentation/vm/slub.rst b/Documentation/vm/slub.rst
index 289d231cee97..77c7a3331eda 100644
--- a/Documentation/vm/slub.rst
+++ b/Documentation/vm/slub.rst
@@ -181,7 +181,7 @@ SLUB Debug output
 Here is a sample of slub debug output::
 
  ====================================================================
- BUG kmalloc-8: Redzone overwritten
+ BUG kmalloc-8: Right Redzone overwritten
  --------------------------------------------------------------------
 
  INFO: 0xc90f6d28-0xc90f6d2b. First byte 0x00 instead of 0xcc
@@ -189,10 +189,10 @@ Here is a sample of slub debug output::
  INFO: Object 0xc90f6d20 @offset=3360 fp=0xc90f6d58
  INFO: Allocated in get_modalias+0x61/0xf5 age=53 cpu=1 pid=554
 
- Bytes b4 0xc90f6d10:  00 00 00 00 00 00 00 00 5a 5a 5a 5a 5a 5a 5a 5a ........ZZZZZZZZ
-   Object 0xc90f6d20:  31 30 31 39 2e 30 30 35                         1019.005
-  Redzone 0xc90f6d28:  00 cc cc cc                                     .
-  Padding 0xc90f6d50:  5a 5a 5a 5a 5a 5a 5a 5a                         ZZZZZZZZ
+ Bytes b4 (0xc90f6d10): 00 00 00 00 00 00 00 00 5a 5a 5a 5a 5a 5a 5a 5a ........ZZZZZZZZ
+ Object   (0xc90f6d20): 31 30 31 39 2e 30 30 35                         1019.005
+ Redzone  (0xc90f6d28): 00 cc cc cc                                     .
+ Padding  (0xc90f6d50): 5a 5a 5a 5a 5a 5a 5a 5a                         ZZZZZZZZ
 
    [<c010523d>] dump_trace+0x63/0x1eb
    [<c01053df>] show_trace_log_lvl+0x1a/0x2f
diff --git a/mm/slub.c b/mm/slub.c
index 6d3574013b2f..f4f1d63f0ab9 100644
--- a/mm/slub.c
+++ b/mm/slub.c
@@ -698,15 +698,15 @@ static void print_trailer(struct kmem_cache *s, struct page *page, u8 *p)
 	       p, p - addr, get_freepointer(s, p));
 
 	if (s->flags & SLAB_RED_ZONE)
-		print_section(KERN_ERR, "Redzone ", p - s->red_left_pad,
+		print_section(KERN_ERR, "Redzone  ", p - s->red_left_pad,
 			      s->red_left_pad);
 	else if (p > addr + 16)
 		print_section(KERN_ERR, "Bytes b4 ", p - 16, 16);
 
-	print_section(KERN_ERR, "Object ", p,
+	print_section(KERN_ERR,         "Object   ", p,
 		      min_t(unsigned int, s->object_size, PAGE_SIZE));
 	if (s->flags & SLAB_RED_ZONE)
-		print_section(KERN_ERR, "Redzone ", p + s->object_size,
+		print_section(KERN_ERR, "Redzone  ", p + s->object_size,
 			s->inuse - s->object_size);
 
 	off = get_info_end(s);
@@ -718,7 +718,7 @@ static void print_trailer(struct kmem_cache *s, struct page *page, u8 *p)
 
 	if (off != size_from_object(s))
 		/* Beginning of the filler is the free pointer */
-		print_section(KERN_ERR, "Padding ", p + off,
+		print_section(KERN_ERR, "Padding  ", p + off,
 			      size_from_object(s) - off);
 
 	dump_stack();
@@ -895,11 +895,11 @@ static int check_object(struct kmem_cache *s, struct page *page,
 	u8 *endobject = object + s->object_size;
 
 	if (s->flags & SLAB_RED_ZONE) {
-		if (!check_bytes_and_report(s, page, object, "Redzone",
+		if (!check_bytes_and_report(s, page, object, "Left Redzone",
 			object - s->red_left_pad, val, s->red_left_pad))
 			return 0;
 
-		if (!check_bytes_and_report(s, page, object, "Redzone",
+		if (!check_bytes_and_report(s, page, object, "Right Redzone",
 			endobject, val, s->inuse - s->object_size))
 			return 0;
 	} else {
@@ -914,7 +914,7 @@ static int check_object(struct kmem_cache *s, struct page *page,
 		if (val != SLUB_RED_ACTIVE && (s->flags & __OBJECT_POISON) &&
 			(!check_bytes_and_report(s, page, p, "Poison", p,
 					POISON_FREE, s->object_size - 1) ||
-			 !check_bytes_and_report(s, page, p, "Poison",
+			 !check_bytes_and_report(s, page, p, "End Poison",
 				p + s->object_size - 1, POISON_END, 1)))
 			return 0;
 		/*
-- 
2.25.1

[PATCH v3 2/3] mm/slub: Fix redzoning for small allocations

[Kees Cook]

The redzone area for SLUB exists between s->object_size and s->inuse
(which is at least the word-aligned object_size). If a cache were created
with an object_size smaller than sizeof(void *), the in-object stored
freelist pointer would overwrite the redzone (e.g. with boot param
"slub_debug=ZF"):

BUG test (Tainted: G    B            ): Right Redzone overwritten
-----------------------------------------------------------------------------

INFO: 0xffff957ead1c05de-0xffff957ead1c05df @offset=1502. First byte 0x1a instead of 0xbb
INFO: Slab 0xffffef3950b47000 objects=170 used=170 fp=0x0000000000000000 flags=0x8000000000000200
INFO: Object 0xffff957ead1c05d8 @offset=1496 fp=0xffff957ead1c0620

Redzone  (____ptrval____): bb bb bb bb bb bb bb bb    ........
Object   (____ptrval____): f6 f4 a5 40 1d e8          ...@..
Redzone  (____ptrval____): 1a aa                      ..
Padding  (____ptrval____): 00 00 00 00 00 00 00 00    ........

Store the freelist pointer out of line when object_size is smaller
than sizeof(void *) and redzoning is enabled. (This object_size is not
actually considered valid, as tested by kmem_cache_sanity_check() under
CONFIG_DEBUG_VM. This is being added for extra robustness, since it IS
possible to build kernels where this is allowed -- why keep foot-guns
around?)

(Note that no caches in this size range are known to exist in the kernel
currently.)

Fixes: 81819f0fc828 ("SLUB core")
Cc: stable@vger.kernel.org
Signed-off-by: Kees Cook <keescook@chromium.org>
---
 mm/slub.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/mm/slub.c b/mm/slub.c
index f4f1d63f0ab9..752fad36522c 100644
--- a/mm/slub.c
+++ b/mm/slub.c
@@ -3682,15 +3682,17 @@ static int calculate_sizes(struct kmem_cache *s, int forced_order)
 	 */
 	s->inuse = size;
 
-	if (((flags & (SLAB_TYPESAFE_BY_RCU | SLAB_POISON)) ||
-		s->ctor)) {
+	if ((flags & (SLAB_TYPESAFE_BY_RCU | SLAB_POISON)) ||
+	    ((flags & SLAB_RED_ZONE) && s->object_size < sizeof(void *)) ||
+	    s->ctor) {
 		/*
 		 * Relocate free pointer after the object if it is not
 		 * permitted to overwrite the first word of the object on
 		 * kmem_cache_free.
 		 *
 		 * This is the case if we do RCU, have a constructor or
-		 * destructor or are poisoning the objects.
+		 * destructor, are poisoning the objects, or are
+		 * redzoning an object smaller than sizeof(void *).
 		 *
 		 * The assumption that s->offset >= s->inuse means free
 		 * pointer is outside of the object is used in the
-- 
2.25.1

[PATCH v3 3/3] mm/slub: Actually fix freelist pointer vs redzoning

[Kees Cook]

It turns out that SLUB redzoning ("slub_debug=Z") checks from
s->object_size rather than from s->inuse (which is normally bumped
to make room for the freelist pointer), so a cache created with an
object size less than 24 would have the freelist pointer written beyond
s->object_size, causing the redzone to be corrupted by the freelist
pointer. This was very visible with "slub_debug=ZF":

BUG test (Tainted: G    B            ): Right Redzone overwritten
-----------------------------------------------------------------------------

INFO: 0xffff957ead1c05de-0xffff957ead1c05df @offset=1502. First byte 0x1a instead of 0xbb
INFO: Slab 0xffffef3950b47000 objects=170 used=170 fp=0x0000000000000000 flags=0x8000000000000200
INFO: Object 0xffff957ead1c05d8 @offset=1496 fp=0xffff957ead1c0620

Redzone  (____ptrval____): bb bb bb bb bb bb bb bb               ........
Object   (____ptrval____): 00 00 00 00 00 f6 f4 a5               ........
Redzone  (____ptrval____): 40 1d e8 1a aa                        @....
Padding  (____ptrval____): 00 00 00 00 00 00 00 00               ........

Adjust the offset to stay within s->object_size.

(Note that no caches in this size range are known to exist in the kernel
currently.)

Reported-by: Marco Elver <elver@google.com>
Link: https://lore.kernel.org/linux-mm/20200807160627.GA1420741@elver.google.com/
Fixes: 89b83f282d8b (slub: avoid redzone when choosing freepointer location)
Cc: stable@vger.kernel.org
Tested-by: Marco Elver <elver@google.com>
Link: https://lore.kernel.org/lkml/CANpmjNOwZ5VpKQn+SYWovTkFB4VsT-RPwyENBmaK0dLcpqStkA@mail.gmail.com
Signed-off-by: Kees Cook <keescook@chromium.org>
Acked-by: Vlastimil Babka <vbabka@suse.cz>
Link: https://lore.kernel.org/lkml/0f7dd7b2-7496-5e2d-9488-2ec9f8e90441@suse.cz/
---
 mm/slub.c | 14 +++-----------
 1 file changed, 3 insertions(+), 11 deletions(-)

diff --git a/mm/slub.c b/mm/slub.c
index 752fad36522c..6f115e56c5d0 100644
--- a/mm/slub.c
+++ b/mm/slub.c
@@ -3637,7 +3637,6 @@ static int calculate_sizes(struct kmem_cache *s, int forced_order)
 {
 	slab_flags_t flags = s->flags;
 	unsigned int size = s->object_size;
-	unsigned int freepointer_area;
 	unsigned int order;
 
 	/*
@@ -3646,13 +3645,6 @@ static int calculate_sizes(struct kmem_cache *s, int forced_order)
 	 * the possible location of the free pointer.
 	 */
 	size = ALIGN(size, sizeof(void *));
-	/*
-	 * This is the area of the object where a freepointer can be
-	 * safely written. If redzoning adds more to the inuse size, we
-	 * can't use that portion for writing the freepointer, so
-	 * s->offset must be limited within this for the general case.
-	 */
-	freepointer_area = size;
 
 #ifdef CONFIG_SLUB_DEBUG
 	/*
@@ -3678,7 +3670,7 @@ static int calculate_sizes(struct kmem_cache *s, int forced_order)
 
 	/*
 	 * With that we have determined the number of bytes in actual use
-	 * by the object. This is the potential offset to the free pointer.
+	 * by the object and redzoning.
 	 */
 	s->inuse = size;
 
@@ -3701,13 +3693,13 @@ static int calculate_sizes(struct kmem_cache *s, int forced_order)
 		 */
 		s->offset = size;
 		size += sizeof(void *);
-	} else if (freepointer_area > sizeof(void *)) {
+	} else {
 		/*
 		 * Store freelist pointer near middle of object to keep
 		 * it away from the edges of the object to avoid small
 		 * sized over/underflows from neighboring allocations.
 		 */
-		s->offset = ALIGN(freepointer_area / 2, sizeof(void *));
+		s->offset = ALIGN_DOWN(s->object_size / 2, sizeof(void *));
 	}
 
 #ifdef CONFIG_SLUB_DEBUG
-- 
2.25.1

Re: [PATCH v3 0/3] Actually fix freelist pointer vs redzoning

[Christopher Lameter]

On Wed, 14 Oct 2020, Kees Cook wrote:

> Note on patch 2: Christopher NAKed it, but I actually think this is a
> reasonable thing to add -- the "too small" check is only made when built
> with CONFIG_DEBUG_VM, so it *is* actually possible for someone to trip
> over this directly, even if it would never make it into a released
> kernel. I see no reason to just leave this foot-gun in place, though, so
> we might as well just fix it too. (Which seems to be what Longman was
> similarly supporting, IIUC.)

Well then remove the duplication of checks. The NAK was there because it
seems that you were not aware of the existing checks.

> Anyway, if patch 2 stays NAKed, that's fine. It's entirely separable,
> and the other 2 can land. :)

Just deal with the old checks too and it will be fine.

Re: [PATCH v3 0/3] Actually fix freelist pointer vs redzoning

[Vlastimil Babka]

On 10/15/20 10:23 AM, Christopher Lameter wrote:
> On Wed, 14 Oct 2020, Kees Cook wrote:
> 
>> Note on patch 2: Christopher NAKed it, but I actually think this is a
>> reasonable thing to add -- the "too small" check is only made when built
>> with CONFIG_DEBUG_VM, so it *is* actually possible for someone to trip
>> over this directly, even if it would never make it into a released
>> kernel. I see no reason to just leave this foot-gun in place, though, so
>> we might as well just fix it too. (Which seems to be what Longman was
>> similarly supporting, IIUC.)
> 
> Well then remove the duplication of checks. The NAK was there because it
> seems that you were not aware of the existing checks.
> 
>> Anyway, if patch 2 stays NAKed, that's fine. It's entirely separable,
>> and the other 2 can land. :)
> 
> Just deal with the old checks too and it will be fine.

Yeah, the existing check is under CONFIG_DEBUG_VM, which means it's not active 
on some configurations. Creating a cache is not exactly fast path operation, so 
I would remove this guard.
As for the minimum size check, I would probably remove it (but watch out if 
SLAB/SLOB can handle it). It's not effective to use slab cache for 4-byte 
objects, but why make it an error.

Re: [PATCH v3 0/3] Actually fix freelist pointer vs redzoning

[Kees Cook]

On Thu, Oct 15, 2020 at 11:44:15AM +0200, Vlastimil Babka wrote:
> On 10/15/20 10:23 AM, Christopher Lameter wrote:
> > On Wed, 14 Oct 2020, Kees Cook wrote:
> > 
> > > Note on patch 2: Christopher NAKed it, but I actually think this is a
> > > reasonable thing to add -- the "too small" check is only made when built
> > > with CONFIG_DEBUG_VM, so it *is* actually possible for someone to trip
> > > over this directly, even if it would never make it into a released
> > > kernel. I see no reason to just leave this foot-gun in place, though, so
> > > we might as well just fix it too. (Which seems to be what Longman was
> > > similarly supporting, IIUC.)
> > 
> > Well then remove the duplication of checks. The NAK was there because it
> > seems that you were not aware of the existing checks.
> > 
> > > Anyway, if patch 2 stays NAKed, that's fine. It's entirely separable,
> > > and the other 2 can land. :)
> > 
> > Just deal with the old checks too and it will be fine.
> 
> Yeah, the existing check is under CONFIG_DEBUG_VM, which means it's not
> active on some configurations. Creating a cache is not exactly fast path
> operation, so I would remove this guard.
> As for the minimum size check, I would probably remove it (but watch out if
> SLAB/SLOB can handle it). It's not effective to use slab cache for 4-byte
> objects, but why make it an error.

Err, why did the check exist to begin with? If the check isn't wanted,
that's one thing, but I was just trying to fix what I saw in the redzone
handling. What is preferred here?

1) drop patch 2
2) keep patch 2, but also:
	a) validate slab/slob can handle < word-sized allocations
	b) remove check in kmem_cache_sanity_check

option 2a seems like it could be fragile if I miss something. I think
I'd rather just take option 1.

-- 
Kees Cook