arm64: untag user pointers passed to the kernel

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.

Quoted 11 lines · 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.

Quoted 2 lines

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?

(quoted diff) +1 -0

@@ -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.
  */

mm/gup.c +4 -0

@@ -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;
 

mm/gup.c +2 -0

@@ -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;

Quoted 10 lines · Kirill A. Shutemov

Quoted 9 lines · Andrey Konovalov

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.

Quoted 8 lines

Quoted 2 lines

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.

Quoted 12 lines

mm/gup.c +2 -0

index 76af4cfeaf68..fb375de7d40d 100644
@@ -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;
 

mm/gup.c struct page *follow_page_mask(struct vm_area_struct *vma, · line 393

Quoted 1 line

mm/gup.c +0 -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).

Quoted 23 lines

mm/gup.c +4 -0

@@ -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;
 

mm/gup.c static __always_inline long __get_user_pages_locked(struct task_struct *tsk, · line 867

Quoted 5 lines

mm/gup.c +2 -0

+     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?

Quoted 14 lines

mm/gup.c +2 -0

@@ -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;
 

mm/gup.c int __get_user_pages_fast(unsigned long start, int nr_pages, int write, · line 1766

Quoted 5 lines

mm/gup.c +2 -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.

Quoted 6 lines · Andrey Konovalov

(quoted diff) +1 -0

@@ -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.
  */

Review static inline void uaccess_enable_not_uao(void) · line 253

Quoted 10 lines

(quoted diff) +2 -0

 #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:

arch/arm64/include/asm/uaccess.h +0 -0

index e66b0fca99c2..ed15bfcbd797 100644
@@ -244,10 +244,11 @@ static inline void __user *__uaccess_mask_ptr(const void __user *ptr)
        void __user *safe_ptr;
 
        asm volatile(

Quoted 3 lines

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.

Quoted 7 lines · Andrey Konovalov

Quoted 3 lines

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?

Quoted 5 lines · Kirill A. Shutemov

Quoted 4 lines · Andrey Konovalov

Quoted 3 lines

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

Quoted 5 lines

Quoted 3 lines

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.

Quoted 11 lines · Andrey Konovalov

Quoted 9 lines · Kirill A. Shutemov

Quoted 7 lines · Andrey Konovalov

Quoted 3 lines

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.

Quoted 3 lines

Quoted 1 line

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.

Quoted 5 lines · Andrey Konovalov

On Wed, May 2, 2018 at 7:25 PM, Andrey Konovalov <[email protected]> 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?

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.

Quoted 9 lines · Kirill A. Shutemov

Quoted 7 lines · Andrey Konovalov

Quoted 3 lines

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.

Quoted 1 line

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?

Quoted 13 lines · Andrey Konovalov

Quoted 10 lines · Kirill A. Shutemov

Quoted 7 lines · Andrey Konovalov

Quoted 3 lines

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?

Quoted 4 lines

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.