[PATCH] ARM: stacktrace: disregard .entry.text when looking for exception frames

[PATCH] ARM: stacktrace: disregard .entry.text when looking for exception frames

[Ard Biesheuvel]

Commit c608906165355089 ("ARM: probes: avoid adding kprobes to sensitive
kernel-entry/exit code") reorganized the section layout to prevent entry
code from being instrumented by kprobes.

This resulted in stack frames referring to back to entry code to be
misidentified as exception frames, resulting in splats like the below
when KASAN is enabled:

  ==================================================================
  BUG: KASAN: stack-out-of-bounds in save_trace+0xc1/0xf8
  Read of size 4 at addr df01f89c by task bash/3421

  CPU: 12 PID: 3421 Comm: bash Not tainted 5.10.0-rc1-kasan+ #219
  Hardware name: QEMU KVM Virtual Machine, BIOS 0.0.0 02/06/2015
  [<c0411765>] (unwind_backtrace) from [<c040ba8b>] (show_stack+0xb/0xc)
  [<c040ba8b>] (show_stack) from [<c121c5e5>] (dump_stack+0x8d/0xa0)
  [<c121c5e5>] (dump_stack) from [<c05bd87b>] (print_address_description.constprop.3+0x2b/0x368)
  [<c05bd87b>] (print_address_description.constprop.3) from [<c05bdd2d>] (kasan_report+0xfd/0x114)
  [<c05bdd2d>] (kasan_report) from [<c040b629>] (save_trace+0xc1/0xf8)
  [<c040b629>] (save_trace) from [<c040b563>] (walk_stackframe+0x1b/0x20)
  [<c040b563>] (walk_stackframe) from [<c040b759>] (__save_stack_trace+0xf9/0x100)
  [<c040b759>] (__save_stack_trace) from [<c04cb169>] (stack_trace_save+0x71/0x88)
  [<c04cb169>] (stack_trace_save) from [<c05bcd9d>] (kasan_save_stack+0x11/0x28)
  [<c05bcd9d>] (kasan_save_stack) from [<c05bcdd1>] (kasan_set_track+0x1d/0x20)
  [<c05bcdd1>] (kasan_set_track) from [<c05be421>] (kasan_set_free_info+0x19/0x20)
  [<c05be421>] (kasan_set_free_info) from [<c05bcd67>] (__kasan_slab_free+0xa7/0xcc)
  [<c05bcd67>] (__kasan_slab_free) from [<c05bbe41>] (kmem_cache_free+0x59/0x21c)
  [<c05bbe41>] (kmem_cache_free) from [<c04c1fdb>] (rcu_core+0x2d7/0x988)
  [<c04c1fdb>] (rcu_core) from [<c04014ab>] (__do_softirq+0x133/0x41c)
  [<c04014ab>] (__do_softirq) from [<c044d4c9>] (irq_exit+0xb5/0xcc)
  [<c044d4c9>] (irq_exit) from [<c04a8503>] (__handle_domain_irq+0x5f/0xa8)
  [<c04a8503>] (__handle_domain_irq) from [<c08d5cb5>] (gic_handle_irq+0x3d/0x8c)
  [<c08d5cb5>] (gic_handle_irq) from [<c0400c11>] (__irq_svc+0x51/0x7c)
  Exception stack(0xdf01f7f8 to 0xdf01f840)
  f7e0:                                                       df01f8e0 df01f8a0
  f800: 00000008 df01fc7c 000000ad df01f960 df01f8e0 00000008 bae03f10 df01fc58
  f820: df01f8a0 0000000a c040b660 df01f848 c04114eb c04110b8 20000033 ffffffff
  [<c0400c11>] (__irq_svc) from [<c04110b8>] (unwind_pop_register+0x0/0x58)
  [<c04110b8>] (unwind_pop_register) from [<c0400161>] (ret_fast_syscall+0x1/0x5a)
  Exception stack(0xdf01f860 to 0xdf01f8a8)
  f860: 41b58ab3 c1f939e0 c04111ec 00000000 d129e800 c0411311 df01fa44 df01fa4c
  f880: 41b58ab3 c1f939e0 c04111ec 00000001 00000003 c040b569 df01f8c0 df01c000
  f8a0: df01fc7c df01ffa8
  ...

  addr df01f89c is located in stack of task bash/3421 at offset 28 in frame:
   unwind_frame+0x0/0x578

Here, the last entry represents a call to unwind_pop_register frame() with
the return address set to ret_fast_syscall, and since in_entry_text() returns
true for that return address, save_trace() treats it as an exception frame,
and attempts to dereference the struct pt_regs pointer to access the ARM_pc
value. With KASAN instrumentation enabled, this results in a read from an
address which is annotated as out of bounds, resulting in the splat.

(Note that the KASAN response is triggered inside the KASAN machinery
itself, which records stack traces for memory allocation and free actions.
While recording such a stack trace, the out of bounds access triggered
the response above, resulting in yet another walk of the call stack, but
this time KASAN was no longer mediating the memory accesses. The same
stack frame is misidentified a second time, which is why the trace above
contains 'Exception stack(0xdf01f860 to 0xdf01f8a8)' which is not really
an exception stack at all.)

So the correct thing to do here is to disregard .entry.text, and only take
true exception frames into account. So we need to use __in_irqentry_text()
instead, in two places: in save_trace(), which KASAN uses to record the
stack traces, and in dump_backtrace_entry(), which prints the exception
stack to the kernel log like above.

Fixes: c608906165355089 ("ARM: probes: avoid adding kprobes to sensitive ...")
Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
---
 arch/arm/kernel/stacktrace.c | 2 +-
 arch/arm/kernel/traps.c      | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/arch/arm/kernel/stacktrace.c b/arch/arm/kernel/stacktrace.c
index 76ea4178a55c..56a7abdc1b96 100644
--- a/arch/arm/kernel/stacktrace.c
+++ b/arch/arm/kernel/stacktrace.c
@@ -112,7 +112,7 @@ static int save_trace(struct stackframe *frame, void *d)
 	if (trace->nr_entries >= trace->max_entries)
 		return 1;
 
-	if (!in_entry_text(frame->pc))
+	if (!__in_irqentry_text(frame->pc))
 		return 0;
 
 	regs = (struct pt_regs *)frame->sp;
diff --git a/arch/arm/kernel/traps.c b/arch/arm/kernel/traps.c
index 17d5a785df28..30628daa80b4 100644
--- a/arch/arm/kernel/traps.c
+++ b/arch/arm/kernel/traps.c
@@ -75,7 +75,7 @@ void dump_backtrace_entry(unsigned long where, unsigned long from,
 		loglvl, where, from);
 #endif
 
-	if (in_entry_text(from) && end <= ALIGN(frame, THREAD_SIZE))
+	if (__in_irqentry_text(from) && end <= ALIGN(frame, THREAD_SIZE))
 		dump_mem(loglvl, "Exception stack", frame + 4, end);
 }
 
