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

[Andrew Cooper <andrew.cooper3@citrix.com>]

[-- Attachment #1: Type: text/plain, Size: 5166 bytes --]

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.

~Andrew