[MODERATED] Re: [PATCH v7 04/10] TAAv7 4

[Josh Poimboeuf <jpoimboe@redhat.com>]

On Tue, Oct 22, 2019 at 11:03:19PM +0100, speck for Andrew Cooper wrote:
> On 22/10/2019 22:44, speck for Josh Poimboeuf wrote:
> > On Tue, Oct 22, 2019 at 10:35:12PM +0100, speck for Andrew Cooper wrote:
> >> On 22/10/2019 22:20, speck for Josh Poimboeuf wrote:
> >>> On Mon, Oct 21, 2019 at 01:26:02PM -0700, speck for Pawan Gupta wrote:
> >>>> diff --git a/arch/x86/include/asm/processor.h b/arch/x86/include/asm/processor.h
> >>>> index 6e0a3b43d027..999b85039128 100644
> >>>> --- a/arch/x86/include/asm/processor.h
> >>>> +++ b/arch/x86/include/asm/processor.h
> >>>> @@ -988,4 +988,11 @@ enum mds_mitigations {
> >>>>  	MDS_MITIGATION_VMWERV,
> >>>>  };
> >>>>  
> >>>> +enum taa_mitigations {
> >>>> +	TAA_MITIGATION_OFF,
> >>>> +	TAA_MITIGATION_UCODE_NEEDED,
> >>>> +	TAA_MITIGATION_VERW,
> >>>> +	TAA_MITIGATION_TSX_DISABLE,
> >>> I would rename "TSX_DISABLE" to "TSX_DISABLED", because
> >>>
> >>>  a) it matches the verb tense of "UCODE_NEEDED", and
> >>>  b) the past tense hopefully helps make it slightly clearer that TSX was
> >>>     disabled separately, not as part of the mitigation code itself.
> >>>
> >>>> @@ -765,7 +871,7 @@ static void update_indir_branch_cond(void)
> >>>>  #undef pr_fmt
> >>>>  #define pr_fmt(fmt) fmt
> >>>>  
> >>>> -/* Update the static key controlling the MDS CPU buffer clear in idle */
> >>>> +/* Update the static key controlling the MDS and TAA CPU buffer clear in idle */
> >>>>  static void update_mds_branch_idle(void)
> >>>>  {
> >>>>  	/*
> >>>> @@ -775,8 +881,11 @@ static void update_mds_branch_idle(void)
> >>>>  	 * The other variants cannot be mitigated when SMT is enabled, so
> >>>>  	 * clearing the buffers on idle just to prevent the Store Buffer
> >>>>  	 * repartitioning leak would be a window dressing exercise.
> >>>> +	 *
> >>>> +	 * Apply idle buffer clearing to TAA affected CPUs also.
> >>>>  	 */
> >>>> -	if (!boot_cpu_has_bug(X86_BUG_MSBDS_ONLY))
> >>>> +	if (!boot_cpu_has_bug(X86_BUG_MSBDS_ONLY) &&
> >>>> +	    !boot_cpu_has_bug(X86_BUG_TAA))
> >>>>  		return;
> >>> Sorry, I was out for most of the last two weeks, so I think I left you
> >>> hanging on this.  For context, here's your answer to my previous
> >>> question about whether the X86_BUG_TAA check makes sense here:
> >>>
> >>>> It does provide protection against the "store buffer" leak. But the
> >>>> other buffers(fill buffer and load port) are still SMT vulnerable. Do
> >>>> you prefer it removed?
> >>> Yes, please remove it, for the same reason we used for MDS.  There's not
> >>> much point in partially mitigating TAA here.
> >>>
> >>>> diff --git a/arch/x86/kernel/cpu/common.c b/arch/x86/kernel/cpu/common.c
> >>>> index 885d4ac2111a..86f22c1e5912 100644
> >>>> --- a/arch/x86/kernel/cpu/common.c
> >>>> +++ b/arch/x86/kernel/cpu/common.c
> >>>> @@ -1128,6 +1128,21 @@ static void __init cpu_set_bug_bits(struct cpuinfo_x86 *c)
> >>>>  	if (!cpu_matches(NO_SWAPGS))
> >>>>  		setup_force_cpu_bug(X86_BUG_SWAPGS);
> >>>>  
> >>>> +	/*
> >>>> +	 * When processor is not mitigated for TAA (TAA_NO=0) set TAA bug when:
> >>>> +	 *	- TSX is supported or
> >>>> +	 *	- TSX_CTRL is supported
> >>>> +	 *
> >>>> +	 * TSX_CTRL check is needed for cases when TSX could be disabled before
> >>>> +	 * the kernel boot e.g. kexec
> >>>> +	 * TSX_CTRL check alone is not sufficient for cases when the microcode
> >>>> +	 * update is not present or running as guest that don't get TSX_CTRL.
> >>>> +	 */
> >>>> +	if (!(ia32_cap & ARCH_CAP_TAA_NO) &&
> >>>> +	    (boot_cpu_has(X86_FEATURE_RTM) ||
> >>>> +	     (ia32_cap & ARCH_CAP_TSX_CTRL_MSR)))
> >>>> +		setup_force_cpu_bug(X86_BUG_TAA);
> >>>> +
> >>> I'm finding this logic to be less than 100% convincing, or at least hard
> >>> to follow.  And it's different from most of the other bug checks in this
> >>> function.
> >>>
> >>> Would the following work instead?
> >>>
> >>> 	if (!cpu_matches(NO_MDS) && !(ia32_cap & ARCH_CAP_TAA_NO))
> >>> 		setup_force_cpu_bug(X86_BUG_TAA);
> >>>
> >>> In other words I would presume all the NO_MDS CPUs listed in
> >>> 'cpu_vuln_whitelist' are also immune to TAA.
> >> From a leakage point of view, TAA is just another way to access stale
> >> date in the load ports, store buffer or fill buffers.
> >>
> >> In practice, the leakage mechanism exists on all MDS-vulnerable,
> >> TSX-enabled parts.  Parts which are not vulnerable to MDS leakage in the
> >> first place will be similarly unaffected.
> >>
> >> On pre MDS_NO=1 parts, the existing VERW and no-smt mitigates TAA as well.
> >>
> >> On the current MDS_NO=1 parts, the silicon fix constituting MDS_NO took
> >> care of the known leakage cases by forcing a register file write-back of
> >> 0 rather than the stale data.  However, the TSX Async Abort case was
> >> missed, which is why this generation of parts are still vulnerable.
> >>
> >> Therefore, the TAA vulnerability only really matters for TSX=1,
> >> MDS_NO=1, TAA_NO=0 parts, because these parts are the ones that are
> >> leaky, but have a lower overhead mitigation option than VERW and no-SMT.
> > I agree with all of that.  So are you saying you agree or disagree with
> > my code snippet?
> 
> Ah - sorry for being unclear.  Disagree.
> 
> Your code snippet will cause some Atom and very old Core parts to also
> report to be TAA vulnerable, as they are on the MDS whitelist.

Ok, so I assume those parts don't have TSX?  Can we clarify which ones
those are?  We could add a "NO_TAA" to the whitelist bits.

-- 
Josh