linux-kernel - Re: [PATCH 3/7] x86/cpu: Disable kernel LASS when patching kernel alternatives

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <Y759AJ/0N9fqwDED@hirez.programming.kicks-ass.net>
Date:   Wed, 11 Jan 2023 10:10:24 +0100
From:   Peter Zijlstra <peterz@...radead.org>
To:     "Chen, Yian" <yian.chen@...el.com>
Cc:     linux-kernel@...r.kernel.org, x86@...nel.org,
        Andy Lutomirski <luto@...nel.org>,
        Dave Hansen <dave.hansen@...ux.intel.com>,
        Ravi Shankar <ravi.v.shankar@...el.com>,
        Tony Luck <tony.luck@...el.com>,
        Sohil Mehta <sohil.mehta@...el.com>,
        Paul Lai <paul.c.lai@...el.com>
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)
 {