linux-mm.kvack.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] lib/stackdepot: fix global out-of-bounds in stack_slabs
@ 2020-02-18 10:29 glider
  2020-02-19 21:37 ` Andrew Morton
  0 siblings, 1 reply; 5+ messages in thread
From: glider @ 2020-02-18 10:29 UTC (permalink / raw)
  To: walter-zh.wu, dvyukov, gregkh, akpm
  Cc: matthias.bgg, tglx, jpoimboe, kstewart, linux-mm, penguin-kernel,
	Alexander Potapenko

Walter Wu has reported a potential case in which init_stack_slab() is
called after stack_slabs[STACK_ALLOC_MAX_SLABS - 1] has already been
initialized. In that case init_stack_slab() will overwrite
stack_slabs[STACK_ALLOC_MAX_SLABS], which may result in a memory
corruption.

Fixes: cd11016e5f521 ("mm, kasan: stackdepot implementation. Enable stackdepot for SLAB")
Reported-by: Walter Wu <walter-zh.wu@mediatek.com>
Cc: Dmitry Vyukov <dvyukov@google.com>
Cc: Matthias Brugger <matthias.bgg@gmail.com>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Josh Poimboeuf <jpoimboe@redhat.com>
Cc: Kate Stewart <kstewart@linuxfoundation.org>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: Andrew Morton <akpm@linux-foundation.org>
Signed-off-by: Alexander Potapenko <glider@google.com>

---
v2:
 - prevent a leak of unused preallocated stack slab (spotted by Tetsuo
   Handa)
---
 lib/stackdepot.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/lib/stackdepot.c b/lib/stackdepot.c
index ed717dd08ff3..81c69c08d1d1 100644
--- a/lib/stackdepot.c
+++ b/lib/stackdepot.c
@@ -83,15 +83,19 @@ static bool init_stack_slab(void **prealloc)
 		return true;
 	if (stack_slabs[depot_index] == NULL) {
 		stack_slabs[depot_index] = *prealloc;
+		*prealloc = NULL;
 	} else {
-		stack_slabs[depot_index + 1] = *prealloc;
+		/* If this is the last depot slab, do not touch the next one. */
+		if (depot_index + 1 < STACK_ALLOC_MAX_SLABS) {
+			stack_slabs[depot_index + 1] = *prealloc;
+			*prealloc = NULL;
+		}
 		/*
 		 * This smp_store_release pairs with smp_load_acquire() from
 		 * |next_slab_inited| above and in stack_depot_save().
 		 */
 		smp_store_release(&next_slab_inited, 1);
 	}
-	*prealloc = NULL;
 	return true;
 }
 
-- 
2.25.0.265.gbab2e86ba0-goog



^ permalink raw reply related	[flat|nested] 5+ messages in thread
* Re: [PATCH] lib/stackdepot: fix global out-of-bounds in stack_slabs
  2020-02-18 10:29 [PATCH] lib/stackdepot: fix global out-of-bounds in stack_slabs glider
@ 2020-02-19 21:37 ` Andrew Morton
  0 siblings, 0 replies; 5+ messages in thread
From: Andrew Morton @ 2020-02-19 21:37 UTC (permalink / raw)
  To: glider
  Cc: walter-zh.wu, dvyukov, gregkh, matthias.bgg, tglx, jpoimboe,
	kstewart, linux-mm, penguin-kernel

On Tue, 18 Feb 2020 11:29:50 +0100 glider@google.com wrote:

> Walter Wu has reported a potential case in which init_stack_slab() is
> called after stack_slabs[STACK_ALLOC_MAX_SLABS - 1] has already been
> initialized. In that case init_stack_slab() will overwrite
> stack_slabs[STACK_ALLOC_MAX_SLABS], which may result in a memory
> corruption.

I added cc:stable to this.  Please let me know it that is undesirable.


^ permalink raw reply	[flat|nested] 5+ messages in thread
* Re: [PATCH] lib/stackdepot: fix global out-of-bounds in stack_slabs
  2020-02-03 11:02 ` Tetsuo Handa
@ 2020-02-03 11:10   ` Alexander Potapenko
  0 siblings, 0 replies; 5+ messages in thread
From: Alexander Potapenko @ 2020-02-03 11:10 UTC (permalink / raw)
  To: Tetsuo Handa
  Cc: Walter Wu, Dmitriy Vyukov, Greg Kroah-Hartman, Andrew Morton,
	Matthias Brugger, Thomas Gleixner, Josh Poimboeuf, Kate Stewart,
	Linux Memory Management List

> > -             stack_slabs[depot_index + 1] = *prealloc;
> > +             /* If this is the last depot slab, do not touch the next one. */
> > +             if (depot_index + 1 < STACK_ALLOC_MAX_SLABS)
> > +                     stack_slabs[depot_index + 1] = *prealloc;
>
> What prevents memory leak (caused by "*prealloc = NULL;")
> when we hit depot_index + 1 >= STACK_ALLOC_MAX_SLABS condition?
>

Nice catch!
We must return from this function instead of setting *prealloc to NULL.


-- 
Alexander Potapenko
Software Engineer

Google Germany GmbH
Erika-Mann-Straße, 33
80636 München

Geschäftsführer: Paul Manicle, Halimah DeLaine Prado
Registergericht und -nummer: Hamburg, HRB 86891
Sitz der Gesellschaft: Hamburg


^ permalink raw reply	[flat|nested] 5+ messages in thread
* Re: [PATCH] lib/stackdepot: fix global out-of-bounds in stack_slabs
  2020-02-03 10:29 glider
@ 2020-02-03 11:02 ` Tetsuo Handa
  2020-02-03 11:10   ` Alexander Potapenko
  0 siblings, 1 reply; 5+ messages in thread
From: Tetsuo Handa @ 2020-02-03 11:02 UTC (permalink / raw)
  To: glider
  Cc: walter-zh.wu, dvyukov, gregkh, akpm, matthias.bgg, tglx,
	jpoimboe, kstewart, linux-mm

On 2020/02/03 19:29, glider@google.com wrote:
> --- a/lib/stackdepot.c
> +++ b/lib/stackdepot.c
> @@ -84,7 +84,9 @@ static bool init_stack_slab(void **prealloc)
>  	if (stack_slabs[depot_index] == NULL) {
>  		stack_slabs[depot_index] = *prealloc;
>  	} else {
> -		stack_slabs[depot_index + 1] = *prealloc;
> +		/* If this is the last depot slab, do not touch the next one. */
> +		if (depot_index + 1 < STACK_ALLOC_MAX_SLABS)
> +			stack_slabs[depot_index + 1] = *prealloc;

What prevents memory leak (caused by "*prealloc = NULL;")
when we hit depot_index + 1 >= STACK_ALLOC_MAX_SLABS condition?

>  		/*
>  		 * This smp_store_release pairs with smp_load_acquire() from
>  		 * |next_slab_inited| above and in stack_depot_save().
> 


^ permalink raw reply	[flat|nested] 5+ messages in thread
* [PATCH] lib/stackdepot: fix global out-of-bounds in stack_slabs
@ 2020-02-03 10:29 glider
  2020-02-03 11:02 ` Tetsuo Handa
  0 siblings, 1 reply; 5+ messages in thread
From: glider @ 2020-02-03 10:29 UTC (permalink / raw)
  To: walter-zh.wu, dvyukov, gregkh, akpm
  Cc: matthias.bgg, tglx, jpoimboe, kstewart, linux-mm, Alexander Potapenko

Walter Wu has reported a potential case in which init_stack_slab() is
called after stack_slabs[STACK_ALLOC_MAX_SLABS - 1] has already been
initialized. In that case init_stack_slab() will overwrite
stack_slabs[STACK_ALLOC_MAX_SLABS], which may result in a memory
corruption.

Fixes: cd11016e5f521 ("mm, kasan: stackdepot implementation. Enable stackdepot for SLAB")
Reported-by: Walter Wu <walter-zh.wu@mediatek.com>
Cc: Dmitry Vyukov <dvyukov@google.com>
Cc: Matthias Brugger <matthias.bgg@gmail.com>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Josh Poimboeuf <jpoimboe@redhat.com>
Cc: Kate Stewart <kstewart@linuxfoundation.org>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: Andrew Morton <akpm@linux-foundation.org>
Signed-off-by: Alexander Potapenko <glider@google.com>
---
 lib/stackdepot.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/lib/stackdepot.c b/lib/stackdepot.c
index ed717dd08ff3..3580e92cac99 100644
--- a/lib/stackdepot.c
+++ b/lib/stackdepot.c
@@ -84,7 +84,9 @@ static bool init_stack_slab(void **prealloc)
 	if (stack_slabs[depot_index] == NULL) {
 		stack_slabs[depot_index] = *prealloc;
 	} else {
-		stack_slabs[depot_index + 1] = *prealloc;
+		/* If this is the last depot slab, do not touch the next one. */
+		if (depot_index + 1 < STACK_ALLOC_MAX_SLABS)
+			stack_slabs[depot_index + 1] = *prealloc;
 		/*
 		 * This smp_store_release pairs with smp_load_acquire() from
 		 * |next_slab_inited| above and in stack_depot_save().
-- 
2.25.0.341.g760bfbb309-goog



^ permalink raw reply related	[flat|nested] 5+ messages in thread
end of thread, other threads:[~2020-02-19 21:37 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-02-18 10:29 [PATCH] lib/stackdepot: fix global out-of-bounds in stack_slabs glider
2020-02-19 21:37 ` Andrew Morton
  -- strict thread matches above, loose matches on Subject: below --
2020-02-03 10:29 glider
2020-02-03 11:02 ` Tetsuo Handa
2020-02-03 11:10   ` Alexander Potapenko

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).