[PATCH] slub: choose the right freelist pointer location when creating small caches

[PATCH] slub: choose the right freelist pointer location when creating small caches

[Lin, Zhenpeng]

When enabling CONFIG_SLUB_DEBUG and booting with "slub_debug=Z", the
kernel crashes at creating caches if the object size is smaller
than 2*sizeof(void*). The problem is due to the wrong calculation
of freepointer_area. The freelist pointer can be stored in the
middle of object only if the object size is not smaller than
2*sizeof(void*). Otherwise, the freelist pointer will be corrupted by
SLUB_RED_ZONE.

Fixes: 3202fa62fb43 ("slub: relocate freelist pointer to middle of object")
Fixes: 89b83f282d8b ("slub: avoid redzone when choosing freepointer location")
Signed-off-by: Zhenpeng Lin <zplin@psu.edu>
---
mm/slub.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/mm/slub.c b/mm/slub.c
index 3f96e099817a..cb23233ee683 100644
--- a/mm/slub.c
+++ b/mm/slub.c
@@ -3704,7 +3704,7 @@ static int calculate_sizes(struct kmem_cache *s, int forced_order)
	 * can't use that portion for writing the freepointer, so
	 * s->offset must be limited within this for the general case.
	 */
-	freepointer_area = size;
+	freepointer_area = s->object_size;

#ifdef CONFIG_SLUB_DEBUG
	/*
@@ -3751,7 +3751,7 @@ static int calculate_sizes(struct kmem_cache *s, int forced_order)
		 */
		s->offset = size;
		size += sizeof(void *);
