2020-04-25 09:16:22

by Changbin Du

[permalink] [raw]
Subject: [PATCH] mm/slub: do not place freelist pointer to middle of object if redzone is on

The recent kernel fails to boot when slub redzone is turned on. This is
caused by commit 3202fa62fb ("slub: relocate freelist pointer to middle of
object") which relocates freelist pointer to middle of object. In this
case, get_track() gets a wrong address and then the redzone is overwritten.

This patch fixes it by relocating freelist pointer after the object, as
what slub posion does.

[ 2.390256][ T0] =============================================================================
[ 2.392816][ T0] BUG kmem_cache_node (Not tainted): Redzone overwritten
[ 2.393735][ T0] -----------------------------------------------------------------------------
[ 2.393735][ T0]
[ 2.395168][ T0] Disabling lock debugging due to kernel taint
[ 2.395923][ T0] INFO: 0xffff88805c000380-0xffff88805c000387 @offset=896. First byte 0x0 instead of 0xbb
[ 2.397175][ T0] INFO: Slab 0xffffea0001700000 objects=25 used=25 fp=0x0000000000000000 flags=0xfffffc0010200
[ 2.398882][ T0] INFO: Object 0xffff88805c000300 @offset=768 fp=0xffff88805c000580
[ 2.398882][ T0]
[ 2.400145][ T0] Redzone ffff88805c000280: bb bb bb bb bb bb bb bb bb bb bb bb bb bb bb bb ................
[ 2.401593][ T0] Redzone ffff88805c000290: bb bb bb bb bb bb bb bb bb bb bb bb bb bb bb bb ................
[ 2.403825][ T0] Redzone ffff88805c0002a0: bb bb bb bb bb bb bb bb bb bb bb bb bb bb bb bb ................
[ 2.405853][ T0] Redzone ffff88805c0002b0: bb bb bb bb bb bb bb bb bb bb bb bb bb bb bb bb ................
[ 2.407714][ T0] Redzone ffff88805c0002c0: bb bb bb bb bb bb bb bb bb bb bb bb bb bb bb bb ................
[ 2.411066][ T0] Redzone ffff88805c0002d0: bb bb bb bb bb bb bb bb bb bb bb bb bb bb bb bb ................
[ 2.413818][ T0] Redzone ffff88805c0002e0: bb bb bb bb bb bb bb bb bb bb bb bb bb bb bb bb ................
[ 2.415482][ T0] Redzone ffff88805c0002f0: bb bb bb bb bb bb bb bb bb bb bb bb bb bb bb bb ................
[ 2.416975][ T0] Object ffff88805c000300: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
[ 2.418445][ T0] Object ffff88805c000310: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
[ 2.420183][ T0] Object ffff88805c000320: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
[ 2.421911][ T0] Object ffff88805c000330: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
[ 2.423491][ T0] Object ffff88805c000340: 00 00 00 00 00 00 00 00 e8 ed 4a b9 09 7b 20 83 ..........J..{ .
[ 2.425186][ T0] Object ffff88805c000350: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
[ 2.426901][ T0] Object ffff88805c000360: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
[ 2.428673][ T0] Object ffff88805c000370: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
[ 2.430431][ T0] Redzone ffff88805c000380: 00 00 00 00 00 00 00 00 ........
[ 2.485141][ T0] Padding ffff88805c000490: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
[ 2.494198][ T0] Padding ffff88805c0004a0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
[ 2.503094][ T0] Padding ffff88805c0004b0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
[ 2.578370][ T0] Padding ffff88805c0004c0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
[ 2.584420][ T0] Padding ffff88805c0004d0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
[ 2.590139][ T0] Padding ffff88805c0004e0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
[ 2.596915][ T0] Padding ffff88805c0004f0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
[ 2.604002][ T0] CPU: 0 PID: 0 Comm: swapper Tainted: G B 5.7.0-rc1+ #53
[ 2.608493][ T0] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS rel-1.13.0-0-gf21b5a4aeb02-prebuilt.qemu.org 04/01/2014
[ 2.612959][ T0] Call Trace:
[ 2.613953][ T0] dump_stack+0x8c/0xc0
[ 2.615063][ T0] check_bytes_and_report.cold+0x60/0x95
[ 2.616572][ T0] check_object+0x1c1/0x280
[ 2.617462][ T0] ? pvclock_clocksource_read+0xf6/0x1c0
[ 2.618646][ T0] ? __kmem_cache_create+0x16a/0x670
[ 2.619692][ T0] alloc_debug_processing+0x129/0x170
[ 2.620766][ T0] ___slab_alloc+0x58c/0x630
[ 2.621804][ T0] ? __kmem_cache_create+0x16a/0x670
[ 2.622631][ T0] ? print_unlock_imbalance_bug+0x40/0x40
[ 2.623630][ T0] ? lock_acquire+0x11f/0x200
[ 2.624421][ T0] ? __kmem_cache_create+0x16a/0x670
[ 2.625342][ T0] ? __slab_alloc+0x1c/0x30
[ 2.626106][ T0] ? ___slab_alloc+0x5/0x630
[ 2.626890][ T0] __slab_alloc+0x1c/0x30
[ 2.627500][ T0] kmem_cache_alloc_node+0xab/0x2e0
[ 2.628222][ T0] __kmem_cache_create+0x16a/0x670
[ 2.628883][ T0] ? mem_init_print_info+0x2af/0x2be
[ 2.629596][ T0] create_boot_cache+0xa4/0xc8
[ 2.630282][ T0] kmem_cache_init+0x80/0x14a
[ 2.631233][ T0] start_kernel+0x67a/0xa2a
[ 2.631905][ T0] ? thread_stack_cache_init+0x6/0x6
[ 2.632553][ T0] ? x86_family+0x5/0x20
[ 2.633085][ T0] ? load_ucode_bsp+0x50/0x216
[ 2.633629][ T0] secondary_startup_64+0xa4/0xb0
[ 2.634243][ T0] FIX kmem_cache_node: Restoring 0xffff88805c000380-0xffff88805c000387=0xbb
[ 2.634243][ T0]
[ 2.635532][ T0] FIX kmem_cache_node: Marking all objects used
[ 2.636287][ T0] =============================================================================

Fixes: 3202fa62fb ("slub: relocate freelist pointer to middle of object")
Cc: Kees Cook <[email protected]>
Signed-off-by: Changbin Du <[email protected]>
---
mm/slub.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/mm/slub.c b/mm/slub.c
index 332d4b459a90..59c5b49038b0 100644
--- a/mm/slub.c
+++ b/mm/slub.c
@@ -3570,7 +3570,7 @@ static int calculate_sizes(struct kmem_cache *s, int forced_order)
*/
s->inuse = size;

- if (((flags & (SLAB_TYPESAFE_BY_RCU | SLAB_POISON)) ||
+ if (((flags & (SLAB_TYPESAFE_BY_RCU | SLAB_RED_ZONE | SLAB_POISON)) ||
s->ctor)) {
/*
* Relocate free pointer after the object if it is not
--
2.25.1


2020-04-25 20:27:59

by Markus Elfring

[permalink] [raw]
Subject: Re: [PATCH] mm/slub: do not place freelist pointer to middle of object if redzone is on

> Fixes: 3202fa62fb ("slub: relocate freelist pointer to middle of object")

Will a longer commit identifier be safer for the final change description?
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst?id=b2768df24ec400dd4f7fa79542f797e904812053#n183

Regards,
Markus

2020-04-25 22:50:47

by Kees Cook

[permalink] [raw]
Subject: Re: [PATCH] mm/slub: do not place freelist pointer to middle of object if redzone is on

On Sat, Apr 25, 2020 at 05:13:38PM +0800, Changbin Du wrote:
> The recent kernel fails to boot when slub redzone is turned on. This is
> caused by commit 3202fa62fb ("slub: relocate freelist pointer to middle of
> object") which relocates freelist pointer to middle of object. In this
> case, get_track() gets a wrong address and then the redzone is overwritten.

Hi! A fix for this is already in -next:

https://www.ozlabs.org/~akpm/mmotm/broken-out/slub-avoid-redzone-when-choosing-freepointer-location.patch

the above doesn't disable the mitigation when using redzones, so I
prefer that to this suggested solution.

--
Kees Cook

2020-04-25 23:52:59

by Changbin Du

[permalink] [raw]
Subject: Re: [PATCH] mm/slub: do not place freelist pointer to middle of object if redzone is on

On Sat, Apr 25, 2020 at 10:24:45PM +0200, Markus Elfring wrote:
> > Fixes: 3202fa62fb ("slub: relocate freelist pointer to middle of object")
>
> Will a longer commit identifier be safer for the final change description?
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst?id=b2768df24ec400dd4f7fa79542f797e904812053#n183
>
I used to give 12 charactors, but this time I lost two. :)

> Regards,
> Markus

--
Cheers,
Changbin Du

2020-04-26 00:10:10

by Changbin Du

[permalink] [raw]
Subject: Re: [PATCH] mm/slub: do not place freelist pointer to middle of object if redzone is on

Hi Kees,
On Sat, Apr 25, 2020 at 03:48:31PM -0700, Kees Cook wrote:
> On Sat, Apr 25, 2020 at 05:13:38PM +0800, Changbin Du wrote:
> > The recent kernel fails to boot when slub redzone is turned on. This is
> > caused by commit 3202fa62fb ("slub: relocate freelist pointer to middle of
> > object") which relocates freelist pointer to middle of object. In this
> > case, get_track() gets a wrong address and then the redzone is overwritten.
>
> Hi! A fix for this is already in -next:
>
> https://www.ozlabs.org/~akpm/mmotm/broken-out/slub-avoid-redzone-when-choosing-freepointer-location.patch
>
> the above doesn't disable the mitigation when using redzones, so I
> prefer that to this suggested solution.
>
Glade to see it's been reported. But I am sorry that your patch cannot fix it.

With your fix, I suppose the layout of slub is:
|obj-fp-obj|redzone|track|...

While get_track():
p = object + s->offset + sizeof(void *);

Then we still get a wrong location. I just tested linux-next and the problem is
still there.

Is the right and left redzone good enough to protect the freepointer? If not,
I will send a patch to fix get_track() along with your patch.

> --
> Kees Cook

--
Cheers,
Changbin Du

2020-04-26 06:44:08

by Markus Elfring

[permalink] [raw]
Subject: Re: mm/slub: do not place freelist pointer to middle of object if redzone is on

>>> Fixes: 3202fa62fb ("slub: relocate freelist pointer to middle of object")

> I used to give 12 charactors, but this time I lost two. :)

Would you like to improve details for the change description
also at another place?

Regards,
Markus

2020-04-28 23:24:04

by Changbin Du

[permalink] [raw]
Subject: Re: mm/slub: do not place freelist pointer to middle of object if redzone is on

On Sun, Apr 26, 2020 at 07:56:43AM +0200, Markus Elfring wrote:
> >>> Fixes: 3202fa62fb ("slub: relocate freelist pointer to middle of object")
> …
> > I used to give 12 charactors, but this time I lost two. :)
>
> Would you like to improve details for the change description
> also at another place?
>
Waiman just posted another fix for this issue. Let's use that patch instead.
Thanks.

> Regards,
> Markus

--
Cheers,
Changbin Du

2020-04-29 08:03:17

by Markus Elfring

[permalink] [raw]
Subject: Re: mm/slub: do not place freelist pointer to middle of object if redzone is on

> Waiman just posted another fix for this issue. Let's use that patch instead.

With which patches should the situation be finally improved
also for this software component?
https://lore.kernel.org/patchwork/project/lkml/list/?submitter=21412

Regards,
Markus

2020-04-29 16:41:28

by Waiman Long

[permalink] [raw]
Subject: Re: mm/slub: do not place freelist pointer to middle of object if redzone is on

On 4/29/20 4:00 AM, Markus Elfring wrote:
>> Waiman just posted another fix for this issue. Let's use that patch instead.
> With which patches should the situation be finally improved
> also for this software component?
> https://lore.kernel.org/patchwork/project/lkml/list/?submitter=21412
>
> Regards,
> Markus
>
The "mm/slub: Fix incorrect interpretation of s->offset" v3 patch that I
sent out this morning should fix the problem.

Cheers,
Longman