On Wed, Oct 16, 2024 at 03:34:11PM -0700, Linus Torvalds wrote:
> On Wed, 16 Oct 2024 at 15:13, Kirill A. Shutemov <kir...@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