-- 
2.17.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] ARM: stacktrace: disregard .entry.text when looking for exception frames

[Ard Biesheuvel]

On Thu, 29 Oct 2020 at 01:18, Ard Biesheuvel <ardb@kernel.org> wrote:
>
> Commit c608906165355089 ("ARM: probes: avoid adding kprobes to sensitive
> kernel-entry/exit code") reorganized the section layout to prevent entry
> code from being instrumented by kprobes.
>
> This resulted in stack frames referring to back to entry code to be
> misidentified as exception frames, resulting in splats like the below
> when KASAN is enabled:
>
>   ==================================================================
>   BUG: KASAN: stack-out-of-bounds in save_trace+0xc1/0xf8
>   Read of size 4 at addr df01f89c by task bash/3421
>
>   CPU: 12 PID: 3421 Comm: bash Not tainted 5.10.0-rc1-kasan+ #219
>   Hardware name: QEMU KVM Virtual Machine, BIOS 0.0.0 02/06/2015
>   [<c0411765>] (unwind_backtrace) from [<c040ba8b>] (show_stack+0xb/0xc)
>   [<c040ba8b>] (show_stack) from [<c121c5e5>] (dump_stack+0x8d/0xa0)
>   [<c121c5e5>] (dump_stack) from [<c05bd87b>] (print_address_description.constprop.3+0x2b/0x368)
>   [<c05bd87b>] (print_address_description.constprop.3) from [<c05bdd2d>] (kasan_report+0xfd/0x114)
>   [<c05bdd2d>] (kasan_report) from [<c040b629>] (save_trace+0xc1/0xf8)
>   [<c040b629>] (save_trace) from [<c040b563>] (walk_stackframe+0x1b/0x20)
>   [<c040b563>] (walk_stackframe) from [<c040b759>] (__save_stack_trace+0xf9/0x100)
>   [<c040b759>] (__save_stack_trace) from [<c04cb169>] (stack_trace_save+0x71/0x88)
>   [<c04cb169>] (stack_trace_save) from [<c05bcd9d>] (kasan_save_stack+0x11/0x28)
>   [<c05bcd9d>] (kasan_save_stack) from [<c05bcdd1>] (kasan_set_track+0x1d/0x20)
>   [<c05bcdd1>] (kasan_set_track) from [<c05be421>] (kasan_set_free_info+0x19/0x20)
>   [<c05be421>] (kasan_set_free_info) from [<c05bcd67>] (__kasan_slab_free+0xa7/0xcc)
>   [<c05bcd67>] (__kasan_slab_free) from [<c05bbe41>] (kmem_cache_free+0x59/0x21c)
>   [<c05bbe41>] (kmem_cache_free) from [<c04c1fdb>] (rcu_core+0x2d7/0x988)
>   [<c04c1fdb>] (rcu_core) from [<c04014ab>] (__do_softirq+0x133/0x41c)
>   [<c04014ab>] (__do_softirq) from [<c044d4c9>] (irq_exit+0xb5/0xcc)
>   [<c044d4c9>] (irq_exit) from [<c04a8503>] (__handle_domain_irq+0x5f/0xa8)
>   [<c04a8503>] (__handle_domain_irq) from [<c08d5cb5>] (gic_handle_irq+0x3d/0x8c)
>   [<c08d5cb5>] (gic_handle_irq) from [<c0400c11>] (__irq_svc+0x51/0x7c)
>   Exception stack(0xdf01f7f8 to 0xdf01f840)
>   f7e0:                                                       df01f8e0 df01f8a0
>   f800: 00000008 df01fc7c 000000ad df01f960 df01f8e0 00000008 bae03f10 df01fc58
>   f820: df01f8a0 0000000a c040b660 df01f848 c04114eb c04110b8 20000033 ffffffff
>   [<c0400c11>] (__irq_svc) from [<c04110b8>] (unwind_pop_register+0x0/0x58)
>   [<c04110b8>] (unwind_pop_register) from [<c0400161>] (ret_fast_syscall+0x1/0x5a)
>   Exception stack(0xdf01f860 to 0xdf01f8a8)
>   f860: 41b58ab3 c1f939e0 c04111ec 00000000 d129e800 c0411311 df01fa44 df01fa4c
>   f880: 41b58ab3 c1f939e0 c04111ec 00000001 00000003 c040b569 df01f8c0 df01c000
>   f8a0: df01fc7c df01ffa8
>   ...
>
>   addr df01f89c is located in stack of task bash/3421 at offset 28 in frame:
>    unwind_frame+0x0/0x578
>
> Here, the last entry represents a call to unwind_pop_register frame() with
> the return address set to ret_fast_syscall, and since in_entry_text() returns
> true for that return address, save_trace() treats it as an exception frame,
> and attempts to dereference the struct pt_regs pointer to access the ARM_pc
> value. With KASAN instrumentation enabled, this results in a read from an
> address which is annotated as out of bounds, resulting in the splat.
>
> (Note that the KASAN response is triggered inside the KASAN machinery
> itself, which records stack traces for memory allocation and free actions.
> While recording such a stack trace, the out of bounds access triggered
> the response above, resulting in yet another walk of the call stack, but
> this time KASAN was no longer mediating the memory accesses. The same
> stack frame is misidentified a second time, which is why the trace above
> contains 'Exception stack(0xdf01f860 to 0xdf01f8a8)' which is not really
> an exception stack at all.)
>
> So the correct thing to do here is to disregard .entry.text, and only take
> true exception frames into account. So we need to use __in_irqentry_text()
> instead, in two places: in save_trace(), which KASAN uses to record the
> stack traces, and in dump_backtrace_entry(), which prints the exception
> stack to the kernel log like above.
>
> Fixes: c608906165355089 ("ARM: probes: avoid adding kprobes to sensitive ...")
> Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
> ---
>  arch/arm/kernel/stacktrace.c | 2 +-
>  arch/arm/kernel/traps.c      | 2 +-
>  2 files changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/arch/arm/kernel/stacktrace.c b/arch/arm/kernel/stacktrace.c
> index 76ea4178a55c..56a7abdc1b96 100644
> --- a/arch/arm/kernel/stacktrace.c
> +++ b/arch/arm/kernel/stacktrace.c
> @@ -112,7 +112,7 @@ static int save_trace(struct stackframe *frame, void *d)
>         if (trace->nr_entries >= trace->max_entries)
>                 return 1;
>
> -       if (!in_entry_text(frame->pc))
> +       if (!__in_irqentry_text(frame->pc))
>                 return 0;
>
>         regs = (struct pt_regs *)frame->sp;
> diff --git a/arch/arm/kernel/traps.c b/arch/arm/kernel/traps.c
> index 17d5a785df28..30628daa80b4 100644
> --- a/arch/arm/kernel/traps.c
> +++ b/arch/arm/kernel/traps.c
> @@ -75,7 +75,7 @@ void dump_backtrace_entry(unsigned long where, unsigned long from,
>                 loglvl, where, from);
>  #endif
>
> -       if (in_entry_text(from) && end <= ALIGN(frame, THREAD_SIZE))
> +       if (__in_irqentry_text(from) && end <= ALIGN(frame, THREAD_SIZE))
>                 dump_mem(loglvl, "Exception stack", frame + 4, end);
>  }
>

