[RFC PATCH 0/6] arm64: untag user pointers passed to the kernel

[RFC PATCH 0/6] arm64: untag user pointers passed to the kernel

[Mark Rutland]

Hi,

[trimming Ccs]

On Fri, Mar 09, 2018 at 03:01:58PM +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.
> 
> This patch makes a few of the kernel interfaces accept tagged user
> pointers. The kernel is already able to handle user faults with tagged
> pointers and has the untagged_addr macro, which this patchset reuses.
> 
> We're not trying to cover all possible ways the kernel accepts user
> pointers in one patchset, so this one should be considered as a start.
> It would be nice to learn about the interfaces that I missed though.

There are many ways that user pointers can be passed to the kernel, and
I'm not sure that it's feasible to catch them all, especially as user
pointers are often passed in data structures (e.g. iovecs) rather than
direct syscall arguments.

If we *really* want the kernel to support taking tagged addresses, anything
with a __user annotation (or cast to something with a __user annotation)
requires tag removal somewhere in the kernel.

It looks like there are plenty uapi structures and syscalls to look at:

[mark at lakrids:~/src/linux]% git grep __user -- include/uapi | wc -l
216
[mark at lakrids:~/src/linux]% git grep __user | grep SYSCALL_DEFINE | wc -l
308

... in addition to special syscalls like ioctl which multiplex a number
of operations with different arguments, where the tag stripping would
have to occur elsewhere (e.g. in particular drivers).

I also wonder if we ever write any of these pointers back to userspace
memory. If so, we have a nasty ABI problem, since we'll have to marshal
the original tag along with the pointer, to ensure userspace pointer
comparisons continue to work.

Thanks,
Mark.

[RFC PATCH 0/6] arm64: untag user pointers passed to the kernel

[Geert Uytterhoeven]

On Fri, Mar 9, 2018 at 3:55 PM, Mark Rutland <mark.rutland@arm.com> wrote:
> On Fri, Mar 09, 2018 at 03:01:58PM +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.
>>
>> This patch makes a few of the kernel interfaces accept tagged user
>> pointers. The kernel is already able to handle user faults with tagged
>> pointers and has the untagged_addr macro, which this patchset reuses.
>>
>> We're not trying to cover all possible ways the kernel accepts user
>> pointers in one patchset, so this one should be considered as a start.
>> It would be nice to learn about the interfaces that I missed though.
>
> There are many ways that user pointers can be passed to the kernel, and
> I'm not sure that it's feasible to catch them all, especially as user
> pointers are often passed in data structures (e.g. iovecs) rather than
> direct syscall arguments.
>
> If we *really* want the kernel to support taking tagged addresses, anything
> with a __user annotation (or cast to something with a __user annotation)
> requires tag removal somewhere in the kernel.
>
> It looks like there are plenty uapi structures and syscalls to look at:
>
> [mark at lakrids:~/src/linux]% git grep __user -- include/uapi | wc -l
> 216
> [mark at lakrids:~/src/linux]% git grep __user | grep SYSCALL_DEFINE | wc -l
> 308
>
> ... in addition to special syscalls like ioctl which multiplex a number
> of operations with different arguments, where the tag stripping would
> have to occur elsewhere (e.g. in particular drivers).

Hence we definitely need good support from e.g. sparse to catch any errors
from happening.

So doing assignments to the same pointer variable like

     addr = untagged_addr(addr);

defeats any checking.

> I also wonder if we ever write any of these pointers back to userspace
> memory. If so, we have a nasty ABI problem, since we'll have to marshal
> the original tag along with the pointer, to ensure userspace pointer
> comparisons continue to work.

Oh, another can of worms...

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert at linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

[RFC PATCH 0/6] arm64: untag user pointers passed to the kernel

[Andrey Konovalov]

