linux-kernel - Re: [PATCH v10 12/22] kasan, arm64: fix up fault handling logic

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]

Date:   Tue, 13 Nov 2018 22:07:29 +0000
From:   Mark Rutland <mark.rutland@....com>
To:     Andrey Konovalov <andreyknvl@...gle.com>
Cc:     Andrey Ryabinin <aryabinin@...tuozzo.com>,
        Alexander Potapenko <glider@...gle.com>,
        Dmitry Vyukov <dvyukov@...gle.com>,
        Catalin Marinas <catalin.marinas@....com>,
        Will Deacon <will.deacon@....com>,
        Christoph Lameter <cl@...ux.com>,
        Andrew Morton <akpm@...ux-foundation.org>,
        Nick Desaulniers <ndesaulniers@...gle.com>,
        Marc Zyngier <marc.zyngier@....com>,
        Dave Martin <dave.martin@....com>,
        Ard Biesheuvel <ard.biesheuvel@...aro.org>,
        "Eric W . Biederman" <ebiederm@...ssion.com>,
        Ingo Molnar <mingo@...nel.org>,
        Paul Lawrence <paullawrence@...gle.com>,
        Geert Uytterhoeven <geert@...ux-m68k.org>,
        Arnd Bergmann <arnd@...db.de>,
        "Kirill A . Shutemov" <kirill.shutemov@...ux.intel.com>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Kate Stewart <kstewart@...uxfoundation.org>,
        Mike Rapoport <rppt@...ux.vnet.ibm.com>,
        kasan-dev@...glegroups.com,
        "open list:DOCUMENTATION" <linux-doc@...r.kernel.org>,
        LKML <linux-kernel@...r.kernel.org>,
        Linux ARM <linux-arm-kernel@...ts.infradead.org>,
        linux-sparse@...r.kernel.org,
        Linux Memory Management List <linux-mm@...ck.org>,
        Linux Kbuild mailing list <linux-kbuild@...r.kernel.org>,
        Kostya Serebryany <kcc@...gle.com>,
        Evgeniy Stepanov <eugenis@...gle.com>,
        Lee Smith <Lee.Smith@....com>,
        Ramana Radhakrishnan <Ramana.Radhakrishnan@....com>,
        Jacob Bramley <Jacob.Bramley@....com>,
        Ruben Ayrapetyan <Ruben.Ayrapetyan@....com>,
        Jann Horn <jannh@...gle.com>,
        Mark Brand <markbrand@...gle.com>,
        Chintan Pandya <cpandya@...eaurora.org>,
        Vishwath Mohan <vishwath@...gle.com>
Subject: Re: [PATCH v10 12/22] kasan, arm64: fix up fault handling logic

On Tue, Nov 13, 2018 at 04:01:27PM +0100, Andrey Konovalov wrote:
> On Thu, Nov 8, 2018 at 1:22 PM, Mark Rutland <mark.rutland@....com> wrote:
> > On Tue, Nov 06, 2018 at 06:30:27PM +0100, Andrey Konovalov wrote:
> >> show_pte in arm64 fault handling relies on the fact that the top byte of
> >> a kernel pointer is 0xff, which isn't always the case with tag-based
> >> KASAN.
> >
> > That's for the TTBR1 check, right?
> >
> > i.e. for the following to work:
> >
> >         if (addr >= VA_START)
> >
> > ... we need the tag bits to be an extension of bit 55...
> >
> >>
> >> This patch resets the top byte in show_pte.
> >>
> >> Reviewed-by: Andrey Ryabinin <aryabinin@...tuozzo.com>
> >> Reviewed-by: Dmitry Vyukov <dvyukov@...gle.com>
> >> Signed-off-by: Andrey Konovalov <andreyknvl@...gle.com>
> >> ---
> >>  arch/arm64/mm/fault.c | 3 +++
> >>  1 file changed, 3 insertions(+)
> >>
> >> diff --git a/arch/arm64/mm/fault.c b/arch/arm64/mm/fault.c
> >> index 7d9571f4ae3d..d9a84d6f3343 100644
> >> --- a/arch/arm64/mm/fault.c
> >> +++ b/arch/arm64/mm/fault.c
> >> @@ -32,6 +32,7 @@
> >>  #include <linux/perf_event.h>
> >>  #include <linux/preempt.h>
> >>  #include <linux/hugetlb.h>
> >> +#include <linux/kasan.h>
> >>
> >>  #include <asm/bug.h>
> >>  #include <asm/cmpxchg.h>
> >> @@ -141,6 +142,8 @@ void show_pte(unsigned long addr)
> >>       pgd_t *pgdp;
> >>       pgd_t pgd;
> >>
> >> +     addr = (unsigned long)kasan_reset_tag((void *)addr);
> >
> > ... but this ORs in (0xffUL << 56), which is not correct for addresses
> > which aren't TTBR1 addresses to begin with, where bit 55 is clear, and
> > throws away useful information.
> >
> > We could use untagged_addr() here, but that wouldn't be right for
> > kernels which don't use TBI1, and we'd erroneously report addresses
> > under the TTBR1 range as being in the TTBR1 range.
> >
> > I also see that the entry assembly for el{1,0}_{da,ia} clears the tag
> > for EL0 addresses.
> >
> > So we could have:
> >
> > static inline bool is_ttbr0_addr(unsigned long addr)
> > {
> >         /* entry assembly clears tags for TTBR0 addrs */
> >         return addr < TASK_SIZE_64;
> > }
> >
> > static inline bool is_ttbr1_addr(unsigned long addr)
> > {
> >         /* TTBR1 addresses may have a tag if HWKASAN is in use */
> >         return arch_kasan_reset_tag(addr) >= VA_START;
> > }
> >
> > ... and use those in the conditionals, leaving the addr as-is for
> > reporting purposes.
> 
> Actually it looks like 276e9327 ("arm64: entry: improve data abort
> handling of tagged pointers") already takes care of both user and
> kernel fault addresses and correctly removes tags from them. So I
> think we need to drop this patch.

The clear_address_tag macro added in that commit only removes tags from TTBR0
addresses, so that's not sufficient if the kernel is used tagged addresses
(which will be in the TTBR1 range).

Thanks,
Mark.
That commit only removes tags from TTBR0 addresses,