From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-9.0 required=3.0 tests=HEADER_FROM_DIFFERENT_DOMAINS, INCLUDES_PATCH,MAILING_LIST_MULTI,SIGNED_OFF_BY,SPF_PASS,USER_AGENT_NEOMUTT autolearn=ham autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 87EFEC43441 for ; Wed, 14 Nov 2018 20:17:45 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.kernel.org (Postfix) with ESMTP id 4BC392251D for ; Wed, 14 Nov 2018 20:17:45 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 4BC392251D Authentication-Results: mail.kernel.org; dmarc=none (p=none dis=none) header.from=arm.com Authentication-Results: mail.kernel.org; spf=none smtp.mailfrom=linux-kernel-owner@vger.kernel.org Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1728683AbeKOGWY (ORCPT ); Thu, 15 Nov 2018 01:22:24 -0500 Received: from usa-sjc-mx-foss1.foss.arm.com ([217.140.101.70]:50526 "EHLO foss.arm.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1725756AbeKOGWW (ORCPT ); Thu, 15 Nov 2018 01:22:22 -0500 Received: from usa-sjc-imap-foss1.foss.arm.com (unknown [10.72.51.249]) by usa-sjc-mx-foss1.foss.arm.com (Postfix) with ESMTP id 252D080D; Wed, 14 Nov 2018 12:17:42 -0800 (PST) Received: from blommer (usa-sjc-mx-foss1.foss.arm.com [217.140.101.70]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id E35AF3F718; Wed, 14 Nov 2018 12:17:40 -0800 (PST) Date: Wed, 14 Nov 2018 20:17:39 +0000 From: Mark Rutland To: Andrey Konovalov 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 Subject: Re: [PATCH v10 12/22] kasan, arm64: fix up fault handling logic Message-ID: <20181114201738.sb2lla7umljsx3qx@blommer> References: <4891a504adf61c0daf1e83642b6f7519328dfd5f.1541525354.git.andreyknvl@google.com> <20181108122228.xqwhpkjritrvqneq@lakrids.cambridge.arm.com> <20181113220728.2h3kz67b2bz36wty@blommer> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: User-Agent: NeoMutt/20170113 (1.7.2) Sender: linux-kernel-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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. Thanks, Mark. From mboxrd@z Thu Jan 1 00:00:00 1970 From: Mark Rutland Subject: Re: [PATCH v10 12/22] kasan, arm64: fix up fault handling logic Date: Wed, 14 Nov 2018 20:17:39 +0000 Message-ID: <20181114201738.sb2lla7umljsx3qx@blommer> References: <4891a504adf61c0daf1e83642b6f7519328dfd5f.1541525354.git.andreyknvl@google.com> <20181108122228.xqwhpkjritrvqneq@lakrids.cambridge.arm.com> <20181113220728.2h3kz67b2bz36wty@blommer> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Return-path: Content-Disposition: inline In-Reply-To: Sender: linux-kernel-owner@vger.kernel.org To: Andrey Konovalov 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 List-Id: linux-sparse@vger.kernel.org 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. Thanks, Mark. From mboxrd@z Thu Jan 1 00:00:00 1970 From: mark.rutland@arm.com (Mark Rutland) Date: Wed, 14 Nov 2018 20:17:39 +0000 Subject: [PATCH v10 12/22] kasan, arm64: fix up fault handling logic In-Reply-To: References: <4891a504adf61c0daf1e83642b6f7519328dfd5f.1541525354.git.andreyknvl@google.com> <20181108122228.xqwhpkjritrvqneq@lakrids.cambridge.arm.com> <20181113220728.2h3kz67b2bz36wty@blommer> Message-ID: <20181114201738.sb2lla7umljsx3qx@blommer> To: linux-arm-kernel@lists.infradead.org List-Id: linux-arm-kernel.lists.infradead.org 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. Thanks, Mark.