On Fri, Mar 9, 2018 at 3:55 PM, Mark Rutland <mark.rutland@arm.com> wrote:
> Hi,
>
> [trimming Ccs]
>
> On Fri, Mar 09, 2018 at 03:01:58PM +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.
>>
>> This patch makes a few of the kernel interfaces accept tagged user
>> pointers. The kernel is already able to handle user faults with tagged
>> pointers and has the untagged_addr macro, which this patchset reuses.
>>
>> We're not trying to cover all possible ways the kernel accepts user
>> pointers in one patchset, so this one should be considered as a start.
>> It would be nice to learn about the interfaces that I missed though.
>
> There are many ways that user pointers can be passed to the kernel, and
> I'm not sure that it's feasible to catch them all, especially as user
> pointers are often passed in data structures (e.g. iovecs) rather than
> direct syscall arguments.
>
> If we *really* want the kernel to support taking tagged addresses, anything
> with a __user annotation (or cast to something with a __user annotation)
> requires tag removal somewhere in the kernel.
>
> It looks like there are plenty uapi structures and syscalls to look at:
>
> [mark at lakrids:~/src/linux]% git grep __user -- include/uapi | wc -l
> 216
> [mark at lakrids:~/src/linux]% git grep __user | grep SYSCALL_DEFINE | wc -l
> 308
>
> ... in addition to special syscalls like ioctl which multiplex a number
> of operations with different arguments, where the tag stripping would
> have to occur elsewhere (e.g. in particular drivers).
>
> I also wonder if we ever write any of these pointers back to userspace
> memory. If so, we have a nasty ABI problem, since we'll have to marshal
> the original tag along with the pointer, to ensure userspace pointer
> comparisons continue to work.
>
> Thanks,
> Mark.

Hi Mark!

This seems to be similar to what you said in reply to one of the other
patches, replied there.

Thanks!

[RFC PATCH 2/6] arm64: untag user addresses in copy_from_user and others

[Mark Rutland]

On Fri, Mar 09, 2018 at 03:02:00PM +0100, Andrey Konovalov wrote:
> copy_from_user (and a few other similar functions) are used to copy data
> from user memory into the kernel memory or vice versa. Since a user can
> provided a tagged pointer to one of the syscalls that use copy_from_user,
> we need to correctly handle such pointers.

I don't think it makes sense to do this in the low-level uaccess
primitives, given we're going to have to untag pointers before common
code can use them, e.g. for comparisons against TASK_SIZE or
user_addr_max().

I think we'll end up with subtle bugs unless we consistently untag
pointers before we get to uaccess primitives. If core code does untag
pointers, then it's redundant to do so here.

Thanks,
Mark.

