Messages in this thread Patch in this message | | | Date | Wed, 11 Jan 2023 10:10:24 +0100 | From | Peter Zijlstra <> | Subject | Re: [PATCH 3/7] x86/cpu: Disable kernel LASS when patching kernel alternatives |
| |
On Tue, Jan 10, 2023 at 05:01:59PM -0800, Chen, Yian wrote:
> > > +/* Deactivate/activate LASS via AC bit in EFLAGS register */ > > > +static __always_inline void low_addr_access_begin(void) > > > +{ > > > + /* Note: a barrier is implicit in alternative() */ > > > + alternative("", __ASM_STAC, X86_FEATURE_LASS); > > > +} > > > + > > > +static __always_inline void low_addr_access_end(void) > > > +{ > > > + /* Note: a barrier is implicit in alternative() */ > > > + alternative("", __ASM_CLAC, X86_FEATURE_LASS); > > > +} > > > > Can't say I like the name. > Indeed, there are alternative ways to name the functions. for example, > enable_kernel_lass()/disable_kernel_lass(), or simply keep no change to use > stac()/clac(). > > I choose this name because it is straight forward to the purpose and helps > in understanding when to use the functions.
Given we normally refer to the kernel address space as negative, it is somewhat confusing.
lass_access_{begin,end}()
or something might be better names.
> Also if you look at bit 63 as a sign bit, > > it's actively wrong since -1 is lower than 0. > This could be a trade-off choice. While considering address manipulation > and calculation, it is likely an unsigned. I would be happy to get input for > better naming.
Note that Documentation/x86/x86_64/mm.rst likes to refer to the kernel range as negative.
Also things like making a canonical address use sign-extention.
> > This is horrific tinkering :/ > > > This part seems difficult to have a perfect solution since function call or > function pointer inside the guard of instruction stac and clac will trigger > objtool warning (stated the reasons in the commit msg)
Yeah, I'm familiar with that objtool warning, I wrote that particular check :-)
Still, this is a horrific solution. Adding something like __inline_mem{set,cpy}() is a much saner option.
Something a little like the completely untested below.
--- diff --git a/arch/x86/include/asm/string_64.h b/arch/x86/include/asm/string_64.h index 888731ccf1f6..f43fc2d9b182 100644 --- a/arch/x86/include/asm/string_64.h +++ b/arch/x86/include/asm/string_64.h @@ -23,6 +23,16 @@ extern void *memcpy(void *to, const void *from, size_t len); #endif extern void *__memcpy(void *to, const void *from, size_t len); +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; +} + #define __HAVE_ARCH_MEMSET #if defined(__SANITIZE_MEMORY__) && defined(__NO_FORTIFY) extern void *__msan_memset(void *s, int c, size_t n); @@ -33,6 +43,17 @@ void *memset(void *s, int c, size_t n); #endif void *__memset(void *s, int c, size_t n); +static __always_inline void *__inline_memset(void *s, int v, size_t n) +{ + void *ret = s; + + asm volatile("rep stosb" + : "+D" (s), "+c" (n) + : "a" ((uint8_t)v) + : "memory"); + return ret; +} + #define __HAVE_ARCH_MEMSET16 static inline void *memset16(uint16_t *s, uint16_t v, size_t n) {
| |