diff mbox series

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

Message ID 6746FEEA-FD69-4792-8DDA-C78F5FE7DA02@psu.edu (mailing list archive)
State New, archived
Headers show
Series slub: choose the right freelist pointer location when creating small caches | expand

Commit Message

Lin, Zhenpeng June 5, 2021, 1:58 a.m. UTC 
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(-)

--
2.17.1

Comments

Kees Cook June 8, 2021, 6:26 p.m. UTC | #1 
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/
Lin, Zhenpeng June 8, 2021, 6:33 p.m. UTC | #2 
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
Kees Cook June 8, 2021, 6:41 p.m. UTC | #3 
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
Lin, Zhenpeng June 8, 2021, 7:06 p.m. UTC | #4 
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
Kees Cook June 8, 2021, 11:14 p.m. UTC | #5 
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.
Lin, Zhenpeng June 10, 2021, 8:20 p.m. UTC | #6 
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
diff mbox series

Patch

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