linux-kernel - Re: [PATCH] x86/smap: Fix the 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>] [day] [month] [year] [list]

Date:   Wed, 16 Sep 2020 10:26:21 +0200
From:   Borislav Petkov <bp@...en8.de>
To:     Andrew Cooper <andrew.cooper3@...rix.com>
Cc:     Andy Lutomirski <luto@...capital.net>,
        Nick Desaulniers <ndesaulniers@...gle.com>,
        Andy Lutomirski <luto@...nel.org>,
        Bill Wendling <morbo@...gle.com>,
        "maintainer:X86 ARCHITECTURE (32-BIT AND 64-BIT)" <x86@...nel.org>,
        LKML <linux-kernel@...r.kernel.org>,
        Greg Thelen <gthelen@...gle.com>,
        John Sperbeck <jsperbeck@...gle.com>,
        "# 3.4.x" <stable@...r.kernel.org>,
        clang-built-linux <clang-built-linux@...glegroups.com>
Subject: Re: [PATCH] x86/smap: Fix the smap_save() asm

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