Re: stack frame unwindind KASAN errors

[Daniel J Blueman]

On 27 February 2017 at 23:47, Josh Poimboeuf <jpoimboe@xxxxxxxxxx> wrote:
> On Mon, Feb 27, 2017 at 12:49:59PM +0800, Daniel J Blueman wrote:
>> On 4.9.13 with KASAN enabled [1], we see a number of stack unwinding
>> errors reported [2,3].
>>
>> This seems to occur at half of boots.
>>
>> Let me know for further debug info/patch testing and thanks,
>>   Daniel
>>
>> [1] https://quora.org/config
>> [2] https://quora.org/dmesg.txt
>
> Hi Daniel,
>
> Can you try the following patch?  It's a backport of the following
> upstream commit:
>
>   09ae68dd0a8d ("x86/unwind: Disable KASAN checks for non-current tasks")
>
> If it fixes it then I'll submit it for 4.9 stable.
>
> ---
>
> From: Josh Poimboeuf <jpoimboe@xxxxxxxxxx>
> Subject: [PATCH] x86/unwind: Disable KASAN checks for non-current tasks
>
> There are a handful of callers to save_stack_trace_tsk() and
> show_stack() which try to unwind the stack of a task other than current.
> In such cases, it's remotely possible that the task is running on one
> CPU while the unwinder is reading its stack from another CPU, causing
> the unwinder to see stack corruption.
>
> These cases seem to be mostly harmless.  The unwinder has checks which
> prevent it from following bad pointers beyond the bounds of the stack.
> So it's not really a bug as long as the caller understands that
> unwinding another task will not always succeed.
>
> In such cases, it's possible that the unwinder may read a KASAN-poisoned
> region of the stack.  Account for that by using READ_ONCE_NOCHECK() when
> reading the stack of another task.
>
> Use READ_ONCE() when reading the stack of the current task, since KASAN
> warnings can still be useful for finding bugs in that case.
>
> Reported-by: Dmitry Vyukov <dvyukov@xxxxxxxxxx>
> Signed-off-by: Josh Poimboeuf <jpoimboe@xxxxxxxxxx>
> Cc: Andy Lutomirski <luto@xxxxxxxxxxxxxx>
> Cc: Andy Lutomirski <luto@xxxxxxxxxx>
> Cc: Borislav Petkov <bp@xxxxxxxxx>
> Cc: Brian Gerst <brgerst@xxxxxxxxx>
> Cc: Dave Jones <davej@xxxxxxxxxxxxxxxxx>
> Cc: Denys Vlasenko <dvlasenk@xxxxxxxxxx>
> Cc: H. Peter Anvin <hpa@xxxxxxxxx>
> Cc: Linus Torvalds <torvalds@xxxxxxxxxxxxxxxxxxxx>
> Cc: Miroslav Benes <mbenes@xxxxxxx>
> Cc: Peter Zijlstra <peterz@xxxxxxxxxxxxx>
> Cc: Thomas Gleixner <tglx@xxxxxxxxxxxxx>
> Link: http://lkml.kernel.org/r/4c575eb288ba9f73d498dfe0acde2f58674598f1.1483978430.git.jpoimboe@xxxxxxxxxx
> Signed-off-by: Ingo Molnar <mingo@xxxxxxxxxx>
> ---
>  arch/x86/include/asm/stacktrace.h |  5 ++++-
>  arch/x86/kernel/unwind_frame.c    | 20 ++++++++++++++++++--
>  2 files changed, 22 insertions(+), 3 deletions(-)
>
> diff --git a/arch/x86/include/asm/stacktrace.h b/arch/x86/include/asm/stacktrace.h
> index 37f2e0b..4141ead 100644
> --- a/arch/x86/include/asm/stacktrace.h
> +++ b/arch/x86/include/asm/stacktrace.h
> @@ -55,13 +55,16 @@ extern int kstack_depth_to_print;
>  static inline unsigned long *
>  get_frame_pointer(struct task_struct *task, struct pt_regs *regs)
>  {
> +       struct inactive_task_frame *frame;
> +
>         if (regs)
>                 return (unsigned long *)regs->bp;
>
>         if (task == current)
>                 return __builtin_frame_address(0);
>
> -       return (unsigned long *)((struct inactive_task_frame *)task->thread.sp)->bp;
> +       frame = (struct inactive_task_frame *)task->thread.sp;
> +       return (unsigned long *)READ_ONCE_NOCHECK(frame->bp);
>  }
>  #else
>  static inline unsigned long *
> diff --git a/arch/x86/kernel/unwind_frame.c b/arch/x86/kernel/unwind_frame.c
> index a2456d4..caff129 100644
> --- a/arch/x86/kernel/unwind_frame.c
> +++ b/arch/x86/kernel/unwind_frame.c
> @@ -6,6 +6,21 @@
>
>  #define FRAME_HEADER_SIZE (sizeof(long) * 2)
>
> +/*
> + * This disables KASAN checking when reading a value from another task's stack,
> + * since the other task could be running on another CPU and could have poisoned
> + * the stack in the meantime.
> + */
> +#define READ_ONCE_TASK_STACK(task, x)                  \
> +({                                                     \
> +       unsigned long val;                              \
> +       if (task == current)                            \
> +               val = READ_ONCE(x);                     \
> +       else                                            \
> +               val = READ_ONCE_NOCHECK(x);             \
> +       val;                                            \
> +})
> +
>  unsigned long unwind_get_return_address(struct unwind_state *state)
>  {
>         unsigned long addr;
> @@ -14,7 +29,8 @@ unsigned long unwind_get_return_address(struct unwind_state *state)
>         if (unwind_done(state))
>                 return 0;
>
> -       addr = ftrace_graph_ret_addr(state->task, &state->graph_idx, *addr_p,
> +       addr = READ_ONCE_TASK_STACK(state->task, *addr_p);
> +       addr = ftrace_graph_ret_addr(state->task, &state->graph_idx, addr,
>                                      addr_p);
>
>         return __kernel_text_address(addr) ? addr : 0;
> @@ -48,7 +64,7 @@ bool unwind_next_frame(struct unwind_state *state)
>         if (unwind_done(state))
>                 return false;
>
> -       next_bp = (unsigned long *)*state->bp;
> +       next_bp = (unsigned long *)READ_ONCE_TASK_STACK(state->task, *state->bp);
>
>         /* make sure the next frame's data is accessible */
>         if (!update_stack_state(state, next_bp, FRAME_HEADER_SIZE))

Thanks Josh!

With this patch, the KASAN warning still occurs, but at
unwind_get_return_address+0x1d3/0x130 instead; the rest of the trace
is identical.

(gdb) list *(unwind_get_return_address+0x1d3)
0xffffffff8112bca3 is in unwind_get_return_address
(./include/linux/compiler.h:243).
238    })
239
240    static __always_inline
241    void __read_once_size(const volatile void *p, void *res, int size)
242    {
243        __READ_ONCE_SIZE;

Thanks,
  Daniel
-- 
Daniel J Blueman