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: <20170719174630.kz5g553evcrnirmr@treble>
Date:   Wed, 19 Jul 2017 12:46:31 -0500
From:   Josh Poimboeuf <jpoimboe@...hat.com>
To:     Matthias Kaehlcke <mka@...omium.org>
Cc:     Andrey Rybainin <ryabinin.a.a@...il.com>,
        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 Thu, Jul 13, 2017 at 02:57:04PM -0700, Matthias Kaehlcke wrote:
> El Thu, Jul 13, 2017 at 04:34:06PM -0500 Josh Poimboeuf ha dit:
> 
> > On Thu, Jul 13, 2017 at 02:12:45PM -0700, Matthias Kaehlcke wrote:
> > > El Thu, Jul 13, 2017 at 03:34:16PM -0500 Josh Poimboeuf ha dit:
> > > > And yet another one to try (clobbering sp) :-)
> > > > 
> > > > diff --git a/arch/x86/include/asm/uaccess.h b/arch/x86/include/asm/uaccess.h
> > > > index 11433f9..21f0c39 100644
> > > > --- a/arch/x86/include/asm/uaccess.h
> > > > +++ b/arch/x86/include/asm/uaccess.h
> > > > @@ -166,12 +166,12 @@ __typeof__(__builtin_choose_expr(sizeof(x) > sizeof(0UL), 0ULL, 0UL))
> > > >  ({									\
> > > >  	int __ret_gu;							\
> > > >  	register __inttype(*(ptr)) __val_gu asm("%"_ASM_DX);		\
> > > > -	register void *__sp asm(_ASM_SP);				\
> > > >  	__chk_user_ptr(ptr);						\
> > > >  	might_fault();							\
> > > > -	asm volatile("call __get_user_%P4"				\
> > > > -		     : "=a" (__ret_gu), "=r" (__val_gu), "+r" (__sp)	\
> > > > -		     : "0" (ptr), "i" (sizeof(*(ptr))));		\
> > > > +	asm volatile("call __get_user_%P3"				\
> > > > +		     : "=a" (__ret_gu), "=r" (__val_gu)			\
> > > > +		     : "0" (ptr), "i" (sizeof(*(ptr)))			\
> > > > +		     : "sp");						\
> > > >  	(x) = (__force __typeof__(*(ptr))) __val_gu;			\
> > > >  	__builtin_expect(__ret_gu, 0);					\
> > > >  })
> > > 
> > > This compiles with both gcc and clang, clang does not corrupt the
> > > stack pointer. I wouldn't be able to tell though if it forces a stack
> > > frame if it doesn't already exist, as the original patch intends.
> > 
> > Whether it forces the stack frame on clang is a very minor issue
> > compared to the double fault.
> 
> I totally agree, I was mainly concerned about not breaking the
> solution that currently works with gcc.
> 
> > That really only matters when you want to use
> > CONFIG_STACK_VALIDATION to get 100% reliable stacktraces with frame
> > pointers.  And that feature is currently very GCC-specific.  So you
> > probably don't need to worry about that for now, at least until you want
> > to do live patching with a clang-compiled kernel.
> 
> Eventually I expect that there will be interest in live patching
> clang-compiled kernels, however at this stage it probably isn't an
> overly important feature.
> 
> > IIRC, clobbering SP does at least force the stack frame on GCC, though I
> > need to double check that.  I can try to work up an official patch in
> > the next week or so (need to do some testing first).
> 
> Sounds great.
> 
> Thanks again for looking into this and coming up with a solution!

After doing some testing, I don't think this approach is going to work
after all.  In addition to forcing the stack frame, it also causes GCC
to add an unnecessary extra instruction to the epilogue of each affected
function:

  lea    -0x10(%rbp),%rsp

We shouldn't be inserting extra instructions like that.  I also don't
think the other suggestion of turning the '__sp' register variable into
a global variable is a very good solution either, as that just wastes
memory for no reason.

It would be nice if both compilers could agree on a way to support this.

-- 
Josh