From: Sohil Mehta <sohil.mehta@intel.com>
For patching, the kernel initializes a temporary mm area in the lower
half of the address range. See commit 4fc19708b165 ("x86/alternatives:
Initialize temporary mm for patching").
Disable LASS enforcement during patching using the stac()/clac()
instructions to avoid triggering a #GP fault.
The objtool warns due to a call to a non-allowed function that exists
outside of the stac/clac guard, or references to any function with a
dynamic function pointer inside the guard. See the Objtool warnings
section #9 in the document tools/objtool/Documentation/objtool.txt.
Considering that patching is usually small, replace the memcpy and
memset functions in the text poking functions with their inline versions
respectively.
Signed-off-by: Sohil Mehta <sohil.mehta@intel.com>
Signed-off-by: Alexander Shishkin <alexander.shishkin@linux.intel.com>
---
arch/x86/kernel/alternative.c | 12 ++++++++++--
1 file changed, 10 insertions(+), 2 deletions(-)
diff --git a/arch/x86/kernel/alternative.c b/arch/x86/kernel/alternative.c
index d17518ca19b8..2dc097014c2d 100644
--- a/arch/x86/kernel/alternative.c
+++ b/arch/x86/kernel/alternative.c
@@ -1841,16 +1841,24 @@ static inline void unuse_temporary_mm(temp_mm_state_t prev_state)
__ro_after_init struct mm_struct *poking_mm;
__ro_after_init unsigned long poking_addr;
+/*
+ * poking_init() initializes the text poking address from the lower half of the
+ * address space. Relax LASS enforcement when accessing the poking address.
+ */
static void text_poke_memcpy(void *dst, const void *src, size_t len)
{
- memcpy(dst, src, len);
+ lass_stac();
+ __inline_memcpy(dst, src, len);
+ lass_clac();
}
static void text_poke_memset(void *dst, const void *src, size_t len)
{
int c = *(const int *)src;
- memset(dst, c, len);
+ lass_stac();
+ __inline_memset(dst, c, len);
+ lass_clac();
}
typedef void text_poke_f(void *dst, const void *src, size_t len);
--
2.45.2On 10/28/24 09:07, Alexander Shishkin wrote:
> static void text_poke_memcpy(void *dst, const void *src, size_t len)
> {
> - memcpy(dst, src, len);
> + lass_stac();
> + __inline_memcpy(dst, src, len);
> + lass_clac();
> }
>
> static void text_poke_memset(void *dst, const void *src, size_t len)
> {
> int c = *(const int *)src;
>
> - memset(dst, c, len);
> + lass_stac();
> + __inline_memset(dst, c, len);
> + lass_clac();
> }
These are the _only_ users of lass_stac/clac() or the new inlines.
First of all, I totally agree that the _existing_ strict objtool
behavior around STAC/CLAC is a good idea.
But text poking really is special and the context is highly unlikely to
result in bugs or exploits. My first instinct here would have been to
tell objtool that the text poking code is OK and to relax objtool's
STAC/CLAC paranoia here.
Looking at objtool, I can see how important it is to keep the STAC/CLAC
code as dirt simple and foolproof as possible. I don't see an obvious
way to except the text poking code without adding at least some complexity.
Basically what I'm asking for is if the goal is to keep objtool simple,
please *SAY* that. Because on the surface this doesn't look like a good
idea.
On Mon, Oct 28, 2024 at 10:49:07AM -0700, Dave Hansen wrote:
> On 10/28/24 09:07, Alexander Shishkin wrote:
> > static void text_poke_memcpy(void *dst, const void *src, size_t len)
> > {
> > - memcpy(dst, src, len);
> > + lass_stac();
> > + __inline_memcpy(dst, src, len);
> > + lass_clac();
> > }
> >
> > static void text_poke_memset(void *dst, const void *src, size_t len)
> > {
> > int c = *(const int *)src;
> >
> > - memset(dst, c, len);
> > + lass_stac();
> > + __inline_memset(dst, c, len);
> > + lass_clac();
> > }
>
> These are the _only_ users of lass_stac/clac() or the new inlines.
For now; I have vague memories of running into trouble with compilers
doing random things with memcpy before, and having these inline versions
gives us more control.
One of the cases I remember running into was KASAN, where a compiler is
SUPPOSED to issue __asan_memcpy calls instead of the regular memcpy
calls, except they weren't all doing that, with the end result that our
regular memcpy implementation grew instrumentation to deal with that.
That got sorted -- by deprecating / breaking all those non-conformant
compilers. But still, I think it would be good to have the option to
force a simple inline memcpy when needed.
> First of all, I totally agree that the _existing_ strict objtool
> behavior around STAC/CLAC is a good idea.
>
> But text poking really is special and the context is highly unlikely to
> result in bugs or exploits. My first instinct here would have been to
> tell objtool that the text poking code is OK and to relax objtool's
> STAC/CLAC paranoia here.
>
> Looking at objtool, I can see how important it is to keep the STAC/CLAC
> code as dirt simple and foolproof as possible. I don't see an obvious
> way to except the text poking code without adding at least some complexity.
>
> Basically what I'm asking for is if the goal is to keep objtool simple,
> please *SAY* that. Because on the surface this doesn't look like a good
> idea.
There is, you can add it to uaccess_safe_builtin[], but I'm not sure we
want to blanked accept memcpy() -- or perhaps that is what you're
saying.
Anyway, looking at this, I see we grew rep_{movs,stos}_alternative, as
used in copy_user_generic() and __clear_user(). Which are all somewhat
similar.
On Tue, Oct 29, 2024 at 12:36:11PM +0100, Peter Zijlstra wrote:
> Anyway, looking at this, I see we grew rep_{movs,stos}_alternative, as
> used in copy_user_generic() and __clear_user(). Which are all somewhat
> similar.
That is, we could consider something like the completely untested and
probably broken, will light your granny on fire and maul pets like
below..
---
diff --git a/arch/x86/include/asm/string.h b/arch/x86/include/asm/string.h
index 9cb5aae7fba9..e25a988360a1 100644
--- a/arch/x86/include/asm/string.h
+++ b/arch/x86/include/asm/string.h
@@ -2,31 +2,50 @@
#ifndef _ASM_X86_STRING_H
#define _ASM_X86_STRING_H
+#include <asm/asm.h>
+#include <asm/alternative.h>
+
#ifdef CONFIG_X86_32
# include <asm/string_32.h>
#else
# include <asm/string_64.h>
#endif
+#ifdef CONFIG_X86_64
+#define ALT_64(orig, alt, feat) ALTERNATIVE(orig, alt, feat)
+#else
+#define ALT_64(orig, alt, feat) orig
+#endif
+
static __always_inline void *__inline_memcpy(void *to, const void *from, size_t len)
{
void *ret = to;
- asm volatile("rep movsb"
- : "+D" (to), "+S" (from), "+c" (len)
- : : "memory");
- return ret;
+ asm volatile("1:\n\t"
+ ALT_64("rep movsb",
+ "call rep_movs_alternative", ALT_NOT(X86_FEATURE_FSRM))
+ "2:\n\t"
+ _ASM_EXTABLE_UA(1b, 2b)
+ : "+D" (to), "+S" (from), "+c" (len), ASM_CALL_CONSTRAINT
+ : : "memory", _ASM_AX);
+
+ return ret + len;
}
static __always_inline void *__inline_memset(void *s, int v, size_t n)
{
void *ret = s;
- asm volatile("rep stosb"
- : "+D" (s), "+c" (n)
+ asm volatile("1:\n\t"
+ ALT_64("rep stosb",
+ "call rep_stos_alternative", ALT_NOT(X86_FEATURE_FSRM))
+ "2:\n\t"
+ _ASM_EXTABLE_UA(1b, 2b)
+ : "+D" (s), "+c" (n), ASM_CALL_CONSTRAINT
: "a" ((uint8_t)v)
- : "memory");
- return ret;
+ : "memory", _ASM_SI);
+
+ return ret + len;
}
#endif /* _ASM_X86_STRING_H */
diff --git a/arch/x86/include/asm/uaccess_64.h b/arch/x86/include/asm/uaccess_64.h
index b0a887209400..9f2d2c2ca731 100644
--- a/arch/x86/include/asm/uaccess_64.h
+++ b/arch/x86/include/asm/uaccess_64.h
@@ -13,6 +13,7 @@
#include <asm/page.h>
#include <asm/percpu.h>
#include <asm/runtime-const.h>
+#include <asm/string.h>
/*
* Virtual variable: there's no actual backing store for this,
@@ -118,21 +119,12 @@ rep_movs_alternative(void *to, const void *from, unsigned len);
static __always_inline __must_check unsigned long
copy_user_generic(void *to, const void *from, unsigned long len)
{
+ void *ret;
+
stac();
- /*
- * If CPU has FSRM feature, use 'rep movs'.
- * Otherwise, use rep_movs_alternative.
- */
- asm volatile(
- "1:\n\t"
- ALTERNATIVE("rep movsb",
- "call rep_movs_alternative", ALT_NOT(X86_FEATURE_FSRM))
- "2:\n"
- _ASM_EXTABLE_UA(1b, 2b)
- :"+c" (len), "+D" (to), "+S" (from), ASM_CALL_CONSTRAINT
- : : "memory", "rax");
+ ret = __inline_memcpy(to, from, len);
clac();
- return len;
+ return ret - to;
}
static __always_inline __must_check unsigned long
@@ -178,25 +170,15 @@ rep_stos_alternative(void __user *addr, unsigned long len);
static __always_inline __must_check unsigned long __clear_user(void __user *addr, unsigned long size)
{
- might_fault();
- stac();
+ void *ret;
- /*
- * No memory constraint because it doesn't change any memory gcc
- * knows about.
- */
- asm volatile(
- "1:\n\t"
- ALTERNATIVE("rep stosb",
- "call rep_stos_alternative", ALT_NOT(X86_FEATURE_FSRS))
- "2:\n"
- _ASM_EXTABLE_UA(1b, 2b)
- : "+c" (size), "+D" (addr), ASM_CALL_CONSTRAINT
- : "a" (0));
+ might_fault();
+ stac();
+ ret = __inline_memset(addr, 0, size);
clac();
- return size;
+ return ret - addr;
}
static __always_inline unsigned long clear_user(void __user *to, unsigned long n)
diff --git a/arch/x86/lib/clear_page_64.S b/arch/x86/lib/clear_page_64.S
index 2760a15fbc00..17d4bf6f50e5 100644
--- a/arch/x86/lib/clear_page_64.S
+++ b/arch/x86/lib/clear_page_64.S
@@ -53,16 +53,22 @@ SYM_FUNC_END(clear_page_erms)
EXPORT_SYMBOL_GPL(clear_page_erms)
/*
- * Default clear user-space.
+ * Default memset
* Input:
* rdi destination
+ * rsi scratch
* rcx count
- * rax is zero
+ * al is value
*
* Output:
- * rcx: uncleared bytes or 0 if successful.
+ * rcx: unset bytes or 0 if successful.
*/
SYM_FUNC_START(rep_stos_alternative)
+
+ movzbl %al, %rsi
+ movabs $0x0101010101010101, %rax
+ mulq %rsi
+
cmpq $64,%rcx
jae .Lunrolled
Peter Zijlstra <peterz@infradead.org> writes:
> On Tue, Oct 29, 2024 at 12:36:11PM +0100, Peter Zijlstra wrote:
> static __always_inline void *__inline_memcpy(void *to, const void *from, size_t len)
> {
> void *ret = to;
>
> - asm volatile("rep movsb"
> - : "+D" (to), "+S" (from), "+c" (len)
> - : : "memory");
> - return ret;
> + asm volatile("1:\n\t"
> + ALT_64("rep movsb",
> + "call rep_movs_alternative", ALT_NOT(X86_FEATURE_FSRM))
I don't know if it matters, but this basically brings in a whole memcpy
to a text_poke situation, which should only be a handful of bytes, and
creates a new stack frame in the !FSRM case, which the __always_inline
was intending to avoid. But given what text_poke is, maybe micro
optimizations don't really matter. And fewer memcpy() implementations
seems like a good idea.
Thanks,
--
Alex
© 2016 - 2024 Red Hat, Inc.