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

[PATCH v2] 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>
---
v2:
- fix 32-bit build

 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                    |  6 ++--
 arch/x86/lib/usercopy_64.c                    |  4 +--
 lib/iov_iter.c                                |  2 +-
 12 files changed, 60 insertions(+), 43 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..185ae8f56477 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);
@@ -331,7 +331,7 @@ do {									\
 
 unsigned long __copy_user_ll(void *to, const void *from, unsigned long n)
 {
-	__uaccess_begin_nospec();
+	__uaccess_begin();
 	if (movsl_is_ok(to, from, n))
 		__copy_user(to, from, n);
 	else
@@ -344,7 +344,7 @@ EXPORT_SYMBOL(__copy_user_ll);
 unsigned long __copy_from_user_ll_nocache_nozero(void *to, const void __user *from,
 					unsigned long n)
 {
-	__uaccess_begin_nospec();
+	__uaccess_begin();
 #ifdef CONFIG_X86_INTEL_USERCOPY
 	if (n > 64 && static_cpu_has(X86_FEATURE_XMM2))
 		n = __copy_user_intel_nocache(to, from, n);
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 v2] x86/uaccess: Use pointer masking to limit uaccess speculation

[Josh Poimboeuf]

Hi x86 maintainers,

Any objection to merging this patch?

On Tue, Sep 01, 2020 at 10:30:19AM -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>
> ---
> v2:
> - fix 32-bit build
> 
>  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                    |  6 ++--
>  arch/x86/lib/usercopy_64.c                    |  4 +--
>  lib/iov_iter.c                                |  2 +-
>  12 files changed, 60 insertions(+), 43 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..185ae8f56477 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);
> @@ -331,7 +331,7 @@ do {									\
>  
>  unsigned long __copy_user_ll(void *to, const void *from, unsigned long n)
>  {
> -	__uaccess_begin_nospec();
> +	__uaccess_begin();
>  	if (movsl_is_ok(to, from, n))
>  		__copy_user(to, from, n);
>  	else
> @@ -344,7 +344,7 @@ EXPORT_SYMBOL(__copy_user_ll);
>  unsigned long __copy_from_user_ll_nocache_nozero(void *to, const void __user *from,
>  					unsigned long n)
>  {
> -	__uaccess_begin_nospec();
> +	__uaccess_begin();
>  #ifdef CONFIG_X86_INTEL_USERCOPY
>  	if (n > 64 && static_cpu_has(X86_FEATURE_XMM2))
>  		n = __copy_user_intel_nocache(to, from, n);
> 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
> 

-- 
Josh

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

[David Laight]

From: Josh Poimboeuf <jpoimboe@redhat.com>
> Sent: 08 September 2020 18:43
> Hi x86 maintainers,
...
> > --- 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

For 64bit the sbb+and pattern can be replaced by an instruction
that clears the high bit (eg btr $63, %rcx).
This isn't dependant on the earlier instructions so can execute
in parallel with them.

I still think that doing the same comparisons in access_ok()
and for the pointer masking is silly - and they should get merged.

While it may be possible to fake 'asm volatile goto with outputs'
by using a local asm register variable and alternative pattern
might be to have access_ok() return 0 (fail) or ~0 (ok).
Then the usage can be (with a load of casts):
	p1 = p & access_ok(p);
	if (!p1 && p)
		return -EFAULT;
	foo(*p1);
With any luck the compiler will use the result of the & for the
!p1 test.

	David

	

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

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

[Josh Poimboeuf]

On Wed, Sep 09, 2020 at 08:20:07AM +0000, David Laight wrote:
> From: Josh Poimboeuf <jpoimboe@redhat.com>
> > Sent: 08 September 2020 18:43
> > Hi x86 maintainers,
> ...
> > > --- 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
> 
> For 64bit the sbb+and pattern can be replaced by an instruction
> that clears the high bit (eg btr $63, %rcx).
> This isn't dependant on the earlier instructions so can execute
> in parallel with them.

Wouldn't that break with KERNEL_DS?

> I still think that doing the same comparisons in access_ok()
> and for the pointer masking is silly - and they should get merged.

I agree, but as I said multiple times already, that will be a much
bigger change (tree-wide and multi-arch) which comes later.

-- 
Josh

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

[Josh Poimboeuf]

On Wed, Sep 09, 2020 at 07:46:51AM -0500, Josh Poimboeuf wrote:
> On Wed, Sep 09, 2020 at 08:20:07AM +0000, David Laight wrote:
> > From: Josh Poimboeuf <jpoimboe@redhat.com>
> > > Sent: 08 September 2020 18:43
> > > Hi x86 maintainers,
> > ...
> > > > --- 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
> > 
> > For 64bit the sbb+and pattern can be replaced by an instruction
> > that clears the high bit (eg btr $63, %rcx).
> > This isn't dependant on the earlier instructions so can execute
> > in parallel with them.
> 
> Wouldn't that break with KERNEL_DS?

Looks like the set_fs() removal patches are now in the vfs tree for
-next.  I doubt this is going in an -rc so I'll rework the patch based
on top of Christoph's changes.

-- 
Josh

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

[David Laight]

From: Josh Poimboeuf
> Sent: 09 September 2020 13:47
> On Wed, Sep 09, 2020 at 08:20:07AM +0000, David Laight wrote:
> > From: Josh Poimboeuf <jpoimboe@redhat.com>
> > > Sent: 08 September 2020 18:43
> > > Hi x86 maintainers,
> > ...
> > > > --- 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
> >
> > For 64bit the sbb+and pattern can be replaced by an instruction
> > that clears the high bit (eg btr $63, %rcx).
> > This isn't dependant on the earlier instructions so can execute
> > in parallel with them.
> 
> Wouldn't that break with KERNEL_DS?

I think that has already all been removed from x86.

	David

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