[PATCH] x86/smap: Fix the smap_save() asm

[PATCH] x86/smap: Fix the smap_save() asm

[Andy Lutomirski]

The old smap_save() code was:

  pushf
  pop %0

with %0 defined by an "=rm" constraint.  This is fine if the
compiler picked the register option, but it was incorrect with an
%rsp-relative memory operand.  With some intentional abuse, I can
get both gcc and clang to generate code along these lines:

  pushfq
  popq 0x8(%rsp)
  mov 0x8(%rsp),%rax

which is incorrect and will not work as intended.

Fix it by removing the memory option.  This issue is exacerbated by
a clang optimization bug:

  https://bugs.llvm.org/show_bug.cgi?id=47530

Fixes: e74deb11931f ("x86/uaccess: Introduce user_access_{save,restore}()")
Cc: stable@vger.kernel.org
Reported-by: Bill Wendling <morbo@google.com> # I think
Signed-off-by: Andy Lutomirski <luto@kernel.org>
---
 arch/x86/include/asm/smap.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/x86/include/asm/smap.h b/arch/x86/include/asm/smap.h
index 8b58d6975d5d..be6d675ae3ac 100644
--- a/arch/x86/include/asm/smap.h
+++ b/arch/x86/include/asm/smap.h
@@ -61,7 +61,7 @@ static __always_inline unsigned long smap_save(void)
 		      ALTERNATIVE("jmp 1f", "", X86_FEATURE_SMAP)
 		      "pushf; pop %0; " __ASM_CLAC "\n\t"
 		      "1:"
-		      : "=rm" (flags) : : "memory", "cc");
+		      : "=r" (flags) : : "memory", "cc");
 
 	return flags;
 }
-- 
2.26.2

Re: [PATCH] x86/smap: Fix the smap_save() asm

[Nick Desaulniers]

On Tue, Sep 15, 2020 at 1:56 PM Andy Lutomirski <luto@kernel.org> wrote:
>
> The old smap_save() code was:
>
>   pushf
>   pop %0
>
> with %0 defined by an "=rm" constraint.  This is fine if the
> compiler picked the register option, but it was incorrect with an
> %rsp-relative memory operand.

It is incorrect because ... (I think mentioning the point about the
red zone would be good, unless there were additional concerns?)

The patch should be fine, so

Reviewed-by: Nick Desaulniers <ndesaulniers@google.com>

regardless of whether or not you choose to amend the commit message.
I suspect that the use of "r" exclusively without "m" could lead to
register exhaustion (though it's more likely the compiler will spill
some other variable to stack), which is why it's common to use it in
conjunction with "m."  As Bill's patch notes, getting the "m" version
is poor for performance of this pattern, or at least for
native_{save|restore}_fl.  Being able to use the compiler builtins is
another possibility here.

> With some intentional abuse, I can
> get both gcc and clang to generate code along these lines:
>
>   pushfq
>   popq 0x8(%rsp)
>   mov 0x8(%rsp),%rax
>
> which is incorrect and will not work as intended.
>
> Fix it by removing the memory option.  This issue is exacerbated by
> a clang optimization bug:
>
>   https://bugs.llvm.org/show_bug.cgi?id=47530

This is something we should fix.  Bill, James, and I are discussing
this internally.  Thank you for filing a bug; I owe you a beer just
for that.

>
> Fixes: e74deb11931f ("x86/uaccess: Introduce user_access_{save,restore}()")
> Cc: stable@vger.kernel.org
> Reported-by: Bill Wendling <morbo@google.com> # I think

LOL, yes, the comment can be dropped...though I guess someone else may
have reported the problem to Bill?

> Signed-off-by: Andy Lutomirski <luto@kernel.org>
> ---
>  arch/x86/include/asm/smap.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/arch/x86/include/asm/smap.h b/arch/x86/include/asm/smap.h
> index 8b58d6975d5d..be6d675ae3ac 100644
> --- a/arch/x86/include/asm/smap.h
> +++ b/arch/x86/include/asm/smap.h
> @@ -61,7 +61,7 @@ static __always_inline unsigned long smap_save(void)
>                       ALTERNATIVE("jmp 1f", "", X86_FEATURE_SMAP)
>                       "pushf; pop %0; " __ASM_CLAC "\n\t"
>                       "1:"
> -                     : "=rm" (flags) : : "memory", "cc");
> +                     : "=r" (flags) : : "memory", "cc");
>
>         return flags;
>  }
> --
> 2.26.2
>


-- 
Thanks,
~Nick Desaulniers

Re: [PATCH] x86/smap: Fix the smap_save() asm

[Bill Wendling]

