From: Borislav Petkov <bp@alien8.de>
To: Andrew Cooper <andrew.cooper3@citrix.com>
Cc: Andy Lutomirski <luto@amacapital.net>,
Nick Desaulniers <ndesaulniers@google.com>,
Andy Lutomirski <luto@kernel.org>,
Bill Wendling <morbo@google.com>,
"maintainer:X86 ARCHITECTURE (32-BIT AND 64-BIT)"
<x86@kernel.org>, LKML <linux-kernel@vger.kernel.org>,
Greg Thelen <gthelen@google.com>,
John Sperbeck <jsperbeck@google.com>,
"# 3.4.x" <stable@vger.kernel.org>,
clang-built-linux <clang-built-linux@googlegroups.com>
Subject: Re: [PATCH] x86/smap: Fix the smap_save() asm
Date: Wed, 16 Sep 2020 10:26:21 +0200 [thread overview]
Message-ID: <20200916082621.GB2643@zn.tnic> (raw)
In-Reply-To: <7233f4cf-5b1d-0fca-0880-f1cf2e6e765b@citrix.com>
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
next prev parent reply other threads:[~2020-09-16 8:26 UTC|newest]
Thread overview: 11+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-09-15 20:55 [PATCH] x86/smap: Fix the smap_save() asm Andy Lutomirski
2020-09-15 21:24 ` Nick Desaulniers
2020-09-15 21:31 ` Bill Wendling
2020-09-15 23:11 ` Andy Lutomirski
[not found] ` <7233f4cf-5b1d-0fca-0880-f1cf2e6e765b@citrix.com>
2020-09-15 23:43 ` Bill Wendling
2020-09-16 8:26 ` Borislav Petkov [this message]
[not found] ` <be498e49-b467-e04c-d833-372f7d83cb1f@citrix.com>
2020-09-17 6:04 ` Borislav Petkov
[not found] ` <ec617df229514fbaa9897683ac88dfda@AcuMS.aculab.com>
2020-09-17 11:57 ` Borislav Petkov
[not found] ` <823af5fadd464c48ade635498d07ba4e@AcuMS.aculab.com>
2020-09-17 14:39 ` Borislav Petkov
2020-09-17 16:32 ` Andy Lutomirski
2020-09-16 7:28 ` peterz
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20200916082621.GB2643@zn.tnic \
--to=bp@alien8.de \
--cc=andrew.cooper3@citrix.com \
--cc=clang-built-linux@googlegroups.com \
--cc=gthelen@google.com \
--cc=jsperbeck@google.com \
--cc=linux-kernel@vger.kernel.org \
--cc=luto@amacapital.net \
--cc=luto@kernel.org \
--cc=morbo@google.com \
--cc=ndesaulniers@google.com \
--cc=stable@vger.kernel.org \
--cc=x86@kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).