This is not quite correct - identifying the exception frame should be
based on 'where' not 'from' when relying on .irqentry.text annotations
but we don't emit those unless CONFIG_FUNCTION_GRAPH_TRACER is
enabled.

I don't understand this well enough to disentangle it, and other
KASAN-enabled architectures simply disable instrumentation for the
stack traversal code, so I propose we do the same for ARM

I.e.,

diff --git a/arch/arm/kernel/Makefile b/arch/arm/kernel/Makefile
index 6a44ffd9c7b4..dad77b0bb734 100644
--- a/arch/arm/kernel/Makefile
+++ b/arch/arm/kernel/Makefile
@@ -21,6 +21,9 @@ obj-y         := elf.o entry-common.o irq.o opcodes.o \
                   setup.o signal.o sigreturn_codes.o \
                   stacktrace.o sys_arm.o time.o traps.o

+KASAN_SANITIZE_stacktrace.o := n
+KASAN_SANITIZE_traps.o := n
+
 ifneq ($(CONFIG_ARM_UNWIND),y)
 obj-$(CONFIG_FRAME_POINTER)    += return_address.o
 endif

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] ARM: stacktrace: disregard .entry.text when looking for exception frames

[Linus Walleij]

On Thu, Oct 29, 2020 at 7:59 PM Ard Biesheuvel <ardb@kernel.org> wrote:

