stackdepot: check depot_index before accessing the stack slab

PATCH In review [email protected] 3 participants 7 messages Feb 20, 2020 – Mar 7, 2020

Subsystems (3)

Memory management Library code KASAN

lore

Share & tools

lore

[email protected]

author reviewer

2020-02-20 14:19 UTC #0 lore raw

PATCH 1/3 stackdepot: check depot_index before accessing the stack slab

Avoid crashes on corrupted stack ids.
Despite stack ID corruption may indicate other bugs in the program, we'd better fail gracefully on such IDs instead of crashing the kernel.

This patch has been previously mailed as part of KMSAN RFC patch series.

Signed-off-by Alexander Potapenko <[email protected]>

---
 lib/stackdepot.c | 17 +++++++++++++++--
 1 file changed, 15 insertions(+), 2 deletions(-)

lib/stackdepot.c +15 -2

index 81c69c08d1d15..a2f6cb900db80 100644
@@ -202,9 +202,22 @@ unsigned int stack_depot_fetch(depot_stack_handle_t handle,
 			       unsigned long **entries)
 {
 	union handle_parts parts = { .handle = handle };
-	void *slab = stack_slabs[parts.slabindex];
+	void *slab;
 	size_t offset = parts.offset << STACK_ALLOC_ALIGN;
-	struct stack_record *stack = slab + offset;
+	struct stack_record *stack;
+
+	if (parts.slabindex > depot_index) {
+		WARN(1, "slab index %d out of bounds (%d) for stack id %08x\n",
+			parts.slabindex, depot_index, handle);
+		*entries = NULL;
+		return 0;
+	}
+	slab = stack_slabs[parts.slabindex];
+	stack = slab + offset;
+	if (!stack) {
+		*entries = NULL;
+		return 0;
+	}
 
 	*entries = stack->entries;
 	return stack->size;

Signature · 1 line

2.25.0.265.gbab2e86ba0-goog

[email protected]

author reviewer

2020-02-20 14:19 UTC #1 lore raw

PATCH 2/3 stackdepot: build with -fno-builtin

Clang may replace stackdepot_memcmp() with a call to instrumented bcmp(), which is exactly what we wanted to avoid creating stackdepot_memcmp(). Building the file with -fno-builtin prevents such optimizations.

This patch has been previously mailed as part of KMSAN RFC patch series.

Signed-off-by Alexander Potapenko <[email protected]>

To: Alexander Potapenko <[email protected]>
Cc: Vegard Nossum <[email protected]>
Cc: Dmitry Vyukov <[email protected]>
Cc: Marco Elver <[email protected]>
Cc: Andrey Konovalov <[email protected]>
Cc: Sergey Senozhatsky <[email protected]>
Cc: Arnd Bergmann <[email protected]>
Cc: Andrey Ryabinin <[email protected]>
Cc: [email protected]

---
 lib/Makefile | 4 ++++
 1 file changed, 4 insertions(+)

lib/Makefile +4 -0

index 23ca78d43d247..390e90d2ee88b 100644
@@ -220,6 +220,10 @@ obj-$(CONFIG_MEMREGION) += memregion.o
 obj-$(CONFIG_STMP_DEVICE) += stmp_device.o
 obj-$(CONFIG_IRQ_POLL) += irq_poll.o
 
+# stackdepot.c should not be instrumented or call instrumented functions.
+# Prevent the compiler from calling builtins like memcmp() or bcmp() from this
+# file.
+CFLAGS_stackdepot.o += -fno-builtin
 obj-$(CONFIG_STACKDEPOT) += stackdepot.o
 KASAN_SANITIZE_stackdepot.o := n
 KCOV_INSTRUMENT_stackdepot.o := n

Signature · 1 line

2.25.0.265.gbab2e86ba0-goog

[email protected]

author reviewer

2020-02-20 14:19 UTC #2 lore raw

PATCH 3/3 kasan: stackdepot: move filter_irq_stacks() to stackdepot.c

In reply to [email protected]

filter_irq_stacks() can be used by other tools (e.g. KMSAN), so it needs to be moved to a common location.
lib/stackdepot.c seems a good place, as filter_irq_stacks() is usually applied to the output of stack_trace_save().

This patch has been previously mailed as part of KMSAN RFC patch series.

Signed-off-by Alexander Potapenko <[email protected]>

To: Alexander Potapenko <[email protected]>
Cc: Vegard Nossum <[email protected]>
Cc: Dmitry Vyukov <[email protected]>
Cc: Marco Elver <[email protected]>
Cc: Andrey Konovalov <[email protected]>
Cc: Andrey Ryabinin <[email protected]>
Cc: [email protected]

---
 include/linux/stackdepot.h |  2 ++
 lib/stackdepot.c           | 24 ++++++++++++++++++++++++
 mm/kasan/common.c          | 23 -----------------------
 3 files changed, 26 insertions(+), 23 deletions(-)

include/linux/stackdepot.h +2 -0

index 3efa97d482cbe..24d49c732341a 100644
@@ -19,4 +19,6 @@ depot_stack_handle_t stack_depot_save(unsigned long *entries,
 unsigned int stack_depot_fetch(depot_stack_handle_t handle,
 			       unsigned long **entries);
 
+unsigned int filter_irq_stacks(unsigned long *entries, unsigned int nr_entries);
+
 #endif

lib/stackdepot.c +24 -0

index a2f6cb900db80..da5d1880bf343 100644
@@ -20,6 +20,7 @@
  */
 
 #include <linux/gfp.h>
+#include <linux/interrupt.h>
 #include <linux/jhash.h>
 #include <linux/kernel.h>
 #include <linux/mm.h>

@@ -318,3 +319,26 @@ depot_stack_handle_t stack_depot_save(unsigned long *entries,
 	return retval;
 }
 EXPORT_SYMBOL_GPL(stack_depot_save);
+
+static inline int in_irqentry_text(unsigned long ptr)
+{
+	return (ptr >= (unsigned long)&__irqentry_text_start &&
+		ptr < (unsigned long)&__irqentry_text_end) ||
+		(ptr >= (unsigned long)&__softirqentry_text_start &&
+		 ptr < (unsigned long)&__softirqentry_text_end);
+}
+
+unsigned int filter_irq_stacks(unsigned long *entries,
+					     unsigned int nr_entries)
+{
+	unsigned int i;
+
+	for (i = 0; i < nr_entries; i++) {
+		if (in_irqentry_text(entries[i])) {
+			/* Include the irqentry function into the stack. */
+			return i + 1;
+		}
+	}
+	return nr_entries;
+}
+EXPORT_SYMBOL_GPL(filter_irq_stacks);

mm/kasan/common.c +0 ~~-23~~

index 6aa51723b92b9..92815ef9c80d3 100644
@@ -15,7 +15,6 @@
  */
 
 #include <linux/export.h>
-#include <linux/interrupt.h>
 #include <linux/init.h>
 #include <linux/kasan.h>
 #include <linux/kernel.h>

@@ -42,28 +41,6 @@
 #include "kasan.h"
 #include "../slab.h"
 
-static inline int in_irqentry_text(unsigned long ptr)
-{
-	return (ptr >= (unsigned long)&__irqentry_text_start &&
-		ptr < (unsigned long)&__irqentry_text_end) ||
-		(ptr >= (unsigned long)&__softirqentry_text_start &&
-		 ptr < (unsigned long)&__softirqentry_text_end);
-}
-
-static inline unsigned int filter_irq_stacks(unsigned long *entries,
-					     unsigned int nr_entries)
-{
-	unsigned int i;
-
-	for (i = 0; i < nr_entries; i++) {
-		if (in_irqentry_text(entries[i])) {
-			/* Include the irqentry function into the stack. */
-			return i + 1;
-		}
-	}
-	return nr_entries;
-}
-
 static inline depot_stack_handle_t save_stack(gfp_t flags)
 {
 	unsigned long entries[KASAN_STACK_DEPTH];

Signature · 1 line

2.25.0.265.gbab2e86ba0-goog

kbuild test robot

build bot

2020-02-25 20:24 UTC #3 lore raw

Re: [PATCH 3/3] kasan: stackdepot: move filter_irq_stacks() to stackdepot.c

I love your patch! Yet something to improve:

Hi,

I love your patch! Yet something to improve:

[auto build test ERROR on linus/master]
[also build test ERROR on v5.6-rc3 next-20200225]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system. BTW, we also suggest to use '--base' option to specify the base tree in git format-patch, please see https://stackoverflow.com/a/37406982]

url:    https://github.com/0day-ci/linux/commits/glider-google-com/stackdepot-check-depot_index-before-accessing-the-stack-slab/20200222-065605
base:   https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git 0c0ddd6ae47c9238c18f475bcca675ca74c9dc31
config: nds32-randconfig-a001-20200225 (attached as .config)
compiler: nds32le-linux-gcc (GCC) 9.2.0

reproduce:
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross chmod +x ~/bin/make.cross
# save the attached .config to linux build tree
GCC_VERSION=9.2.0 make.cross ARCH=nds32

If you fix the issue, kindly add following tag

Reported-by kbuild test robot <[email protected]>

All errors (new ones prefixed by >>):

nds32le-linux-ld: lib/stackdepot.o: in function `filter_irq_stacks':

>> stackdepot.c:(.text+0xc6): undefined reference to `__softirqentry_text_start'
>> nds32le-linux-ld: stackdepot.c:(.text+0xca): undefined reference to `__softirqentry_text_start'
>> nds32le-linux-ld: stackdepot.c:(.text+0xce): undefined reference to `__softirqentry_text_end'

nds32le-linux-ld: stackdepot.c:(.text+0xd2): undefined reference to `__softirqentry_text_end'

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all(a)lists.01.org

Alexander Potapenko

author reviewer

2020-02-26 09:53 UTC #4 lore raw

Re: [PATCH 3/3] kasan: stackdepot: move filter_irq_stacks() to stackdepot.c

Hi Andes maintainers,

Quoted 24 lines · kbuild test robot

Hi,
I love your patch! Yet something to improve:
[auto build test ERROR on linus/master]
[also build test ERROR on v5.6-rc3 next-20200225]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system. BTW, we also suggest to use '--base' option to specify the base tree in git format-patch, please see https://stackoverflow.com/a/37406982]
url:    https://github.com/0day-ci/linux/commits/glider-google-com/stackdepot-check-depot_index-before-accessing-the-stack-slab/20200222-065605
base:   https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git 0c0ddd6ae47c9238c18f475bcca675ca74c9dc31
config: nds32-randconfig-a001-20200225 (attached as .config)
compiler: nds32le-linux-gcc (GCC) 9.2.0
reproduce:
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross chmod +x ~/bin/make.cross
# save the attached .config to linux build tree
GCC_VERSION=9.2.0 make.cross ARCH=nds32
If you fix the issue, kindly add following tag
Reported-by kbuild test robot <[email protected]>

Quoted 7 lines

All errors (new ones prefixed by >>):
nds32le-linux-ld: lib/stackdepot.o: in function `filter_irq_stacks':
Quoted 3 lines
Quoted 3 lines
stackdepot.c:(.text+0xc6): undefined reference to `__softirqentry_text_start'
nds32le-linux-ld: stackdepot.c:(.text+0xca): undefined reference to `__softirqentry_text_start'
nds32le-linux-ld: stackdepot.c:(.text+0xce): undefined reference to `__softirqentry_text_end'
nds32le-linux-ld: stackdepot.c:(.text+0xd2): undefined reference to `__softirqentry_text_end'

Can you please help me with this?
I couldn't find out where this section needs to be declared, and I don't really know whether nds32 is supposed to have softirqs or not. Maybe filter_irq_stacks() should be no-op on nds32?

Quoted 3 lines

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all(a)lists.01.org

Signature · 10 lines

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

Greentime Hu

2020-02-27 03:16 UTC #5 lore raw

Re: [PATCH 3/3] kasan: stackdepot: move filter_irq_stacks() to stackdepot.c

Alexander Potapenko <[email protected]> 於 2020年2月26日週三下午5:53寫道：

Quoted 34 lines

Hi Andes maintainers,
Quoted 30 lines · kbuild test robot
Hi,
I love your patch! Yet something to improve:
[auto build test ERROR on linus/master]
[also build test ERROR on v5.6-rc3 next-20200225]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system. BTW, we also suggest to use '--base' option to specify the base tree in git format-patch, please see https://stackoverflow.com/a/37406982]
url:    https://github.com/0day-ci/linux/commits/glider-google-com/stackdepot-check-depot_index-before-accessing-the-stack-slab/20200222-065605
base:   https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git 0c0ddd6ae47c9238c18f475bcca675ca74c9dc31
config: nds32-randconfig-a001-20200225 (attached as .config)
compiler: nds32le-linux-gcc (GCC) 9.2.0
reproduce:
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross chmod +x ~/bin/make.cross
# save the attached .config to linux build tree
GCC_VERSION=9.2.0 make.cross ARCH=nds32
If you fix the issue, kindly add following tag
Reported-by kbuild test robot <[email protected]>
All errors (new ones prefixed by >>):
nds32le-linux-ld: lib/stackdepot.o: in function `filter_irq_stacks':
Quoted 3 lines
Quoted 3 lines
stackdepot.c:(.text+0xc6): undefined reference to `__softirqentry_text_start'
nds32le-linux-ld: stackdepot.c:(.text+0xca): undefined reference to `__softirqentry_text_start'
nds32le-linux-ld: stackdepot.c:(.text+0xce): undefined reference to `__softirqentry_text_end'

Quoted 6 lines

Quoted 1 line
   nds32le-linux-ld: stackdepot.c:(.text+0xd2): undefined reference to `__softirqentry_text_end'
Can you please help me with this?
I couldn't find out where this section needs to be declared, and I don't really know whether nds32 is supposed to have softirqs or not. Maybe filter_irq_stacks() should be no-op on nds32?

Hi Alexander,

I've disccussed this with Nick.
There might be 2 solutions, but we are not sure which one is better or maybe there are another better solutions.
Since nds32 doesn't support KASAN, we might not need to use this section.

Solution 1.
We may just add the dependency in STACKDEPOT depend on KASAN

Solution 2.
Just add SOFTIRQENTRY_TEXT this section to nds32's linker script

What do you think? :)

Alexander Potapenko

author reviewer

2020-03-07 13:37 UTC #6 lore raw

Re: [PATCH 3/3] kasan: stackdepot: move filter_irq_stacks() to stackdepot.c

Quoted 50 lines · Greentime Hu

Alexander Potapenko <[email protected]> 於 2020年2月26日週三下午5:53寫道：
Quoted 34 lines
Hi Andes maintainers,
Quoted 30 lines · kbuild test robot
Hi,
I love your patch! Yet something to improve:
[auto build test ERROR on linus/master]
[also build test ERROR on v5.6-rc3 next-20200225]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system. BTW, we also suggest to use '--base' option to specify the base tree in git format-patch, please see https://stackoverflow.com/a/37406982]
url:    https://github.com/0day-ci/linux/commits/glider-google-com/stackdepot-check-depot_index-before-accessing-the-stack-slab/20200222-065605
base:   https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git 0c0ddd6ae47c9238c18f475bcca675ca74c9dc31
config: nds32-randconfig-a001-20200225 (attached as .config)
compiler: nds32le-linux-gcc (GCC) 9.2.0
reproduce:
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross chmod +x ~/bin/make.cross
# save the attached .config to linux build tree
GCC_VERSION=9.2.0 make.cross ARCH=nds32
If you fix the issue, kindly add following tag
Reported-by kbuild test robot <[email protected]>
All errors (new ones prefixed by >>):
nds32le-linux-ld: lib/stackdepot.o: in function `filter_irq_stacks':
Quoted 3 lines
Quoted 3 lines
stackdepot.c:(.text+0xc6): undefined reference to `__softirqentry_text_start'
nds32le-linux-ld: stackdepot.c:(.text+0xca): undefined reference to `__softirqentry_text_start'
nds32le-linux-ld: stackdepot.c:(.text+0xce): undefined reference to `__softirqentry_text_end'
Quoted 6 lines
Quoted 1 line
   nds32le-linux-ld: stackdepot.c:(.text+0xd2): undefined reference to `__softirqentry_text_end'
Can you please help me with this?
I couldn't find out where this section needs to be declared, and I don't really know whether nds32 is supposed to have softirqs or not. Maybe filter_irq_stacks() should be no-op on nds32?
Hi Alexander,
I've disccussed this with Nick.
There might be 2 solutions, but we are not sure which one is better or maybe there are another better solutions.
Since nds32 doesn't support KASAN, we might not need to use this section.

Quoted 7 lines

Solution 1.
We may just add the dependency in STACKDEPOT depend on KASAN
Solution 2.
Just add SOFTIRQENTRY_TEXT this section to nds32's linker script
What do you think? :)

Hi,

I think the latter solution actually makes sense.

Signature · 10 lines

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