All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] slub: choose the right freelist pointer location when creating small caches
@ 2021-06-05  1:58 ` Lin, Zhenpeng
  0 siblings, 0 replies; 10+ messages in thread
From: Lin, Zhenpeng @ 2021-06-05  1:58 UTC (permalink / raw)
  Cc: Christoph Lameter, Pekka Enberg, David Rientjes, Joonsoo Kim,
	Andrew Morton, Vlastimil Babka, Kees Cook, linux-mm,
	linux-kernel

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



^ permalink raw reply	[flat|nested] 10+ messages in thread

* [PATCH] slub: choose the right freelist pointer location when creating small caches
@ 2021-06-05  1:58 ` Lin, Zhenpeng
  0 siblings, 0 replies; 10+ messages in thread
From: Lin, Zhenpeng @ 2021-06-05  1:58 UTC (permalink / raw)
  Cc: Christoph Lameter, Pekka Enberg, David Rientjes, Joonsoo Kim,
	Andrew Morton, Vlastimil Babka, Kees Cook, linux-mm,
	linux-kernel

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



^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] slub: choose the right freelist pointer location when creating small caches
  2021-06-05  1:58 ` Lin, Zhenpeng
  (?)
@ 2021-06-08 18:26 ` Kees Cook
  2021-06-08 18:33   ` Lin, Zhenpeng
  -1 siblings, 1 reply; 10+ messages in thread
From: Kees Cook @ 2021-06-08 18:26 UTC (permalink / raw)
  To: Lin, Zhenpeng
  Cc: Christoph Lameter, Pekka Enberg, David Rientjes, Joonsoo Kim,
	Andrew Morton, Vlastimil Babka, linux-mm, linux-kernel

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

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] slub: choose the right freelist pointer location when creating small caches
  2021-06-08 18:26 ` Kees Cook
@ 2021-06-08 18:33   ` Lin, Zhenpeng
  2021-06-08 18:41     ` Kees Cook
  0 siblings, 1 reply; 10+ messages in thread
From: Lin, Zhenpeng @ 2021-06-08 18:33 UTC (permalink / raw)
  To: Kees Cook
  Cc: Christoph Lameter, Pekka Enberg, David Rientjes, Joonsoo Kim,
	Andrew Morton, Vlastimil Babka, linux-mm, linux-kernel

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


^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] slub: choose the right freelist pointer location when creating small caches
  2021-06-08 18:33   ` Lin, Zhenpeng
@ 2021-06-08 18:41     ` Kees Cook
  2021-06-08 19:06       ` Lin, Zhenpeng
  0 siblings, 1 reply; 10+ messages in thread
From: Kees Cook @ 2021-06-08 18:41 UTC (permalink / raw)
  To: Lin, Zhenpeng
  Cc: Christoph Lameter, Pekka Enberg, David Rientjes, Joonsoo Kim,
	Andrew Morton, Vlastimil Babka, linux-mm, linux-kernel

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

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] slub: choose the right freelist pointer location when creating small caches
  2021-06-08 18:41     ` Kees Cook
@ 2021-06-08 19:06       ` Lin, Zhenpeng
  2021-06-08 23:14         ` Kees Cook
  0 siblings, 1 reply; 10+ messages in thread
From: Lin, Zhenpeng @ 2021-06-08 19:06 UTC (permalink / raw)
  To: Kees Cook
  Cc: Christoph Lameter, Pekka Enberg, David Rientjes, Joonsoo Kim,
	Andrew Morton, Vlastimil Babka, linux-mm, linux-kernel

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


^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] slub: choose the right freelist pointer location when creating small caches
  2021-06-08 19:06       ` Lin, Zhenpeng
@ 2021-06-08 23:14         ` Kees Cook
  2021-06-10 20:20           ` Lin, Zhenpeng
  0 siblings, 1 reply; 10+ messages in thread
From: Kees Cook @ 2021-06-08 23:14 UTC (permalink / raw)
  To: Lin, Zhenpeng
  Cc: Christoph Lameter, Pekka Enberg, David Rientjes, Joonsoo Kim,
	Andrew Morton, Vlastimil Babka, linux-mm, linux-kernel

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

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] slub: choose the right freelist pointer location when creating small caches
  2021-06-08 23:14         ` Kees Cook
@ 2021-06-10 20:20           ` Lin, Zhenpeng
  2021-06-10 23:01             ` Kees Cook
  0 siblings, 1 reply; 10+ messages in thread
From: Lin, Zhenpeng @ 2021-06-10 20:20 UTC (permalink / raw)
  To: Kees Cook
  Cc: Christoph Lameter, Pekka Enberg, David Rientjes, Joonsoo Kim,
	Andrew Morton, Vlastimil Babka, linux-mm, linux-kernel

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


^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] slub: choose the right freelist pointer location when creating small caches
  2021-06-10 20:20           ` Lin, Zhenpeng
@ 2021-06-10 23:01             ` Kees Cook
  2021-06-11  4:22               ` Lin, Zhenpeng
  0 siblings, 1 reply; 10+ messages in thread
From: Kees Cook @ 2021-06-10 23:01 UTC (permalink / raw)
  To: Lin, Zhenpeng
  Cc: Christoph Lameter, Pekka Enberg, David Rientjes, Joonsoo Kim,
	Andrew Morton, Vlastimil Babka, linux-mm, linux-kernel

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

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] slub: choose the right freelist pointer location when creating small caches
  2021-06-10 23:01             ` Kees Cook
@ 2021-06-11  4:22               ` Lin, Zhenpeng
  0 siblings, 0 replies; 10+ messages in thread
From: Lin, Zhenpeng @ 2021-06-11  4:22 UTC (permalink / raw)
  To: Kees Cook
  Cc: Christoph Lameter, Pekka Enberg, David Rientjes, Joonsoo Kim,
	Andrew Morton, Vlastimil Babka, linux-mm, linux-kernel

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


^ permalink raw reply	[flat|nested] 10+ messages in thread

end of thread, other threads:[~2021-06-11  4:22 UTC | newest]

Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-06-05  1:58 [PATCH] slub: choose the right freelist pointer location when creating small caches Lin, Zhenpeng
2021-06-05  1:58 ` Lin, Zhenpeng
2021-06-08 18:26 ` Kees Cook
2021-06-08 18:33   ` Lin, Zhenpeng
2021-06-08 18:41     ` Kees Cook
2021-06-08 19:06       ` Lin, Zhenpeng
2021-06-08 23:14         ` Kees Cook
2021-06-10 20:20           ` Lin, Zhenpeng
2021-06-10 23:01             ` Kees Cook
2021-06-11  4:22               ` Lin, Zhenpeng

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.