diff mbox

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

Message ID 20180309155829.2fzgevhsxj3gnyly@armageddon.cambridge.arm.com (mailing list archive)
State New, archived
Headers show

Commit Message

Catalin Marinas March 9, 2018, 3:58 p.m. UTC 
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(+)

Comments

Andrey Konovalov March 9, 2018, 5:57 p.m. UTC | #1 
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.
diff mbox

Patch

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