From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-it1-f198.google.com (mail-it1-f198.google.com [209.85.166.198]) by kanga.kvack.org (Postfix) with ESMTP id 433546B000D for ; Tue, 13 Nov 2018 10:01:30 -0500 (EST) Received: by mail-it1-f198.google.com with SMTP id p78-v6so17244183itb.1 for ; Tue, 13 Nov 2018 07:01:30 -0800 (PST) Received: from mail-sor-f65.google.com (mail-sor-f65.google.com. [209.85.220.65]) by mx.google.com with SMTPS id c188-v6sor20368679itc.35.2018.11.13.07.01.28 for (Google Transport Security); Tue, 13 Nov 2018 07:01:28 -0800 (PST) MIME-Version: 1.0 In-Reply-To: <20181108122228.xqwhpkjritrvqneq@lakrids.cambridge.arm.com> References: <4891a504adf61c0daf1e83642b6f7519328dfd5f.1541525354.git.andreyknvl@google.com> <20181108122228.xqwhpkjritrvqneq@lakrids.cambridge.arm.com> From: Andrey Konovalov Date: Tue, 13 Nov 2018 16:01:27 +0100 Message-ID: Subject: Re: [PATCH v10 12/22] kasan, arm64: fix up fault handling logic Content-Type: text/plain; charset="UTF-8" Sender: owner-linux-mm@kvack.org List-ID: To: Mark Rutland Cc: Andrey Ryabinin , Alexander Potapenko , Dmitry Vyukov , Catalin Marinas , Will Deacon , Christoph Lameter , Andrew Morton , Nick Desaulniers , Marc Zyngier , Dave Martin , Ard Biesheuvel , "Eric W . Biederman" , Ingo Molnar , Paul Lawrence , Geert Uytterhoeven , Arnd Bergmann , "Kirill A . Shutemov" , Greg Kroah-Hartman , Kate Stewart , Mike Rapoport , kasan-dev@googlegroups.com, "open list:DOCUMENTATION" , LKML , Linux ARM , linux-sparse@vger.kernel.org, Linux Memory Management List , Linux Kbuild mailing list , Kostya Serebryany , Evgeniy Stepanov , Lee Smith , Ramana Radhakrishnan , Jacob Bramley , Ruben Ayrapetyan , Jann Horn , Mark Brand , Chintan Pandya , Vishwath Mohan On Thu, Nov 8, 2018 at 1:22 PM, Mark Rutland 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 >> Reviewed-by: Dmitry Vyukov >> Signed-off-by: Andrey Konovalov >> --- >> 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 >> #include >> #include >> +#include >> >> #include >> #include >> @@ -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.