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

Thu Oct 29 14:59:28 EDT 2020

On Thu, 29 Oct 2020 at 01:18, Ard Biesheuvel <ardb at 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 at 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