> 
> Do this by untagging user pointers in access_ok and in __uaccess_mask_ptr.
> 
> Signed-off-by: Andrey Konovalov <andreyknvl@google.com>
> ---
>  arch/arm64/include/asm/uaccess.h | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/arm64/include/asm/uaccess.h b/arch/arm64/include/asm/uaccess.h
> index 2d6451cbaa86..24a221678fe3 100644
> --- a/arch/arm64/include/asm/uaccess.h
> +++ b/arch/arm64/include/asm/uaccess.h
> @@ -105,7 +105,8 @@ static inline unsigned long __range_ok(const void __user *addr, unsigned long si
>  #define untagged_addr(addr)		\
>  	((__typeof__(addr))sign_extend64((__u64)(addr), 55))
>  
> -#define access_ok(type, addr, size)	__range_ok(addr, size)
> +#define access_ok(type, addr, size)	\
> +	__range_ok(untagged_addr(addr), size)
>  #define user_addr_max			get_fs
>  
>  #define _ASM_EXTABLE(from, to)						\
> @@ -238,12 +239,15 @@ static inline void uaccess_enable_not_uao(void)
>  /*
>   * Sanitise a uaccess pointer such that it becomes NULL if above the
>   * current addr_limit.
> + * Also untag user pointers that have the top byte tag set.
>   */
>  #define uaccess_mask_ptr(ptr) (__typeof__(ptr))__uaccess_mask_ptr(ptr)
>  static inline void __user *__uaccess_mask_ptr(const void __user *ptr)
>  {
>  	void __user *safe_ptr;
>  
> +	ptr = untagged_addr(ptr);
> +
>  	asm volatile(
>  	"	bics	xzr, %1, %2\n"
>  	"	csel	%0, %1, xzr, eq\n"
> -- 
> 2.16.2.395.g2e18187dfd-goog
> 

[RFC PATCH 2/6] arm64: untag user addresses in copy_from_user and others

[Catalin Marinas]

On Fri, Mar 09, 2018 at 03:03:09PM +0000, Mark Rutland wrote:
> On Fri, Mar 09, 2018 at 03:02:00PM +0100, Andrey Konovalov wrote:
> > copy_from_user (and a few other similar functions) are used to copy data
> > from user memory into the kernel memory or vice versa. Since a user can
> > provided a tagged pointer to one of the syscalls that use copy_from_user,
> > we need to correctly handle such pointers.
> 
> I don't think it makes sense to do this in the low-level uaccess
> primitives, given we're going to have to untag pointers before common
> code can use them, e.g. for comparisons against TASK_SIZE or
> user_addr_max().
> 
> I think we'll end up with subtle bugs unless we consistently untag
> pointers before we get to uaccess primitives. If core code does untag
> pointers, then it's redundant to do so here.

A quick "hack" below clears the tag on syscall entry (where the argument
is a __user pointer). However, we still have cases in core code where
the pointer is read from a structure or even passed as an unsigned long
as part of a command + argument (like in ptrace).

The "hack":

---------------------------------8<--------------------------
>From 6df503651f73c923d91eb695e56f977ddcc52d43 Mon Sep 17 00:00:00 2001
From: Catalin Marinas <catalin.marinas@arm.com>
Date: Tue, 6 Feb 2018 17:54:05 +0000
Subject: [PATCH] arm64: Allow user pointer tags to be passed into the kernel

The current tagged pointer ABI disallows the top byte of a user pointer
to be non-zero when invoking a syscall. This patch allows such pointer
to be passed into the kernel and the kernel will mask them out
automatically. Page-based syscall ABI (mmap, mprotect, madvise etc.)
expect the pointer tag to be 0 (see include/linux/syscalls.h for the ABI
functions taking __user pointers).

Signed-off-by: Catalin Marinas <catalin.marinas@arm.com>
---
 arch/arm64/include/asm/unistd.h | 9 +++++++++
 include/linux/syscalls.h        | 2 ++
 2 files changed, 11 insertions(+)

diff --git a/arch/arm64/include/asm/unistd.h b/arch/arm64/include/asm/unistd.h
index a0baa9af5487..cd68ad969e3a 100644
--- a/arch/arm64/include/asm/unistd.h
+++ b/arch/arm64/include/asm/unistd.h
@@ -53,3 +53,12 @@
 #endif
 
 #define NR_syscalls (__NR_syscalls)
+
+/* copied from arch/s390/ */
+#define __TYPE_IS_PTR(t) (!__builtin_types_compatible_p( \
+				typeof(0?(__force t)0:0ULL), u64))
+/* sign-extend bit 55 to mask out the pointer tag */
+#define __SC_CAST(t, a)						\
+	(__TYPE_IS_PTR(t)					\
+		? (__force t)((__s64)((__u64)a << 8) >> 8)	\
+		: (__force t)a)
diff --git a/include/linux/syscalls.h b/include/linux/syscalls.h
index a78186d826d7..279497207a31 100644
--- a/include/linux/syscalls.h
+++ b/include/linux/syscalls.h
@@ -105,7 +105,9 @@ union bpf_attr;
 #define __TYPE_IS_UL(t)	(__TYPE_AS(t, 0UL))
 #define __TYPE_IS_LL(t) (__TYPE_AS(t, 0LL) || __TYPE_AS(t, 0ULL))
 #define __SC_LONG(t, a) __typeof(__builtin_choose_expr(__TYPE_IS_LL(t), 0LL, 0L)) a
+#ifndef __SC_CAST
 #define __SC_CAST(t, a)	(__force t) a
+#endif
 #define __SC_ARGS(t, a)	a
 #define __SC_TEST(t, a) (void)BUILD_BUG_ON_ZERO(!__TYPE_IS_LL(t) && sizeof(t) > sizeof(long))
 
-- 
Catalin

[RFC PATCH 2/6] arm64: untag user addresses in copy_from_user and others

[Andrey Konovalov]

On Fri, Mar 9, 2018 at 4:58 PM, Catalin Marinas <catalin.marinas@arm.com> wrote:
> On Fri, Mar 09, 2018 at 03:03:09PM +0000, Mark Rutland wrote:
>> On Fri, Mar 09, 2018 at 03:02:00PM +0100, Andrey Konovalov wrote:
>> > copy_from_user (and a few other similar functions) are used to copy data
>> > from user memory into the kernel memory or vice versa. Since a user can
>> > provided a tagged pointer to one of the syscalls that use copy_from_user,
>> > we need to correctly handle such pointers.
>>
>> I don't think it makes sense to do this in the low-level uaccess
>> primitives, given we're going to have to untag pointers before common
>> code can use them, e.g. for comparisons against TASK_SIZE or
>> user_addr_max().
>>
>> I think we'll end up with subtle bugs unless we consistently untag
>> pointers before we get to uaccess primitives. If core code does untag
>> pointers, then it's redundant to do so here.

There are two different approaches to untagging the user pointers that I see:

1. Untag user pointers right after they are passed to the kernel.

While this might be possible for pointers that are passed to syscalls
as arguments (Catalin's "hack"), this leaves user pointers, that are
embedded into for example structs that are passed to the kernel. Since
there's no specification of the interface between user space and the
kernel, different kernel parts handle user pointers differently and I
don't see an easy way to cover them all.

2. Untag user pointers where they are used in the kernel.

Although there's no specification on the interface between the user
space and the kernel, the kernel still has to use one of a few
specific ways to access user data (copy_from_user, etc.). So the idea
here is to add untagging into them. This patchset mostly takes this
approach (with the exception of memory subsystem syscalls).

If there's a better approach, I'm open to suggestions.

[RFC PATCH 3/6] mm, arm64: untag user addresses in memory syscalls

[Catalin Marinas]

On Fri, Mar 09, 2018 at 03:02:01PM +0100, Andrey Konovalov wrote:
> Memory subsystem syscalls accept user addresses as arguments, but don't use
> copy_from_user and other similar functions, so we need to handle this case
> separately.
> 
> Untag user pointers passed to madvise, mbind, get_mempolicy, mincore,
> mlock, mlock2, brk, mmap_pgoff, old_mmap, munmap, remap_file_pages,
> mprotect, pkey_mprotect, mremap and msync.
> 
> Signed-off-by: Andrey Konovalov <andreyknvl@google.com>

Please keep the cc list small (maybe linux-arch, linux-arm-kernel) as
I'm sure some lists would consider this spam.

>  mm/madvise.c   | 2 ++
>  mm/mempolicy.c | 6 ++++++
>  mm/mincore.c   | 2 ++
>  mm/mlock.c     | 5 +++++
>  mm/mmap.c      | 9 +++++++++
>  mm/mprotect.c  | 2 ++
>  mm/mremap.c    | 2 ++
>  mm/msync.c     | 3 +++

I'm not yet convinced these functions need to allow tagged pointers.
They are not doing memory accesses but rather dealing with the memory
range, hence an untagged pointer is better suited. There is probably a
reason why the "start" argument is "unsigned long" vs "void __user *"
(in the kernel, not the man page).

-- 
Catalin

[RFC PATCH 3/6] mm, arm64: untag user addresses in memory syscalls

[Andrey Konovalov]

On Fri, Mar 9, 2018 at 4:53 PM, Catalin Marinas <catalin.marinas@arm.com> wrote:
> On Fri, Mar 09, 2018 at 03:02:01PM +0100, Andrey Konovalov wrote:
>> Memory subsystem syscalls accept user addresses as arguments, but don't use
>> copy_from_user and other similar functions, so we need to handle this case
>> separately.
>>
>> Untag user pointers passed to madvise, mbind, get_mempolicy, mincore,
>> mlock, mlock2, brk, mmap_pgoff, old_mmap, munmap, remap_file_pages,
>> mprotect, pkey_mprotect, mremap and msync.
>>
>> Signed-off-by: Andrey Konovalov <andreyknvl@google.com>
>
> Please keep the cc list small (maybe linux-arch, linux-arm-kernel) as
> I'm sure some lists would consider this spam.

OK.

>
>>  mm/madvise.c   | 2 ++
>>  mm/mempolicy.c | 6 ++++++
>>  mm/mincore.c   | 2 ++
>>  mm/mlock.c     | 5 +++++
>>  mm/mmap.c      | 9 +++++++++
>>  mm/mprotect.c  | 2 ++
>>  mm/mremap.c    | 2 ++
>>  mm/msync.c     | 3 +++
>
> I'm not yet convinced these functions need to allow tagged pointers.
> They are not doing memory accesses but rather dealing with the memory
> range, hence an untagged pointer is better suited. There is probably a
> reason why the "start" argument is "unsigned long" vs "void __user *"
> (in the kernel, not the man page).

So that would make the user to untag pointers before passing to these syscalls.

Evgeniy, would that be possible to untag pointers in HWASan before
using memory subsystem syscalls? Is there a reason for untagging them
in the kernel?

>
> --
> Catalin

[RFC PATCH 3/6] mm, arm64: untag user addresses in memory syscalls

[Evgenii Stepanov]

On Fri, Mar 9, 2018 at 9:31 AM, Andrey Konovalov <andreyknvl@google.com> wrote:
> On Fri, Mar 9, 2018 at 4:53 PM, Catalin Marinas <catalin.marinas@arm.com> wrote:
>> On Fri, Mar 09, 2018 at 03:02:01PM +0100, Andrey Konovalov wrote:
>>> Memory subsystem syscalls accept user addresses as arguments, but don't use
>>> copy_from_user and other similar functions, so we need to handle this case
>>> separately.
>>>
>>> Untag user pointers passed to madvise, mbind, get_mempolicy, mincore,
>>> mlock, mlock2, brk, mmap_pgoff, old_mmap, munmap, remap_file_pages,
>>> mprotect, pkey_mprotect, mremap and msync.
>>>
>>> Signed-off-by: Andrey Konovalov <andreyknvl@google.com>
>>
>> Please keep the cc list small (maybe linux-arch, linux-arm-kernel) as
>> I'm sure some lists would consider this spam.
>
> OK.
>
>>
>>>  mm/madvise.c   | 2 ++
>>>  mm/mempolicy.c | 6 ++++++
>>>  mm/mincore.c   | 2 ++
>>>  mm/mlock.c     | 5 +++++
>>>  mm/mmap.c      | 9 +++++++++
>>>  mm/mprotect.c  | 2 ++
>>>  mm/mremap.c    | 2 ++
>>>  mm/msync.c     | 3 +++
>>
>> I'm not yet convinced these functions need to allow tagged pointers.
>> They are not doing memory accesses but rather dealing with the memory
>> range, hence an untagged pointer is better suited. There is probably a
>> reason why the "start" argument is "unsigned long" vs "void __user *"
>> (in the kernel, not the man page).
>
> So that would make the user to untag pointers before passing to these syscalls.
>
> Evgeniy, would that be possible to untag pointers in HWASan before
> using memory subsystem syscalls? Is there a reason for untagging them
> in the kernel?

Generally, no. It's possible to intercept a libc call using symbol
interposition, but I don't know how to rewrite arguments of a raw
system call other than through ptrace, and that creates more problems
than it solves.

AFAIU, it's valid for a program to pass an address obtained from
malloc or, better, posix_memalign to an mm syscall like mprotect().
These arguments are pointers from the userspace point of view.

[RFC PATCH 3/6] mm, arm64: untag user addresses in memory syscalls

[Andrey Konovalov]

On Fri, Mar 9, 2018 at 6:42 PM, Evgenii Stepanov <eugenis@google.com> wrote:
> On Fri, Mar 9, 2018 at 9:31 AM, Andrey Konovalov <andreyknvl@google.com> wrote:
>> On Fri, Mar 9, 2018 at 4:53 PM, Catalin Marinas <catalin.marinas@arm.com> wrote:
>>> I'm not yet convinced these functions need to allow tagged pointers.
>>> They are not doing memory accesses but rather dealing with the memory
>>> range, hence an untagged pointer is better suited. There is probably a
>>> reason why the "start" argument is "unsigned long" vs "void __user *"
>>> (in the kernel, not the man page).
>>
>> So that would make the user to untag pointers before passing to these syscalls.
>>
>> Evgeniy, would that be possible to untag pointers in HWASan before
>> using memory subsystem syscalls? Is there a reason for untagging them
>> in the kernel?
>
> Generally, no. It's possible to intercept a libc call using symbol
> interposition, but I don't know how to rewrite arguments of a raw
> system call other than through ptrace, and that creates more problems
> than it solves.
>
> AFAIU, it's valid for a program to pass an address obtained from
> malloc or, better, posix_memalign to an mm syscall like mprotect().
> These arguments are pointers from the userspace point of view.

Catalin, do you think this is a good reason to have the untagging done
in the kernel?

[RFC PATCH 3/6] mm, arm64: untag user addresses in memory syscalls

[Catalin Marinas]

On Wed, Mar 14, 2018 at 04:45:20PM +0100, Andrey Konovalov wrote:
> On Fri, Mar 9, 2018 at 6:42 PM, Evgenii Stepanov <eugenis@google.com> wrote:
> > On Fri, Mar 9, 2018 at 9:31 AM, Andrey Konovalov <andreyknvl@google.com> wrote:
> >> On Fri, Mar 9, 2018 at 4:53 PM, Catalin Marinas <catalin.marinas@arm.com> wrote:
> >>> I'm not yet convinced these functions need to allow tagged pointers.
> >>> They are not doing memory accesses but rather dealing with the memory
> >>> range, hence an untagged pointer is better suited. There is probably a
> >>> reason why the "start" argument is "unsigned long" vs "void __user *"
> >>> (in the kernel, not the man page).
> >>
> >> So that would make the user to untag pointers before passing to these syscalls.
> >>
> >> Evgeniy, would that be possible to untag pointers in HWASan before
> >> using memory subsystem syscalls? Is there a reason for untagging them
> >> in the kernel?
> >
> > Generally, no. It's possible to intercept a libc call using symbol
> > interposition, but I don't know how to rewrite arguments of a raw
> > system call other than through ptrace, and that creates more problems
> > than it solves.

With these patches, we are trying to relax the user/kernel ABI so that
tagged pointers can be passed into the kernel. Since this is a new ABI
(or an extension to the existing one), it might be ok to change the libc
so that the top byte is zeroed on specific syscalls before issuing the
SVC.

I agree that it is problematic for HWASan if it only relies on
overriding malloc/free.

> > AFAIU, it's valid for a program to pass an address obtained from
> > malloc or, better, posix_memalign to an mm syscall like mprotect().
> > These arguments are pointers from the userspace point of view.
> 
> Catalin, do you think this is a good reason to have the untagging done
> in the kernel?

malloc() or posix_memalign() are C library implementations and it's the
C library (or overridden functions) setting a tag on the returned
pointers. Since the TBI hardware feature allows memory accesses with a
non-zero tag, we could allow them in the kernel for syscalls performing
such accesses on behalf of the user (e.g. get_user/put_user would not
need to clear the tag).

madvise(), OTOH, does not perform a memory access on behalf of the user,
it's just advising the kernel about a range of virtual addresses. That's
where I think, from an ABI perspective, it doesn't make much sense to
allow tags into the kernel for these syscalls (even if it's simpler from
a user space perspective).

(but I don't have a very strong opinion on this ;))

-- 
Catalin

[RFC PATCH 3/6] mm, arm64: untag user addresses in memory syscalls

[Evgenii Stepanov]

On Wed, Mar 14, 2018 at 10:44 AM, Catalin Marinas
<catalin.marinas@arm.com> wrote:
> On Wed, Mar 14, 2018 at 04:45:20PM +0100, Andrey Konovalov wrote:
>> On Fri, Mar 9, 2018 at 6:42 PM, Evgenii Stepanov <eugenis@google.com> wrote:
>> > On Fri, Mar 9, 2018 at 9:31 AM, Andrey Konovalov <andreyknvl@google.com> wrote:
>> >> On Fri, Mar 9, 2018 at 4:53 PM, Catalin Marinas <catalin.marinas@arm.com> wrote:
>> >>> I'm not yet convinced these functions need to allow tagged pointers.
>> >>> They are not doing memory accesses but rather dealing with the memory
>> >>> range, hence an untagged pointer is better suited. There is probably a
>> >>> reason why the "start" argument is "unsigned long" vs "void __user *"
>> >>> (in the kernel, not the man page).
>> >>
>> >> So that would make the user to untag pointers before passing to these syscalls.
>> >>
>> >> Evgeniy, would that be possible to untag pointers in HWASan before
>> >> using memory subsystem syscalls? Is there a reason for untagging them
>> >> in the kernel?
>> >
>> > Generally, no. It's possible to intercept a libc call using symbol
>> > interposition, but I don't know how to rewrite arguments of a raw
>> > system call other than through ptrace, and that creates more problems
>> > than it solves.
>
> With these patches, we are trying to relax the user/kernel ABI so that
> tagged pointers can be passed into the kernel. Since this is a new ABI
> (or an extension to the existing one), it might be ok to change the libc
> so that the top byte is zeroed on specific syscalls before issuing the
> SVC.
>
> I agree that it is problematic for HWASan if it only relies on
> overriding malloc/free.
>
>> > AFAIU, it's valid for a program to pass an address obtained from
>> > malloc or, better, posix_memalign to an mm syscall like mprotect().
>> > These arguments are pointers from the userspace point of view.
>>
>> Catalin, do you think this is a good reason to have the untagging done
>> in the kernel?
>
> malloc() or posix_memalign() are C library implementations and it's the
> C library (or overridden functions) setting a tag on the returned
> pointers. Since the TBI hardware feature allows memory accesses with a
> non-zero tag, we could allow them in the kernel for syscalls performing
> such accesses on behalf of the user (e.g. get_user/put_user would not
> need to clear the tag).
>
> madvise(), OTOH, does not perform a memory access on behalf of the user,
> it's just advising the kernel about a range of virtual addresses. That's
> where I think, from an ABI perspective, it doesn't make much sense to
> allow tags into the kernel for these syscalls (even if it's simpler from
> a user space perspective).
>
> (but I don't have a very strong opinion on this ;))

I don't have a strong opinion on this, either.
Ideally, I would like tags to be fully transparent for user space
code. MM syscalls used on a malloc/memalign address are not a very
common pattern, so it might be OK to not allow tags there. But all
such code will have to be changed with explicit knowledge of TBI.