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

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

[Andrey Konovalov]

Hi!

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.

Thanks!

[1] http://clang.llvm.org/docs/HardwareAssistedAddressSanitizerDesign.html

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 a??mm, arm64: untag user addresses in memory syscallsa??.
- Rebased onto 3eb2ce82 (4.16-rc7).

Andrey Konovalov (6):
  arm64: add type casts to untagged_addr macro
  uaccess: add untagged_addr definition for other arches
  arm64: untag user addresses in copy_from_user and others
  mm, arm64: untag user addresses in mm/gup.c
  lib, arm64: untag addrs passed to strncpy_from_user and strnlen_user
  arm64: update Documentation/arm64/tagged-pointers.txt

 Documentation/arm64/tagged-pointers.txt |  5 +++--
 arch/arm64/include/asm/uaccess.h        |  9 +++++++--
 include/linux/uaccess.h                 |  4 ++++
 lib/strncpy_from_user.c                 |  2 ++
 lib/strnlen_user.c                      |  2 ++
 mm/gup.c                                | 12 ++++++++++++
 6 files changed, 30 insertions(+), 4 deletions(-)

-- 
2.17.0.484.g0c8726318c-goog

[PATCH 1/6] arm64: add type casts to untagged_addr macro

[Andrey Konovalov]

This patch makes the untagged_addr macro accept all kinds of address types
(void *, unsigned long, etc.) and allows not to specify type casts in each
place where it is used. This is done by using __typeof__.

Signed-off-by: Andrey Konovalov <andreyknvl@google.com>
---
 arch/arm64/include/asm/uaccess.h | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/arch/arm64/include/asm/uaccess.h b/arch/arm64/include/asm/uaccess.h
