From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-it1-f199.google.com (mail-it1-f199.google.com [209.85.166.199]) by kanga.kvack.org (Postfix) with ESMTP id 425626B0320 for ; Thu, 15 Nov 2018 08:33:05 -0500 (EST) Received: by mail-it1-f199.google.com with SMTP id p73-v6so23603358itb.7 for ; Thu, 15 Nov 2018 05:33:05 -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 d68sor3448806iof.126.2018.11.15.05.33.03 for (Google Transport Security); Thu, 15 Nov 2018 05:33:03 -0800 (PST) MIME-Version: 1.0 In-Reply-To: <20181114201738.sb2lla7umljsx3qx@blommer> References: <4891a504adf61c0daf1e83642b6f7519328dfd5f.1541525354.git.andreyknvl@google.com> <20181108122228.xqwhpkjritrvqneq@lakrids.cambridge.arm.com> <20181113220728.2h3kz67b2bz36wty@blommer> <20181114201738.sb2lla7umljsx3qx@blommer> From: Andrey Konovalov Date: Thu, 15 Nov 2018 14:33:02 +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 Wed, Nov 14, 2018 at 9:17 PM, Mark Rutland wrote: > On Wed, Nov 14, 2018 at 09:06:23PM +0100, Andrey Konovalov wrote: >> On Tue, Nov 13, 2018 at 11:07 PM, Mark Rutland wrote: >> > On Tue, Nov 13, 2018 at 04:01:27PM +0100, Andrey Konovalov wrote: >> >> 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. >> > >> > 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). >> >> Do I understand correctly that TTBR0 means user space addresses and >> TTBR1 means kernel addresses? > > Effectively, yes. The address space is split into two halves (with a gap in the > middle). The high half (where we map the kernel) is covered by TTBR1, and the > low half (where we map userspace) is covered by TTBR0. > > The TTBRs are the Translation Table Base Registers -- the two halves have > separate page tables. > >> In that commit I see that the clear_address_tag() macro is used in el0_da and >> in el1_da, which means that it untags both user and kernel addresses (on data >> aborts). Do I misunderstand something? > > It's called for faults taken from EL0 and EL1, but it only removes the tags > from addresses covered by TTBR0. The logic is: > > .macro clear_address_tag, dst, addr > tst \addr, #(1 << 55) > bic \dst, \addr, #(0xff << 56) > csel \dst, \dst, \addr, eq > .endm > > ... which in C would be: > > if (!(addr & (1UL << 55))) { > addr &= ~(0xffUL << 56); > } > > ... and therefore does not affect TTBR1 addresses. Got it, will fix in v11, thanks!