On Tue, Sep 15, 2020 at 2:24 PM Nick Desaulniers
<ndesaulniers@google.com> wrote:
> On Tue, Sep 15, 2020 at 1:56 PM Andy Lutomirski <luto@kernel.org> wrote:
> > Fixes: e74deb11931f ("x86/uaccess: Introduce user_access_{save,restore}()")
> > Cc: stable@vger.kernel.org
> > Reported-by: Bill Wendling <morbo@google.com> # I think
>
> LOL, yes, the comment can be dropped...though I guess someone else may
> have reported the problem to Bill?
>
I found this instance, but not the general issue. :-)

-bw

Re: [PATCH] x86/smap: Fix the smap_save() asm

[Andy Lutomirski]

> On Sep 15, 2020, at 2:24 PM, Nick Desaulniers <ndesaulniers@google.com> wrote:
> 
> On Tue, Sep 15, 2020 at 1:56 PM Andy Lutomirski <luto@kernel.org> wrote:
>> 
>> The old smap_save() code was:
>> 
>>  pushf
>>  pop %0
>> 
>> with %0 defined by an "=rm" constraint.  This is fine if the
>> compiler picked the register option, but it was incorrect with an
>> %rsp-relative memory operand.
> 
> It is incorrect because ... (I think mentioning the point about the
> red zone would be good, unless there were additional concerns?)

This isn’t a red zone issue — it’s a just-plain-wrong issue.  The popf is storing the result in the wrong place in memory — it’s RSP-relative, but RSP is whatever the compiler thinks it should be minus 8, because the compiler doesn’t know that pushfq changed RSP.

> 
> This is something we should fix.  Bill, James, and I are discussing
> this internally.  Thank you for filing a bug; I owe you a beer just
> for that.

I’m looking forward to the day that beers can be exchanged in person again :)

> 
>> 
>> Fixes: e74deb11931f ("x86/uaccess: Introduce user_access_{save,restore}()")
>> Cc: stable@vger.kernel.org
>> Reported-by: Bill Wendling <morbo@google.com> # I think
> 
> LOL, yes, the comment can be dropped...though I guess someone else may
> have reported the problem to Bill?

The “I think” is because I’m not sure whether Bill reported this particular issue. But I’m fine with dropping it.

Re: [PATCH] x86/smap: Fix the smap_save() asm

[Bill Wendling]

On Tue, Sep 15, 2020 at 4:40 PM Andrew Cooper <andrew.cooper3@citrix.com> wrote:
>
> On 16/09/2020 00:11, Andy Lutomirski wrote:
> >> On Sep 15, 2020, at 2:24 PM, Nick Desaulniers <ndesaulniers@google.com> wrote:
> >>
> >> On Tue, Sep 15, 2020 at 1:56 PM Andy Lutomirski <luto@kernel.org> wrote:
> >>> The old smap_save() code was:
> >>>
> >>>  pushf
> >>>  pop %0
> >>>
> >>> with %0 defined by an "=rm" constraint.  This is fine if the
> >>> compiler picked the register option, but it was incorrect with an
> >>> %rsp-relative memory operand.
> >> It is incorrect because ... (I think mentioning the point about the
> >> red zone would be good, unless there were additional concerns?)
> > This isn’t a red zone issue — it’s a just-plain-wrong issue.  The popf is storing the result in the wrong place in memory — it’s RSP-relative, but RSP is whatever the compiler thinks it should be minus 8, because the compiler doesn’t know that pushfq changed RSP.
>
> It's worse than that.  Even when stating that %rsp is modified in the
> asm, the generated code sequence is still buggy, for recent Clang and GCC.
>
> https://godbolt.org/z/ccz9v7
>
> It's clearly not safe to ever use memory operands with pushf/popf asm
> fragments.
>
Would this apply to native_save_fl() and native_restore_fl in
arch/x86/include/asm/irqflags.h? It was like that two revisions ago,
but it was changed (back) to "=rm" with a comment about it being safe.

> >> This is something we should fix.  Bill, James, and I are discussing
> >> this internally.  Thank you for filing a bug; I owe you a beer just
> >> for that.
> > I’m looking forward to the day that beers can be exchanged in person again :)
>
> +1 to that.
>
+100

-bw

Re: [PATCH] x86/smap: Fix the smap_save() asm

[Borislav Petkov]

On Wed, Sep 16, 2020 at 12:40:30AM +0100, Andrew Cooper wrote:
> It's worse than that.  Even when stating that %rsp is modified in the
> asm, the generated code sequence is still buggy, for recent Clang and GCC.
> 
> https://godbolt.org/z/ccz9v7
> 
> It's clearly not safe to ever use memory operands with pushf/popf asm
> fragments.

So I went and singlestepped your snippet in gdb. And it all seems to
work - it is simply a bit confusing: :-)

eflags         0x246               [ PF ZF IF ]

