[PATCH] x86/uaccess: Avoid barrier_nospec() in copy_from_user()

[Kirill A. Shutemov]

On Wed, Oct 16, 2024 at 03:34:11PM -0700, Linus Torvalds wrote:
> On Wed, 16 Oct 2024 at 15:13, Kirill A. Shutemov <kirill at shutemov.name> wrote:
> >
> > It is worse than that. If we get LAM_SUP enabled (there's KASAN patchset
> > in works) this check will allow arbitrary kernel addresses.
> 
> Ugh. I haven't seen the LAM_SUP patches.
> 
> But yeah, I assume any LAM_SUP model would basically then make the GP
> fault due to non-canonical addresses go away.

Actually, I've got this wrong.

I based my comment about incompatibility with LAM_SUP on the approach
description that stated zeroing bits 48/57, but actual masks zeroing bits
47/56 which is compatible with all LAM modes. It will trigger #GP for
LAM_SUP too for kernel addresses: bit 63 has to be equal to bit 47/56.

I think it is worth looking at this approach again as it gets better
code: removes two instructions from get_user() and doesn't get flags
involved.

Below is patch on top of current mainline.

The mask only clears bit 47/56. It gets us stronger canonicality check on
machines with LAM off.

I am not entirely sure about my take on valid_user_address() here.

Any comments?

diff --git a/arch/x86/include/asm/runtime-const.h b/arch/x86/include/asm/runtime-const.h
index 6652ebddfd02..8d983cfd06ea 100644
--- a/arch/x86/include/asm/runtime-const.h
+++ b/arch/x86/include/asm/runtime-const.h
@@ -2,6 +2,18 @@
 #ifndef _ASM_RUNTIME_CONST_H
 #define _ASM_RUNTIME_CONST_H
 
+#ifdef __ASSEMBLY__
+
+.macro RUNTIME_CONST_PTR sym reg
+	movq	$0x0123456789abcdef, %\reg
+	1:
+	.pushsection runtime_ptr_\sym, "a"
+	.long	1b - 8 - .
+	.popsection
+.endm
+
+#else /* __ASSEMBLY__ */
+
 #define runtime_const_ptr(sym) ({				\
 	typeof(sym) __ret;					\
 	asm_inline("mov %1,%0\n1:\n"				\
@@ -58,4 +70,5 @@ static inline void runtime_const_fixup(void (*fn)(void *, unsigned long),
 	}
 }
 
+#endif /* __ASSEMBLY__ */
 #endif
diff --git a/arch/x86/include/asm/uaccess_64.h b/arch/x86/include/asm/uaccess_64.h
index b0a887209400..77acef272b0d 100644
--- a/arch/x86/include/asm/uaccess_64.h
+++ b/arch/x86/include/asm/uaccess_64.h
@@ -16,9 +16,9 @@
 
 /*
  * Virtual variable: there's no actual backing store for this,
- * it can purely be used as 'runtime_const_ptr(USER_PTR_MAX)'
+ * it can purely be used as 'runtime_const_ptr(USER_CANONICAL_MASK)'
  */
-extern unsigned long USER_PTR_MAX;
+extern unsigned long USER_CANONICAL_MASK;
 
 #ifdef CONFIG_ADDRESS_MASKING
 /*
@@ -54,7 +54,7 @@ static inline unsigned long __untagged_addr_remote(struct mm_struct *mm,
 #endif
 
 #define valid_user_address(x) \
-	((__force unsigned long)(x) <= runtime_const_ptr(USER_PTR_MAX))
+	(!((__force unsigned long)(x) & ~runtime_const_ptr(USER_CANONICAL_MASK)))
 
 /*
  * Masking the user address is an alternative to a conditional
@@ -63,13 +63,8 @@ static inline unsigned long __untagged_addr_remote(struct mm_struct *mm,
  */
 static inline void __user *mask_user_address(const void __user *ptr)
 {
-	unsigned long mask;
-	asm("cmp %1,%0\n\t"
-	    "sbb %0,%0"
-		:"=r" (mask)
-		:"r" (ptr),
-		 "0" (runtime_const_ptr(USER_PTR_MAX)));
-	return (__force void __user *)(mask | (__force unsigned long)ptr);
+	unsigned long mask = runtime_const_ptr(USER_CANONICAL_MASK);
+	return (__force void __user *)(mask & (__force unsigned long)ptr);
 }
 #define masked_user_access_begin(x) ({				\
 	__auto_type __masked_ptr = (x);				\
diff --git a/arch/x86/kernel/cpu/common.c b/arch/x86/kernel/cpu/common.c
index a5f221ea5688..9d25f4f146f4 100644
--- a/arch/x86/kernel/cpu/common.c
+++ b/arch/x86/kernel/cpu/common.c
@@ -2390,14 +2390,9 @@ void __init arch_cpu_finalize_init(void)
 	alternative_instructions();
 
 	if (IS_ENABLED(CONFIG_X86_64)) {
-		unsigned long USER_PTR_MAX = TASK_SIZE_MAX-1;
+		unsigned long USER_CANONICAL_MASK = ~(1UL << __VIRTUAL_MASK_SHIFT);
 
-		/*
-		 * Enable this when LAM is gated on LASS support
-		if (cpu_feature_enabled(X86_FEATURE_LAM))
-			USER_PTR_MAX = (1ul << 63) - PAGE_SIZE - 1;
-		 */
-		runtime_const_init(ptr, USER_PTR_MAX);
+		runtime_const_init(ptr, USER_CANONICAL_MASK);
 
 		/*
 		 * Make sure the first 2MB area is not mapped by huge pages
diff --git a/arch/x86/kernel/vmlinux.lds.S b/arch/x86/kernel/vmlinux.lds.S
index b8c5741d2fb4..65d2f4cad0aa 100644
--- a/arch/x86/kernel/vmlinux.lds.S
+++ b/arch/x86/kernel/vmlinux.lds.S
@@ -358,7 +358,7 @@ SECTIONS
 #endif
 
 	RUNTIME_CONST_VARIABLES
-	RUNTIME_CONST(ptr, USER_PTR_MAX)
+	RUNTIME_CONST(ptr, USER_CANONICAL_MASK)
 
 	. = ALIGN(PAGE_SIZE);
 
diff --git a/arch/x86/lib/getuser.S b/arch/x86/lib/getuser.S
index 4357ec2a0bfc..b93f0282c6c9 100644
--- a/arch/x86/lib/getuser.S
+++ b/arch/x86/lib/getuser.S
@@ -32,6 +32,7 @@
 #include <asm/errno.h>
 #include <asm/asm-offsets.h>
 #include <asm/thread_info.h>
+#include <asm/runtime-const.h>
 #include <asm/asm.h>
 #include <asm/smap.h>
 
@@ -39,14 +40,8 @@
 
 .macro check_range size:req
 .if IS_ENABLED(CONFIG_X86_64)
-	movq $0x0123456789abcdef,%rdx
-  1:
-  .pushsection runtime_ptr_USER_PTR_MAX,"a"
-	.long 1b - 8 - .
-  .popsection
-	cmp %rax, %rdx
-	sbb %rdx, %rdx
-	or %rdx, %rax
+	RUNTIME_CONST_PTR sym=USER_CANONICAL_MASK reg=rdx
+	and %rdx, %rax
 .else
 	cmp $TASK_SIZE_MAX-\size+1, %eax
 	jae .Lbad_get_user
-- 
  Kiryl Shutsemau / Kirill A. Shutemov