linux-kernel - Re: [RFC PATCH 04/21] x86/hweight: Add stack frame dependency for __arch

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: <20150717171726.GA21568@nazgul.tnic>
Date:	Fri, 17 Jul 2015 19:17:26 +0200
From:	Borislav Petkov <bp@...en8.de>
To:	Josh Poimboeuf <jpoimboe@...hat.com>
Cc:	Thomas Gleixner <tglx@...utronix.de>,
	Ingo Molnar <mingo@...hat.com>,
	"H. Peter Anvin" <hpa@...or.com>, Michal Marek <mmarek@...e.cz>,
	Peter Zijlstra <peterz@...radead.org>,
	Andy Lutomirski <luto@...nel.org>,
	Linus Torvalds <torvalds@...ux-foundation.org>,
	Andi Kleen <andi@...stfloor.org>,
	Pedro Alves <palves@...hat.com>, x86@...nel.org,
	live-patching@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [RFC PATCH 04/21] x86/hweight: Add stack frame dependency for
 __arch_hweight*()

On Fri, Jul 17, 2015 at 11:47:20AM -0500, Josh Poimboeuf wrote:
> If __arch_hweight32() or __arch_hweight64() is inlined at the beginning
> of a function, gcc can insert the call instruction before setting up a
> stack frame, which breaks frame pointer convention if
> CONFIG_FRAME_POINTER is enabled and can result in a bad stack trace.
> 
> Force a stack frame to be created if CONFIG_FRAME_POINTER is enabled by
> listing the stack pointer as an output operand for the inline asm
> statement.
> 
> Signed-off-by: Josh Poimboeuf <jpoimboe@...hat.com>
> ---
>  arch/x86/include/asm/arch_hweight.h | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/x86/include/asm/arch_hweight.h b/arch/x86/include/asm/arch_hweight.h
> index 9686c3d..e438a0d 100644
> --- a/arch/x86/include/asm/arch_hweight.h
> +++ b/arch/x86/include/asm/arch_hweight.h
> @@ -23,10 +23,11 @@
>   */
>  static inline unsigned int __arch_hweight32(unsigned int w)
>  {
> +	register void *__sp asm("esp");
>  	unsigned int res = 0;
>  
>  	asm (ALTERNATIVE("call __sw_hweight32", POPCNT32, X86_FEATURE_POPCNT)
> -		     : "="REG_OUT (res)
> +		     : "="REG_OUT (res), "+r" (__sp)
>  		     : REG_IN (w));
>  
>  	return res;
> @@ -44,6 +45,7 @@ static inline unsigned int __arch_hweight8(unsigned int w)
>  
>  static inline unsigned long __arch_hweight64(__u64 w)
>  {
> +	register void __maybe_unused *__sp asm("rsp");
>  	unsigned long res = 0;
>  
>  #ifdef CONFIG_X86_32
> @@ -51,7 +53,7 @@ static inline unsigned long __arch_hweight64(__u64 w)
>  		__arch_hweight32((u32)(w >> 32));
>  #else
>  	asm (ALTERNATIVE("call __sw_hweight64", POPCNT64, X86_FEATURE_POPCNT)
> -		     : "="REG_OUT (res)
> +		     : "="REG_OUT (res), "+r" (__sp)
>  		     : REG_IN (w));
>  #endif /* CONFIG_X86_32 */

Eeew, useless code so that some compile-time validation is done. Let's
not add this clutter please.

In this particular case, the majority of CPUs out there will get POPCNT
patched in and that CALL is gone. And for the remaining cases where we
do end up using the __sw_* variants, I'd prefer to rather not do the
validation instead of polluting the code with that fake rsp dependency.

-- 
Regards/Gruss,
    Boris.

ECO tip #101: Trim your mails when you reply.
--
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/