Messages in this thread | | | Date | Tue, 10 Jan 2023 22:04:39 +0100 | From | Peter Zijlstra <> | Subject | Re: [PATCH 3/7] x86/cpu: Disable kernel LASS when patching kernel alternatives |
| |
On Mon, Jan 09, 2023 at 09:52:00PM -0800, Yian Chen wrote: > Most of the kernel is mapped at virtual addresses > in the upper half of the address range. But kernel > deliberately initialized a temporary mm area > within the lower half of the address range > for text poking, see commit 4fc19708b165 > ("x86/alternatives: Initialize temporary mm > for patching"). > > LASS stops access to a lower half address in kernel, > and this can be deactivated if AC bit in EFLAGS > register is set. Hence use stac and clac instructions > around access to the address to avoid triggering a > LASS #GP fault. > > Kernel objtool validation warns if the binary calls > to a non-whitelisted function that exists outside of > the stac/clac guard, or references any function with a > dynamic function pointer inside the guard; see section > 9 in the document tools/objtool/Documentation/objtool.txt. > > For these reasons, also considering text poking size is > usually small, simple modifications have been done > in function text_poke_memcpy() and text_poke_memset() to > avoid non-whitelisted function calls inside the stac/clac > guard. > > Gcc may detect and replace the target with its built-in > functions. However, the replacement would break the > objtool validation criteria. Hence, add compiler option > -fno-builtin for the file.
Please reflow to 72 characters consistently, this is silly.
> Co-developed-by: Tony Luck <tony.luck@intel.com> > Signed-off-by: Tony Luck <tony.luck@intel.com> > Signed-off-by: Yian Chen <yian.chen@intel.com> > --- > arch/x86/include/asm/smap.h | 13 +++++++++++++ > arch/x86/kernel/Makefile | 2 ++ > arch/x86/kernel/alternative.c | 21 +++++++++++++++++++-- > tools/objtool/arch/x86/special.c | 2 ++ > 4 files changed, 36 insertions(+), 2 deletions(-) > > diff --git a/arch/x86/include/asm/smap.h b/arch/x86/include/asm/smap.h > index bab490379c65..6f7ac0839b10 100644 > --- a/arch/x86/include/asm/smap.h > +++ b/arch/x86/include/asm/smap.h > @@ -39,6 +39,19 @@ static __always_inline void stac(void) > alternative("", __ASM_STAC, X86_FEATURE_SMAP); > } > > +/* 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. Also if you look at bit 63 as a sign bit, it's actively wrong since -1 is lower than 0.
> + > static __always_inline unsigned long smap_save(void) > { > unsigned long flags; > diff --git a/arch/x86/kernel/Makefile b/arch/x86/kernel/Makefile > index 96d51bbc2bd4..f8a455fc56a2 100644 > --- a/arch/x86/kernel/Makefile > +++ b/arch/x86/kernel/Makefile > @@ -7,6 +7,8 @@ extra-y += vmlinux.lds > > CPPFLAGS_vmlinux.lds += -U$(UTS_MACHINE) > > +CFLAGS_alternative.o += -fno-builtin > + > ifdef CONFIG_FUNCTION_TRACER > # Do not profile debug and lowlevel utilities > CFLAGS_REMOVE_tsc.o = -pg > diff --git a/arch/x86/kernel/alternative.c b/arch/x86/kernel/alternative.c > index 7d8c3cbde368..4de8b54fb5f2 100644 > --- a/arch/x86/kernel/alternative.c > +++ b/arch/x86/kernel/alternative.c > @@ -1530,14 +1530,31 @@ __ro_after_init unsigned long poking_addr; > > static void text_poke_memcpy(void *dst, const void *src, size_t len) > { > - memcpy(dst, src, len); > + const char *s = src; > + char *d = dst; > + > + /* The parameter dst ends up referencing to the global variable > + * poking_addr, which is mapped to the low half address space. > + * In kernel, accessing the low half address range is prevented > + * by LASS. So relax LASS prevention while accessing the memory > + * range. > + */ > + low_addr_access_begin(); > + while (len-- > 0) > + *d++ = *s++; > + low_addr_access_end(); > } > > static void text_poke_memset(void *dst, const void *src, size_t len) > { > int c = *(const int *)src; > + char *d = dst; > > - memset(dst, c, len); > + /* The same comment as it is in function text_poke_memcpy */ > + low_addr_access_begin(); > + while (len-- > 0) > + *d++ = c; > + low_addr_access_end(); > }
This is horrific tinkering :/
Also, what about the EFI mm? IIRC EFI also lives in the user address space.
| |