diff mbox series

[v3,03/12] x86/alternatives: Disable LASS when patching kernel alternatives

Message ID 20230609183632.48706-4-alexander.shishkin@linux.intel.com
State New
Headers
Series Enable Linear Address Space Separation support |

Commit Message

Alexander Shishkin June 9, 2023, 6:36 p.m. UTC 
  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(-)

Comments

Edgecombe, Rick P July 31, 2023, 10:41 p.m. UTC | #1 
On Fri, 2023-06-09 at 21:36 +0300, Alexander Shishkin wrote:
> +/*
> + * 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);
> +       stac();
> +       __inline_memcpy(dst, src, len);
> +       clac();
>  }
>  
>  static void text_poke_memset(void *dst, const void *src, size_t len)
>  {
>         int c = *(const int *)src;
>  
> -       memset(dst, c, len);
> +       stac();
> +       __inline_memset(dst, c, len);
> +       clac();
>  }

Why not do stac/clac in a single place inside __text_poke()?
Sohil Mehta Aug. 1, 2023, 9:10 p.m. UTC | #2 
> Why not do stac/clac in a single place inside __text_poke()?

It would mostly look something like this:
> diff --git a/arch/x86/kernel/alternative.c b/arch/x86/kernel/alternative.c
> index 0fbf8a631306..02ef08e2575d 100644
> --- a/arch/x86/kernel/alternative.c
> +++ b/arch/x86/kernel/alternative.c
> @@ -1781,7 +1781,9 @@ static void *__text_poke(text_poke_f func, void *addr, const void *src, size_t l
>         prev = use_temporary_mm(poking_mm);
> 
>         kasan_disable_current();
> +       stac();
>         func((u8 *)poking_addr + offset_in_page(addr), src, len);
> +       clac();
>         kasan_enable_current();
> 
>         /*

Since, __text_poke() uses a dynamic function to call into
text_poke_memcpy() and text_poke_memset(), objtool would still complain.

> arch/x86/kernel/alternative.o: warning: objtool: __text_poke+0x259: call to {dynamic}() with UACCESS enabled

We could change __text_poke() to not use the dynamic func but it might
be a bit heavy handed to save a couple of lines of stac/clac calls. The
current trade-off seems reasonable to me.

Did you have something different in mind?

Sohil
Edgecombe, Rick P Aug. 1, 2023, 9:50 p.m. UTC | #3 
On Tue, 2023-08-01 at 14:10 -0700, Sohil Mehta wrote:
> > Why not do stac/clac in a single place inside __text_poke()?
> 
> It would mostly look something like this:
> > diff --git a/arch/x86/kernel/alternative.c
> > b/arch/x86/kernel/alternative.c
> > index 0fbf8a631306..02ef08e2575d 100644
> > --- a/arch/x86/kernel/alternative.c
> > +++ b/arch/x86/kernel/alternative.c
> > @@ -1781,7 +1781,9 @@ static void *__text_poke(text_poke_f func,
> > void *addr, const void *src, size_t l
> >          prev = use_temporary_mm(poking_mm);
> > 
> >          kasan_disable_current();
> > +       stac();
> >          func((u8 *)poking_addr + offset_in_page(addr), src, len);
> > +       clac();
> >          kasan_enable_current();
> > 
> >          /*
> 
> Since, __text_poke() uses a dynamic function to call into
> text_poke_memcpy() and text_poke_memset(), objtool would still
> complain.
> 
> > arch/x86/kernel/alternative.o: warning: objtool: __text_poke+0x259:
> > call to {dynamic}() with UACCESS enabled
> 
> We could change __text_poke() to not use the dynamic func but it
> might
> be a bit heavy handed to save a couple of lines of stac/clac calls.
> The
> current trade-off seems reasonable to me.
> 
> Did you have something different in mind?

I wondered if it might be something like that. Yes, seems like an ok
tradeoff.
diff mbox series

Patch

diff --git a/arch/x86/kernel/alternative.c b/arch/x86/kernel/alternative.c
index f615e0cb6d93..eac6a5406d39 100644
--- a/arch/x86/kernel/alternative.c
+++ b/arch/x86/kernel/alternative.c
@@ -1526,16 +1526,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);
+	stac();
+	__inline_memcpy(dst, src, len);
+	clac();
 }
 
 static void text_poke_memset(void *dst, const void *src, size_t len)
 {
 	int c = *(const int *)src;
 
-	memset(dst, c, len);
+	stac();
+	__inline_memset(dst, c, len);
+	clac();
 }
 
 typedef void text_poke_f(void *dst, const void *src, size_t len);