[PATCH] x86/CPUID: don't override tool stack decision to hide STIBP

[PATCH] x86/CPUID: don't override tool stack decision to hide STIBP

[Jan Beulich]

Other than in the feature sets, where we indeed want to offer the
feature even if not enumerated on hardware, we shouldn't dictate the
feature being available if tool stack or host admin have decided not
to expose it (for whatever [questionable?] reason).

Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
This is effectively accompanying the discussion rooted at the 4.8/4.7
patch at
https://lists.xenproject.org/archives/html/xen-devel/2018-05/msg01028.html 
dealing with a feature leveling issue.

--- a/xen/arch/x86/cpuid.c
+++ b/xen/arch/x86/cpuid.c
@@ -642,14 +642,6 @@ void recalculate_cpuid_policy(struct dom
     recalculate_xstate(p);
     recalculate_misc(p);
 
-    /*
-     * Override STIBP to match IBRS.  Guests can safely use STIBP
-     * functionality on non-HT hardware, but can't necesserily protect
-     * themselves from SP2/Spectre/Branch Target Injection if STIBP is hidden
-     * on HT-capable hardware.
-     */
-    p->feat.stibp = p->feat.ibrsb;
-
     for ( i = 0; i < ARRAY_SIZE(p->cache.raw); ++i )
     {
         if ( p->cache.subleaf[i].type >= 1 &&



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[PATCH] x86/CPUID: don't override tool stack decision to hide STIBP

[Andrew Cooper]

[-- Attachment #1.1: Type: text/plain, Size: 2539 bytes --]

On 22/05/18 11:33, Jan Beulich wrote:
> Other than in the feature sets, where we indeed want to offer the
> feature even if not enumerated on hardware, we shouldn't dictate the
> feature being available if tool stack or host admin have decided not
> to expose it (for whatever [questionable?] reason).
>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> ---
> This is effectively accompanying the discussion rooted at the 4.8/4.7
> patch at
> https://lists.xenproject.org/archives/html/xen-devel/2018-05/msg01028.html 
> dealing with a feature leveling issue.
>
> --- a/xen/arch/x86/cpuid.c
> +++ b/xen/arch/x86/cpuid.c
> @@ -642,14 +642,6 @@ void recalculate_cpuid_policy(struct dom
>      recalculate_xstate(p);
>      recalculate_misc(p);
>  
> -    /*
> -     * Override STIBP to match IBRS.  Guests can safely use STIBP
> -     * functionality on non-HT hardware, but can't necesserily protect
> -     * themselves from SP2/Spectre/Branch Target Injection if STIBP is hidden
> -     * on HT-capable hardware.
> -     */
> -    p->feat.stibp = p->feat.ibrsb;

You've deleted a comment explaining why this is needed for safety
reasons, without addressing the safety argument.  Simply "because we
shouldn't override the toolstack" isn't a reasonable argument.

With the SP2 microcode, we have the following situations which can occur:
 * No mitigations
 * IBRSB visible
 * IBRSB and STIBP visible

IBSRB enumerates MSR_SPEC_CTRL, MSR_SPEC_CTRL.IBRS, and MSR_PRED_CMD.
STIBP enumerates MSR_SPEC_CTRL.STIBP

SPEC_CTRL.STIBP is specified as usable (albeit, as a nop) even if STIBP
isn't enumerated.  This is deliberately and explicitly for heterogeneous
migration scenarios, as it won't be a nop on other processors.  In
practice, this is so hypervisors can offer the feature unilaterally, and
have it usable on non-HT hardware.

However, to safely level it, dom0 needs to see it set in the information
used to construct the guest policies.  In principle, this should just be
in the guest policy which needs adjusting.

However, due to still not having got the CPUID policy improvements
finished, dom0 is still excluded from cpuid faulting for the exclusive
benefit of the CPUID logic in the domain builder, because it uses native
CPUID to construct the guests CPUID policy.  Therefore, dom0 is unable
to create a safe CPUID policy for the guest, and Xen must override the
setting.

If you try and make this change, you will introduce a guest security
issue on some hardware by hiding STIBP which is necessary for safety.

~Andrew

[-- Attachment #1.2: Type: text/html, Size: 3265 bytes --]

[-- Attachment #2: Type: text/plain, Size: 157 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH] x86/CPUID: don't override tool stack decision to hide STIBP

[Jan Beulich]

>>> On 24.05.18 at 18:44, <andrew.cooper3@citrix.com> wrote:
> On 22/05/18 11:33, Jan Beulich wrote:
>> Other than in the feature sets, where we indeed want to offer the
>> feature even if not enumerated on hardware, we shouldn't dictate the
>> feature being available if tool stack or host admin have decided not
>> to expose it (for whatever [questionable?] reason).
>>
>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>> ---
>> This is effectively accompanying the discussion rooted at the 4.8/4.7
>> patch at
>> https://lists.xenproject.org/archives/html/xen-devel/2018-05/msg01028.html 
>> dealing with a feature leveling issue.
>>
>> --- a/xen/arch/x86/cpuid.c
>> +++ b/xen/arch/x86/cpuid.c
>> @@ -642,14 +642,6 @@ void recalculate_cpuid_policy(struct dom
>>      recalculate_xstate(p);
>>      recalculate_misc(p);
>>  
>> -    /*
>> -     * Override STIBP to match IBRS.  Guests can safely use STIBP
>> -     * functionality on non-HT hardware, but can't necesserily protect
>> -     * themselves from SP2/Spectre/Branch Target Injection if STIBP is hidden
>> -     * on HT-capable hardware.
>> -     */
>> -    p->feat.stibp = p->feat.ibrsb;
> 
> You've deleted a comment explaining why this is needed for safety
> reasons, without addressing the safety argument.  Simply "because we
> shouldn't override the toolstack" isn't a reasonable argument.

It very much is: Policy decisions belong, as far as possible, in the tool
stack rather than the hypervisor, and in the admin's hands rather than
pre-programmed tool stack behavior.

> With the SP2 microcode, we have the following situations which can occur:
>  * No mitigations
>  * IBRSB visible
>  * IBRSB and STIBP visible
> 
> IBSRB enumerates MSR_SPEC_CTRL, MSR_SPEC_CTRL.IBRS, and MSR_PRED_CMD.
> STIBP enumerates MSR_SPEC_CTRL.STIBP
> 
> SPEC_CTRL.STIBP is specified as usable (albeit, as a nop) even if STIBP
> isn't enumerated.  This is deliberately and explicitly for heterogeneous
> migration scenarios, as it won't be a nop on other processors.  In
> practice, this is so hypervisors can offer the feature unilaterally, and
> have it usable on non-HT hardware.
> 
> However, to safely level it, dom0 needs to see it set in the information
> used to construct the guest policies.  In principle, this should just be
> in the guest policy which needs adjusting.
> 
> However, due to still not having got the CPUID policy improvements
> finished, dom0 is still excluded from cpuid faulting for the exclusive
> benefit of the CPUID logic in the domain builder, because it uses native
> CPUID to construct the guests CPUID policy.  Therefore, dom0 is unable
> to create a safe CPUID policy for the guest, and Xen must override the
> setting.

I don't understand this. Both xc_cpuid_{hvm,pv}_policy() have

    case 0x00000007: /* Intel-defined CPU features */
        if ( input[1] == 0 )
        {
            regs[1] = info->featureset[featureword_of(X86_FEATURE_FSGSBASE)];
            regs[2] = info->featureset[featureword_of(X86_FEATURE_PREFETCHWT1)];
            regs[3] = info->featureset[featureword_of(X86_FEATURE_AVX512_4VNNIW)];
        }

Where is the use of native CPUID here?

Jan



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH] x86/CPUID: don't override tool stack decision to hide STIBP

[Andrew Cooper]

On 25/05/18 08:17, Jan Beulich wrote:
>>>> On 24.05.18 at 18:44, <andrew.cooper3@citrix.com> wrote:
>> On 22/05/18 11:33, Jan Beulich wrote:
>>> Other than in the feature sets, where we indeed want to offer the
>>> feature even if not enumerated on hardware, we shouldn't dictate the
>>> feature being available if tool stack or host admin have decided not
>>> to expose it (for whatever [questionable?] reason).
>>>
>>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>>> ---
>>> This is effectively accompanying the discussion rooted at the 4.8/4.7
>>> patch at
>>> https://lists.xenproject.org/archives/html/xen-devel/2018-05/msg01028.html 
>>> dealing with a feature leveling issue.
>>>
>>> --- a/xen/arch/x86/cpuid.c
>>> +++ b/xen/arch/x86/cpuid.c
>>> @@ -642,14 +642,6 @@ void recalculate_cpuid_policy(struct dom
>>>      recalculate_xstate(p);
>>>      recalculate_misc(p);
>>>  
>>> -    /*
>>> -     * Override STIBP to match IBRS.  Guests can safely use STIBP
>>> -     * functionality on non-HT hardware, but can't necesserily protect
>>> -     * themselves from SP2/Spectre/Branch Target Injection if STIBP is hidden
>>> -     * on HT-capable hardware.
>>> -     */
>>> -    p->feat.stibp = p->feat.ibrsb;
>> You've deleted a comment explaining why this is needed for safety
>> reasons, without addressing the safety argument.  Simply "because we
>> shouldn't override the toolstack" isn't a reasonable argument.
> It very much is: Policy decisions belong, as far as possible, in the tool
> stack rather than the hypervisor, and in the admin's hands rather than
> pre-programmed tool stack behavior.

Policy decisions, absolutely, but a patch like this needs justify why it
is deleting a safety check.  One valid justification would be "the
safety property this override is trying to achieve is actually already
safe because of $X".

>
>> With the SP2 microcode, we have the following situations which can occur:
>>  * No mitigations
>>  * IBRSB visible
>>  * IBRSB and STIBP visible
>>
>> IBSRB enumerates MSR_SPEC_CTRL, MSR_SPEC_CTRL.IBRS, and MSR_PRED_CMD.
>> STIBP enumerates MSR_SPEC_CTRL.STIBP
>>
>> SPEC_CTRL.STIBP is specified as usable (albeit, as a nop) even if STIBP
>> isn't enumerated.  This is deliberately and explicitly for heterogeneous
>> migration scenarios, as it won't be a nop on other processors.  In
>> practice, this is so hypervisors can offer the feature unilaterally, and
>> have it usable on non-HT hardware.
>>
>> However, to safely level it, dom0 needs to see it set in the information
>> used to construct the guest policies.  In principle, this should just be
>> in the guest policy which needs adjusting.
>>
>> However, due to still not having got the CPUID policy improvements
>> finished, dom0 is still excluded from cpuid faulting for the exclusive
>> benefit of the CPUID logic in the domain builder, because it uses native
>> CPUID to construct the guests CPUID policy.  Therefore, dom0 is unable
>> to create a safe CPUID policy for the guest, and Xen must override the
>> setting.
> I don't understand this. Both xc_cpuid_{hvm,pv}_policy() have
>
>     case 0x00000007: /* Intel-defined CPU features */
>         if ( input[1] == 0 )
>         {
>             regs[1] = info->featureset[featureword_of(X86_FEATURE_FSGSBASE)];
>             regs[2] = info->featureset[featureword_of(X86_FEATURE_PREFETCHWT1)];
>             regs[3] = info->featureset[featureword_of(X86_FEATURE_AVX512_4VNNIW)];
>         }
>
> Where is the use of native CPUID here?

Oh - I'd completely forgotten I'd fixed up the xc_cpuid_set() path
(which libxl uses for cpuid=[]) as well as the xc_cpuid_apply_policy()
path which is used for "I'd like a default setup please".

Therefore, c/s 3e0c8272f200457d9a735070b66a0da808ac3924 looks to be the
point after which libxc becomes safe (WRT to these native CPUID
concerns), so long as the featureset do have a IBRSB -> STIBP override
visible in them.

In staging at the moment, the IBRSB -> STIBP override is performed in
guest_common_feature_adjustments(), but this gets rather more
complicated on older branches.  Perhaps the easiest way to test is with
`cpuid=no-stibp` and looking at xen-cpuid.

Here is a sample from one of my test boxes:

[root@fusebot ~]# xen-cpuid 
nr_features: 10
                      KEY 1d       1c       e1d      e1c      Da1      7b0      7c0      e7d      e8b      7d0      

Static sets:
Known                     bfebfbff:fffef3ff:ee500800:2469bfff:0000000f:fdbfffff:0040401f:00000500:00001001:8c00000c
Special                   10000200:88200000:00000000:00000002:00000000:00002040:00000010:00000000:00000000:08000000
PV Mask                   1fc9cbf5:f6f83203:e2500800:042109e3:00000007:fdaf0b39:00404003:00000000:00001001:8c00000c
HVM Shadow Mask           1fcbfbff:f7f83223:ea500800:042189f7:0000000f:fdbf4bbb:00404007:00000000:00001001:8c00000c
HVM Hap Mask              1fcbfbff:f7fa3223:ee500800:042189f7:0000000f:fdbf4fbb:0040400f:00000000:00001001:8c00000c

Dynamic sets:
Raw                       bfebfbff:7ffafbff:2c100800:00000021:00000001:000027ab:00000000:00000100:00000000:8c000000
Host                      bfebfbff:77faf3ff:2c100800:00000021:00000001:000027ab:00000000:00000100:00000000:84000000
PV                        1fc9cbf5:f6f83203:20100800:00000021:00000001:00000329:00000000:00000000:00001000:8c000000
HVM                       1fcbfbff:f7fa3223:2c100800:00000021:00000001:000007ab:00000000:00000000:00001000:8c000000

In the raw dynamic set, we see IBRSB, STIBP (and SSBD, seeing as I've
got some microcode).  STIBP is clear in the Host policy (because of the
cpuid= override), yet still set in the PV and HVM featuresets.

Therefore, a default guest constructed on non-HT hardware will see STIBP
visible, per the intention of its special case.

However, if you are going to make this change, then you're missing the
following hunk:

diff --git a/xen/include/public/arch-x86/cpufeatureset.h b/xen/include/public/arch-x86/cpufeatureset.h
index c721c12..f1a5ed9 100644
--- a/xen/include/public/arch-x86/cpufeatureset.h
+++ b/xen/include/public/arch-x86/cpufeatureset.h
@@ -243,7 +243,7 @@ XEN_CPUFEATURE(IBPB,          8*32+12) /*A  IBPB support only (no IBRS, used by
 XEN_CPUFEATURE(AVX512_4VNNIW, 9*32+ 2) /*A  AVX512 Neural Network Instructions */
 XEN_CPUFEATURE(AVX512_4FMAPS, 9*32+ 3) /*A  AVX512 Multiply Accumulation Single Precision */
 XEN_CPUFEATURE(IBRSB,         9*32+26) /*A  IBRS and IBPB support (used by Intel) */
-XEN_CPUFEATURE(STIBP,         9*32+27) /*A! STIBP */
+XEN_CPUFEATURE(STIBP,         9*32+27) /*A  STIBP */
 XEN_CPUFEATURE(ARCH_CAPS,     9*32+29) /*   IA32_ARCH_CAPABILITIES MSR */
 XEN_CPUFEATURE(SSBD,          9*32+31) /*A  MSR_SPEC_CTRL.SSBD available */
 

which is the signal that:

...
 * Special: '!'
 *   This bit has special properties and is not a straight indication of a
 *   piece of new functionality.  Xen will handle these differently,
 *   and may override toolstack settings completely.
...

~Andrew

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH] x86/CPUID: don't override tool stack decision to hide STIBP

[Jan Beulich]

>>> On 25.05.18 at 15:52, <andrew.cooper3@citrix.com> wrote:
> On 25/05/18 08:17, Jan Beulich wrote:
>>>>> On 24.05.18 at 18:44, <andrew.cooper3@citrix.com> wrote:
>>> On 22/05/18 11:33, Jan Beulich wrote:
>>>> Other than in the feature sets, where we indeed want to offer the
>>>> feature even if not enumerated on hardware, we shouldn't dictate the
>>>> feature being available if tool stack or host admin have decided not
>>>> to expose it (for whatever [questionable?] reason).
>>>>
>>>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>>>> ---
>>>> This is effectively accompanying the discussion rooted at the 4.8/4.7
>>>> patch at
>>>> https://lists.xenproject.org/archives/html/xen-devel/2018-05/msg01028.html 
>>>> dealing with a feature leveling issue.
>>>>
>>>> --- a/xen/arch/x86/cpuid.c
>>>> +++ b/xen/arch/x86/cpuid.c
>>>> @@ -642,14 +642,6 @@ void recalculate_cpuid_policy(struct dom
>>>>      recalculate_xstate(p);
>>>>      recalculate_misc(p);
>>>>  
>>>> -    /*
>>>> -     * Override STIBP to match IBRS.  Guests can safely use STIBP
>>>> -     * functionality on non-HT hardware, but can't necesserily protect
>>>> -     * themselves from SP2/Spectre/Branch Target Injection if STIBP is hidden
>>>> -     * on HT-capable hardware.
>>>> -     */
>>>> -    p->feat.stibp = p->feat.ibrsb;
>>> You've deleted a comment explaining why this is needed for safety
>>> reasons, without addressing the safety argument.  Simply "because we
>>> shouldn't override the toolstack" isn't a reasonable argument.
>> It very much is: Policy decisions belong, as far as possible, in the tool
>> stack rather than the hypervisor, and in the admin's hands rather than
>> pre-programmed tool stack behavior.
> 
> Policy decisions, absolutely, but a patch like this needs justify why it
> is deleting a safety check.  One valid justification would be "the
> safety property this override is trying to achieve is actually already
> safe because of $X".

That's what I would have hoped the initial part of the description conveys.

> However, if you are going to make this change, then you're missing the
> following hunk:
> 
> diff --git a/xen/include/public/arch-x86/cpufeatureset.h 
> b/xen/include/public/arch-x86/cpufeatureset.h
> index c721c12..f1a5ed9 100644
> --- a/xen/include/public/arch-x86/cpufeatureset.h
> +++ b/xen/include/public/arch-x86/cpufeatureset.h
> @@ -243,7 +243,7 @@ XEN_CPUFEATURE(IBPB,          8*32+12) /*A  IBPB support only (no IBRS, used by
>  XEN_CPUFEATURE(AVX512_4VNNIW, 9*32+ 2) /*A  AVX512 Neural Network Instructions */
>  XEN_CPUFEATURE(AVX512_4FMAPS, 9*32+ 3) /*A  AVX512 Multiply Accumulation Single Precision */
>  XEN_CPUFEATURE(IBRSB,         9*32+26) /*A  IBRS and IBPB support (used by Intel) */
> -XEN_CPUFEATURE(STIBP,         9*32+27) /*A! STIBP */
> +XEN_CPUFEATURE(STIBP,         9*32+27) /*A  STIBP */
>  XEN_CPUFEATURE(ARCH_CAPS,     9*32+29) /*   IA32_ARCH_CAPABILITIES MSR */
>  XEN_CPUFEATURE(SSBD,          9*32+31) /*A  MSR_SPEC_CTRL.SSBD available */
>  
> 
> which is the signal that:
> 
> ...
>  * Special: '!'
>  *   This bit has special properties and is not a straight indication of a
>  *   piece of new functionality.  Xen will handle these differently,
>  *   and may override toolstack settings completely.
> ...

I did consider this before submitting, but the special casing in the feature set
construction left me uncertain how special "special" really means. I notice I
forgot to add a respective remark after the --- marker. I'm fine dropping the !.

Now that I think we've managed to settle on a way forward for this patch,
what about the proposed alternative patch for 4.8 and 4.7? It has the same
net effect as the one here, after all (and afaict).

Jan



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel