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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <CACT4Y+ay_0e6GSsaYXwLGRkBmmBGSA-gy_TEUu+FsL8JfRHG9g@mail.gmail.com>
Date:   Wed, 13 Nov 2019 16:43:43 +0100
From:   Dmitry Vyukov <dvyukov@...gle.com>
To:     Andrey Konovalov <andreyknvl@...gle.com>
Cc:     Jann Horn <jannh@...gle.com>, Thomas Gleixner <tglx@...utronix.de>,
        Ingo Molnar <mingo@...hat.com>, Borislav Petkov <bp@...en8.de>,
        "H. Peter Anvin" <hpa@...or.com>,
        "the arch/x86 maintainers" <x86@...nel.org>,
        Andrey Ryabinin <aryabinin@...tuozzo.com>,
        Alexander Potapenko <glider@...gle.com>,
        kasan-dev <kasan-dev@...glegroups.com>,
        LKML <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH 3/3] x86/kasan: Print original address on #GP

On Wed, Nov 13, 2019 at 4:19 PM Andrey Konovalov <andreyknvl@...gle.com> wrote:
>
> On Wed, Nov 13, 2019 at 11:11 AM 'Dmitry Vyukov' via kasan-dev
> <kasan-dev@...glegroups.com> wrote:
> >
> > On Tue, Nov 12, 2019 at 10:10 PM 'Jann Horn' via kasan-dev
> > <kasan-dev@...glegroups.com> 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
> > >     [...]
>
> Would it make sense to use the common "BUG: KASAN: <bug-type>" report
> format here? Something like:
>
> BUG: KASAN: invalid-ptr-deref in range ...


Currently this line is not the official bug title. The official bug
title is "general protection fault:" line that follows.
If we add "BUG: KASAN:" before that we need to be super careful wrt
effect on syzbot but parsing/reporting.



> Otherwise this looks amazing, distinguishing NULL pointer accesses
> from wild memory accesses is much more convenient with this. Thanks
> Jann!
>
> >
> > Nice!
> >
> > +Andrey, do you see any issues for TAGS mode? Or, Jann, did you test
> > it by any chance?
>
> Hm, this looks like x86-specific change, so I don't think it
> interferes with the TAGS mode.
>
> >
> >
> > > Signed-off-by: Jann Horn <jannh@...gle.com>
> > > ---
> > >  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.
>
> I think we should use the same naming scheme here as in
> get_wild_bug_type(): null-ptr-deref, user-memory-access and
> wild-memory-access.
>
> >
> > > +       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@...glegroups.com.
> > > To view this discussion on the web visit https://groups.google.com/d/msgid/kasan-dev/20191112211002.128278-3-jannh%40google.com.
> >
> > --
> > 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@...glegroups.com.
> > To view this discussion on the web visit https://groups.google.com/d/msgid/kasan-dev/CACT4Y%2BaojSsss3%2BY2FB9Rw%3DOPxXgsFrGF0YiAJ9eo2wJM0ruWg%40mail.gmail.com.