index e66b0fca99c2..2d6451cbaa86 100644
--- a/arch/arm64/include/asm/uaccess.h
+++ b/arch/arm64/include/asm/uaccess.h
@@ -102,7 +102,8 @@ static inline unsigned long __range_ok(const void __user *addr, unsigned long si
  * up with a tagged userland pointer. Clear the tag to get a sane pointer to
  * pass on to access_ok(), for instance.
  */
-#define untagged_addr(addr)		sign_extend64(addr, 55)
+#define untagged_addr(addr)		\
+	((__typeof__(addr))sign_extend64((__u64)(addr), 55))
 
 #define access_ok(type, addr, size)	__range_ok(addr, size)
 #define user_addr_max			get_fs
-- 
2.17.0.484.g0c8726318c-goog

[PATCH 1/6] arm64: add type casts to untagged_addr macro

[Andrey Konovalov]

This patch makes the untagged_addr macro accept all kinds of address types
(void *, unsigned long, etc.) and allows not to specify type casts in each
place where it is used. This is done by using __typeof__.

Signed-off-by: Andrey Konovalov <andreyknvl@google.com>
---
 arch/arm64/include/asm/uaccess.h | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/arch/arm64/include/asm/uaccess.h b/arch/arm64/include/asm/uaccess.h
index e66b0fca99c2..2d6451cbaa86 100644
--- a/arch/arm64/include/asm/uaccess.h
+++ b/arch/arm64/include/asm/uaccess.h
@@ -102,7 +102,8 @@ static inline unsigned long __range_ok(const void __user *addr, unsigned long si
  * up with a tagged userland pointer. Clear the tag to get a sane pointer to
  * pass on to access_ok(), for instance.
  */
-#define untagged_addr(addr)		sign_extend64(addr, 55)
+#define untagged_addr(addr)		\
+	((__typeof__(addr))sign_extend64((__u64)(addr), 55))
 
 #define access_ok(type, addr, size)	__range_ok(addr, size)
 #define user_addr_max			get_fs
-- 
2.17.0.484.g0c8726318c-goog

[PATCH 2/6] uaccess: add untagged_addr definition for other arches

[Andrey Konovalov]

To allow arm64 syscalls accept tagged pointers from userspace, we must
untag them when they are passed to the kernel. Since untagging is done in
generic parts of the kernel (like the mm subsystem), the untagged_addr
macro should be defined for all architectures.

Define it as a noop for other architectures besides arm64.

Signed-off-by: Andrey Konovalov <andreyknvl@google.com>
---
 include/linux/uaccess.h | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/include/linux/uaccess.h b/include/linux/uaccess.h
index efe79c1cdd47..c045b4eff95e 100644
--- a/include/linux/uaccess.h
+++ b/include/linux/uaccess.h
@@ -13,6 +13,10 @@
 
 #include <asm/uaccess.h>
 
+#ifndef untagged_addr
+#define untagged_addr(addr) addr
+#endif
+
 /*
  * Architectures should provide two primitives (raw_copy_{to,from}_user())
  * and get rid of their private instances of copy_{to,from}_user() and
-- 
2.17.0.484.g0c8726318c-goog

[PATCH 2/6] uaccess: add untagged_addr definition for other arches

[Andrey Konovalov]

To allow arm64 syscalls accept tagged pointers from userspace, we must
untag them when they are passed to the kernel. Since untagging is done in
generic parts of the kernel (like the mm subsystem), the untagged_addr
macro should be defined for all architectures.

Define it as a noop for other architectures besides arm64.

Signed-off-by: Andrey Konovalov <andreyknvl@google.com>
---
 include/linux/uaccess.h | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/include/linux/uaccess.h b/include/linux/uaccess.h
index efe79c1cdd47..c045b4eff95e 100644
--- a/include/linux/uaccess.h
+++ b/include/linux/uaccess.h
@@ -13,6 +13,10 @@
 
 #include <asm/uaccess.h>
 
+#ifndef untagged_addr
+#define untagged_addr(addr) addr
+#endif
+
 /*
  * Architectures should provide two primitives (raw_copy_{to,from}_user())
  * and get rid of their private instances of copy_{to,from}_user() and
-- 
2.17.0.484.g0c8726318c-goog

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

[Andrey Konovalov]

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.

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.17.0.484.g0c8726318c-goog

Re: [PATCH 3/6] arm64: untag user addresses in copy_from_user and others

[Catalin Marinas]

On Wed, Apr 18, 2018 at 08:53:12PM +0200, Andrey Konovalov wrote:
> @@ -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"

First of all, passing a tagged user pointer throughout the kernel is
safe with uaccess routines but not suitable for find_vma() etc.

With this change, we may have an inconsistent behaviour on the tag
masking, depending on whether the entry code uses __uaccess_mask_ptr()
or not. We could preserve the tag with something like:

diff --git a/arch/arm64/include/asm/uaccess.h b/arch/arm64/include/asm/uaccess.h
index e66b0fca99c2..ed15bfcbd797 100644
--- a/arch/arm64/include/asm/uaccess.h
+++ b/arch/arm64/include/asm/uaccess.h
@@ -244,10 +244,11 @@ static inline void __user *__uaccess_mask_ptr(const void __user *ptr)
 	void __user *safe_ptr;
 
 	asm volatile(
-	"	bics	xzr, %1, %2\n"
+	"	bics	xzr, %3, %2\n"
 	"	csel	%0, %1, xzr, eq\n"
 	: "=&r" (safe_ptr)
-	: "r" (ptr), "r" (current_thread_info()->addr_limit)
+	: "r" (ptr), "r" (current_thread_info()->addr_limit),
+	  "r" (untagged_addr(ptr))
 	: "cc");
 
 	csdb();

-- 
Catalin

Re: [PATCH 3/6] arm64: untag user addresses in copy_from_user and others

[Andrey Konovalov]

On Thu, Apr 26, 2018 at 5:47 PM, Catalin Marinas
<catalin.marinas@arm.com> wrote:
> On Wed, Apr 18, 2018 at 08:53:12PM +0200, Andrey Konovalov wrote:
>> @@ -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"
>
> First of all, passing a tagged user pointer throughout the kernel is
> safe with uaccess routines but not suitable for find_vma() etc.
>
> With this change, we may have an inconsistent behaviour on the tag
> masking, depending on whether the entry code uses __uaccess_mask_ptr()
> or not. We could preserve the tag with something like:
>
> diff --git a/arch/arm64/include/asm/uaccess.h b/arch/arm64/include/asm/uaccess.h
> index e66b0fca99c2..ed15bfcbd797 100644
> --- a/arch/arm64/include/asm/uaccess.h
> +++ b/arch/arm64/include/asm/uaccess.h
> @@ -244,10 +244,11 @@ static inline void __user *__uaccess_mask_ptr(const void __user *ptr)
>         void __user *safe_ptr;
>
>         asm volatile(
> -       "       bics    xzr, %1, %2\n"
> +       "       bics    xzr, %3, %2\n"
>         "       csel    %0, %1, xzr, eq\n"
>         : "=&r" (safe_ptr)
> -       : "r" (ptr), "r" (current_thread_info()->addr_limit)
> +       : "r" (ptr), "r" (current_thread_info()->addr_limit),
> +         "r" (untagged_addr(ptr))
>         : "cc");
>
>         csdb();

Just to make sure I understood this assembly snippet correctly, this
change will result in checking untagged address against addr_limit,
and returning the original tagged address if the check passes. Sure,
sounds good, I'll do that.

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

[Andrey Konovalov]

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.

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.17.0.484.g0c8726318c-goog

[PATCH 4/6] mm, arm64: untag user addresses in mm/gup.c

[Andrey Konovalov]

mm/gup.c provides a kernel interface that accepts user addresses and
manipulates user pages directly (for example get_user_pages, that is used
by the futex syscall). Here we also need to handle the case of tagged user
pointers.

Untag addresses passed to this interface.

Signed-off-by: Andrey Konovalov <andreyknvl@google.com>
---
 mm/gup.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/mm/gup.c b/mm/gup.c
index 76af4cfeaf68..fb375de7d40d 100644
--- a/mm/gup.c
+++ b/mm/gup.c
@@ -386,6 +386,8 @@ struct page *follow_page_mask(struct vm_area_struct *vma,
 	struct page *page;
 	struct mm_struct *mm = vma->vm_mm;
 
+	address = untagged_addr(address);
+
 	*page_mask = 0;
 
 	/* make this handle hugepd */
@@ -647,6 +649,8 @@ static long __get_user_pages(struct task_struct *tsk, struct mm_struct *mm,
 	if (!nr_pages)
 		return 0;
 
+	start = untagged_addr(start);
+
 	VM_BUG_ON(!!pages != !!(gup_flags & FOLL_GET));
 
 	/*
@@ -801,6 +805,8 @@ int fixup_user_fault(struct task_struct *tsk, struct mm_struct *mm,
 	struct vm_area_struct *vma;
 	int ret, major = 0;
 
+	address = untagged_addr(address);
+
 	if (unlocked)
 		fault_flags |= FAULT_FLAG_ALLOW_RETRY;
 
@@ -854,6 +860,8 @@ static __always_inline long __get_user_pages_locked(struct task_struct *tsk,
 	long ret, pages_done;
 	bool lock_dropped;
 
+	start = untagged_addr(start);
+
 	if (locked) {
 		/* if VM_FAULT_RETRY can be returned, vmas become invalid */
 		BUG_ON(vmas);
@@ -1751,6 +1759,8 @@ int __get_user_pages_fast(unsigned long start, int nr_pages, int write,
 	unsigned long flags;
 	int nr = 0;
 
+	start = untagged_addr(start);
+
 	start &= PAGE_MASK;
 	addr = start;
 	len = (unsigned long) nr_pages << PAGE_SHIFT;
@@ -1803,6 +1813,8 @@ int get_user_pages_fast(unsigned long start, int nr_pages, int write,
 	unsigned long addr, len, end;
 	int nr = 0, ret = 0;
 
+	start = untagged_addr(start);
+
 	start &= PAGE_MASK;
 	addr = start;
 	len = (unsigned long) nr_pages << PAGE_SHIFT;
-- 
2.17.0.484.g0c8726318c-goog

Re: [PATCH 4/6] mm, arm64: untag user addresses in mm/gup.c

[Catalin Marinas]

On Wed, Apr 18, 2018 at 08:53:13PM +0200, Andrey Konovalov wrote:
> diff --git a/mm/gup.c b/mm/gup.c
> index 76af4cfeaf68..fb375de7d40d 100644
> --- a/mm/gup.c
> +++ b/mm/gup.c
> @@ -386,6 +386,8 @@ struct page *follow_page_mask(struct vm_area_struct *vma,
>  	struct page *page;
>  	struct mm_struct *mm = vma->vm_mm;
>  
> +	address = untagged_addr(address);
> +
>  	*page_mask = 0;
>  
>  	/* make this handle hugepd */

Does having a tagged address here makes any difference? I couldn't hit a
failure with my simple tests (LD_PRELOAD a library that randomly adds
tags to pointers returned by malloc).

> @@ -647,6 +649,8 @@ static long __get_user_pages(struct task_struct *tsk, struct mm_struct *mm,
>  	if (!nr_pages)
>  		return 0;
>  
> +	start = untagged_addr(start);
> +
>  	VM_BUG_ON(!!pages != !!(gup_flags & FOLL_GET));
>  
>  	/*
> @@ -801,6 +805,8 @@ int fixup_user_fault(struct task_struct *tsk, struct mm_struct *mm,
>  	struct vm_area_struct *vma;
>  	int ret, major = 0;
>  
> +	address = untagged_addr(address);
> +
>  	if (unlocked)
>  		fault_flags |= FAULT_FLAG_ALLOW_RETRY;
>  
> @@ -854,6 +860,8 @@ static __always_inline long __get_user_pages_locked(struct task_struct *tsk,
>  	long ret, pages_done;
>  	bool lock_dropped;
>  
> +	start = untagged_addr(start);
> +
>  	if (locked) {
>  		/* if VM_FAULT_RETRY can be returned, vmas become invalid */
>  		BUG_ON(vmas);

Isn't __get_user_pages() untagging enough to cover this case as well?
Can this function not cope with tagged pointers?

> @@ -1751,6 +1759,8 @@ int __get_user_pages_fast(unsigned long start, int nr_pages, int write,
>  	unsigned long flags;
>  	int nr = 0;
>  
> +	start = untagged_addr(start);
> +
>  	start &= PAGE_MASK;
>  	addr = start;
>  	len = (unsigned long) nr_pages << PAGE_SHIFT;
> @@ -1803,6 +1813,8 @@ int get_user_pages_fast(unsigned long start, int nr_pages, int write,
>  	unsigned long addr, len, end;
>  	int nr = 0, ret = 0;
>  
> +	start = untagged_addr(start);
> +
>  	start &= PAGE_MASK;
>  	addr = start;
>  	len = (unsigned long) nr_pages << PAGE_SHIFT;

Have you hit a problem with the fast gup functions and tagged pointers?
The page table walking macros (e.g. p*d_index()) should mask the tag out
already.

-- 
Catalin

Re: [PATCH 4/6] mm, arm64: untag user addresses in mm/gup.c

[Andrey Konovalov]

On Thu, Apr 26, 2018 at 7:47 PM, Catalin Marinas
<catalin.marinas@arm.com> wrote:

My approach with this was to add untagging to every gup.c function
that is exposed for external use, but perhaps adding untagging only
where it is actually required is a better approach.

> On Wed, Apr 18, 2018 at 08:53:13PM +0200, Andrey Konovalov wrote:
>> diff --git a/mm/gup.c b/mm/gup.c
>> index 76af4cfeaf68..fb375de7d40d 100644
>> --- a/mm/gup.c
>> +++ b/mm/gup.c
>> @@ -386,6 +386,8 @@ struct page *follow_page_mask(struct vm_area_struct *vma,
>>       struct page *page;
>>       struct mm_struct *mm = vma->vm_mm;
>>
>> +     address = untagged_addr(address);
>> +
>>       *page_mask = 0;
>>
>>       /* make this handle hugepd */
>
> Does having a tagged address here makes any difference? I couldn't hit a
> failure with my simple tests (LD_PRELOAD a library that randomly adds
> tags to pointers returned by malloc).

I think you're right, follow_page_mask is only called from
__get_user_pages, which already untagged the address. I'll remove
untagging here.

>
>> @@ -647,6 +649,8 @@ static long __get_user_pages(struct task_struct *tsk, struct mm_struct *mm,
>>       if (!nr_pages)
>>               return 0;
>>
>> +     start = untagged_addr(start);
>> +
>>       VM_BUG_ON(!!pages != !!(gup_flags & FOLL_GET));
>>
>>       /*
>> @@ -801,6 +805,8 @@ int fixup_user_fault(struct task_struct *tsk, struct mm_struct *mm,
>>       struct vm_area_struct *vma;
>>       int ret, major = 0;
>>
>> +     address = untagged_addr(address);
>> +
>>       if (unlocked)
>>               fault_flags |= FAULT_FLAG_ALLOW_RETRY;
>>
>> @@ -854,6 +860,8 @@ static __always_inline long __get_user_pages_locked(struct task_struct *tsk,
>>       long ret, pages_done;
>>       bool lock_dropped;
>>
>> +     start = untagged_addr(start);
>> +
>>       if (locked) {
>>               /* if VM_FAULT_RETRY can be returned, vmas become invalid */
>>               BUG_ON(vmas);
>
> Isn't __get_user_pages() untagging enough to cover this case as well?
> Can this function not cope with tagged pointers?

Yes, I think you're right here as well. I'll remove untagging here.

>
>> @@ -1751,6 +1759,8 @@ int __get_user_pages_fast(unsigned long start, int nr_pages, int write,
>>       unsigned long flags;
>>       int nr = 0;
>>
>> +     start = untagged_addr(start);
>> +
>>       start &= PAGE_MASK;
>>       addr = start;
>>       len = (unsigned long) nr_pages << PAGE_SHIFT;
>> @@ -1803,6 +1813,8 @@ int get_user_pages_fast(unsigned long start, int nr_pages, int write,
>>       unsigned long addr, len, end;
>>       int nr = 0, ret = 0;
>>
>> +     start = untagged_addr(start);
>> +
>>       start &= PAGE_MASK;
>>       addr = start;
>>       len = (unsigned long) nr_pages << PAGE_SHIFT;
>
> Have you hit a problem with the fast gup functions and tagged pointers?
> The page table walking macros (e.g. p*d_index()) should mask the tag out
> already.

I didn't hit a problem, but the plan was to add untagging to all gup.c
interface functions as I mentioned above. Here get_user_pages_fast can
cope with tagged addresses as long as gup_pgd_range can. And looks
like the latter can indeed do that since it only uses addr through the
page table walking macros you mentioned. I'll remove untagging here as
well.

Thanks!

Re: [PATCH 4/6] mm, arm64: untag user addresses in mm/gup.c

[Kirill A. Shutemov]

On Wed, May 02, 2018 at 02:38:42PM +0000, Andrey Konovalov wrote:
> > Does having a tagged address here makes any difference? I couldn't hit a
> > failure with my simple tests (LD_PRELOAD a library that randomly adds
> > tags to pointers returned by malloc).
> 
> I think you're right, follow_page_mask is only called from
> __get_user_pages, which already untagged the address. I'll remove
> untagging here.

It also called from follow_page(). Have you covered all its callers?

-- 
 Kirill A. Shutemov

Re: [PATCH 4/6] mm, arm64: untag user addresses in mm/gup.c

[Andrey Konovalov]

On Wed, May 2, 2018 at 5:36 PM, Kirill A. Shutemov
<kirill.shutemov@linux.intel.com> wrote:
> On Wed, May 02, 2018 at 02:38:42PM +0000, Andrey Konovalov wrote:
>> > Does having a tagged address here makes any difference? I couldn't hit a
>> > failure with my simple tests (LD_PRELOAD a library that randomly adds
>> > tags to pointers returned by malloc).
>>
>> I think you're right, follow_page_mask is only called from
>> __get_user_pages, which already untagged the address. I'll remove
>> untagging here.
>
> It also called from follow_page(). Have you covered all its callers?

Oh, missed that, will take a look.

Thinking about that, would it make sense to add untagging to find_vma
(and others) instead of trying to cover all find_vma callers?

Re: [PATCH 4/6] mm, arm64: untag user addresses in mm/gup.c

[Andrey Konovalov]

On Wed, May 2, 2018 at 7:25 PM, Andrey Konovalov <andreyknvl@google.com> wrote:
> On Wed, May 2, 2018 at 5:36 PM, Kirill A. Shutemov
> <kirill.shutemov@linux.intel.com> wrote:
>> On Wed, May 02, 2018 at 02:38:42PM +0000, Andrey Konovalov wrote:
>>> > Does having a tagged address here makes any difference? I couldn't hit a
>>> > failure with my simple tests (LD_PRELOAD a library that randomly adds
>>> > tags to pointers returned by malloc).
>>>
>>> I think you're right, follow_page_mask is only called from
>>> __get_user_pages, which already untagged the address. I'll remove
>>> untagging here.
>>
>> It also called from follow_page(). Have you covered all its callers?
>
> Oh, missed that, will take a look.

I wasn't able to find anything that calls follow_page with pointers
passed from userspace except for the memory subsystem syscalls, and we
deliberately don't add untagging in those.

Re: [PATCH 4/6] mm, arm64: untag user addresses in mm/gup.c

[Kirill A. Shutemov]

On Thu, May 03, 2018 at 04:09:56PM +0200, Andrey Konovalov wrote:
> On Wed, May 2, 2018 at 7:25 PM, Andrey Konovalov <andreyknvl@google.com> wrote:
> > On Wed, May 2, 2018 at 5:36 PM, Kirill A. Shutemov
> > <kirill.shutemov@linux.intel.com> wrote:
> >> On Wed, May 02, 2018 at 02:38:42PM +0000, Andrey Konovalov wrote:
> >>> > Does having a tagged address here makes any difference? I couldn't hit a
> >>> > failure with my simple tests (LD_PRELOAD a library that randomly adds
> >>> > tags to pointers returned by malloc).
> >>>
> >>> I think you're right, follow_page_mask is only called from
> >>> __get_user_pages, which already untagged the address. I'll remove
> >>> untagging here.
> >>
> >> It also called from follow_page(). Have you covered all its callers?
> >
> > Oh, missed that, will take a look.
> 
> I wasn't able to find anything that calls follow_page with pointers
> passed from userspace except for the memory subsystem syscalls, and we
> deliberately don't add untagging in those.

I guess I missed this part, but could you elaborate on this? Why?
Not yet or not ever?

Also I wounder if we can find (with sparse?) all places where we cast out
__user. This would give a nice list of places where to pay attention.

-- 
 Kirill A. Shutemov

Re: [PATCH 4/6] mm, arm64: untag user addresses in mm/gup.c

[Andrey Konovalov]

On Thu, May 3, 2018 at 5:24 PM, Kirill A. Shutemov <kirill@shutemov.name> wrote:
> On Thu, May 03, 2018 at 04:09:56PM +0200, Andrey Konovalov wrote:
>> On Wed, May 2, 2018 at 7:25 PM, Andrey Konovalov <andreyknvl@google.com> wrote:

>> I wasn't able to find anything that calls follow_page with pointers
>> passed from userspace except for the memory subsystem syscalls, and we
>> deliberately don't add untagging in those.
>
> I guess I missed this part, but could you elaborate on this? Why?
> Not yet or not ever?

Check out the discussion here:
https://www.spinics.net/lists/arm-kernel/msg640936.html

>
> Also I wounder if we can find (with sparse?) all places where we cast out
> __user. This would give a nice list of places where to pay attention.

The way I tested this is I added BUG_ON(top byte tag is set) to
find_vma and find_extend_vma and ran a modified version of syzkaller
that embeds tags into pointers overnight. The only crashes that I saw
were coming from memory subsystem syscalls. I then temporarily added
untagging to suppress those crashes
(https://gist.github.com/xairy/3aa1f57798fa62522c8ac53fad9b74ca), and
didn't see any crashes after that.

Re: [PATCH 4/6] mm, arm64: untag user addresses in mm/gup.c

[Catalin Marinas]

On Wed, May 02, 2018 at 07:25:17PM +0200, Andrey Konovalov wrote:
> On Wed, May 2, 2018 at 5:36 PM, Kirill A. Shutemov
> <kirill.shutemov@linux.intel.com> wrote:
> > On Wed, May 02, 2018 at 02:38:42PM +0000, Andrey Konovalov wrote:
> >> > Does having a tagged address here makes any difference? I couldn't hit a
> >> > failure with my simple tests (LD_PRELOAD a library that randomly adds
> >> > tags to pointers returned by malloc).
> >>
> >> I think you're right, follow_page_mask is only called from
> >> __get_user_pages, which already untagged the address. I'll remove
> >> untagging here.
> >
> > It also called from follow_page(). Have you covered all its callers?
> 
> Oh, missed that, will take a look.
> 
> Thinking about that, would it make sense to add untagging to find_vma
> (and others) instead of trying to cover all find_vma callers?

I don't think adding the untagging to find_vma() is sufficient. In many
cases the caller does a subsequent check like 'start < vma->vm_start'
(see sys_msync() as an example, there are a few others as well). What I
did in my tests was a WARN_ON_ONCE() in find_vma() if the address is
tagged.

-- 
Catalin

Re: [PATCH 4/6] mm, arm64: untag user addresses in mm/gup.c

[Andrey Konovalov]

On Tue, May 8, 2018 at 5:11 PM, Catalin Marinas <catalin.marinas@arm.com> wrote:
> On Wed, May 02, 2018 at 07:25:17PM +0200, Andrey Konovalov wrote:
>> On Wed, May 2, 2018 at 5:36 PM, Kirill A. Shutemov
>> <kirill.shutemov@linux.intel.com> wrote:
>> > On Wed, May 02, 2018 at 02:38:42PM +0000, Andrey Konovalov wrote:
>> >> > Does having a tagged address here makes any difference? I couldn't hit a
>> >> > failure with my simple tests (LD_PRELOAD a library that randomly adds
>> >> > tags to pointers returned by malloc).
>> >>
>> >> I think you're right, follow_page_mask is only called from
>> >> __get_user_pages, which already untagged the address. I'll remove
>> >> untagging here.
>> >
>> > It also called from follow_page(). Have you covered all its callers?
>>
>> Oh, missed that, will take a look.
>>
>> Thinking about that, would it make sense to add untagging to find_vma
>> (and others) instead of trying to cover all find_vma callers?
>
> I don't think adding the untagging to find_vma() is sufficient. In many
> cases the caller does a subsequent check like 'start < vma->vm_start'
> (see sys_msync() as an example, there are a few others as well).

OK.

> What I
> did in my tests was a WARN_ON_ONCE() in find_vma() if the address is
> tagged.

So this is similar to what I did.

Do you think trying to find "all places where we cast out __user" with
static analysis as Kirill suggested is something I should pursue? Or
is this patchset is good as is as the first approximation, since we
can fix more things where untagging is needed as we discover them one
by one?

[PATCH 4/6] mm, arm64: untag user addresses in mm/gup.c

[Andrey Konovalov]

mm/gup.c provides a kernel interface that accepts user addresses and
manipulates user pages directly (for example get_user_pages, that is used
by the futex syscall). Here we also need to handle the case of tagged user
pointers.

Untag addresses passed to this interface.

Signed-off-by: Andrey Konovalov <andreyknvl@google.com>
---
 mm/gup.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/mm/gup.c b/mm/gup.c
index 76af4cfeaf68..fb375de7d40d 100644
--- a/mm/gup.c
+++ b/mm/gup.c
@@ -386,6 +386,8 @@ struct page *follow_page_mask(struct vm_area_struct *vma,
 	struct page *page;
 	struct mm_struct *mm = vma->vm_mm;
 
+	address = untagged_addr(address);
+
 	*page_mask = 0;
 
 	/* make this handle hugepd */
@@ -647,6 +649,8 @@ static long __get_user_pages(struct task_struct *tsk, struct mm_struct *mm,
 	if (!nr_pages)
 		return 0;
 
+	start = untagged_addr(start);
+
 	VM_BUG_ON(!!pages != !!(gup_flags & FOLL_GET));
 
 	/*
@@ -801,6 +805,8 @@ int fixup_user_fault(struct task_struct *tsk, struct mm_struct *mm,
 	struct vm_area_struct *vma;
 	int ret, major = 0;
 
+	address = untagged_addr(address);
+
 	if (unlocked)
 		fault_flags |= FAULT_FLAG_ALLOW_RETRY;
 
@@ -854,6 +860,8 @@ static __always_inline long __get_user_pages_locked(struct task_struct *tsk,
 	long ret, pages_done;
 	bool lock_dropped;
 
+	start = untagged_addr(start);
+
 	if (locked) {
 		/* if VM_FAULT_RETRY can be returned, vmas become invalid */
 		BUG_ON(vmas);
@@ -1751,6 +1759,8 @@ int __get_user_pages_fast(unsigned long start, int nr_pages, int write,
 	unsigned long flags;
 	int nr = 0;
 
+	start = untagged_addr(start);
+
 	start &= PAGE_MASK;
 	addr = start;
 	len = (unsigned long) nr_pages << PAGE_SHIFT;
@@ -1803,6 +1813,8 @@ int get_user_pages_fast(unsigned long start, int nr_pages, int write,
 	unsigned long addr, len, end;
 	int nr = 0, ret = 0;
 
+	start = untagged_addr(start);
+
 	start &= PAGE_MASK;
 	addr = start;
 	len = (unsigned long) nr_pages << PAGE_SHIFT;
-- 
2.17.0.484.g0c8726318c-goog

[PATCH 5/6] lib, arm64: untag addrs passed to strncpy_from_user and strnlen_user

[Andrey Konovalov]

strncpy_from_user and strnlen_user accept user addresses as arguments, and
do not go through the same path as copy_from_user and others, so here we
need to separately handle the case of tagged user addresses as well.

Untag user pointers passed to these functions.

Signed-off-by: Andrey Konovalov <andreyknvl@google.com>
---
 lib/strncpy_from_user.c | 2 ++
 lib/strnlen_user.c      | 2 ++
 2 files changed, 4 insertions(+)

diff --git a/lib/strncpy_from_user.c b/lib/strncpy_from_user.c
index b53e1b5d80f4..97467cd2bc59 100644
--- a/lib/strncpy_from_user.c
+++ b/lib/strncpy_from_user.c
@@ -106,6 +106,8 @@ long strncpy_from_user(char *dst, const char __user *src, long count)
 	if (unlikely(count <= 0))
 		return 0;
 
+	src = untagged_addr(src);
+
 	max_addr = user_addr_max();
 	src_addr = (unsigned long)src;
 	if (likely(src_addr < max_addr)) {
diff --git a/lib/strnlen_user.c b/lib/strnlen_user.c
index 60d0bbda8f5e..8b5f56466e00 100644
--- a/lib/strnlen_user.c
+++ b/lib/strnlen_user.c
@@ -108,6 +108,8 @@ long strnlen_user(const char __user *str, long count)
 	if (unlikely(count <= 0))
 		return 0;
 
+	str = untagged_addr(str);
+
 	max_addr = user_addr_max();
 	src_addr = (unsigned long)str;
 	if (likely(src_addr < max_addr)) {
-- 
2.17.0.484.g0c8726318c-goog

[PATCH 5/6] lib, arm64: untag addrs passed to strncpy_from_user and strnlen_user

[Andrey Konovalov]

strncpy_from_user and strnlen_user accept user addresses as arguments, and
do not go through the same path as copy_from_user and others, so here we
need to separately handle the case of tagged user addresses as well.

Untag user pointers passed to these functions.

Signed-off-by: Andrey Konovalov <andreyknvl@google.com>
---
 lib/strncpy_from_user.c | 2 ++
 lib/strnlen_user.c      | 2 ++
 2 files changed, 4 insertions(+)

diff --git a/lib/strncpy_from_user.c b/lib/strncpy_from_user.c
index b53e1b5d80f4..97467cd2bc59 100644
--- a/lib/strncpy_from_user.c
+++ b/lib/strncpy_from_user.c
@@ -106,6 +106,8 @@ long strncpy_from_user(char *dst, const char __user *src, long count)
 	if (unlikely(count <= 0))
 		return 0;
 
+	src = untagged_addr(src);
+
 	max_addr = user_addr_max();
 	src_addr = (unsigned long)src;
 	if (likely(src_addr < max_addr)) {
diff --git a/lib/strnlen_user.c b/lib/strnlen_user.c
index 60d0bbda8f5e..8b5f56466e00 100644
--- a/lib/strnlen_user.c
+++ b/lib/strnlen_user.c
@@ -108,6 +108,8 @@ long strnlen_user(const char __user *str, long count)
 	if (unlikely(count <= 0))
 		return 0;
 
+	str = untagged_addr(str);
+
 	max_addr = user_addr_max();
 	src_addr = (unsigned long)str;
 	if (likely(src_addr < max_addr)) {
-- 
2.17.0.484.g0c8726318c-goog

[PATCH 6/6] arm64: update Documentation/arm64/tagged-pointers.txt

[Andrey Konovalov]

Add a note that work on passing tagged user pointers to the kernel via
syscalls has started, but might not be complete yet.

Signed-off-by: Andrey Konovalov <andreyknvl@google.com>
---
 Documentation/arm64/tagged-pointers.txt | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/Documentation/arm64/tagged-pointers.txt b/Documentation/arm64/tagged-pointers.txt
index a25a99e82bb1..361481283f00 100644
--- a/Documentation/arm64/tagged-pointers.txt
+++ b/Documentation/arm64/tagged-pointers.txt
@@ -35,8 +35,9 @@ Using non-zero address tags in any of these locations may result in an
 error code being returned, a (fatal) signal being raised, or other modes
 of failure.
 
-For these reasons, passing non-zero address tags to the kernel via
-system calls is forbidden, and using a non-zero address tag for sp is
+Some initial work for supporting non-zero address tags passed to the
+kernel via system calls has been done, but the kernel doesn't provide
+any guarantees at this point. Using a non-zero address tag for sp is
 strongly discouraged.
 
 Programs maintaining a frame pointer and frame records that use non-zero
-- 
2.17.0.484.g0c8726318c-goog

[PATCH 6/6] arm64: update Documentation/arm64/tagged-pointers.txt

[Andrey Konovalov]

Add a note that work on passing tagged user pointers to the kernel via
syscalls has started, but might not be complete yet.

Signed-off-by: Andrey Konovalov <andreyknvl@google.com>
---
 Documentation/arm64/tagged-pointers.txt | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/Documentation/arm64/tagged-pointers.txt b/Documentation/arm64/tagged-pointers.txt
index a25a99e82bb1..361481283f00 100644
--- a/Documentation/arm64/tagged-pointers.txt
+++ b/Documentation/arm64/tagged-pointers.txt
@@ -35,8 +35,9 @@ Using non-zero address tags in any of these locations may result in an
 error code being returned, a (fatal) signal being raised, or other modes
 of failure.
 
-For these reasons, passing non-zero address tags to the kernel via
-system calls is forbidden, and using a non-zero address tag for sp is
+Some initial work for supporting non-zero address tags passed to the
+kernel via system calls has been done, but the kernel doesn't provide
+any guarantees at this point. Using a non-zero address tag for sp is
 strongly discouraged.
 
 Programs maintaining a frame pointer and frame records that use non-zero
-- 
2.17.0.484.g0c8726318c-goog

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

[Kirill A. Shutemov]

On Wed, Apr 18, 2018 at 08:53:09PM +0200, Andrey Konovalov wrote:
> Hi!
> 
> 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.

How many changes do you anticipate?

This patchset looks small and reasonable, but I see a potential to become a
boilerplate. Would we need to change every driver which implements ioctl()
to strip these bits?

-- 
 Kirill A. Shutemov

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

[Andrey Konovalov]

On Thu, Apr 19, 2018 at 11:33 AM, Kirill A. Shutemov
<kirill@shutemov.name> wrote:
> On Wed, Apr 18, 2018 at 08:53:09PM +0200, Andrey Konovalov wrote:
>> Hi!
>>
>> 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.
>
> How many changes do you anticipate?
>
> This patchset looks small and reasonable, but I see a potential to become a
> boilerplate. Would we need to change every driver which implements ioctl()
> to strip these bits?

I've replied to somewhat similar question in one of the previous
versions of the patchset.

"""
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.
"""

So if we go with the first way, we'll need to go through every syscall
and ioctl handler, which doesn't seem feasible.

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

[Catalin Marinas]

On Wed, Apr 25, 2018 at 04:45:37PM +0200, Andrey Konovalov wrote:
> On Thu, Apr 19, 2018 at 11:33 AM, Kirill A. Shutemov
> <kirill@shutemov.name> wrote:
> > On Wed, Apr 18, 2018 at 08:53:09PM +0200, 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.
> >
> > How many changes do you anticipate?
> >
> > This patchset looks small and reasonable, but I see a potential to become a
> > boilerplate. Would we need to change every driver which implements ioctl()
> > to strip these bits?
> 
> I've replied to somewhat similar question in one of the previous
> versions of the patchset.
> 
> """
> 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.
> """
> 
> So if we go with the first way, we'll need to go through every syscall
> and ioctl handler, which doesn't seem feasible.

I agree with you that (1) isn't feasible. My hack is sufficient for the
pointer arguments but doesn't help with pointers in user structures
passed to the kernel.

Now, since the hardware allows access to user pointers with non-zero top
8-bit, the kernel uaccess routines can also use such pointers directly.
What's needed, as per your patches, is the access_ok() macro and
whatever ends up calling find_vma() (at a first look, there may be other
cases). I don't think drivers need changing as long as the in-kernel API
they use performs the untagging (e.g. get_user_pages()).

-- 
Catalin