linux-kernel - Re: [PATCH] x86/retpoline: Avoid return buffer underflows on context switch

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 for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20180108221121.GU29822@worktop.programming.kicks-ass.net>
Date:   Mon, 8 Jan 2018 23:11:21 +0100
From:   Peter Zijlstra <peterz@...radead.org>
To:     Andi Kleen <andi@...stfloor.org>
Cc:     dwmw@...zon.co.uk, pjt@...gle.com, linux-kernel@...r.kernel.org,
        gregkh@...ux-foundation.org, tim.c.chen@...ux.intel.com,
        dave.hansen@...el.com, tglx@...utronix.de, luto@...capital.net,
        Andi Kleen <ak@...ux.intel.com>
Subject: Re: [PATCH] x86/retpoline: Avoid return buffer underflows on context
 switch

On Mon, Jan 08, 2018 at 12:15:31PM -0800, Andi Kleen wrote:
> diff --git a/arch/x86/include/asm/nospec-branch.h b/arch/x86/include/asm/nospec-branch.h
> index b8c8eeacb4be..e84e231248c2 100644
> --- a/arch/x86/include/asm/nospec-branch.h
> +++ b/arch/x86/include/asm/nospec-branch.h
> @@ -53,6 +53,35 @@
>  #endif
>  .endm
>  
> +/*
> + * We use 32-N: 32 is the max return buffer size,
> + * but there should have been at a minimum two
> + * controlled calls already: one into the kernel
> + * from entry*.S and another into the function
> + * containing this macro. So N=2, thus 30.
> + */
> +#define NUM_BRANCHES_TO_FILL	30
> +
> +/*
> + * Fill the CPU return branch buffer to prevent
> + * indirect branch prediction on underflow.
> + * Caller should check for X86_FEATURE_SMEP and X86_FEATURE_RETPOLINE
> + */
> +.macro FILL_RETURN_BUFFER
> +#ifdef CONFIG_RETPOLINE
> +	.rept	NUM_BRANCHES_TO_FILL
> +	call	1221f
> +	pause	/* stop speculation */
> +1221:
> +	.endr
> +#ifdef CONFIG_64BIT
> +	addq	$8*NUM_BRANCHES_TO_FILL, %rsp
> +#else
> +	addl    $4*NUM_BRANCHES_TO_FILL, %esp
> +#endif
> +#endif
> +.endm

So pjt did alignment, a single unroll and per discussion earlier today
(CET) or late last night (PST), he only does 16.

Why is none of that done here? Also, can we pretty please stop using
those retarded number labels, they make this stuff unreadable.

Also, pause is unlikely to stop speculation, that comment doesn't make
sense. Looking at PJT's version there used to be a speculation trap in
there, but I can't see that here.