Re: [PATCH 1/3] x86: fix access_ok() and valid_user_address() using wrong USER_PTR_MAX in modules

[Linus Torvalds]

[ Adding x86 maintainers - I had added Thomas earlier, but I guess at
least Borislav might actually care and have input too ]

So I think the patch I will commit would look like the attached: it's
similar to your suggestion, but without the renaming of USER_PTR_MAX,
and with just a

  #ifdef MODULE
    #define runtime_const_ptr(sym) (sym)
  #else
    #include <asm/runtime-const.h>
  #endif

in the x86 asm/uaccess_64.h header file and an added '#error' for the
MODULE case in the actual x86 runtime-const.h file.

As it is, this bug really only affects modular code that uses
access_ok() and __{get,put}_user(), which is a really broken pattern
to begin with these days, and is happily fairly rare.

That is an old optimization that is no longer an optimization at all
(since a plain "get_user()" is actually *faster* than the access_ok()
and __get_user() these days), and I wish we didn't have any such code
any more, but there are a handful of things that have never been
converted to the modern world order.

So it is what it is, and we have to deal with it.

Also, even that kind of rare and broken code actually *works*,
although the whole "non-canonical reads can speculatively leak
possibly kernel data" does end up being an issue (largely theoretical
because it's now limited to just a couple of odd-ball code sequences)

And yes, it works just because I picked a runtime-const value that is
non-canonical. I'd say it's "by luck", but I did pick that value
partly *because* it's non-canonical, so it's not _entirely_ just luck.
But mostly.

That was all a long explanation for why I am planning on committing
this as a real fix, even if the actual impact of it is largely
theoretical.

Borislav - comments? Generating this patch took longer than it should
have, but I had travel and jetlag and a flight that I expected to have
wifi but didn't...  And properly it should probably be committed by
x86 maintainers rather than me, but I did mess this code up in the
first place.

The patch *looks* very straightforward, but since I'm on the road I am
doing this on my laptop and haven't actually tested it yet (well, I've
built this, and booted it, but nothing past that).

Mateusz - I'd like to just credit you with this, since your comment
about modules was why I started looking into this all in the first
place (and you then wrote a similar patch). But I'm not going to do
that without your ack.

               Linus
 arch/x86/include/asm/runtime-const.h |  4 ++++
 arch/x86/include/asm/uaccess_64.h    | 10 +++++-----
 arch/x86/kernel/cpu/common.c         |  6 +++++-
 3 files changed, 14 insertions(+), 6 deletions(-)

diff --git a/arch/x86/include/asm/runtime-const.h b/arch/x86/include/asm/runtime-const.h
index 8d983cfd06ea..e5a13dc8816e 100644
--- a/arch/x86/include/asm/runtime-const.h
+++ b/arch/x86/include/asm/runtime-const.h
@@ -2,6 +2,10 @@
 #ifndef _ASM_RUNTIME_CONST_H
 #define _ASM_RUNTIME_CONST_H
 
+#ifdef MODULE
+  #error "Cannot use runtime-const infrastructure from modules"
+#endif
+
 #ifdef __ASSEMBLY__
 
 .macro RUNTIME_CONST_PTR sym reg
diff --git a/arch/x86/include/asm/uaccess_64.h b/arch/x86/include/asm/uaccess_64.h
index c8a5ae35c871..641f45c22f9d 100644
--- a/arch/x86/include/asm/uaccess_64.h
+++ b/arch/x86/include/asm/uaccess_64.h
@@ -12,12 +12,12 @@
 #include <asm/cpufeatures.h>
 #include <asm/page.h>
 #include <asm/percpu.h>
-#include <asm/runtime-const.h>
 
-/*
- * Virtual variable: there's no actual backing store for this,
- * it can purely be used as 'runtime_const_ptr(USER_PTR_MAX)'
- */
+#ifdef MODULE
+  #define runtime_const_ptr(sym) (sym)
+#else
+  #include <asm/runtime-const.h>
+#endif
 extern unsigned long USER_PTR_MAX;
 
 #ifdef CONFIG_ADDRESS_MASKING
diff --git a/arch/x86/kernel/cpu/common.c b/arch/x86/kernel/cpu/common.c
index c7d3512914ca..02d97834a1d4 100644
--- a/arch/x86/kernel/cpu/common.c
+++ b/arch/x86/kernel/cpu/common.c
@@ -78,6 +78,10 @@
 DEFINE_PER_CPU_READ_MOSTLY(struct cpuinfo_x86, cpu_info);
 EXPORT_PER_CPU_SYMBOL(cpu_info);
 
+/* Used for modules: built-in code uses runtime constants */
+unsigned long USER_PTR_MAX;
+EXPORT_SYMBOL(USER_PTR_MAX);
+
 u32 elf_hwcap2 __read_mostly;
 
 /* Number of siblings per CPU package */
@@ -2579,7 +2583,7 @@ void __init arch_cpu_finalize_init(void)
 	alternative_instructions();
 
 	if (IS_ENABLED(CONFIG_X86_64)) {
-		unsigned long USER_PTR_MAX = TASK_SIZE_MAX;
+		USER_PTR_MAX = TASK_SIZE_MAX;
 
 		/*
 		 * Enable this when LAM is gated on LASS support