linux-kernel - Re: [PATCH v2 03/14] x86,smap: Fix smap

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: <20200429101802.GI13592@hirez.programming.kicks-ass.net>
Date:   Wed, 29 Apr 2020 12:18:02 +0200
From:   Peter Zijlstra <peterz@...radead.org>
To:     Brian Gerst <brgerst@...il.com>
Cc:     Josh Poimboeuf <jpoimboe@...hat.com>,
        Alexandre Chartre <alexandre.chartre@...cle.com>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        jthierry@...hat.com, Thomas Gleixner <tglx@...utronix.de>,
        the arch/x86 maintainers <x86@...nel.org>,
        Miroslav Benes <mbenes@...e.cz>
Subject: Re: [PATCH v2 03/14] x86,smap: Fix smap_{save,restore}() alternatives

On Wed, Apr 29, 2020 at 10:30:53AM +0200, Peter Zijlstra wrote:
> > POPF is an expensive instruction that should be avoided if possible.
> > A better solution would be to have the alternative jump over the
> > push/pop when SMAP is disabled.
> 
> Yeah. I think I had that, but then confused myself again. I don't think
> it matters much if you look at where it's used though.
> 
> Still, let me try the jmp thing again..

Here goes..

---
Subject: x86,smap: Fix smap_{save,restore}() alternatives
From: Peter Zijlstra <peterz@...radead.org>
Date: Tue Apr 28 19:57:59 CEST 2020

As reported by objtool:

  lib/ubsan.o: warning: objtool: .altinstr_replacement+0x0: alternative modifies stack
  lib/ubsan.o: warning: objtool: .altinstr_replacement+0x7: alternative modifies stack

the smap_{save,restore}() alternatives violate (the newly enforced)
rule on stack invariance. That is, due to there only being a single
ORC table it must be valid to any alternative. These alternatives
violate this with the direct result that unwinds will not be correct
when it hits between the PUSH and POP instructions.

Rewrite the functions to only have a conditional jump.

Signed-off-by: Peter Zijlstra (Intel) <peterz@...radead.org>
---
 arch/x86/include/asm/smap.h |   11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

--- a/arch/x86/include/asm/smap.h
+++ b/arch/x86/include/asm/smap.h
@@ -57,8 +57,10 @@ static __always_inline unsigned long sma
 {
 	unsigned long flags;
 
-	asm volatile (ALTERNATIVE("", "pushf; pop %0; " __ASM_CLAC,
-				  X86_FEATURE_SMAP)
+	asm volatile ("# smap_save\n\t"
+		      ALTERNATIVE("jmp 1f", "", X86_FEATURE_SMAP)
+		      "pushf; pop %0; " __ASM_CLAC "\n\t"
+		      "1:"
 		      : "=rm" (flags) : : "memory", "cc");
 
 	return flags;
@@ -66,7 +68,10 @@ static __always_inline unsigned long sma
 
 static __always_inline void smap_restore(unsigned long flags)
 {
-	asm volatile (ALTERNATIVE("", "push %0; popf", X86_FEATURE_SMAP)
+	asm volatile ("# smap_restore\n\t"
+		      ALTERNATIVE("jmp 1f", "", X86_FEATURE_SMAP)
+		      "push %0; popf\n\t"
+		      "1:"
 		      : : "g" (flags) : "memory", "cc");
 }