-	} else if (freepointer_area > sizeof(void *)) {
+	} else if (freepointer_area >= 2 * sizeof(void *)) {
		/*
		 * Store freelist pointer near middle of object to keep
		 * it away from the edges of the object to avoid small
--
2.17.1

[PATCH] slub: choose the right freelist pointer location when creating small caches

[Lin, Zhenpeng]

When enabling CONFIG_SLUB_DEBUG and booting with "slub_debug=Z", the
kernel crashes at creating caches if the object size is smaller
than 2*sizeof(void*). The problem is due to the wrong calculation
of freepointer_area. The freelist pointer can be stored in the
middle of object only if the object size is not smaller than
2*sizeof(void*). Otherwise, the freelist pointer will be corrupted by
SLUB_RED_ZONE.

Fixes: 3202fa62fb43 ("slub: relocate freelist pointer to middle of object")
Fixes: 89b83f282d8b ("slub: avoid redzone when choosing freepointer location")
Signed-off-by: Zhenpeng Lin <zplin@psu.edu>
---
mm/slub.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/mm/slub.c b/mm/slub.c
index 3f96e099817a..cb23233ee683 100644
--- a/mm/slub.c
+++ b/mm/slub.c
@@ -3704,7 +3704,7 @@ static int calculate_sizes(struct kmem_cache *s, int forced_order)
	 * can't use that portion for writing the freepointer, so
	 * s->offset must be limited within this for the general case.
	 */
-	freepointer_area = size;
+	freepointer_area = s->object_size;

#ifdef CONFIG_SLUB_DEBUG
	/*
@@ -3751,7 +3751,7 @@ static int calculate_sizes(struct kmem_cache *s, int forced_order)
		 */
		s->offset = size;
		size += sizeof(void *);
-	} else if (freepointer_area > sizeof(void *)) {
+	} else if (freepointer_area >= 2 * sizeof(void *)) {
		/*
		 * Store freelist pointer near middle of object to keep
		 * it away from the edges of the object to avoid small
--
2.17.1

Re: [PATCH] slub: choose the right freelist pointer location when creating small caches

[Kees Cook]

On Sat, Jun 05, 2021 at 01:58:13AM +0000, Lin, Zhenpeng wrote:
> When enabling CONFIG_SLUB_DEBUG and booting with "slub_debug=Z", the
> kernel crashes at creating caches if the object size is smaller
> than 2*sizeof(void*). The problem is due to the wrong calculation
> of freepointer_area. The freelist pointer can be stored in the
> middle of object only if the object size is not smaller than
> 2*sizeof(void*). Otherwise, the freelist pointer will be corrupted by
> SLUB_RED_ZONE.
> 
> Fixes: 3202fa62fb43 ("slub: relocate freelist pointer to middle of object")
> Fixes: 89b83f282d8b ("slub: avoid redzone when choosing freepointer location")
> Signed-off-by: Zhenpeng Lin <zplin@psu.edu>
> ---
> mm/slub.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/mm/slub.c b/mm/slub.c
> index 3f96e099817a..cb23233ee683 100644
> --- a/mm/slub.c
> +++ b/mm/slub.c
> @@ -3704,7 +3704,7 @@ static int calculate_sizes(struct kmem_cache *s, int forced_order)
> 	 * can't use that portion for writing the freepointer, so
> 	 * s->offset must be limited within this for the general case.
> 	 */
> -	freepointer_area = size;
> +	freepointer_area = s->object_size;
> 
> #ifdef CONFIG_SLUB_DEBUG
> 	/*
> @@ -3751,7 +3751,7 @@ static int calculate_sizes(struct kmem_cache *s, int forced_order)
> 		 */
> 		s->offset = size;
> 		size += sizeof(void *);
> -	} else if (freepointer_area > sizeof(void *)) {
> +	} else if (freepointer_area >= 2 * sizeof(void *)) {
> 		/*
> 		 * Store freelist pointer near middle of object to keep
> 		 * it away from the edges of the object to avoid small
> --
> 2.17.1

NAK, I'd prefer this get cleaned up more completely, especially since
there are no objects of that size in the kernel currently:

https://lore.kernel.org/lkml/20201015033712.1491731-1-keescook@chromium.org/

-- 
Kees Cook

Re: [PATCH] slub: choose the right freelist pointer location when creating small caches

[Lin, Zhenpeng]

There do exist objects whose size is smaller than 2*sizeof(void*). E.g. struct ccid in DCCP module.

On 6/8/21, 2:26 PM, "Kees Cook" <keescook@chromium.org> wrote:

    On Sat, Jun 05, 2021 at 01:58:13AM +0000, Lin, Zhenpeng wrote:
    > When enabling CONFIG_SLUB_DEBUG and booting with "slub_debug=Z", the
    > kernel crashes at creating caches if the object size is smaller
    > than 2*sizeof(void*). The problem is due to the wrong calculation
    > of freepointer_area. The freelist pointer can be stored in the
    > middle of object only if the object size is not smaller than
    > 2*sizeof(void*). Otherwise, the freelist pointer will be corrupted by
    > SLUB_RED_ZONE.
    > 
    > Fixes: 3202fa62fb43 ("slub: relocate freelist pointer to middle of object")
    > Fixes: 89b83f282d8b ("slub: avoid redzone when choosing freepointer location")
    > Signed-off-by: Zhenpeng Lin <zplin@psu.edu>
    > ---
    > mm/slub.c | 4 ++--
    > 1 file changed, 2 insertions(+), 2 deletions(-)
    > 
    > diff --git a/mm/slub.c b/mm/slub.c
    > index 3f96e099817a..cb23233ee683 100644
    > --- a/mm/slub.c
    > +++ b/mm/slub.c
    > @@ -3704,7 +3704,7 @@ static int calculate_sizes(struct kmem_cache *s, int forced_order)
    > 	 * can't use that portion for writing the freepointer, so
    > 	 * s->offset must be limited within this for the general case.
    > 	 */
    > -	freepointer_area = size;
    > +	freepointer_area = s->object_size;
    > 
    > #ifdef CONFIG_SLUB_DEBUG
    > 	/*
    > @@ -3751,7 +3751,7 @@ static int calculate_sizes(struct kmem_cache *s, int forced_order)
    > 		 */
    > 		s->offset = size;
    > 		size += sizeof(void *);
    > -	} else if (freepointer_area > sizeof(void *)) {
    > +	} else if (freepointer_area >= 2 * sizeof(void *)) {
    > 		/*
    > 		 * Store freelist pointer near middle of object to keep
    > 		 * it away from the edges of the object to avoid small
    > --
    > 2.17.1

    NAK, I'd prefer this get cleaned up more completely, especially since
    there are no objects of that size in the kernel currently:

    https://nam10.safelinks.protection.outlook.com/?url=https%3A%2F%2Flore.kernel.org%2Flkml%2F20201015033712.1491731-1-keescook%40chromium.org%2F&amp;data=04%7C01%7Czplin%40psu.edu%7C28b6f3c5a3b149be56e808d92aaafd26%7C7cf48d453ddb4389a9c1c115526eb52e%7C0%7C0%7C637587736155493816%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&amp;sdata=%2F8CXVkqlhA7RnfX%2BDP07%2F4t1NIw1CHsUpuuWrsLyU9o%3D&amp;reserved=0

    -- 
    Kees Cook

Re: [PATCH] slub: choose the right freelist pointer location when creating small caches

[Kees Cook]

On Tue, Jun 08, 2021 at 06:33:01PM +0000, Lin, Zhenpeng wrote:
> There do exist objects whose size is smaller than 2*sizeof(void*). E.g. struct ccid in DCCP module.

Yes, sorry, I meant sizeof(void *). I've sent an updated v4 series and
CCed you. Are you able to test that and see if it fixes it for you too?

Thanks for the push to dust this series off again! :)

-Kees

-- 
Kees Cook

Re: [PATCH] slub: choose the right freelist pointer location when creating small caches

[Lin, Zhenpeng]

No problem. Just took a look and tested the patch, it looks good to me!

On 6/8/21, 2:41 PM, "Kees Cook" <keescook@chromium.org> wrote:

    On Tue, Jun 08, 2021 at 06:33:01PM +0000, Lin, Zhenpeng wrote:
    > There do exist objects whose size is smaller than 2*sizeof(void*). E.g. struct ccid in DCCP module.

    Yes, sorry, I meant sizeof(void *). I've sent an updated v4 series and
    CCed you. Are you able to test that and see if it fixes it for you too?

    Thanks for the push to dust this series off again! :)

    -Kees

    -- 
    Kees Cook

Re: [PATCH] slub: choose the right freelist pointer location when creating small caches

[Kees Cook]

On Tue, Jun 08, 2021 at 07:06:45PM +0000, Lin, Zhenpeng wrote:
> No problem. Just took a look and tested the patch, it looks good to me!

Great; thank you! Sorry I dropped the ball on this series. I got
distracted. :) It looks like akpm took it into -mm now, so this should
be fixed in -next soon.

-- 
Kees Cook

Re: [PATCH] slub: choose the right freelist pointer location when creating small caches

[Lin, Zhenpeng]

Sounds good. But I would suggest this to go to -stable as soon as possible. Because this bug is affecting the basic functionality of DCCP. It crashes kernel whenever a new socket in this module is created.

Best,
Zhenpeng

-----Original Message-----
From: Kees Cook <keescook@chromium.org>
Date: Tuesday, June 8, 2021 at 7:14 PM
To: "Lin, Zhenpeng" <zplin@psu.edu>
Cc: Christoph Lameter <cl@linux.com>, Pekka Enberg <penberg@kernel.org>, David Rientjes <rientjes@google.com>, Joonsoo Kim <iamjoonsoo.kim@lge.com>, Andrew Morton <akpm@linux-foundation.org>, Vlastimil Babka <vbabka@suse.cz>, "linux-mm@kvack.org" <linux-mm@kvack.org>, "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH] slub: choose the right freelist pointer location when creating small caches

    On Tue, Jun 08, 2021 at 07:06:45PM +0000, Lin, Zhenpeng wrote:
    > No problem. Just took a look and tested the patch, it looks good to me!

    Great; thank you! Sorry I dropped the ball on this series. I got
    distracted. :) It looks like akpm took it into -mm now, so this should
    be fixed in -next soon.

    -- 
    Kees Cook

Re: [PATCH] slub: choose the right freelist pointer location when creating small caches

[Kees Cook]

On Thu, Jun 10, 2021 at 08:20:31PM +0000, Lin, Zhenpeng wrote:
> Sounds good. But I would suggest this to go to -stable as soon as possible. Because this bug is affecting the basic functionality of DCCP. It crashes kernel whenever a new socket in this module is created.

But only when redzoning is enabled, yes?

-- 
Kees Cook

Re: [PATCH] slub: choose the right freelist pointer location when creating small caches

[Lin, Zhenpeng]

Yes, that should be enabled to trigger. 

-----Original Message-----
From: Kees Cook <keescook@chromium.org>
Date: Thursday, June 10, 2021 at 7:01 PM
To: "Lin, Zhenpeng" <zplin@psu.edu>
Cc: Christoph Lameter <cl@linux.com>, Pekka Enberg <penberg@kernel.org>, David Rientjes <rientjes@google.com>, Joonsoo Kim <iamjoonsoo.kim@lge.com>, Andrew Morton <akpm@linux-foundation.org>, Vlastimil Babka <vbabka@suse.cz>, "linux-mm@kvack.org" <linux-mm@kvack.org>, "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH] slub: choose the right freelist pointer location when creating small caches

    On Thu, Jun 10, 2021 at 08:20:31PM +0000, Lin, Zhenpeng wrote:
    > Sounds good. But I would suggest this to go to -stable as soon as possible. Because this bug is affecting the basic functionality of DCCP. It crashes kernel whenever a new socket in this module is created.

    But only when redzoning is enabled, yes?

    -- 
    Kees Cook