=> 0x000055555555505d <main+13>:        9c      pushfq
0x7fffffffe440: 0x00007fffffffe540      0x0000000000000000
0x7fffffffe450: 0x0000000000000000      0x00007ffff7e0ecca
0x7fffffffe460: 0x00007fffffffe548      0x00000001ffffe7c9
0x7fffffffe470: 0x0000555555555050      0x00007ffff7e0e8f8
0x7fffffffe480: 0x0000000000000000      0x0c710afd7e78681b

those lines under the "=>" line are the stack contents printed with

$ x/10gx $sp

Then, we will pop into 0x8(%rsp):

=> 0x55555555505e <main+14>:    popq   0x8(%rsp)
0x7fffffffe438: 0x0000000000000346      0x00007fffffffe540
0x7fffffffe448: 0x0000000000000000      0x0000000000000000
0x7fffffffe458: 0x00007ffff7e0ecca      0x00007fffffffe548
0x7fffffffe468: 0x00000001ffffe7c9      0x0000555555555050
0x7fffffffe478: 0x00007ffff7e0e8f8      0x0000000000000000

Now, POP copies the value pointed to by %rsp, *increments* the stack
pointer and *then* computes the effective address of the operand. It
says so in the SDM too (thanks peterz!):

"If the ESP register is used as a base register for addressing a
destination operand in memory, the POP instruction computes the
effective address of the operand after it increments the ESP register."

*That*s why, FLAGS is in 0x7fffffffe448! which is %rsp + 8.

Basically flags is there *twice* on the stack:

(gdb) x/10x 0x7fffffffe438
0x7fffffffe438: 0x0000000000000346      0x00007fffffffe540
		^^^^^^^^^^^^^^^^^^
0x7fffffffe448: 0x0000000000000346      0x0000000000000000
		^^^^^^^^^^^^^^^^^^
0x7fffffffe458: 0x00007ffff7e0ecca      0x00007fffffffe548
0x7fffffffe468: 0x00000001ffffe7c9      0x0000555555555050
0x7fffffffe478: 0x00007ffff7e0e8f8      0x0000000000000000

and now we read the second copy into %rsi.

=> 0x555555555062 <main+18>:    mov    0x8(%rsp),%rsi
0x7fffffffe440: 0x00007fffffffe540      0x0000000000000346
0x7fffffffe450: 0x0000000000000000      0x00007ffff7e0ecca
0x7fffffffe460: 0x00007fffffffe548      0x00000001ffffe7c9
0x7fffffffe470: 0x0000555555555050      0x00007ffff7e0e8f8
0x7fffffffe480: 0x0000000000000000      0x0c710afd7e78681b

Looks like it works as designed.

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

Re: [PATCH] x86/smap: Fix the smap_save() asm

[Borislav Petkov]

On Wed, Sep 16, 2020 at 11:48:42PM +0100, Andrew Cooper wrote:
> Every day is a school day.

Tell me about it...

> This is very definitely one to be filed in obscure x86 corner cases.
> 
> The code snippet above is actually wrong for the kernel, as it uses one
> slot of the red-zone.  Recompiling with -mno-red-zone makes something
> which looks safe stack-wise, give or take this behaviour.

Right, we recently disabled red zone in the early decompression stage,
for SEV-ES:

https://git.kernel.org/tip/6ba0efa46047936afa81460489cfd24bc95dd863

I probably should go audit that for similar funnies:

$ objdump -d arch/x86/boot/compressed/vmlinux | grep -E "pop.*\(%[er]?sp"
$

Nope, nothing. Because building your snippet with:

$ gcc -Wall -O2 -mno-red-zone -o flags{,.c}

still does use that one slot:

0000000000001050 <main>:
    1050:       48 83 ec 18             sub    $0x18,%rsp
    1054:       48 8d 3d a9 0f 00 00    lea    0xfa9(%rip),%rdi        # 2004 <_IO_stdin_used+0x4>
    105b:       31 c0                   xor    %eax,%eax
    105d:       9c                      pushfq
    105e:       8f 44 24 08             popq   0x8(%rsp)
    1062:       48 8b 74 24 08          mov    0x8(%rsp),%rsi

Wonder if that flag -mno-red-zone even does anything...

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

Re: [PATCH] x86/smap: Fix the smap_save() asm

[Borislav Petkov]

On Thu, Sep 17, 2020 at 10:05:37AM +0000, David Laight wrote:
> The 'red-zone' allows leaf function to use stack memory for locals
> that is below (ie the wrong side of) the stack pointer.

After looking at

"Figure 3.3: Stack Frame with Base Pointer"

in the x86-64 ABI doc, you're probably right:

0(%rbp)
-8(%rbp)
...
0(%rsp)
.. red zone
-128(%rsp)

i.e., rsp-relative addresses with negative offsets are in the red zone.
So it looks like the compiler actually knows very well what's going on
here and allocates room on the stack for that 0x8(%rsp) slot.

