[PATCH] x86/uaccess: Use pointer masking to limit uaccess speculation

[PATCH] x86/uaccess: Use pointer masking to limit uaccess speculation

[Josh Poimboeuf]

The x86 uaccess code uses barrier_nospec() in various places to prevent
speculative dereferencing of user-controlled pointers (which might be
combined with further gadgets or CPU bugs to leak data).

There are some issues with the current implementation:

- The barrier_nospec() in copy_from_user() was inadvertently removed
  with: 4b842e4e25b1 ("x86: get rid of small constant size cases in
  raw_copy_{to,from}_user()")

- copy_to_user() and friends should also have a speculation barrier,
  because a speculative write to a user-controlled address can still
  populate the cache line with the original data.

- The LFENCE in barrier_nospec() is overkill, when more lightweight user
  pointer masking can be used instead.

Remove all existing barrier_nospec() usage, and instead do user pointer
masking, throughout the x86 uaccess code.  This is similar to what arm64
is already doing.

barrier_nospec() is now unused, and can be removed.

Fixes: 4b842e4e25b1 ("x86: get rid of small constant size cases in raw_copy_{to,from}_user()")
Suggested-by: Will Deacon <will@kernel.org>
Signed-off-by: Josh Poimboeuf <jpoimboe@redhat.com>
---

Andy just pointed out Christoph's series to me, I think those patches
will have a minor conflict since uaccess_mask_ptr() relies on
user_addr_max().

 Documentation/admin-guide/hw-vuln/spectre.rst |  6 ++--
 arch/x86/include/asm/barrier.h                |  3 --
 arch/x86/include/asm/checksum_32.h            |  6 ++--
 arch/x86/include/asm/futex.h                  |  5 +++
 arch/x86/include/asm/uaccess.h                | 31 ++++++++++++-------
 arch/x86/include/asm/uaccess_64.h             | 16 +++++-----
 arch/x86/lib/csum-wrappers_64.c               |  6 ++--
 arch/x86/lib/getuser.S                        | 10 +++---
 arch/x86/lib/putuser.S                        |  8 +++++
 arch/x86/lib/usercopy_32.c                    |  2 +-
 arch/x86/lib/usercopy_64.c                    |  4 +--
 lib/iov_iter.c                                |  2 +-
 12 files changed, 58 insertions(+), 41 deletions(-)

diff --git a/Documentation/admin-guide/hw-vuln/spectre.rst b/Documentation/admin-guide/hw-vuln/spectre.rst
index e05e581af5cf..849c613afb35 100644
--- a/Documentation/admin-guide/hw-vuln/spectre.rst
+++ b/Documentation/admin-guide/hw-vuln/spectre.rst
@@ -426,9 +426,9 @@ Spectre variant 1
    <spec_ref2>` to avoid any usable disclosure gadgets. However, it may
    not cover all attack vectors for Spectre variant 1.
 
-   Copy-from-user code has an LFENCE barrier to prevent the access_ok()
-   check from being mis-speculated.  The barrier is done by the
-   barrier_nospec() macro.
+   Usercopy code uses user pointer masking to prevent the access_ok()
+   check from being mis-speculated in the success path with a kernel
+   address.  The masking is done by the uaccess_mask_ptr() macro.
 
    For the swapgs variant of Spectre variant 1, LFENCE barriers are
    added to interrupt, exception and NMI entry where needed.  These
diff --git a/arch/x86/include/asm/barrier.h b/arch/x86/include/asm/barrier.h
index 7f828fe49797..d158ea1fa250 100644
--- a/arch/x86/include/asm/barrier.h
+++ b/arch/x86/include/asm/barrier.h
@@ -48,9 +48,6 @@ static inline unsigned long array_index_mask_nospec(unsigned long index,
 /* Override the default implementation from linux/nospec.h. */
 #define array_index_mask_nospec array_index_mask_nospec
 
-/* Prevent speculative execution past this barrier. */
-#define barrier_nospec() alternative("", "lfence", X86_FEATURE_LFENCE_RDTSC)
-
 #define dma_rmb()	barrier()
 #define dma_wmb()	barrier()
 
diff --git a/arch/x86/include/asm/checksum_32.h b/arch/x86/include/asm/checksum_32.h
index 11624c8a9d8d..d2bbea5b0334 100644
--- a/arch/x86/include/asm/checksum_32.h
+++ b/arch/x86/include/asm/checksum_32.h
@@ -56,8 +56,8 @@ static inline __wsum csum_and_copy_from_user(const void __user *src,
 			*err_ptr = -EFAULT;
 		return sum;
 	}
-	ret = csum_partial_copy_generic((__force void *)src, dst,
-					len, sum, err_ptr, NULL);
+	ret = csum_partial_copy_generic((__force void *)uaccess_mask_ptr(src),
+					dst, len, sum, err_ptr, NULL);
 	user_access_end();
 
 	return ret;
@@ -185,7 +185,7 @@ static inline __wsum csum_and_copy_to_user(const void *src,
 
 	might_sleep();
 	if (user_access_begin(dst, len)) {
-		ret = csum_partial_copy_generic(src, (__force void *)dst,
+		ret = csum_partial_copy_generic(src, (__force void *)uaccess_mask_ptr(dst),
 						len, sum, NULL, err_ptr);
 		user_access_end();
 		return ret;
diff --git a/arch/x86/include/asm/futex.h b/arch/x86/include/asm/futex.h
index f9c00110a69a..89e7e37c2b25 100644
--- a/arch/x86/include/asm/futex.h
+++ b/arch/x86/include/asm/futex.h
@@ -59,6 +59,8 @@ static __always_inline int arch_futex_atomic_op_inuser(int op, int oparg, int *o
 	if (!user_access_begin(uaddr, sizeof(u32)))
 		return -EFAULT;
 
+	uaddr = uaccess_mask_ptr(uaddr);
+
 	switch (op) {
 	case FUTEX_OP_SET:
 		unsafe_atomic_op1("xchgl %0, %2", oval, uaddr, oparg, Efault);
@@ -94,6 +96,9 @@ static inline int futex_atomic_cmpxchg_inatomic(u32 *uval, u32 __user *uaddr,
 
 	if (!user_access_begin(uaddr, sizeof(u32)))
 		return -EFAULT;
+
+	uaddr = uaccess_mask_ptr(uaddr);
+
 	asm volatile("\n"
 		"1:\t" LOCK_PREFIX "cmpxchgl %4, %2\n"
 		"2:\n"
diff --git a/arch/x86/include/asm/uaccess.h b/arch/x86/include/asm/uaccess.h
index ecefaffd15d4..e697b42d0533 100644
--- a/arch/x86/include/asm/uaccess.h
+++ b/arch/x86/include/asm/uaccess.h
@@ -6,6 +6,7 @@
  */
 #include <linux/compiler.h>
 #include <linux/kasan-checks.h>
+#include <linux/nospec.h>
 #include <linux/string.h>
 #include <asm/asm.h>
 #include <asm/page.h>
@@ -96,6 +97,15 @@ static inline bool pagefault_disabled(void);
 	likely(!__range_not_ok(addr, size, user_addr_max()));		\
 })
 
+/*
+ * Sanitize a uaccess pointer such that it becomes NULL if it's not a valid
+ * user pointer.  This blocks speculative dereferences of user-controlled
+ * pointers.
+ */
+#define uaccess_mask_ptr(ptr) \
+	(__typeof__(ptr)) array_index_nospec((__force unsigned long)ptr, user_addr_max())
+
+
 /*
  * These are the main single-value transfer routines.  They automatically
  * use the right size if we just have the right pointer type.
@@ -119,11 +129,6 @@ extern int __get_user_bad(void);
 
 #define __uaccess_begin() stac()
 #define __uaccess_end()   clac()
-#define __uaccess_begin_nospec()	\
-({					\
-	stac();				\
-	barrier_nospec();		\
-})
 
 /*
  * This is the smallest unsigned integer type that can fit a value
@@ -357,7 +362,7 @@ do {									\
 	__label__ __pu_label;					\
 	int __pu_err = -EFAULT;					\
 	__typeof__(*(ptr)) __pu_val = (x);			\
-	__typeof__(ptr) __pu_ptr = (ptr);			\
+	__typeof__(ptr) __pu_ptr = uaccess_mask_ptr(ptr);	\
 	__typeof__(size) __pu_size = (size);			\
 	__uaccess_begin();					\
 	__put_user_size(__pu_val, __pu_ptr, __pu_size, __pu_label);	\
@@ -371,9 +376,9 @@ __pu_label:							\
 ({									\
 	int __gu_err;							\
 	__inttype(*(ptr)) __gu_val;					\
-	__typeof__(ptr) __gu_ptr = (ptr);				\
+	__typeof__(ptr) __gu_ptr = uaccess_mask_ptr(ptr);		\
 	__typeof__(size) __gu_size = (size);				\
-	__uaccess_begin_nospec();					\
+	__uaccess_begin();						\
 	__get_user_size(__gu_val, __gu_ptr, __gu_size, __gu_err);	\
 	__uaccess_end();						\
 	(x) = (__force __typeof__(*(ptr)))__gu_val;			\
@@ -482,7 +487,7 @@ static __must_check __always_inline bool user_access_begin(const void __user *pt
 {
 	if (unlikely(!access_ok(ptr,len)))
 		return 0;
-	__uaccess_begin_nospec();
+	__uaccess_begin();
 	return 1;
 }
 #define user_access_begin(a,b)	user_access_begin(a,b)
@@ -491,14 +496,16 @@ static __must_check __always_inline bool user_access_begin(const void __user *pt
 #define user_access_save()	smap_save()
 #define user_access_restore(x)	smap_restore(x)
 
-#define unsafe_put_user(x, ptr, label)	\
-	__put_user_size((__typeof__(*(ptr)))(x), (ptr), sizeof(*(ptr)), label)
+#define unsafe_put_user(x, ptr, label)						\
+	__put_user_size((__typeof__(*(ptr)))(x), uaccess_mask_ptr(ptr),		\
+			sizeof(*(ptr)), label)
 
 #define unsafe_get_user(x, ptr, err_label)					\
 do {										\
 	int __gu_err;								\
 	__inttype(*(ptr)) __gu_val;						\
-	__get_user_size(__gu_val, (ptr), sizeof(*(ptr)), __gu_err);		\
+	__get_user_size(__gu_val, uaccess_mask_ptr(ptr), sizeof(*(ptr)),	\
+			__gu_err);						\
 	(x) = (__force __typeof__(*(ptr)))__gu_val;				\
 	if (unlikely(__gu_err)) goto err_label;					\
 } while (0)
diff --git a/arch/x86/include/asm/uaccess_64.h b/arch/x86/include/asm/uaccess_64.h
index bc10e3dc64fe..ef859e1ae956 100644
--- a/arch/x86/include/asm/uaccess_64.h
+++ b/arch/x86/include/asm/uaccess_64.h
@@ -47,7 +47,7 @@ copy_user_generic(void *to, const void *from, unsigned len)
 }
 
 static __always_inline __must_check unsigned long
-copy_to_user_mcsafe(void *to, const void *from, unsigned len)
+copy_to_user_mcsafe(void __user *to, const void *from, size_t len)
 {
 	unsigned long ret;
 
@@ -57,7 +57,7 @@ copy_to_user_mcsafe(void *to, const void *from, unsigned len)
 	 * handle exceptions / faults.  memcpy_mcsafe() may fall back to
 	 * memcpy() which lacks this handling.
 	 */
-	ret = __memcpy_mcsafe(to, from, len);
+	ret = __memcpy_mcsafe((__force void *)uaccess_mask_ptr(to), from, len);
 	__uaccess_end();
 	return ret;
 }
@@ -65,20 +65,20 @@ copy_to_user_mcsafe(void *to, const void *from, unsigned len)
 static __always_inline __must_check unsigned long
 raw_copy_from_user(void *dst, const void __user *src, unsigned long size)
 {
-	return copy_user_generic(dst, (__force void *)src, size);
+	return copy_user_generic(dst, (__force void *)uaccess_mask_ptr(src), size);
 }
 
 static __always_inline __must_check unsigned long
 raw_copy_to_user(void __user *dst, const void *src, unsigned long size)
 {
-	return copy_user_generic((__force void *)dst, src, size);
+	return copy_user_generic((__force void *)uaccess_mask_ptr(dst), src, size);
 }
 
 static __always_inline __must_check
 unsigned long raw_copy_in_user(void __user *dst, const void __user *src, unsigned long size)
 {
-	return copy_user_generic((__force void *)dst,
-				 (__force void *)src, size);
+	return copy_user_generic((__force void *)uaccess_mask_ptr(dst),
+				 (__force void *)uaccess_mask_ptr(src), size);
 }
 
 extern long __copy_user_nocache(void *dst, const void __user *src,
@@ -93,14 +93,14 @@ __copy_from_user_inatomic_nocache(void *dst, const void __user *src,
 				  unsigned size)
 {
 	kasan_check_write(dst, size);
-	return __copy_user_nocache(dst, src, size, 0);
+	return __copy_user_nocache(dst, uaccess_mask_ptr(src), size, 0);
 }
 
 static inline int
 __copy_from_user_flushcache(void *dst, const void __user *src, unsigned size)
 {
 	kasan_check_write(dst, size);
-	return __copy_user_flushcache(dst, src, size);
+	return __copy_user_flushcache(dst, uaccess_mask_ptr(src), size);
 }
 
 unsigned long
diff --git a/arch/x86/lib/csum-wrappers_64.c b/arch/x86/lib/csum-wrappers_64.c
index ee63d7576fd2..d677b91a7e84 100644
--- a/arch/x86/lib/csum-wrappers_64.c
+++ b/arch/x86/lib/csum-wrappers_64.c
@@ -52,8 +52,8 @@ csum_and_copy_from_user(const void __user *src, void *dst,
 			len -= 2;
 		}
 	}
-	isum = csum_partial_copy_generic((__force const void *)src,
-				dst, len, isum, errp, NULL);
+	isum = csum_partial_copy_generic((__force const void *)uaccess_mask_ptr(src),
+					 dst, len, isum, errp, NULL);
 	user_access_end();
 	if (unlikely(*errp))
 		goto out_err;
@@ -108,7 +108,7 @@ csum_and_copy_to_user(const void *src, void __user *dst,
 	}
 
 	*errp = 0;
-	ret = csum_partial_copy_generic(src, (void __force *)dst,
+	ret = csum_partial_copy_generic(src, (void __force *)uaccess_mask_ptr(dst),
 					len, isum, NULL, errp);
 	user_access_end();
 	return ret;
diff --git a/arch/x86/lib/getuser.S b/arch/x86/lib/getuser.S
index c8a85b512796..90ae5be8af23 100644
--- a/arch/x86/lib/getuser.S
+++ b/arch/x86/lib/getuser.S
@@ -40,7 +40,7 @@ SYM_FUNC_START(__get_user_1)
 	mov PER_CPU_VAR(current_task), %_ASM_DX
 	cmp TASK_addr_limit(%_ASM_DX),%_ASM_AX
 	jae bad_get_user
-	sbb %_ASM_DX, %_ASM_DX		/* array_index_mask_nospec() */
+	sbb %_ASM_DX, %_ASM_DX		/* uaccess_mask_ptr() */
 	and %_ASM_DX, %_ASM_AX
 	ASM_STAC
 1:	movzbl (%_ASM_AX),%edx
@@ -56,7 +56,7 @@ SYM_FUNC_START(__get_user_2)
 	mov PER_CPU_VAR(current_task), %_ASM_DX
 	cmp TASK_addr_limit(%_ASM_DX),%_ASM_AX
 	jae bad_get_user
-	sbb %_ASM_DX, %_ASM_DX		/* array_index_mask_nospec() */
+	sbb %_ASM_DX, %_ASM_DX		/* uaccess_mask_ptr() */
 	and %_ASM_DX, %_ASM_AX
 	ASM_STAC
 2:	movzwl -1(%_ASM_AX),%edx
@@ -72,7 +72,7 @@ SYM_FUNC_START(__get_user_4)
 	mov PER_CPU_VAR(current_task), %_ASM_DX
 	cmp TASK_addr_limit(%_ASM_DX),%_ASM_AX
 	jae bad_get_user
-	sbb %_ASM_DX, %_ASM_DX		/* array_index_mask_nospec() */
+	sbb %_ASM_DX, %_ASM_DX		/* uaccess_mask_ptr() */
 	and %_ASM_DX, %_ASM_AX
 	ASM_STAC
 3:	movl -3(%_ASM_AX),%edx
@@ -89,7 +89,7 @@ SYM_FUNC_START(__get_user_8)
 	mov PER_CPU_VAR(current_task), %_ASM_DX
 	cmp TASK_addr_limit(%_ASM_DX),%_ASM_AX
 	jae bad_get_user
-	sbb %_ASM_DX, %_ASM_DX		/* array_index_mask_nospec() */
+	sbb %_ASM_DX, %_ASM_DX		/* uaccess_mask_ptr() */
 	and %_ASM_DX, %_ASM_AX
 	ASM_STAC
 4:	movq -7(%_ASM_AX),%rdx
@@ -102,7 +102,7 @@ SYM_FUNC_START(__get_user_8)
 	mov PER_CPU_VAR(current_task), %_ASM_DX
 	cmp TASK_addr_limit(%_ASM_DX),%_ASM_AX
 	jae bad_get_user_8
-	sbb %_ASM_DX, %_ASM_DX		/* array_index_mask_nospec() */
+	sbb %_ASM_DX, %_ASM_DX		/* uaccess_mask_ptr() */
 	and %_ASM_DX, %_ASM_AX
 	ASM_STAC
 4:	movl -7(%_ASM_AX),%edx
diff --git a/arch/x86/lib/putuser.S b/arch/x86/lib/putuser.S
index 7c7c92db8497..a4384b0ce013 100644
--- a/arch/x86/lib/putuser.S
+++ b/arch/x86/lib/putuser.S
@@ -38,6 +38,8 @@ SYM_FUNC_START(__put_user_1)
 	ENTER
 	cmp TASK_addr_limit(%_ASM_BX),%_ASM_CX
 	jae .Lbad_put_user
+	sbb %_ASM_BX, %_ASM_BX		/* uaccess_mask_ptr() */
+	and %_ASM_BX, %_ASM_CX
 	ASM_STAC
 1:	movb %al,(%_ASM_CX)
 	xor %eax,%eax
@@ -52,6 +54,8 @@ SYM_FUNC_START(__put_user_2)
 	sub $1,%_ASM_BX
 	cmp %_ASM_BX,%_ASM_CX
 	jae .Lbad_put_user
+	sbb %_ASM_BX, %_ASM_BX		/* uaccess_mask_ptr() */
+	and %_ASM_BX, %_ASM_CX
 	ASM_STAC
 2:	movw %ax,(%_ASM_CX)
 	xor %eax,%eax
@@ -66,6 +70,8 @@ SYM_FUNC_START(__put_user_4)
 	sub $3,%_ASM_BX
 	cmp %_ASM_BX,%_ASM_CX
 	jae .Lbad_put_user
+	sbb %_ASM_BX, %_ASM_BX		/* uaccess_mask_ptr() */
+	and %_ASM_BX, %_ASM_CX
 	ASM_STAC
 3:	movl %eax,(%_ASM_CX)
 	xor %eax,%eax
@@ -80,6 +86,8 @@ SYM_FUNC_START(__put_user_8)
 	sub $7,%_ASM_BX
 	cmp %_ASM_BX,%_ASM_CX
 	jae .Lbad_put_user
+	sbb %_ASM_BX, %_ASM_BX		/* uaccess_mask_ptr() */
+	and %_ASM_BX, %_ASM_CX
 	ASM_STAC
 4:	mov %_ASM_AX,(%_ASM_CX)
 #ifdef CONFIG_X86_32
diff --git a/arch/x86/lib/usercopy_32.c b/arch/x86/lib/usercopy_32.c
index 7d290777246d..ea0eae7a68fa 100644
--- a/arch/x86/lib/usercopy_32.c
+++ b/arch/x86/lib/usercopy_32.c
@@ -68,7 +68,7 @@ clear_user(void __user *to, unsigned long n)
 {
 	might_fault();
 	if (access_ok(to, n))
-		__do_clear_user(to, n);
+		__do_clear_user(uaccess_mask_ptr(to), n);
 	return n;
 }
 EXPORT_SYMBOL(clear_user);
diff --git a/arch/x86/lib/usercopy_64.c b/arch/x86/lib/usercopy_64.c
index b0dfac3d3df7..abe5d2d1c49a 100644
--- a/arch/x86/lib/usercopy_64.c
+++ b/arch/x86/lib/usercopy_64.c
@@ -51,7 +51,7 @@ EXPORT_SYMBOL(__clear_user);
 unsigned long clear_user(void __user *to, unsigned long n)
 {
 	if (access_ok(to, n))
-		return __clear_user(to, n);
+		return __clear_user(uaccess_mask_ptr(to), n);
 	return n;
 }
 EXPORT_SYMBOL(clear_user);
@@ -108,7 +108,7 @@ EXPORT_SYMBOL_GPL(arch_wb_cache_pmem);
 long __copy_user_flushcache(void *dst, const void __user *src, unsigned size)
 {
 	unsigned long flushed, dest = (unsigned long) dst;
-	long rc = __copy_user_nocache(dst, src, size, 0);
+	long rc = __copy_user_nocache(dst, uaccess_mask_ptr(src), size, 0);
 
 	/*
 	 * __copy_user_nocache() uses non-temporal stores for the bulk
diff --git a/lib/iov_iter.c b/lib/iov_iter.c
index 5e40786c8f12..175cc8320220 100644
--- a/lib/iov_iter.c
+++ b/lib/iov_iter.c
@@ -642,7 +642,7 @@ static int copyout_mcsafe(void __user *to, const void *from, size_t n)
 {
 	if (access_ok(to, n)) {
 		instrument_copy_to_user(to, from, n);
-		n = copy_to_user_mcsafe((__force void *) to, from, n);
+		n = copy_to_user_mcsafe(to, from, n);
 	}
 	return n;
 }
-- 
2.25.4

Re: [PATCH] x86/uaccess: Use pointer masking to limit uaccess speculation

[Andy Lutomirski]

On Wed, Aug 19, 2020 at 7:50 AM Josh Poimboeuf <jpoimboe@redhat.com> wrote:
> +/*
> + * Sanitize a uaccess pointer such that it becomes NULL if it's not a valid
> + * user pointer.  This blocks speculative dereferences of user-controlled
> + * pointers.
> + */
> +#define uaccess_mask_ptr(ptr) \
> +       (__typeof__(ptr)) array_index_nospec((__force unsigned long)ptr, user_addr_max())
> +

If I dug through all the macros correctly, this is generating a fairly
complex pile of math to account for the fact that user_addr_max() is
variable and that it's a nasty number.

But I don't think there's any particular need to use the real maximum
user address here.  Allowing a mis-speculated user access to a
non-canonical address or to the top guard page of the lower canonical
region is harmless.  With current kernels, a sequence like:

if (likely((long)addr > 0) {
  masked_addr = addr & 0x7FFFFFFFFFFFFFFFUL;
} else {
  if (kernel fs) {
    masked_addr = addr;
  } else {
    EFAULT;
  }
}

could plausibly be better.  But Christoph's series fixes this whole
mess, and I think that this should be:

#define uaccess_mask_ptr(ptr) ((__typeof___(ptr)) (__force unsigned
long)ptr & USER_ADDR_MASK))

where USER_ADDR_MASK is the appropriate value for 32-bit or 64-bit.

Re: [PATCH] x86/uaccess: Use pointer masking to limit uaccess speculation

[Josh Poimboeuf]

On Wed, Aug 19, 2020 at 09:39:10AM -0700, Andy Lutomirski wrote:
> On Wed, Aug 19, 2020 at 7:50 AM Josh Poimboeuf <jpoimboe@redhat.com> wrote:
> > +/*
> > + * Sanitize a uaccess pointer such that it becomes NULL if it's not a valid
> > + * user pointer.  This blocks speculative dereferences of user-controlled
> > + * pointers.
> > + */
> > +#define uaccess_mask_ptr(ptr) \
> > +       (__typeof__(ptr)) array_index_nospec((__force unsigned long)ptr, user_addr_max())
> > +
> 
> If I dug through all the macros correctly, this is generating a fairly
> complex pile of math to account for the fact that user_addr_max() is
> variable and that it's a nasty number.

The math is actually pretty simple.  It's identical to what getuser.S is
doing:

	cmp TASK_addr_limit(%_ASM_DX),%_ASM_AX
	sbb %_ASM_DX, %_ASM_DX
	and %_ASM_DX, %_ASM_AX

> But I don't think there's any particular need to use the real maximum
> user address here.  Allowing a mis-speculated user access to a
> non-canonical address or to the top guard page of the lower canonical
> region is harmless.  With current kernels, a sequence like:
> 
> if (likely((long)addr > 0) {
>   masked_addr = addr & 0x7FFFFFFFFFFFFFFFUL;
> } else {
>   if (kernel fs) {
>     masked_addr = addr;
>   } else {
>     EFAULT;
>   }
> }

The masking has to be done without conditional branches, otherwise it
defeats the point.

> could plausibly be better.  But Christoph's series fixes this whole
> mess, and I think that this should be:
> 
> #define uaccess_mask_ptr(ptr) ((__typeof___(ptr)) (__force unsigned
> long)ptr & USER_ADDR_MASK))
> 
> where USER_ADDR_MASK is the appropriate value for 32-bit or 64-bit.

Yeah, we could do that.  Though in the meantime, the simple merge
conflict resolution with Christoph's patches would be
s/user_addr_max/TASK_SIZE_MAX/ in my uaccess_mask_ptr() macro.

-- 
Josh

RE: [PATCH] x86/uaccess: Use pointer masking to limit uaccess speculation

[David Laight]

From: Josh Poimboeuf
> Sent: 19 August 2020 18:02
> 
> On Wed, Aug 19, 2020 at 09:39:10AM -0700, Andy Lutomirski wrote:
> > On Wed, Aug 19, 2020 at 7:50 AM Josh Poimboeuf <jpoimboe@redhat.com> wrote:
> > > +/*
> > > + * Sanitize a uaccess pointer such that it becomes NULL if it's not a valid
> > > + * user pointer.  This blocks speculative dereferences of user-controlled
> > > + * pointers.
> > > + */
> > > +#define uaccess_mask_ptr(ptr) \
> > > +       (__typeof__(ptr)) array_index_nospec((__force unsigned long)ptr, user_addr_max())
> > > +
> >
> > If I dug through all the macros correctly, this is generating a fairly
> > complex pile of math to account for the fact that user_addr_max() is
> > variable and that it's a nasty number.
> 
> The math is actually pretty simple.  It's identical to what getuser.S is
> doing:
> 
> 	cmp TASK_addr_limit(%_ASM_DX),%_ASM_AX
> 	sbb %_ASM_DX, %_ASM_DX
> 	and %_ASM_DX, %_ASM_AX
> 
> > But I don't think there's any particular need to use the real maximum
> > user address here.  Allowing a mis-speculated user access to a
> > non-canonical address or to the top guard page of the lower canonical
> > region is harmless.  With current kernels, a sequence like:
> >
> > if (likely((long)addr > 0) {
> >   masked_addr = addr & 0x7FFFFFFFFFFFFFFFUL;
> > } else {
> >   if (kernel fs) {
> >     masked_addr = addr;
> >   } else {
> >     EFAULT;
> >   }
> > }
> 
> The masking has to be done without conditional branches, otherwise it
> defeats the point.
> 
> > could plausibly be better.  But Christoph's series fixes this whole
> > mess, and I think that this should be:
> >
> > #define uaccess_mask_ptr(ptr) ((__typeof___(ptr)) (__force unsigned
> > long)ptr & USER_ADDR_MASK))
> >
> > where USER_ADDR_MASK is the appropriate value for 32-bit or 64-bit.
> 
> Yeah, we could do that.  Though in the meantime, the simple merge
> conflict resolution with Christoph's patches would be
> s/user_addr_max/TASK_SIZE_MAX/ in my uaccess_mask_ptr() macro.

For access_ok(ptr, size) I think you can do:
	(ptr | (ptr + size)) & (64bit ? 1 << 63 : 3 << 30)

Masking on 32bit is harder, something like.
Subtract 0xc0000000 (sets carry for user addresses)
sbb reg,reg to generate 0 (kernel) or ~0 (user).
And with the address - kernel addresses are now zero.

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)

Re: [PATCH] x86/uaccess: Use pointer masking to limit uaccess speculation

[Andy Lutomirski]

On Wed, Aug 19, 2020 at 10:02 AM Josh Poimboeuf <jpoimboe@redhat.com> wrote:
>
> On Wed, Aug 19, 2020 at 09:39:10AM -0700, Andy Lutomirski wrote:
> > On Wed, Aug 19, 2020 at 7:50 AM Josh Poimboeuf <jpoimboe@redhat.com> wrote:
> > > +/*
> > > + * Sanitize a uaccess pointer such that it becomes NULL if it's not a valid
> > > + * user pointer.  This blocks speculative dereferences of user-controlled
> > > + * pointers.
> > > + */
> > > +#define uaccess_mask_ptr(ptr) \
> > > +       (__typeof__(ptr)) array_index_nospec((__force unsigned long)ptr, user_addr_max())
> > > +
> >
> > If I dug through all the macros correctly, this is generating a fairly
> > complex pile of math to account for the fact that user_addr_max() is
> > variable and that it's a nasty number.
>
> The math is actually pretty simple.  It's identical to what getuser.S is
> doing:
>
>         cmp TASK_addr_limit(%_ASM_DX),%_ASM_AX
>         sbb %_ASM_DX, %_ASM_DX
>         and %_ASM_DX, %_ASM_AX
>
> > But I don't think there's any particular need to use the real maximum
> > user address here.  Allowing a mis-speculated user access to a
> > non-canonical address or to the top guard page of the lower canonical
> > region is harmless.  With current kernels, a sequence like:
> >
> > if (likely((long)addr > 0) {
> >   masked_addr = addr & 0x7FFFFFFFFFFFFFFFUL;
> > } else {
> >   if (kernel fs) {
> >     masked_addr = addr;
> >   } else {
> >     EFAULT;
> >   }
> > }
>
> The masking has to be done without conditional branches, otherwise it
> defeats the point.

Ugh.  Fair enough.

> Yeah, we could do that.  Though in the meantime, the simple merge
> conflict resolution with Christoph's patches would be
> s/user_addr_max/TASK_SIZE_MAX/ in my uaccess_mask_ptr() macro.

Okay.  I will try to help remember :)

--Andy

Re: [PATCH] x86/uaccess: Use pointer masking to limit uaccess speculation

[Josh Poimboeuf]

On Wed, Aug 19, 2020 at 09:50:06AM -0500, Josh Poimboeuf wrote:
> The x86 uaccess code uses barrier_nospec() in various places to prevent
> speculative dereferencing of user-controlled pointers (which might be
> combined with further gadgets or CPU bugs to leak data).
> 
> There are some issues with the current implementation:
> 
> - The barrier_nospec() in copy_from_user() was inadvertently removed
>   with: 4b842e4e25b1 ("x86: get rid of small constant size cases in
>   raw_copy_{to,from}_user()")
> 
> - copy_to_user() and friends should also have a speculation barrier,
>   because a speculative write to a user-controlled address can still
>   populate the cache line with the original data.
> 
> - The LFENCE in barrier_nospec() is overkill, when more lightweight user
>   pointer masking can be used instead.
> 
> Remove all existing barrier_nospec() usage, and instead do user pointer
> masking, throughout the x86 uaccess code.  This is similar to what arm64
> is already doing.
> 
> barrier_nospec() is now unused, and can be removed.
> 
> Fixes: 4b842e4e25b1 ("x86: get rid of small constant size cases in raw_copy_{to,from}_user()")
> Suggested-by: Will Deacon <will@kernel.org>
> Signed-off-by: Josh Poimboeuf <jpoimboe@redhat.com>

Ping?

-- 
Josh

RE: [PATCH] x86/uaccess: Use pointer masking to limit uaccess speculation

[David Laight]

From: Josh Poimboeuf
> Sent: 28 August 2020 20:29
> 
> On Wed, Aug 19, 2020 at 09:50:06AM -0500, Josh Poimboeuf wrote:
> > The x86 uaccess code uses barrier_nospec() in various places to prevent
> > speculative dereferencing of user-controlled pointers (which might be
> > combined with further gadgets or CPU bugs to leak data).
> >
> > There are some issues with the current implementation:
> >
> > - The barrier_nospec() in copy_from_user() was inadvertently removed
> >   with: 4b842e4e25b1 ("x86: get rid of small constant size cases in
> >   raw_copy_{to,from}_user()")
> >
> > - copy_to_user() and friends should also have a speculation barrier,
> >   because a speculative write to a user-controlled address can still
> >   populate the cache line with the original data.
> >
> > - The LFENCE in barrier_nospec() is overkill, when more lightweight user
> >   pointer masking can be used instead.
> >
> > Remove all existing barrier_nospec() usage, and instead do user pointer
> > masking, throughout the x86 uaccess code.  This is similar to what arm64
> > is already doing.
> >
> > barrier_nospec() is now unused, and can be removed.
> >
> > Fixes: 4b842e4e25b1 ("x86: get rid of small constant size cases in raw_copy_{to,from}_user()")
> > Suggested-by: Will Deacon <will@kernel.org>
> > Signed-off-by: Josh Poimboeuf <jpoimboe@redhat.com>
> 
> Ping?

Rereading the patch it looks like a lot of bloat (as well as a
lot of changes).
Does the array_mask even work on 32bit archs where the kernel
base address is 0xc0000000?
I'm sure there is something much simpler.

If access_ok() generates ~0u or 0 without a conditional then
the address can be masked with the result.
So you probably need to change access_ok() to take the address
of the user pointer - so the callers become like:
	if (access_ok(&user_buffer, len))
		return -EFAULT
	__put_user(user_buffer, value);

It would be easier if NULL were guaranteed to be an invalid
user address (is it?).
Then access_ok() could return the modified pointer.
So you get something like:
	user_buffer = access_ok(user_buffer, len);
	if (!user_buffer)
		return -EFAULT.

Provided the 'last' user page is never allocated (it can't
be on i386 due to cpu prefetch issues) something like:
(and with the asm probably all broken)

static inline void __user * access_ok(void __user *b, size_t len)
{
	unsigned long x = (long)b | (long)(b + len);
	unsigned long lim = 64_bit ? 1u << 63 : 0x40000000;
	asm volatile (" add %1, %0\n"
			" sbb $0, %0", "=r" (x), "r" (lim));
	return (void __user *)(long)b & ~x);
}

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)

RE: [PATCH] x86/uaccess: Use pointer masking to limit uaccess speculation

[David Laight]

From: David Laight
> Sent: 29 August 2020 14:21
> 
> From: Josh Poimboeuf
> > Sent: 28 August 2020 20:29
> >
> > On Wed, Aug 19, 2020 at 09:50:06AM -0500, Josh Poimboeuf wrote:
> > > The x86 uaccess code uses barrier_nospec() in various places to prevent
> > > speculative dereferencing of user-controlled pointers (which might be
> > > combined with further gadgets or CPU bugs to leak data).
> > >
> > > There are some issues with the current implementation:
> > >
> > > - The barrier_nospec() in copy_from_user() was inadvertently removed
> > >   with: 4b842e4e25b1 ("x86: get rid of small constant size cases in
> > >   raw_copy_{to,from}_user()")
> > >
> > > - copy_to_user() and friends should also have a speculation barrier,
> > >   because a speculative write to a user-controlled address can still
> > >   populate the cache line with the original data.
> > >
> > > - The LFENCE in barrier_nospec() is overkill, when more lightweight user
> > >   pointer masking can be used instead.
> > >
> > > Remove all existing barrier_nospec() usage, and instead do user pointer
> > > masking, throughout the x86 uaccess code.  This is similar to what arm64
> > > is already doing.
> > >
> > > barrier_nospec() is now unused, and can be removed.
> > >
> > > Fixes: 4b842e4e25b1 ("x86: get rid of small constant size cases in raw_copy_{to,from}_user()")
> > > Suggested-by: Will Deacon <will@kernel.org>
> > > Signed-off-by: Josh Poimboeuf <jpoimboe@redhat.com>
> >
> > Ping?
> 
> Rereading the patch it looks like a lot of bloat (as well as a
> lot of changes).
> Does the array_mask even work on 32bit archs where the kernel
> base address is 0xc0000000?
> I'm sure there is something much simpler.
> 
> If access_ok() generates ~0u or 0 without a conditional then
> the address can be masked with the result.
> So you probably need to change access_ok() to take the address
> of the user pointer - so the callers become like:
> 	if (access_ok(&user_buffer, len))
> 		return -EFAULT
> 	__put_user(user_buffer, value);
> 
> It would be easier if NULL were guaranteed to be an invalid
> user address (is it?).
> Then access_ok() could return the modified pointer.
> So you get something like:
> 	user_buffer = access_ok(user_buffer, len);
> 	if (!user_buffer)
> 		return -EFAULT.
> 
> Provided the 'last' user page is never allocated (it can't
> be on i386 due to cpu prefetch issues) something like:
> (and with the asm probably all broken)
> 
> static inline void __user * access_ok(void __user *b, size_t len)
> {
> 	unsigned long x = (long)b | (long)(b + len);
> 	unsigned long lim = 64_bit ? 1u << 63 : 0x40000000;
> 	asm volatile (" add %1, %0\n"
> 			" sbb $0, %0", "=r" (x), "r" (lim));
> 	return (void __user *)(long)b & ~x);
> }

Actually, thinking further, if:
1) the access_ok() immediately precedes the user copy (as it should).
2) the user-copies use a sensible 'increasing address' copy.
and
3) there is a 'guard page' between valid user and kernel addresses.
Then access_ok() only need check the base address of the user buffer.

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)

Re: [PATCH] x86/uaccess: Use pointer masking to limit uaccess speculation

[Josh Poimboeuf]

On Sat, Aug 29, 2020 at 07:31:20PM +0000, David Laight wrote:
> > Rereading the patch it looks like a lot of bloat (as well as a
> > lot of changes).
> > Does the array_mask even work on 32bit archs where the kernel
> > base address is 0xc0000000?

Why wouldn't it on work on 32-bit?  My patch does have a minor compile
bug on 32-bit, but otherwise it seems to work (i.e., the asm looks ok,
and it boots).

> > I'm sure there is something much simpler.
> > 
> > If access_ok() generates ~0u or 0 without a conditional then
> > the address can be masked with the result.
> > So you probably need to change access_ok() to take the address
> > of the user pointer - so the callers become like:
> > 	if (access_ok(&user_buffer, len))
> > 		return -EFAULT
> > 	__put_user(user_buffer, value);
> > 
> > It would be easier if NULL were guaranteed to be an invalid
> > user address (is it?).
> > Then access_ok() could return the modified pointer.
> > So you get something like:
> > 	user_buffer = access_ok(user_buffer, len);
> > 	if (!user_buffer)
> > 		return -EFAULT.
> > 
> > Provided the 'last' user page is never allocated (it can't
> > be on i386 due to cpu prefetch issues) something like:
> > (and with the asm probably all broken)
> > 
> > static inline void __user * access_ok(void __user *b, size_t len)
> > {
> > 	unsigned long x = (long)b | (long)(b + len);
> > 	unsigned long lim = 64_bit ? 1u << 63 : 0x40000000;
> > 	asm volatile (" add %1, %0\n"
> > 			" sbb $0, %0", "=r" (x), "r" (lim));
> > 	return (void __user *)(long)b & ~x);
> > }
> 
> Actually, thinking further, if:
> 1) the access_ok() immediately precedes the user copy (as it should).
> 2) the user-copies use a sensible 'increasing address' copy.
> and
> 3) there is a 'guard page' between valid user and kernel addresses.
> Then access_ok() only need check the base address of the user buffer.

Yes, it would make sense to put the masking in access_ok() somehow.  But
to do it properly, I think we'd first need to make access_ok() generic.
Maybe that's do-able, but it would be a much bigger patch set.

First I'd prefer to just fix x86, like my patch does.  Then we could do
an access_ok() rework.

-- 
Josh

RE: [PATCH] x86/uaccess: Use pointer masking to limit uaccess speculation

[David Laight]

From: Josh Poimboeuf
> Sent: 31 August 2020 18:31
> On Sat, Aug 29, 2020 at 07:31:20PM +0000, David Laight wrote:
> > > Rereading the patch it looks like a lot of bloat (as well as a
> > > lot of changes).
> > > Does the array_mask even work on 32bit archs where the kernel
> > > base address is 0xc0000000?
> 
> Why wouldn't it on work on 32-bit?  My patch does have a minor compile
> bug on 32-bit, but otherwise it seems to work (i.e., the asm looks ok,
> and it boots).

As usual I hadn't looked closely enough into the masked_array internals.

...
> > Actually, thinking further, if:
> > 1) the access_ok() immediately precedes the user copy (as it should).
> > 2) the user-copies use a sensible 'increasing address' copy.
> > and
> > 3) there is a 'guard page' between valid user and kernel addresses.
> > Then access_ok() only need check the base address of the user buffer.
> 
> Yes, it would make sense to put the masking in access_ok() somehow.  But
> to do it properly, I think we'd first need to make access_ok() generic.
> Maybe that's do-able, but it would be a much bigger patch set.
> 
> First I'd prefer to just fix x86, like my patch does.  Then we could do
> an access_ok() rework.

If you do a modified access_ok() you get to (slowly) collect all
the important paths.
No point replicating the same test.

A lot of the access_ok() can be deleted - maybe remove some __
from the following functions.
Or change to the variants that enable user-space accesses.

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)

Re: [PATCH] x86/uaccess: Use pointer masking to limit uaccess speculation

[Mark Rutland]

On Wed, Aug 19, 2020 at 09:50:06AM -0500, Josh Poimboeuf wrote:
> The x86 uaccess code uses barrier_nospec() in various places to prevent
> speculative dereferencing of user-controlled pointers (which might be
> combined with further gadgets or CPU bugs to leak data).
> 
> There are some issues with the current implementation:
> 
> - The barrier_nospec() in copy_from_user() was inadvertently removed
>   with: 4b842e4e25b1 ("x86: get rid of small constant size cases in
>   raw_copy_{to,from}_user()")
> 
> - copy_to_user() and friends should also have a speculation barrier,
>   because a speculative write to a user-controlled address can still
>   populate the cache line with the original data.
> 
> - The LFENCE in barrier_nospec() is overkill, when more lightweight user
>   pointer masking can be used instead.
> 
> Remove all existing barrier_nospec() usage, and instead do user pointer
> masking, throughout the x86 uaccess code.  This is similar to what arm64
> is already doing.
> 
> barrier_nospec() is now unused, and can be removed.

One thing to consider is whether you need a speculation barrier after
set_fs(). Otherwise for code like:

| fs = get_fs();
| if (cond)
|	set_fs(KERNEL_DS);
| copy_to_user(...)
| set_fs(fs)

... the set_fs() can occur speculatively, and may be able to satisfy
the masking logic if forwarded within the cpu.

See arm64 commit:

  c2f0ad4fc089cff8 ("arm64: uaccess: Prevent speculative use of the current addr_limit")

Thanks,
Mark.

Re: [PATCH] x86/uaccess: Use pointer masking to limit uaccess speculation

[Josh Poimboeuf]

On Tue, Sep 01, 2020 at 03:02:08PM +0100, Mark Rutland wrote:
> d instead do user pointer
> > masking, throughout the x86 uaccess code.  This is similar to what arm64
> > is already doing.
> > 
> > barrier_nospec() is now unused, and can be removed.
> 
> One thing to consider is whether you need a speculation barrier after
> set_fs(). Otherwise for code like:
> 
> | fs = get_fs();
> | if (cond)
> |	set_fs(KERNEL_DS);
> | copy_to_user(...)
> | set_fs(fs)
> 
> ... the set_fs() can occur speculatively, and may be able to satisfy
> the masking logic if forwarded within the cpu.
> 
> See arm64 commit:
> 
>   c2f0ad4fc089cff8 ("arm64: uaccess: Prevent speculative use of the current addr_limit")

Do you have any examples of that conditional set_fs(KERNEL_DS) pattern?
I wasn't able to find any.

-- 
Josh

Re: [PATCH] x86/uaccess: Use pointer masking to limit uaccess speculation

[Josh Poimboeuf]

On Tue, Sep 01, 2020 at 08:32:20AM +0000, David Laight wrote:
> > Yes, it would make sense to put the masking in access_ok() somehow.  But
> > to do it properly, I think we'd first need to make access_ok() generic.
> > Maybe that's do-able, but it would be a much bigger patch set.
> > 
> > First I'd prefer to just fix x86, like my patch does.  Then we could do
> > an access_ok() rework.
> 
> If you do a modified access_ok() you get to (slowly) collect all
> the important paths.
> No point replicating the same test.
> 
> A lot of the access_ok() can be deleted - maybe remove some __
> from the following functions.
> Or change to the variants that enable user-space accesses.

Well, yes, but that's a much bigger job which can be done later.

-- 
Josh

Re: [PATCH] x86/uaccess: Use pointer masking to limit uaccess speculation

[Christoph Hellwig]

On Tue, Sep 01, 2020 at 03:02:08PM +0100, Mark Rutland wrote:
> One thing to consider is whether you need a speculation barrier after
> set_fs(). Otherwise for code like:

FYI, at least for x86 and powerpc I have a pending series to kill
set_fs().  I'd love to see someone help with the arm/arm64 side, otherwise
I'll try to get to it eventually.

Re: [PATCH] x86/uaccess: Use pointer masking to limit uaccess speculation

[Mark Rutland]

On Tue, Sep 01, 2020 at 09:21:58AM -0500, Josh Poimboeuf wrote:
> On Tue, Sep 01, 2020 at 03:02:08PM +0100, Mark Rutland wrote:
> > d instead do user pointer
> > > masking, throughout the x86 uaccess code.  This is similar to what arm64
> > > is already doing.
> > > 
> > > barrier_nospec() is now unused, and can be removed.
> > 
> > One thing to consider is whether you need a speculation barrier after
> > set_fs(). Otherwise for code like:
> > 
> > | fs = get_fs();
> > | if (cond)
> > |	set_fs(KERNEL_DS);
> > | copy_to_user(...)
> > | set_fs(fs)
> > 
> > ... the set_fs() can occur speculatively, and may be able to satisfy
> > the masking logic if forwarded within the cpu.
> > 
> > See arm64 commit:
> > 
> >   c2f0ad4fc089cff8 ("arm64: uaccess: Prevent speculative use of the current addr_limit")
> 
> Do you have any examples of that conditional set_fs(KERNEL_DS) pattern?
> I wasn't able to find any.

I'm afraid not -- we used to in arm64 in some memory dump code, but that
is now gone. It might be that this is no longer necessary.

If set_fs() goes entirely, that's even better...

Thanks,
Mark.

Re: [PATCH] x86/uaccess: Use pointer masking to limit uaccess speculation

[Mark Rutland]

On Tue, Sep 01, 2020 at 04:46:41PM +0200, Christoph Hellwig wrote:
> On Tue, Sep 01, 2020 at 03:02:08PM +0100, Mark Rutland wrote:
> > One thing to consider is whether you need a speculation barrier after
> > set_fs(). Otherwise for code like:
> 
> FYI, at least for x86 and powerpc I have a pending series to kill
> set_fs().  I'd love to see someone help with the arm/arm64 side, otherwise
> I'll try to get to it eventually.

Is there anything in particular that's tricky, or do you just want
someone to look generally? From a quick grep arch/arm64/* looks clean, but
I suspect that's misleading.

Thanks,
Mark.

RE: [PATCH] x86/uaccess: Use pointer masking to limit uaccess speculation

[David Laight]

From: Josh Poimboeuf
> Sent: 01 September 2020 15:27
> 
> On Tue, Sep 01, 2020 at 08:32:20AM +0000, David Laight wrote:
> > > Yes, it would make sense to put the masking in access_ok() somehow.  But
> > > to do it properly, I think we'd first need to make access_ok() generic.
> > > Maybe that's do-able, but it would be a much bigger patch set.
> > >
> > > First I'd prefer to just fix x86, like my patch does.  Then we could do
> > > an access_ok() rework.
> >
> > If you do a modified access_ok() you get to (slowly) collect all
> > the important paths.
> > No point replicating the same test.
> >
> > A lot of the access_ok() can be deleted - maybe remove some __
> > from the following functions.
> > Or change to the variants that enable user-space accesses.
> 
> Well, yes, but that's a much bigger job which can be done later.

Isn't this all rather difficult to exploit though?
(Unlike the original Spectre which trivially let kernel
memory be read.)
Don't you need to manage to 'preset' the branch predictor and BTB
to the right state and then manage some kind of timing attack
on L1 cache?

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)

Re: [PATCH] x86/uaccess: Use pointer masking to limit uaccess speculation

[Christoph Hellwig]

On Tue, Sep 01, 2020 at 03:54:42PM +0100, Mark Rutland wrote:
> On Tue, Sep 01, 2020 at 04:46:41PM +0200, Christoph Hellwig wrote:
> > On Tue, Sep 01, 2020 at 03:02:08PM +0100, Mark Rutland wrote:
> > > One thing to consider is whether you need a speculation barrier after
> > > set_fs(). Otherwise for code like:
> > 
> > FYI, at least for x86 and powerpc I have a pending series to kill
> > set_fs().  I'd love to see someone help with the arm/arm64 side, otherwise
> > I'll try to get to it eventually.
> 
> Is there anything in particular that's tricky, or do you just want
> someone to look generally? From a quick grep arch/arm64/* looks clean, but
> I suspect that's misleading.

Yes, it should be mostly trivial.  I just bet the maintainers are
better at optimizing the low-level assembly code with the variable
address limit gone than I am.  (See Linus comments on the x86 version
for example).  And I don't have a physical arm64 to test with so I'd
have to rely on qemu for any testing.

Re: [PATCH] x86/uaccess: Use pointer masking to limit uaccess speculation

[Josh Poimboeuf]

On Tue, Sep 01, 2020 at 03:00:44PM +0000, David Laight wrote:
> e user-space accesses.
> > 
> > Well, yes, but that's a much bigger job which can be done later.
> 
> Isn't this all rather difficult to exploit though?
> (Unlike the original Spectre which trivially let kernel
> memory be read.)
> Don't you need to manage to 'preset' the branch predictor and BTB
> to the right state and then manage some kind of timing attack
> on L1 cache?

In-place training of the branch predictor, and L1 timing attacks are
both trivial.

The harder part is finding another gadget to build on the speculative
load.  But there have been no shortage of new and creative ways to do
that over the last three years.

Also, since Spectre v1 affects many architectures, and may never get
fixed in HW, it's wise to harden these classes of attacks, especially if
we can do so easily.

Yes, it's not as efficient as it could be, but at least I think it's
better than the existing LFENCE.  And it will get more efficient after
Christoph's set_fs() removal, and then even more so when we move the
masking to inside access_ok().

-- 
Josh

Re: [PATCH] x86/uaccess: Use pointer masking to limit uaccess speculation

[Christoph Hellwig]

On Tue, Sep 01, 2020 at 05:05:53PM +0200, Christoph Hellwig wrote:
> > Is there anything in particular that's tricky, or do you just want
> > someone to look generally? From a quick grep arch/arm64/* looks clean, but
> > I suspect that's misleading.
> 
> Yes, it should be mostly trivial.  I just bet the maintainers are
> better at optimizing the low-level assembly code with the variable
> address limit gone than I am.  (See Linus comments on the x86 version
> for example).  And I don't have a physical arm64 to test with so I'd
> have to rely on qemu for any testing.

So I looked at the arm64 code and I don't think it is entirely trivial,
due to the orig_addr_limit saving in the syscall entry path, and due
to all the UAO stuff.  On the plus side it looks to me like
CONFIG_ARM64_UAO and all the code relate to it can go away entirely
if set_fs() is gone.

So if I can trick you guys into submiting a patch on top of:

   http://git.infradead.org/users/hch/misc.git/shortlog/refs/heads/set_fs-removal

that would make my life a lot simpler.

Re: [PATCH] x86/uaccess: Use pointer masking to limit uaccess speculation

[Mark Rutland]

On Tue, Sep 01, 2020 at 05:46:29PM +0200, Christoph Hellwig wrote:
> On Tue, Sep 01, 2020 at 05:05:53PM +0200, Christoph Hellwig wrote:
> > > Is there anything in particular that's tricky, or do you just want
> > > someone to look generally? From a quick grep arch/arm64/* looks clean, but
> > > I suspect that's misleading.
> > 
> > Yes, it should be mostly trivial.  I just bet the maintainers are
> > better at optimizing the low-level assembly code with the variable
> > address limit gone than I am.  (See Linus comments on the x86 version
> > for example).  And I don't have a physical arm64 to test with so I'd
> > have to rely on qemu for any testing.

Makes sense.

I'll take a look if Will doesn't beat me to it, and I'm happy to test
the result regardless.

> So I looked at the arm64 code and I don't think it is entirely trivial,
> due to the orig_addr_limit saving in the syscall entry path, and due
> to all the UAO stuff.  On the plus side it looks to me like
> CONFIG_ARM64_UAO and all the code relate to it can go away entirely
> if set_fs() is gone.

I *think* removing that should be largely mechanical for someone
for someone familiar with it, and it'd be nice to see it go.

> So if I can trick you guys into submiting a patch on top of:
> 
>    http://git.infradead.org/users/hch/misc.git/shortlog/refs/heads/set_fs-removal
> 
> that would make my life a lot simpler.

I'll see what I can do.

At first glance it looks like we might need to flesh out or refactor the
arm64 kernel maccess routines first (since we want the user maccess
routines to use LDTR/STTR instructions that can't access kernel memory),
but after that I think the rest is largely mechanical.

Mark.

Re: [PATCH] x86/uaccess: Use pointer masking to limit uaccess speculation

[Christoph Hellwig]

On Wed, Sep 02, 2020 at 12:43:48PM +0100, Mark Rutland wrote:
> I'll see what I can do.
> 
> At first glance it looks like we might need to flesh out or refactor the
> arm64 kernel maccess routines first (since we want the user maccess
> routines to use LDTR/STTR instructions that can't access kernel memory),
> but after that I think the rest is largely mechanical.

Yes, the first thing is to implement __get_kernel_nofaul and
__put_kernel_nofault.  I think they should mostly look like the existing
non-UAO versions of get_user and put_user with a fixed address space
limit.

Re: [PATCH] x86/uaccess: Use pointer masking to limit uaccess speculation

[Mark Rutland]

On Wed, Sep 02, 2020 at 03:32:31PM +0200, Christoph Hellwig wrote:
> On Wed, Sep 02, 2020 at 12:43:48PM +0100, Mark Rutland wrote:
> > I'll see what I can do.
> > 
> > At first glance it looks like we might need to flesh out or refactor the
> > arm64 kernel maccess routines first (since we want the user maccess
> > routines to use LDTR/STTR instructions that can't access kernel memory),
> > but after that I think the rest is largely mechanical.
> 
> Yes, the first thing is to implement __get_kernel_nofaul and
> __put_kernel_nofault.  I think they should mostly look like the existing
> non-UAO versions of get_user and put_user with a fixed address space
> limit.

I've pushed an initial/incomplete/WIP stab (just the kernel accessors)
to:

https://git.kernel.org/pub/scm/linux/kernel/git/mark/linux.git/log/?h=arm64/set_fs-removal

... and doing that made it clear that the necessary arm64 rework is a
bit more fractal than I thought (e.g. SDEI bits), so it might be a short
while before I post a series.

It might be handy to have a stable branch with the common bits so that
the arm64 rework could go via the arm64 tree in case there's any
fallout.

Thanks for all of this!

Mark.

Re: [PATCH] x86/uaccess: Use pointer masking to limit uaccess speculation

[Christoph Hellwig]

On Wed, Sep 02, 2020 at 06:23:30PM +0100, Mark Rutland wrote:
> I've pushed an initial/incomplete/WIP stab (just the kernel accessors)
> to:
> 
> https://git.kernel.org/pub/scm/linux/kernel/git/mark/linux.git/log/?h=arm64/set_fs-removal

Thanks!

> ... and doing that made it clear that the necessary arm64 rework is a
> bit more fractal than I thought (e.g. SDEI bits), so it might be a short
> while before I post a series.

SDEI is just forcing back a KERNEL_DS to a normal USER_DS, isn't it?

> 
> It might be handy to have a stable branch with the common bits so that
> the arm64 rework could go via the arm64 tree in case there's any
> fallout.

The series will need at least one more repost, but hopefully I'll have
s stable branch soon.

Re: [PATCH] x86/uaccess: Use pointer masking to limit uaccess speculation

[Mark Rutland]

On Thu, Sep 03, 2020 at 08:56:13AM +0200, Christoph Hellwig wrote:
> On Wed, Sep 02, 2020 at 06:23:30PM +0100, Mark Rutland wrote:
> > I've pushed an initial/incomplete/WIP stab (just the kernel accessors)
> > to:
> > 
> > https://git.kernel.org/pub/scm/linux/kernel/git/mark/linux.git/log/?h=arm64/set_fs-removal
> 
> Thanks!
> 
> > ... and doing that made it clear that the necessary arm64 rework is a
> > bit more fractal than I thought (e.g. SDEI bits), so it might be a short
> > while before I post a series.
> 
> SDEI is just forcing back a KERNEL_DS to a normal USER_DS, isn't it?
 
It's a bit more involved -- we also need to fiddle with some HW state
(PSTATE.PAN, PSTATE.UAO, and TTBR0 for SW_PAN) because SDEI isn't a real
exception, so we have to simulate what HW (or the usual exception code)
does for exception entry/return.

It's simple enough to handle, but requires some refactoring as we now
rely on force_uaccess_{begin,end}() having those HW side-effects. I'll
probably factor that out into new arm64-specific helpers for simulating
exception entry/return (which we used to open-code) since it's the only
special case and that'll be clearer than repurposing generic helpers to
do so.

> > It might be handy to have a stable branch with the common bits so that
> > the arm64 rework could go via the arm64 tree in case there's any
> > fallout.
> 
> The series will need at least one more repost, but hopefully I'll have
> s stable branch soon.

I'll keep an eye out!

Mark.