Re: [patch 17/24] x86/speculation: Move IBPB control out of switch_mm()

[Ingo Molnar <mingo@kernel.org>]

* Thomas Gleixner <tglx@linutronix.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@linutronix.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