| Message ID | cover.1544445454.git.andreyknvl@google.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | arm64: untag user pointers passed to the kernel | expand |
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. Do you have an idea of how much of the user/kernel interface is covered by this workload? > 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. There may already be some background on these topics -- can you throw me a link if so? Cheers ---Dave
On Wed, Dec 12, 2018 at 6:01 PM Dave Martin <Dave.Martin@arm.com> 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!
Hi Dave, On Wed, Dec 12, 2018 at 05:01:12PM +0000, Dave P 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. [...] > 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. In summary, based on last summer's analysis, there are two main (and rather broad) scenarios of __user pointers use in the kernel: (a) uaccess macros, together with access_ok() checks and (b) identifying of user address ranges (find_vma() and related, some ioctls). The patches here handle the former by allowing sign-extension in access_ok() and subsequent uaccess routines work fine with tagged pointers. Identifying the latter is a bit more problematic and the approach we took was tracking down pointer to long conversion which seems to cover the majority of cases. However, this approach doesn't scale as, for example, we'd rather change get_user_pages() to sign-extend the address rather than all the callers. In lots of other cases we don't even need untagging as we don't expect user space to tag such pointers (i.e. mmap() of device memory). We might be able to improve the static analysis by introducing a virt_addr_t but that's significant effort and we still won't cover all cases (e.g. it doesn't necessarily catch tcp_zerocopy_receive() which wouldn't use a pointer, just a u64 for address). > 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 agree. > 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)? Defining an opt-in is not a problem, however, rejecting all syscalls that we haven't whitelisted is not feasible. We can have an opt-in per process (that's what we were going to do with MTE) but the only thing we can reasonably do is change the behaviour of access_ok(). That's too big a knob and a new syscall that we haven't got around to whitelist may just work. This eventually leads to de-facto ABI and our whitelist would simply be ignored. I'm not really keen on a big syscall shim in the arm64 kernel which checks syscall arguments, including in-struct values. If we are to do this, I'd rather keep it in user space as part of the C library. > 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) Without these patches, passing non-address bits in pointers is considered broken. I couldn't find a case where it would still work with non-zero tag but maybe I haven't looked hard enough. > * 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. That's what we are aiming with the pointer origins, to move away from a syscall whitelist to a generic definition. That said, the two approaches are orthogonal, we can use the pointer origins as the base rule for which syscalls can be whitelisted. If we step back a bit to look at the use-case for TBI (and MTE), the normal application programmer shouldn't really care about this ABI (well, most of the time). The app gets a tagged pointer from the C library as a result of a malloc()/realloc() (possibly alloca()) call and it expects to be able to pass it back into the kernel (usually via the C library) without any awareness of the non-address bits. Now, we can't define a user/kernel ABI based on the provenance of the pointer in user space (i.e. we only support tags for heap and stack), so we are trying to generalise this based where the pointer originated from in the kernel (e.g. anonymous mmap()). > There may already be some background on these topics -- can you throw me > a link if so? That's an interesting sub-thread to read: https://lore.kernel.org/lkml/5d54526e5ff2e5ad63d0dfdd9ab17cf359afa4f2.1535629099.git.andreyknvl@google.com/
On Mon, Feb 11, 2019 at 11:35:12AM +0000, Catalin Marinas wrote: > Hi Dave, > > On Wed, Dec 12, 2018 at 05:01:12PM +0000, Dave P 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. > [...] > > 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. > > In summary, based on last summer's analysis, there are two main (and > rather broad) scenarios of __user pointers use in the kernel: (a) > uaccess macros, together with access_ok() checks and (b) identifying > of user address ranges (find_vma() and related, some ioctls). The > patches here handle the former by allowing sign-extension in access_ok() > and subsequent uaccess routines work fine with tagged pointers. > Identifying the latter is a bit more problematic and the approach we > took was tracking down pointer to long conversion which seems to cover > the majority of cases. However, this approach doesn't scale as, for > example, we'd rather change get_user_pages() to sign-extend the address > rather than all the callers. In lots of other cases we don't even need > untagging as we don't expect user space to tag such pointers (i.e. > mmap() of device memory). > > We might be able to improve the static analysis by introducing a > virt_addr_t but that's significant effort and we still won't cover all > cases (e.g. it doesn't necessarily catch tcp_zerocopy_receive() which > wouldn't use a pointer, just a u64 for address). > > > 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 agree. > > > 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)? > > Defining an opt-in is not a problem, however, rejecting all syscalls > that we haven't whitelisted is not feasible. We can have an opt-in per > process (that's what we were going to do with MTE) but the only thing > we can reasonably do is change the behaviour of access_ok(). That's too > big a knob and a new syscall that we haven't got around to whitelist may > just work. This eventually leads to de-facto ABI and our whitelist would > simply be ignored. > > I'm not really keen on a big syscall shim in the arm64 kernel which > checks syscall arguments, including in-struct values. If we are to do > this, I'd rather keep it in user space as part of the C library. > > > 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) > > Without these patches, passing non-address bits in pointers is > considered broken. I couldn't find a case where it would still work with > non-zero tag but maybe I haven't looked hard enough. > > > * 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. > > That's what we are aiming with the pointer origins, to move away from a > syscall whitelist to a generic definition. That said, the two approaches > are orthogonal, we can use the pointer origins as the base rule for > which syscalls can be whitelisted. > > If we step back a bit to look at the use-case for TBI (and MTE), the > normal application programmer shouldn't really care about this ABI > (well, most of the time). The app gets a tagged pointer from the C > library as a result of a malloc()/realloc() (possibly alloca()) call and > it expects to be able to pass it back into the kernel (usually via the C > library) without any awareness of the non-address bits. Now, we can't > define a user/kernel ABI based on the provenance of the pointer in user > space (i.e. we only support tags for heap and stack), so we are trying > to generalise this based where the pointer originated from in the kernel > (e.g. anonymous mmap()). This sounds generally reasonable. It is not adequate for describing changing the tag on already-tagged memory (which a memory allocator will definitely do), but we may be able to come up with some weasel words to cover that. It is also not adequete for describing tagging (and retagging) regions of the stack -- but as you say, we can rule that use-case out for now in the interest of simplicity, since we know we wouldn't be able to deploy it widely for now anyway due to the incompability with non-MTE- capable hardware. Ideally we would clarify user/kernel interface semantics in terms of object and pointer lifetimes and accessibility, but that's a larger project that should be pursued separately (if at all). I could also quibble about whether "anonymous mmap" is the right thing here -- we should still give specific examples of things that do / don't qualify, to make it clear what we mean. Cheers ---Dave
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. This patchset is a prerequisite for ARM's memory tagging hardware feature support [3]. Thanks! [1] http://clang.llvm.org/docs/HardwareAssistedAddressSanitizerDesign.html [2] https://github.com/lucvoo/sparse-dev/commit/5f960cb10f56ec2017c128ef9d16060e0145f292 [3] https://community.arm.com/processors/b/blog/posts/arm-a-profile-architecture-2018-developments-armv85a Changes in v9: - Rebased onto 4.20-rc6. - Used u64 instead of __u64 in type casts in the untagged_addr macro for arm64. - Added braces around (addr) in the untagged_addr macro for other arches. Changes in v8: - Rebased onto 65102238 (4.20-rc1). - Added a note to the cover letter on why syscall wrappers/shims that untag user pointers won't work. - Added a note to the cover letter that this patchset has been merged into the Pixel 2 kernel tree. - Documentation fixes, in particular added a list of syscalls that don't support tagged user pointers. Changes in v7: - Rebased onto 17b57b18 (4.19-rc6). - Dropped the "arm64: untag user address in __do_user_fault" patch, since the existing patches already handle user faults properly. - Dropped the "usb, arm64: untag user addresses in devio" patch, since the passed pointer must come from a vma and therefore be untagged. - Dropped the "arm64: annotate user pointers casts detected by sparse" patch (see the discussion to the replies of the v6 of this patchset). - Added more context to the cover letter. - Updated Documentation/arm64/tagged-pointers.txt. Changes in v6: 1 From 502466b9652c57a23af3bd72124144319212f30b Mon Sep 17 00:00:00 2001 - Added annotations for user pointer casts found by sparse. 1 From 502466b9652c57a23af3bd72124144319212f30b Mon Sep 17 00:00:00 2001 - Rebased onto 050cdc6c (4.19-rc1+). 1 From 502466b9652c57a23af3bd72124144319212f30b Mon Sep 17 00:00:00 2001 Changes in v5: - Added 3 new patches that add untagging to places found with static analysis. - Rebased onto 44c929e1 (4.18-rc8). Changes in v4: - Added a selftest for checking that passing tagged pointers to the kernel succeeds. - Rebased onto 81e97f013 (4.18-rc1+). Changes in v3: - Rebased onto e5c51f30 (4.17-rc6+). - Added linux-arch@ to the list of recipients. Changes in v2: - Rebased onto 2d618bdf (4.17-rc3+). - Removed excessive untagging in gup.c. - Removed untagging pointers returned from __uaccess_mask_ptr. Changes in v1: - Rebased onto 4.17-rc1. Changes in RFC v2: - Added "#ifndef untagged_addr..." fallback in linux/uaccess.h instead of defining it for each arch individually. - Updated Documentation/arm64/tagged-pointers.txt. - Dropped "mm, arm64: untag user addresses in memory syscalls". - Rebased onto 3eb2ce82 (4.16-rc7). Reviewed-by: Luc Van Oostenryck <luc.vanoostenryck@gmail.com> Signed-off-by: Andrey Konovalov <andreyknvl@google.com> Andrey Konovalov (8): arm64: add type casts to untagged_addr macro uaccess: add untagged_addr definition for other arches arm64: untag user addresses in access_ok and __uaccess_mask_ptr mm, arm64: untag user addresses in mm/gup.c lib, arm64: untag addrs passed to strncpy_from_user and strnlen_user fs, arm64: untag user address in copy_mount_options arm64: update Documentation/arm64/tagged-pointers.txt selftests, arm64: add a selftest for passing tagged pointers to kernel Documentation/arm64/tagged-pointers.txt | 25 +++++++++++-------- arch/arm64/include/asm/uaccess.h | 14 +++++++---- fs/namespace.c | 2 +- include/linux/uaccess.h | 4 +++ lib/strncpy_from_user.c | 2 ++ lib/strnlen_user.c | 2 ++ mm/gup.c | 4 +++ tools/testing/selftests/arm64/.gitignore | 1 + tools/testing/selftests/arm64/Makefile | 11 ++++++++ .../testing/selftests/arm64/run_tags_test.sh | 12 +++++++++ tools/testing/selftests/arm64/tags_test.c | 19 ++++++++++++++ 11 files changed, 80 insertions(+), 16 deletions(-) create mode 100644 tools/testing/selftests/arm64/.gitignore create mode 100644 tools/testing/selftests/arm64/Makefile create mode 100755 tools/testing/selftests/arm64/run_tags_test.sh create mode 100644 tools/testing/selftests/arm64/tags_test.c