Re: [PATCH 3/3] x86/kasan: Print original address on #GP

[Dmitry Vyukov]

On Tue, Nov 12, 2019 at 10:10 PM 'Jann Horn' via kasan-dev
<kasan-dev@xxxxxxxxxxxxxxxx> wrote:
>
> Make #GP exceptions caused by out-of-bounds KASAN shadow accesses easier
> to understand by computing the address of the original access and
> printing that. More details are in the comments in the patch.
>
> This turns an error like this:
>
>     kasan: CONFIG_KASAN_INLINE enabled
>     kasan: GPF could be caused by NULL-ptr deref or user memory access
>     traps: dereferencing non-canonical address 0xe017577ddf75b7dd
>     general protection fault: 0000 [#1] PREEMPT SMP KASAN PTI
>
> into this:
>
>     traps: dereferencing non-canonical address 0xe017577ddf75b7dd
>     kasan: maybe dereferencing invalid pointer in range
>             [0x00badbeefbadbee8-0x00badbeefbadbeef]
>     general protection fault: 0000 [#3] PREEMPT SMP KASAN PTI
>     [...]

Nice!

+Andrey, do you see any issues for TAGS mode? Or, Jann, did you test
it by any chance?


> Signed-off-by: Jann Horn <jannh@xxxxxxxxxx>
> ---
>  arch/x86/include/asm/kasan.h |  6 +++++
>  arch/x86/kernel/traps.c      |  2 ++
>  arch/x86/mm/kasan_init_64.c  | 52 +++++++++++++++++++++++++-----------
>  3 files changed, 44 insertions(+), 16 deletions(-)
>
> diff --git a/arch/x86/include/asm/kasan.h b/arch/x86/include/asm/kasan.h
> index 13e70da38bed..eaf624a758ed 100644
> --- a/arch/x86/include/asm/kasan.h
> +++ b/arch/x86/include/asm/kasan.h
> @@ -25,6 +25,12 @@
>
>  #ifndef __ASSEMBLY__
>
> +#ifdef CONFIG_KASAN_INLINE
> +void kasan_general_protection_hook(unsigned long addr);
> +#else
> +static inline void kasan_general_protection_hook(unsigned long addr) { }
> +#endif
> +
>  #ifdef CONFIG_KASAN
>  void __init kasan_early_init(void);
>  void __init kasan_init(void);
> diff --git a/arch/x86/kernel/traps.c b/arch/x86/kernel/traps.c
> index 479cfc6e9507..e271a5a1ddd4 100644
> --- a/arch/x86/kernel/traps.c
> +++ b/arch/x86/kernel/traps.c
> @@ -58,6 +58,7 @@
>  #include <asm/umip.h>
>  #include <asm/insn.h>
>  #include <asm/insn-eval.h>
> +#include <asm/kasan.h>
>
>  #ifdef CONFIG_X86_64
>  #include <asm/x86_init.h>
> @@ -544,6 +545,7 @@ static void print_kernel_gp_address(struct pt_regs *regs)
>                 return;
>
>         pr_alert("dereferencing non-canonical address 0x%016lx\n", addr_ref);
> +       kasan_general_protection_hook(addr_ref);
>  #endif
>  }
>
> diff --git a/arch/x86/mm/kasan_init_64.c b/arch/x86/mm/kasan_init_64.c
> index 296da58f3013..9ef099309489 100644
> --- a/arch/x86/mm/kasan_init_64.c
> +++ b/arch/x86/mm/kasan_init_64.c
> @@ -246,20 +246,44 @@ static void __init kasan_map_early_shadow(pgd_t *pgd)
>  }
>
>  #ifdef CONFIG_KASAN_INLINE
> -static int kasan_die_handler(struct notifier_block *self,
> -                            unsigned long val,
> -                            void *data)
> +/*
> + * With CONFIG_KASAN_INLINE, accesses to bogus pointers (outside the high
> + * canonical half of the address space) cause out-of-bounds shadow memory reads
> + * before the actual access. For addresses in the low canonical half of the
> + * address space, as well as most non-canonical addresses, that out-of-bounds
> + * shadow memory access lands in the non-canonical part of the address space,
> + * causing #GP to be thrown.
> + * Help the user figure out what the original bogus pointer was.
> + */
> +void kasan_general_protection_hook(unsigned long addr)
>  {
> -       if (val == DIE_GPF) {
> -               pr_emerg("CONFIG_KASAN_INLINE enabled\n");
> -               pr_emerg("GPF could be caused by NULL-ptr deref or user memory access\n");
> -       }
> -       return NOTIFY_OK;
> -}
> +       unsigned long orig_addr;
> +       const char *addr_type;
> +
> +       if (addr < KASAN_SHADOW_OFFSET)
> +               return;

Thinking how much sense it makes to compare addr with KASAN_SHADOW_END...
If the addr is > KASAN_SHADOW_END, we know it's not a KASAN access,
but do we ever get GP on canonical addresses?

>
> -static struct notifier_block kasan_die_notifier = {
> -       .notifier_call = kasan_die_handler,
> -};
> +       orig_addr = (addr - KASAN_SHADOW_OFFSET) << KASAN_SHADOW_SCALE_SHIFT;
> +       /*
> +        * For faults near the shadow address for NULL, we can be fairly certain
> +        * that this is a KASAN shadow memory access.
> +        * For faults that correspond to shadow for low canonical addresses, we
> +        * can still be pretty sure - that shadow region is a fairly narrow
> +        * chunk of the non-canonical address space.
> +        * But faults that look like shadow for non-canonical addresses are a
> +        * really large chunk of the address space. In that case, we still
> +        * print the decoded address, but make it clear that this is not
> +        * necessarily what's actually going on.
> +        */
> +       if (orig_addr < PAGE_SIZE)
> +               addr_type = "dereferencing kernel NULL pointer";
> +       else if (orig_addr < TASK_SIZE_MAX)
> +               addr_type = "probably dereferencing invalid pointer";

This is access to user memory, right? In outline mode we call it
"user-memory-access". We could say about "user" part here as well.

> +       else
> +               addr_type = "maybe dereferencing invalid pointer";
> +       pr_alert("%s in range [0x%016lx-0x%016lx]\n", addr_type,
> +                orig_addr, orig_addr + (1 << KASAN_SHADOW_SCALE_SHIFT) - 1);

"(1 << KASAN_SHADOW_SCALE_SHIFT) - 1)" part may be replaced with
KASAN_SHADOW_MASK.
Overall it can make sense to move this mm/kasan/report.c b/c we are
open-coding a number of things here (e.g. reverse address mapping). If
another arch will do the same, it will need all of this code too (?).

But in general I think it's a very good usability improvement for KASAN.

> +}
>  #endif
>
>  void __init kasan_early_init(void)
> @@ -298,10 +322,6 @@ void __init kasan_init(void)
>         int i;
>         void *shadow_cpu_entry_begin, *shadow_cpu_entry_end;
>
> -#ifdef CONFIG_KASAN_INLINE
> -       register_die_notifier(&kasan_die_notifier);
> -#endif
> -
>         memcpy(early_top_pgt, init_top_pgt, sizeof(early_top_pgt));
>
>         /*
> --
> 2.24.0.432.g9d3f5f5b63-goog
>
> --
> You received this message because you are subscribed to the Google Groups "kasan-dev" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to kasan-dev+unsubscribe@xxxxxxxxxxxxxxxxx
> To view this discussion on the web visit https://groups.google.com/d/msgid/kasan-dev/20191112211002.128278-3-jannh%40google.com.