linux-kernel - Re: [PATCH] Revert "x86/uaccess: Add stack frame output operand in get

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: <20170712221242.puv5illztsla4nph@treble>
Date:   Wed, 12 Jul 2017 17:12:42 -0500
From:   Josh Poimboeuf <jpoimboe@...hat.com>
To:     Matthias Kaehlcke <mka@...omium.org>
Cc:     Chris J Arges <chris.j.arges@...onical.com>,
        Borislav Petkov <bp@...e.de>,
        Thomas Gleixner <tglx@...utronix.de>,
        Ingo Molnar <mingo@...hat.com>,
        "H . Peter Anvin" <hpa@...or.com>, x86@...nel.org,
        linux-kernel@...r.kernel.org,
        Douglas Anderson <dianders@...omium.org>,
        Michael Davidson <md@...gle.com>,
        Greg Hackmann <ghackmann@...gle.com>,
        Nick Desaulniers <ndesaulniers@...gle.com>,
        Stephen Hines <srhines@...gle.com>,
        Kees Cook <keescook@...omium.org>,
        Arnd Bergmann <arnd@...db.de>, Bernhard.Rosenkranzer@...aro.org
Subject: Re: [PATCH] Revert "x86/uaccess: Add stack frame output operand in
 get_user() inline asm"

On Wed, Jul 12, 2017 at 02:27:44PM -0700, Matthias Kaehlcke wrote:
> Commit f05058c4d652 supposedly "forces a stack frame to be created before
> the inline asm code if CONFIG_FRAME_POINTER is enabled by listing the
> stack pointer as an output operand for the get_user() inline assembly
> statement.". This doesn't work as intended, at least with gcc v4.9.2 and
> x86-64 the generated code is exactly the same with and without the patch.
> However clang adds an extra instruction that adjusts %rsp, which ends up
> causing double faults all over the place.

I don't think reverting it is the right approach, because that will
still break frame pointers in certain cases.

The original commit probably should have clarified:

 " ... forces a stack frame *if it doesn't already exist*."

In *most* cases it will have no effect, as you saw, because users of
get_user() tend to do other function calls beforehand, so they will have
already saved the frame pointer before calling it.

However, that isn't always the case.  We found that certain configs
change GCC's behavior such that, for certain get_user() call sites, the
containing function doesn't saved the frame pointer before inserting
get_user()'s inline asm.

GCC completely ignores inline asm, so it has no idea that it has a call
instruction in it.  So in general, *any* inline asm with a call
instruction needs this constraint, to force the frame pointer to be
saved, if it hasn't already.

This is admittedly an awkward way of achieving this goal, but it's the
only way I know how to do it with GCC.

What extra instruction does clang add?

-- 
Josh