| 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
| ||
|
Message-ID: <20181122075206.GG41788@gmail.com>
Date: Thu, 22 Nov 2018 08:52:06 +0100
From: Ingo Molnar <mingo@...nel.org>
To: Thomas Gleixner <tglx@...utronix.de>
Cc: LKML <linux-kernel@...r.kernel.org>, x86@...nel.org,
Peter Zijlstra <peterz@...radead.org>,
Andy Lutomirski <luto@...nel.org>,
Linus Torvalds <torvalds@...ux-foundation.org>,
Jiri Kosina <jkosina@...e.cz>,
Tom Lendacky <thomas.lendacky@....com>,
Josh Poimboeuf <jpoimboe@...hat.com>,
Andrea Arcangeli <aarcange@...hat.com>,
David Woodhouse <dwmw@...zon.co.uk>,
Andi Kleen <ak@...ux.intel.com>,
Dave Hansen <dave.hansen@...el.com>,
Casey Schaufler <casey.schaufler@...el.com>,
Asit Mallick <asit.k.mallick@...el.com>,
Arjan van de Ven <arjan@...ux.intel.com>,
Jon Masters <jcm@...hat.com>,
Waiman Long <longman9394@...il.com>,
Greg KH <gregkh@...uxfoundation.org>,
Dave Stewart <david.c.stewart@...el.com>,
Kees Cook <keescook@...omium.org>
Subject: Re: [patch 17/24] x86/speculation: Move IBPB control out of
switch_mm()
* Thomas Gleixner <tglx@...utronix.de> wrote:
> IBPB control is currently in switch_mm() to avoid issuing IBPB when
> switching between tasks of the same process.
>
> But that's not covering the case of sandboxed tasks which get the
> TIF_SPEC_IB flag set via seccomp. There the barrier is required when the
> potentially malicious task is switched out because the task which is
> switched in might have it not set and would still be attackable.
>
> For tasks which mark themself with TIF_SPEC_IB via the prctl, the barrier
> needs to be when the tasks switches in because the previous one might be an
> attacker.
s/themself
/themselves
>
> Move the code out of switch_mm() and evaluate the TIF bit in
> switch_to(). Make it an inline function so it can be used both in 32bit and
> 64bit code.
s/32bit
/32-bit
s/64bit
/64-bit
>
> This loses the optimization of switching back to the same process, but
> that's wrong in the context of seccomp anyway as it does not protect tasks
> of the same process against each other.
>
> This could be optimized by keeping track of the last user task per cpu and
> avoiding the barrier when the task is immediately scheduled back and the
> thread inbetween was a kernel thread. It's dubious whether that'd be worth
> the extra load/store and conditional operations. Keep it optimized for the
> common case where the TIF bit is not set.
s/cpu/CPU
>
> Signed-off-by: Thomas Gleixner <tglx@...utronix.de>
> ---
> arch/x86/include/asm/nospec-branch.h | 2 +
> arch/x86/include/asm/spec-ctrl.h | 46 +++++++++++++++++++++++++++++++++++
> arch/x86/include/asm/tlbflush.h | 2 -
> arch/x86/kernel/cpu/bugs.c | 16 +++++++++++-
> arch/x86/kernel/process_32.c | 11 ++++++--
> arch/x86/kernel/process_64.c | 11 ++++++--
> arch/x86/mm/tlb.c | 39 -----------------------------
> 7 files changed, 81 insertions(+), 46 deletions(-)
>
> --- a/arch/x86/include/asm/nospec-branch.h
> +++ b/arch/x86/include/asm/nospec-branch.h
> @@ -312,6 +312,8 @@ do { \
> } while (0)
>
> DECLARE_STATIC_KEY_FALSE(switch_to_cond_stibp);
> +DECLARE_STATIC_KEY_FALSE(switch_to_cond_ibpb);
> +DECLARE_STATIC_KEY_FALSE(switch_to_always_ibpb);
>
> #endif /* __ASSEMBLY__ */
>
> --- a/arch/x86/include/asm/spec-ctrl.h
> +++ b/arch/x86/include/asm/spec-ctrl.h
> @@ -76,6 +76,52 @@ static inline u64 ssbd_tif_to_amd_ls_cfg
> return (tifn & _TIF_SSBD) ? x86_amd_ls_cfg_ssbd_mask : 0ULL;
> }
>
> +/**
> + * switch_to_ibpb - Issue IBPB on task switch
> + * @next: Pointer to the next task
> + * @prev_tif: Threadinfo flags of the previous task
> + * @next_tif: Threadinfo flags of the next task
> + *
> + * IBPB flushes the branch predictor, which stops Spectre-v2 attacks
> + * between user space tasks. Depending on the mode the flush is made
> + * conditional.
> + */
> +static inline void switch_to_ibpb(struct task_struct *next,
> + unsigned long prev_tif,
> + unsigned long next_tif)
> +{
> + if (static_branch_unlikely(&switch_to_always_ibpb)) {
> + /* Only flush when switching to a user task. */
> + if (next->mm)
> + indirect_branch_prediction_barrier();
> + }
> +
> + if (static_branch_unlikely(&switch_to_cond_ibpb)) {
> + /*
> + * Both tasks' threadinfo flags are checked for TIF_SPEC_IB.
> + *
> + * For an outgoing sandboxed task which has TIF_SPEC_IB set
> + * via seccomp this is needed because it might be malicious
> + * and the next user task switching in might not have it
> + * set.
> + *
> + * For an incoming task which has set TIF_SPEC_IB itself
> + * via prctl() this is needed because the previous user
> + * task might be malicious and have the flag unset.
> + *
> + * This could be optimized by keeping track of the last
> + * user task per cpu and avoiding the barrier when the task
> + * is immediately scheduled back and the thread inbetween
> + * was a kernel thread. It's dubious whether that'd be
> + * worth the extra load/store and conditional operations.
> + * Keep it optimized for the common case where the TIF bit
> + * is not set.
> + */
> + if ((prev_tif | next_tif) & _TIF_SPEC_IB)
> + indirect_branch_prediction_barrier();
s/cpu/CPU
> +
> + switch (mode) {
> + case SPECTRE_V2_APP2APP_STRICT:
> + static_branch_enable(&switch_to_always_ibpb);
> + break;
> + default:
> + break;
> + }
> +
> + pr_info("mitigation: Enabling %s Indirect Branch Prediction Barrier\n",
> + mode == SPECTRE_V2_APP2APP_STRICT ? "forced" : "conditional");
Maybe s/forced/always-on, to better match the code?
> @@ -617,11 +619,16 @@ void compat_start_thread(struct pt_regs
> /* Reload sp0. */
> update_task_stack(next_p);
>
> + prev_tif = task_thread_info(prev_p)->flags;
> + next_tif = task_thread_info(next_p)->flags;
> + /* Indirect branch prediction barrier control */
> + switch_to_ibpb(next_p, prev_tif, next_tif);
> +
> /*
> * Now maybe reload the debug registers and handle I/O bitmaps
> */
> - if (unlikely(task_thread_info(next_p)->flags & _TIF_WORK_CTXSW_NEXT ||
> - task_thread_info(prev_p)->flags & _TIF_WORK_CTXSW_PREV))
> + if (unlikely(next_tif & _TIF_WORK_CTXSW_NEXT ||
> + prev_tif & _TIF_WORK_CTXSW_PREV))
> __switch_to_xtra(prev_p, next_p, tss);
Hm, the repetition between process_32.c and process_64.c is getting
stronger - could some of this be unified into process.c? (in later
patches)
Thanks,
Ingo
Powered by blists - more mailing lists