From mboxrd@z Thu Jan 1 00:00:00 1970 From: Andrey Konovalov Subject: Re: [PATCH v9 0/8] arm64: untag user pointers passed to the kernel Date: Tue, 18 Dec 2018 18:17:31 +0100 Message-ID: References: <20181212170108.GZ3505@e103592.cambridge.arm.com> Mime-Version: 1.0 Content-Type: text/plain; charset="UTF-8" Return-path: In-Reply-To: <20181212170108.GZ3505@e103592.cambridge.arm.com> Sender: linux-kernel-owner@vger.kernel.org To: Dave Martin Cc: Catalin Marinas , Will Deacon , Mark Rutland , Robin Murphy , Kees Cook , Kate Stewart , Greg Kroah-Hartman , Andrew Morton , Ingo Molnar , "Kirill A. Shutemov" , Shuah Khan , Linux ARM , "open list:DOCUMENTATION" , Linux Memory Management List , linux-arch , "open list:KERNEL SELFTEST FRAMEWORK" , LKML C List-Id: linux-arch.vger.kernel.org On Wed, Dec 12, 2018 at 6:01 PM Dave Martin wrote: > > On Mon, Dec 10, 2018 at 01:50:57PM +0100, Andrey Konovalov wrote: > > arm64 has a feature called Top Byte Ignore, which allows to embed pointer > > tags into the top byte of each pointer. Userspace programs (such as > > HWASan, a memory debugging tool [1]) might use this feature and pass > > tagged user pointers to the kernel through syscalls or other interfaces. > > > > Right now the kernel is already able to handle user faults with tagged > > pointers, due to these patches: > > > > 1. 81cddd65 ("arm64: traps: fix userspace cache maintenance emulation on a > > tagged pointer") > > 2. 7dcd9dd8 ("arm64: hw_breakpoint: fix watchpoint matching for tagged > > pointers") > > 3. 276e9327 ("arm64: entry: improve data abort handling of tagged > > pointers") > > > > When passing tagged pointers to syscalls, there's a special case of such a > > pointer being passed to one of the memory syscalls (mmap, mprotect, etc.). > > These syscalls don't do memory accesses but rather deal with memory > > ranges, hence an untagged pointer is better suited. > > > > This patchset extends tagged pointer support to non-memory syscalls. This > > is done by reusing the untagged_addr macro to untag user pointers when the > > kernel performs pointer checking to find out whether the pointer comes > > from userspace (most notably in access_ok). The untagging is done only > > when the pointer is being checked, the tag is preserved as the pointer > > makes its way through the kernel. > > > > One of the alternative approaches to untagging that was considered is to > > completely strip the pointer tag as the pointer enters the kernel with > > some kind of a syscall wrapper, but that won't work with the countless > > number of different ioctl calls. With this approach we would need a custom > > wrapper for each ioctl variation, which doesn't seem practical. > > > > The following testing approaches has been taken to find potential issues > > with user pointer untagging: > > > > 1. Static testing (with sparse [2] and separately with a custom static > > analyzer based on Clang) to track casts of __user pointers to integer > > types to find places where untagging needs to be done. > > > > 2. Dynamic testing: adding BUG_ON(has_tag(addr)) to find_vma() and running > > a modified syzkaller version that passes tagged pointers to the kernel. > > > > Based on the results of the testing the requried patches have been added > > to the patchset. > > > > This patchset has been merged into the Pixel 2 kernel tree and is now > > being used to enable testing of Pixel 2 phones with HWASan. Hi, Dave, > > Do you have an idea of how much of the user/kernel interface is covered > by this workload? Not really. I don't even know what kind of measurements can be used to obtain this estimate. But Pixel 2 kernel with these patches + Android runtime instrumented with HWASan works. > > > This patchset is a prerequisite for ARM's memory tagging hardware feature > > support [3]. > > It looks like there's been a lot of progress made here towards smoking > out most of the sites in the kernel where pointers need to be untagged. > > However, I do think that we need a clear policy for how existing kernel > interfaces are to be interpreted in the presence of tagged pointers. > Unless we have that nailed down, we are likely to be able to make only > vague guarantees to userspace about what works, and the ongoing risk > of ABI regressions and inconsistencies seems high. > > I don't really see how we can advertise a full system interface if we > know some subset of it doesn't work for foreseeable userspace > environments. I feel that presenting the current changes as an ABI > relaxation may be a recipe for future problems, since the forwards > compatibility guarantees we're able to make today are few and rather > vague. > > Can we define an opt-in for tagged-pointer userspace, that rejects all > syscalls that we haven't checked and whitelisted (or that are > uncheckable like ioctl)? This reflects the reality that we don't have > a regular userspace environment in which standards-compliant software > that uses address tags in a reasonable way will just work. > > It might be feasible to promote this to be enabled by default later on, > if it becomes sufficiently complete. > > > In the meantime, I think we really need to nail down the kernel's > policies on > > * in the default configuration (without opt-in), is the presence of > non-address bits in pointers exchanged with the kernel simply > considered broken? (Even with this series, the de factor answer > generally seems to be "yes", although many specific things will now > work fine) > > * if not, how do we tighten syscall / interface specifications to > describe what happens with pointers containing non-address bits, while > keeping the existing behaviour for untagged pointers? > > We would want a general recipe that gives clear guidance on what > userspace should expect an arbitrarily chosen syscall to do with its > pointers, without having to enumerate each and every case. > > To be sustainable, we would also need to solve that in a way that > doesn't need to be reintented per-arch. As I understand your main concern is userspace/kernel ABI changes these patches introduce. This concern was already pointed out by Catalin, and working out the details is still in progress. > > There may already be some background on these topics -- can you throw me > a link if so? I don't have a single link, I would suggest to look at the comments for all the previous versions of this patchset. I see you saw the pathset by Vincenzo, it also has some information about this. > > Cheers > ---Dave Thanks! From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-pl1-f193.google.com ([209.85.214.193]:37865 "EHLO mail-pl1-f193.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1727005AbeLRRRo (ORCPT ); Tue, 18 Dec 2018 12:17:44 -0500 Received: by mail-pl1-f193.google.com with SMTP id b5so8122406plr.4 for ; Tue, 18 Dec 2018 09:17:44 -0800 (PST) MIME-Version: 1.0 References: <20181212170108.GZ3505@e103592.cambridge.arm.com> In-Reply-To: <20181212170108.GZ3505@e103592.cambridge.arm.com> From: Andrey Konovalov Date: Tue, 18 Dec 2018 18:17:31 +0100 Message-ID: Subject: Re: [PATCH v9 0/8] arm64: untag user pointers passed to the kernel Content-Type: text/plain; charset="UTF-8" Sender: linux-arch-owner@vger.kernel.org List-ID: To: Dave Martin Cc: Catalin Marinas , Will Deacon , Mark Rutland , Robin Murphy , Kees Cook , Kate Stewart , Greg Kroah-Hartman , Andrew Morton , Ingo Molnar , "Kirill A. Shutemov" , Shuah Khan , Linux ARM , "open list:DOCUMENTATION" , Linux Memory Management List , linux-arch , "open list:KERNEL SELFTEST FRAMEWORK" , LKML , Chintan Pandya , Jacob Bramley , Ruben Ayrapetyan , Lee Smith , Kostya Serebryany , Dmitry Vyukov , Ramana Radhakrishnan , Luc Van Oostenryck , Evgenii Stepanov , Vincenzo Frascino Message-ID: <20181218171731.-05g_HaK--xc5SoCnShkLpiYyH-79TP8u-82C9-NH2o@z> On Wed, Dec 12, 2018 at 6:01 PM Dave Martin wrote: > > On Mon, Dec 10, 2018 at 01:50:57PM +0100, Andrey Konovalov wrote: > > arm64 has a feature called Top Byte Ignore, which allows to embed pointer > > tags into the top byte of each pointer. Userspace programs (such as > > HWASan, a memory debugging tool [1]) might use this feature and pass > > tagged user pointers to the kernel through syscalls or other interfaces. > > > > Right now the kernel is already able to handle user faults with tagged > > pointers, due to these patches: > > > > 1. 81cddd65 ("arm64: traps: fix userspace cache maintenance emulation on a > > tagged pointer") > > 2. 7dcd9dd8 ("arm64: hw_breakpoint: fix watchpoint matching for tagged > > pointers") > > 3. 276e9327 ("arm64: entry: improve data abort handling of tagged > > pointers") > > > > When passing tagged pointers to syscalls, there's a special case of such a > > pointer being passed to one of the memory syscalls (mmap, mprotect, etc.). > > These syscalls don't do memory accesses but rather deal with memory > > ranges, hence an untagged pointer is better suited. > > > > This patchset extends tagged pointer support to non-memory syscalls. This > > is done by reusing the untagged_addr macro to untag user pointers when the > > kernel performs pointer checking to find out whether the pointer comes > > from userspace (most notably in access_ok). The untagging is done only > > when the pointer is being checked, the tag is preserved as the pointer > > makes its way through the kernel. > > > > One of the alternative approaches to untagging that was considered is to > > completely strip the pointer tag as the pointer enters the kernel with > > some kind of a syscall wrapper, but that won't work with the countless > > number of different ioctl calls. With this approach we would need a custom > > wrapper for each ioctl variation, which doesn't seem practical. > > > > The following testing approaches has been taken to find potential issues > > with user pointer untagging: > > > > 1. Static testing (with sparse [2] and separately with a custom static > > analyzer based on Clang) to track casts of __user pointers to integer > > types to find places where untagging needs to be done. > > > > 2. Dynamic testing: adding BUG_ON(has_tag(addr)) to find_vma() and running > > a modified syzkaller version that passes tagged pointers to the kernel. > > > > Based on the results of the testing the requried patches have been added > > to the patchset. > > > > This patchset has been merged into the Pixel 2 kernel tree and is now > > being used to enable testing of Pixel 2 phones with HWASan. Hi, Dave, > > Do you have an idea of how much of the user/kernel interface is covered > by this workload? Not really. I don't even know what kind of measurements can be used to obtain this estimate. But Pixel 2 kernel with these patches + Android runtime instrumented with HWASan works. > > > This patchset is a prerequisite for ARM's memory tagging hardware feature > > support [3]. > > It looks like there's been a lot of progress made here towards smoking > out most of the sites in the kernel where pointers need to be untagged. > > However, I do think that we need a clear policy for how existing kernel > interfaces are to be interpreted in the presence of tagged pointers. > Unless we have that nailed down, we are likely to be able to make only > vague guarantees to userspace about what works, and the ongoing risk > of ABI regressions and inconsistencies seems high. > > I don't really see how we can advertise a full system interface if we > know some subset of it doesn't work for foreseeable userspace > environments. I feel that presenting the current changes as an ABI > relaxation may be a recipe for future problems, since the forwards > compatibility guarantees we're able to make today are few and rather > vague. > > Can we define an opt-in for tagged-pointer userspace, that rejects all > syscalls that we haven't checked and whitelisted (or that are > uncheckable like ioctl)? This reflects the reality that we don't have > a regular userspace environment in which standards-compliant software > that uses address tags in a reasonable way will just work. > > It might be feasible to promote this to be enabled by default later on, > if it becomes sufficiently complete. > > > In the meantime, I think we really need to nail down the kernel's > policies on > > * in the default configuration (without opt-in), is the presence of > non-address bits in pointers exchanged with the kernel simply > considered broken? (Even with this series, the de factor answer > generally seems to be "yes", although many specific things will now > work fine) > > * if not, how do we tighten syscall / interface specifications to > describe what happens with pointers containing non-address bits, while > keeping the existing behaviour for untagged pointers? > > We would want a general recipe that gives clear guidance on what > userspace should expect an arbitrarily chosen syscall to do with its > pointers, without having to enumerate each and every case. > > To be sustainable, we would also need to solve that in a way that > doesn't need to be reintented per-arch. As I understand your main concern is userspace/kernel ABI changes these patches introduce. This concern was already pointed out by Catalin, and working out the details is still in progress. > > There may already be some background on these topics -- can you throw me > a link if so? I don't have a single link, I would suggest to look at the comments for all the previous versions of this patchset. I see you saw the pathset by Vincenzo, it also has some information about this. > > Cheers > ---Dave Thanks!