Good.

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

Re: [PATCH] x86/smap: Fix the smap_save() asm

[Borislav Petkov]

On Thu, Sep 17, 2020 at 02:25:50PM +0000, David Laight wrote:
> I actually wonder if there is any code that really benefits from
> the red-zone.

The kernel has been without a red zone since 2002 at least:

  commit 47f16da277d10ef9494f3e9da2a9113bb22bcd75
  Author: Andi Kleen <ak@muc.de>
  Date:   Tue Feb 12 20:17:35 2002 -0800

      [PATCH] x86_64 merge: arch + asm

      This adds the x86_64 arch and asm directories and a Documentation/x86_64.

  ...
  +CFLAGS += $(shell if $(CC) -mno-red-zone -S -o /dev/null -xc /dev/null >/dev/null 2>&1; then echo "-mno-red-zone"; fi )


Also, from the ABI doc:

"A.2.2 Stack Layout

The Linux kernel may align the end of the input argument area to a
8, instead of 16, byte boundary. It does not honor the red zone (see
section 3.2.2) and therefore this area is not allowed to be used by
kernel code. Kernel code should be compiled by GCC with the option
-mno-red-zone."

so forget the red zone.

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

Re: [PATCH] x86/smap: Fix the smap_save() asm

[Andy Lutomirski]

On Thu, Sep 17, 2020 at 7:39 AM Borislav Petkov <bp@alien8.de> wrote:
>
> On Thu, Sep 17, 2020 at 02:25:50PM +0000, David Laight wrote:
> > I actually wonder if there is any code that really benefits from
> > the red-zone.
>
> The kernel has been without a red zone since 2002 at least:
>
>   commit 47f16da277d10ef9494f3e9da2a9113bb22bcd75
>   Author: Andi Kleen <ak@muc.de>
>   Date:   Tue Feb 12 20:17:35 2002 -0800
>
>       [PATCH] x86_64 merge: arch + asm
>
>       This adds the x86_64 arch and asm directories and a Documentation/x86_64.
>
>   ...
>   +CFLAGS += $(shell if $(CC) -mno-red-zone -S -o /dev/null -xc /dev/null >/dev/null 2>&1; then echo "-mno-red-zone"; fi )
>
>
> Also, from the ABI doc:
>
> "A.2.2 Stack Layout
>
> The Linux kernel may align the end of the input argument area to a
> 8, instead of 16, byte boundary. It does not honor the red zone (see
> section 3.2.2) and therefore this area is not allowed to be used by
> kernel code. Kernel code should be compiled by GCC with the option
> -mno-red-zone."
>
> so forget the red zone.
>
> --
> Regards/Gruss,
>     Boris.
>
> https://people.kernel.org/tglx/notes-about-netiquette

Regardless of anything that any docs may or may not say, the kernel
*can't* use a redzone -- an interrupt would overwrite it.

Re: [PATCH] x86/smap: Fix the smap_save() asm

[peterz]

On Tue, Sep 15, 2020 at 01:55:58PM -0700, Andy Lutomirski wrote:
> The old smap_save() code was:
> 
>   pushf
>   pop %0
> 
> with %0 defined by an "=rm" constraint.  This is fine if the
> compiler picked the register option, but it was incorrect with an
> %rsp-relative memory operand.  With some intentional abuse, I can
> get both gcc and clang to generate code along these lines:
> 
>   pushfq
>   popq 0x8(%rsp)
>   mov 0x8(%rsp),%rax
> 
> which is incorrect and will not work as intended.

We need another constraint :-)

> Fix it by removing the memory option.  This issue is exacerbated by
> a clang optimization bug:
> 
>   https://bugs.llvm.org/show_bug.cgi?id=47530
> 
> Fixes: e74deb11931f ("x86/uaccess: Introduce user_access_{save,restore}()")
> Cc: stable@vger.kernel.org
> Reported-by: Bill Wendling <morbo@google.com> # I think
> Signed-off-by: Andy Lutomirski <luto@kernel.org>
> ---
>  arch/x86/include/asm/smap.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/arch/x86/include/asm/smap.h b/arch/x86/include/asm/smap.h
> index 8b58d6975d5d..be6d675ae3ac 100644
> --- a/arch/x86/include/asm/smap.h
> +++ b/arch/x86/include/asm/smap.h
> @@ -61,7 +61,7 @@ static __always_inline unsigned long smap_save(void)
>  		      ALTERNATIVE("jmp 1f", "", X86_FEATURE_SMAP)
>  		      "pushf; pop %0; " __ASM_CLAC "\n\t"
>  		      "1:"
> -		      : "=rm" (flags) : : "memory", "cc");
> +		      : "=r" (flags) : : "memory", "cc");

native_save_fl() has the exact same code; you'll need to fix both.