> This is not quite correct - identifying the exception frame should be
> based on 'where' not 'from' when relying on .irqentry.text annotations
> but we don't emit those unless CONFIG_FUNCTION_GRAPH_TRACER is
> enabled.
>
> I don't understand this well enough to disentangle it, and other
> KASAN-enabled architectures simply disable instrumentation for the
> stack traversal code, so I propose we do the same for ARM
>
> I.e.,
>
> diff --git a/arch/arm/kernel/Makefile b/arch/arm/kernel/Makefile
> index 6a44ffd9c7b4..dad77b0bb734 100644
> --- a/arch/arm/kernel/Makefile
> +++ b/arch/arm/kernel/Makefile
> @@ -21,6 +21,9 @@ obj-y         := elf.o entry-common.o irq.o opcodes.o \
>                    setup.o signal.o sigreturn_codes.o \
>                    stacktrace.o sys_arm.o time.o traps.o
>
> +KASAN_SANITIZE_stacktrace.o := n
> +KASAN_SANITIZE_traps.o := n

Reviewed-by: Linus Walleij <linus.walleij@linaro.org>

for this approach. Just cook this into a proper patch and then put it in
Russell's patch tracker I'd say.

If people really want to probe around this code then they can invest the
time to analyze the code paths.

Yours,
Linus Walleij

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel