[PATCH for-4.7/4.8] x86: Fix "x86: further CPUID handling adjustments"

[PATCH for-4.7/4.8] x86: Fix "x86: further CPUID handling adjustments"

[Andrew Cooper]

c/s f9616884e (a backport of c/s 0d703a701 "x86/feature: Definitions for
Indirect Branch Controls") missed a CPUID adjustment when calculating the raw
featureset.  This impacts host administrator diagnostics.

Signed-off-by: Sergey Dyasli <sergey.dyasli@citrix.com>

c/s 62b187969 "x86: further CPUID handling adjustments" make some adjustments.
However, it breaks levelling of guests, making it impossible for the toolstack
to hide STIBP or IBPB from guests on hardware with up-to-date microcode.

Also, I don't see any link between the change and the commit message.  With
the microcode installed, STIBP and IBPB are already visible to dom0.

The only required adjustment is to force STIBP == IBRSB, which must be done
after applying the pv_featureset[] mask to the toolstack's choice of value.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
---
 xen/arch/x86/cpuid.c   | 2 +-
 xen/arch/x86/hvm/hvm.c | 8 +++++---
 xen/arch/x86/traps.c   | 8 +++++---
 3 files changed, 11 insertions(+), 7 deletions(-)

diff --git a/xen/arch/x86/cpuid.c b/xen/arch/x86/cpuid.c
index 451952c..fffcecd 100644
--- a/xen/arch/x86/cpuid.c
+++ b/xen/arch/x86/cpuid.c
@@ -113,7 +113,7 @@ static void __init calculate_raw_featureset(void)
         cpuid_count(0x7, 0, &tmp,
                     &raw_featureset[FEATURESET_7b0],
                     &raw_featureset[FEATURESET_7c0],
-                    &tmp);
+                    &raw_featureset[FEATURESET_7d0]);
     if ( max >= 0xd )
         cpuid_count(0xd, 1,
                     &raw_featureset[FEATURESET_Da1],
diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
index ff1c6fa..0a1d4a9 100644
--- a/xen/arch/x86/hvm/hvm.c
+++ b/xen/arch/x86/hvm/hvm.c
@@ -3496,10 +3496,13 @@ void hvm_cpuid(unsigned int input, unsigned int *eax, unsigned int *ebx,
                      special_features[FEATURESET_7b0]);
 
             *ecx &= hvm_featureset[FEATURESET_7c0];
-
-            *edx |= cpufeat_mask(X86_FEATURE_STIBP);
             *edx &= hvm_featureset[FEATURESET_7d0];
 
+            /* Force STIBP equal to IBRSB */
+            *edx &= ~cpufeat_mask(X86_FEATURE_STIBP);
+            if ( *edx & cpufeat_mask(X86_FEATURE_IBRSB) )
+                *edx |= cpufeat_mask(X86_FEATURE_STIBP);
+
             /* Don't expose HAP-only features to non-hap guests. */
             if ( !hap_enabled(d) )
             {
@@ -3657,7 +3660,6 @@ void hvm_cpuid(unsigned int input, unsigned int *eax, unsigned int *ebx,
         hvm_cpuid(0x80000001, NULL, NULL, NULL, &_edx);
         *eax |= (_edx & cpufeat_mask(X86_FEATURE_LM) ? vaddr_bits : 32) << 8;
 
-        *ebx |= cpufeat_mask(X86_FEATURE_IBPB);
         *ebx &= hvm_featureset[FEATURESET_e8b];
         break;
     }
diff --git a/xen/arch/x86/traps.c b/xen/arch/x86/traps.c
index 0f34b21..da26749 100644
--- a/xen/arch/x86/traps.c
+++ b/xen/arch/x86/traps.c
@@ -1088,10 +1088,13 @@ void pv_cpuid(struct cpu_user_regs *regs)
                   special_features[FEATURESET_7b0]);
 
             c &= pv_featureset[FEATURESET_7c0];
-
-            d |= cpufeat_mask(X86_FEATURE_STIBP);
             d &= pv_featureset[FEATURESET_7d0];
 
+            /* Force STIBP equal to IBRSB */
+            d &= ~cpufeat_mask(X86_FEATURE_STIBP);
+            if ( d & cpufeat_mask(X86_FEATURE_IBRSB) )
+                d |= cpufeat_mask(X86_FEATURE_STIBP);
+
             if ( !is_pvh_domain(currd) )
             {
                 /*
@@ -1188,7 +1191,6 @@ void pv_cpuid(struct cpu_user_regs *regs)
 
     case 0x80000008:
         a = paddr_bits | (vaddr_bits << 8);
-        b |= cpufeat_mask(X86_FEATURE_IBPB);
         b &= pv_featureset[FEATURESET_e8b];
         break;
 
-- 
2.1.4


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

Re: [PATCH for-4.7/4.8] x86: Fix "x86: further CPUID handling adjustments"

[Jan Beulich]

>>> On 15.05.18 at 19:54, <andrew.cooper3@citrix.com> wrote:
> Also, I don't see any link between the change and the commit message.  With
> the microcode installed, STIBP and IBPB are already visible to dom0.

They reportedly weren't (and I was able to confirm that), and given this
original (prior to that change) code

            }
        }
        else
            b = c = 0;
        a = d = 0;

I also can't see how IBRSB and STIBP could have been visible. I agree I
had wrongly extended that to IBPB.

> The only required adjustment is to force STIBP == IBRSB, which must be done
> after applying the pv_featureset[] mask to the toolstack's choice of value.

I can see how I've got that part wrong from a leveling perspective (I was
really too focused on Dom0 back then), but I don't see how reporting IBPB
when IBRSB is available in hardware (implying IBPB itself isn't) would work
with your change in place.

I'm also not convinced assimilating Sergey's original change into this one is
appropriate - raw_featureset[] isn't used for anything except the sysctl.

Jan



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

Re: [PATCH for-4.7/4.8] x86: Fix "x86: further CPUID handling adjustments"

[Andrew Cooper]

On 16/05/18 09:14, Jan Beulich wrote:
>>>> On 15.05.18 at 19:54, <andrew.cooper3@citrix.com> wrote:
>> Also, I don't see any link between the change and the commit message.  With
>> the microcode installed, STIBP and IBPB are already visible to dom0.
> They reportedly weren't (and I was able to confirm that), and given this
> original (prior to that change) code
>
>             }
>         }
>         else
>             b = c = 0;
>         a = d = 0;
>
> I also can't see how IBRSB and STIBP could have been visible. I agree I
> had wrongly extended that to IBPB.
>
>> The only required adjustment is to force STIBP == IBRSB, which must be done
>> after applying the pv_featureset[] mask to the toolstack's choice of value.
> I can see how I've got that part wrong from a leveling perspective (I was
> really too focused on Dom0 back then), but I don't see how reporting IBPB
> when IBRSB is available in hardware (implying IBPB itself isn't) would work
> with your change in place.

I've submitted v2 with an updated commit message, now I understand where
the dom0 comment came from.

>
> I'm also not convinced assimilating Sergey's original change into this one is
> appropriate - raw_featureset[] isn't used for anything except the sysctl.

You regressed a feature with an incorrect backport, in a way which
directly impacts a tool which administrators will use to see if they've
got the microcode applied properly.

It was broken in a security patch, therefore it is going to get fixed.

~Andrew

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

[PATCH v2 for-4.7/4.8] x86: Fix "x86: further CPUID handling adjustments"

[Andrew Cooper]

c/s f9616884e (a backport of c/s 0d703a701 "x86/feature: Definitions for
Indirect Branch Controls") missed a CPUID adjustment when calculating the raw
featureset.  This impacts host administrator diagnostics.

Signed-off-by: Sergey Dyasli <sergey.dyasli@citrix.com>

c/s 62b187969 "x86: further CPUID handling adjustments" make some adjustments.
However, it breaks levelling of guests, making it impossible for the toolstack
to hide STIBP or IBPB from guests on hardware with up-to-date microcode.

The dom0 issue referenced in the commit message was fixed by the hunk
adjusting the zeroing alone.  STIBP and IBPB don't need (and indeed, must not
be for levelling purposes) OR'd into the leaf.

One final item which was missed in backport was the need to ignore the
toolstack choice of STIBP, and set it equal to IBRSB.  This needs doing after
the mask has been applied.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>

v2:
 * Update the commit message, how I understand the dom0 aspect of the previous
   commit message.
---
 xen/arch/x86/cpuid.c   | 2 +-
 xen/arch/x86/hvm/hvm.c | 8 +++++---
 xen/arch/x86/traps.c   | 8 +++++---
 3 files changed, 11 insertions(+), 7 deletions(-)

diff --git a/xen/arch/x86/cpuid.c b/xen/arch/x86/cpuid.c
index 451952c..fffcecd 100644
--- a/xen/arch/x86/cpuid.c
+++ b/xen/arch/x86/cpuid.c
@@ -113,7 +113,7 @@ static void __init calculate_raw_featureset(void)
         cpuid_count(0x7, 0, &tmp,
                     &raw_featureset[FEATURESET_7b0],
                     &raw_featureset[FEATURESET_7c0],
-                    &tmp);
+                    &raw_featureset[FEATURESET_7d0]);
     if ( max >= 0xd )
         cpuid_count(0xd, 1,
                     &raw_featureset[FEATURESET_Da1],
diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
index ff1c6fa..0a1d4a9 100644
--- a/xen/arch/x86/hvm/hvm.c
+++ b/xen/arch/x86/hvm/hvm.c
@@ -3496,10 +3496,13 @@ void hvm_cpuid(unsigned int input, unsigned int *eax, unsigned int *ebx,
                      special_features[FEATURESET_7b0]);
 
             *ecx &= hvm_featureset[FEATURESET_7c0];
-
-            *edx |= cpufeat_mask(X86_FEATURE_STIBP);
             *edx &= hvm_featureset[FEATURESET_7d0];
 
+            /* Force STIBP equal to IBRSB */
+            *edx &= ~cpufeat_mask(X86_FEATURE_STIBP);
+            if ( *edx & cpufeat_mask(X86_FEATURE_IBRSB) )
+                *edx |= cpufeat_mask(X86_FEATURE_STIBP);
+
             /* Don't expose HAP-only features to non-hap guests. */
             if ( !hap_enabled(d) )
             {
@@ -3657,7 +3660,6 @@ void hvm_cpuid(unsigned int input, unsigned int *eax, unsigned int *ebx,
         hvm_cpuid(0x80000001, NULL, NULL, NULL, &_edx);
         *eax |= (_edx & cpufeat_mask(X86_FEATURE_LM) ? vaddr_bits : 32) << 8;
 
-        *ebx |= cpufeat_mask(X86_FEATURE_IBPB);
         *ebx &= hvm_featureset[FEATURESET_e8b];
         break;
     }
diff --git a/xen/arch/x86/traps.c b/xen/arch/x86/traps.c
index 0f34b21..da26749 100644
--- a/xen/arch/x86/traps.c
+++ b/xen/arch/x86/traps.c
@@ -1088,10 +1088,13 @@ void pv_cpuid(struct cpu_user_regs *regs)
                   special_features[FEATURESET_7b0]);
 
             c &= pv_featureset[FEATURESET_7c0];
-
-            d |= cpufeat_mask(X86_FEATURE_STIBP);
             d &= pv_featureset[FEATURESET_7d0];
 
+            /* Force STIBP equal to IBRSB */
+            d &= ~cpufeat_mask(X86_FEATURE_STIBP);
+            if ( d & cpufeat_mask(X86_FEATURE_IBRSB) )
+                d |= cpufeat_mask(X86_FEATURE_STIBP);
+
             if ( !is_pvh_domain(currd) )
             {
                 /*
@@ -1188,7 +1191,6 @@ void pv_cpuid(struct cpu_user_regs *regs)
 
     case 0x80000008:
         a = paddr_bits | (vaddr_bits << 8);
-        b |= cpufeat_mask(X86_FEATURE_IBPB);
         b &= pv_featureset[FEATURESET_e8b];
         break;
 
-- 
2.1.4


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

Re: [PATCH v2 for-4.7/4.8] x86: Fix "x86: further CPUID handling adjustments"

[Jan Beulich]

>>> On 16.05.18 at 19:27, <andrew.cooper3@citrix.com> wrote:
> c/s f9616884e (a backport of c/s 0d703a701 "x86/feature: Definitions for
> Indirect Branch Controls") missed a CPUID adjustment when calculating the raw
> featureset.  This impacts host administrator diagnostics.
> 
> Signed-off-by: Sergey Dyasli <sergey.dyasli@citrix.com>

I continue to think this should remain to be a separate patch.

> c/s 62b187969 "x86: further CPUID handling adjustments" make some adjustments.
> However, it breaks levelling of guests, making it impossible for the toolstack
> to hide STIBP or IBPB from guests on hardware with up-to-date microcode.
> 
> The dom0 issue referenced in the commit message was fixed by the hunk
> adjusting the zeroing alone.  STIBP and IBPB don't need (and indeed, must not
> be for levelling purposes) OR'd into the leaf.
> 
> One final item which was missed in backport was the need to ignore the
> toolstack choice of STIBP, and set it equal to IBRSB.  This needs doing after
> the mask has been applied.

This last paragraph at least partly contradicts the first talking about tool stack
chosen hiding of STIBP. The intended net effect, aiui, is
- expose STIBP independent of tool stack choice when the tool stack has
  elected to expose IBRSB,
- hide STIBP according to tool stack choice when IBRSB is also hidden.

A similar implication (as mentioned before, and see below) is supposed to
exist from IBRSB to IBPB, I think.

Also this aspect wasn't missed in the original backport, but done wrongly: The
ORing in ahead of the masking was meant to take care of this, utilizing what
calculate_{hvm,pv}_featureset() do (overlooking the fact that the feature sets
may have the bits set while the tool stack may have cleared them for the given
domain).

> @@ -1188,7 +1191,6 @@ void pv_cpuid(struct cpu_user_regs *regs)
>  
>      case 0x80000008:
>          a = paddr_bits | (vaddr_bits << 8);
> -        b |= cpufeat_mask(X86_FEATURE_IBPB);
>          b &= pv_featureset[FEATURESET_e8b];
>          break;

You didn't address my respective v1 review comment, neither by adding code
here, nor verbally: How is Dom0 supposed to know of IBPB being available if
IBSRB is (in hardware), but IBPB isn't? The whole purpose of the respective
chunk of code in calculate_pv_featureset() is just that. And if we  I think this similarly
should be done for DomU-s, i.e. also in the HVM variant of this code.

Jan



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

Re: [PATCH v2 for-4.7/4.8] x86: Fix "x86: further CPUID handling adjustments"

[Jan Beulich]

>>> On 16.05.18 at 19:27, <andrew.cooper3@citrix.com> wrote:
> c/s 62b187969 "x86: further CPUID handling adjustments" make some adjustments.
> However, it breaks levelling of guests, making it impossible for the toolstack
> to hide STIBP or IBPB from guests on hardware with up-to-date microcode.
> 
> The dom0 issue referenced in the commit message was fixed by the hunk
> adjusting the zeroing alone.  STIBP and IBPB don't need (and indeed, must not
> be for levelling purposes) OR'd into the leaf.
> 
> One final item which was missed in backport was the need to ignore the
> toolstack choice of STIBP, and set it equal to IBRSB.  This needs doing after
> the mask has been applied.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

What about the patch below instead? This then allows the tool stack to override
STIBP independent of IBRSB.

Jan

x86: correct "further CPUID handling adjustments"

Commit 62b187969 "x86: further CPUID handling adjustments" went too far,
breaking feature levelling of DomU-s. Restrict the PV overrides to just
Dom0 and undo the HVM overrides.

Reported-by: Andrew Cooper <andrew.cooper3@citrix.com>
Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/xen/arch/x86/hvm/hvm.c
+++ b/xen/arch/x86/hvm/hvm.c
@@ -3587,7 +3587,6 @@ void hvm_cpuid(unsigned int input, unsig
 
             *ecx &= hvm_featureset[FEATURESET_7c0];
 
-            *edx |= cpufeat_mask(X86_FEATURE_STIBP);
             *edx &= hvm_featureset[FEATURESET_7d0];
 
             /* Don't expose HAP-only features to non-hap guests. */
@@ -3761,7 +3760,6 @@ void hvm_cpuid(unsigned int input, unsig
         hvm_cpuid(0x80000001, NULL, NULL, NULL, &_edx);
         *eax |= (_edx & cpufeat_mask(X86_FEATURE_LM) ? vaddr_bits : 32) << 8;
 
-        *ebx |= cpufeat_mask(X86_FEATURE_IBPB);
         *ebx &= hvm_featureset[FEATURESET_e8b];
         break;
     }
--- a/xen/arch/x86/traps.c
+++ b/xen/arch/x86/traps.c
@@ -1156,7 +1156,8 @@ void pv_cpuid(struct cpu_user_regs *regs
 
             c &= pv_featureset[FEATURESET_7c0];
 
-            d |= cpufeat_mask(X86_FEATURE_STIBP);
+            if ( is_hardware_domain(currd) || is_control_domain(currd) )
+                d |= cpufeat_mask(X86_FEATURE_STIBP);
             d &= pv_featureset[FEATURESET_7d0];
 
             if ( !is_pvh_domain(currd) )
@@ -1271,7 +1272,8 @@ void pv_cpuid(struct cpu_user_regs *regs
 
     case 0x80000008:
         a = paddr_bits | (vaddr_bits << 8);
-        b |= cpufeat_mask(X86_FEATURE_IBPB);
+        if ( is_hardware_domain(currd) || is_control_domain(currd) )
+            b |= cpufeat_mask(X86_FEATURE_IBPB);
         b &= pv_featureset[FEATURESET_e8b];
         break;
 


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

Re: [PATCH v2 for-4.7/4.8] x86: Fix "x86: further CPUID handling adjustments"

[Andrew Cooper]

On 17/05/18 13:23, Jan Beulich wrote:
>>>> On 16.05.18 at 19:27, <andrew.cooper3@citrix.com> wrote:
>> c/s 62b187969 "x86: further CPUID handling adjustments" make some adjustments.
>> However, it breaks levelling of guests, making it impossible for the toolstack
>> to hide STIBP or IBPB from guests on hardware with up-to-date microcode.
>>
>> The dom0 issue referenced in the commit message was fixed by the hunk
>> adjusting the zeroing alone.  STIBP and IBPB don't need (and indeed, must not
>> be for levelling purposes) OR'd into the leaf.
>>
>> One final item which was missed in backport was the need to ignore the
>> toolstack choice of STIBP, and set it equal to IBRSB.  This needs doing after
>> the mask has been applied.
>>
>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> What about the patch below instead? This then allows the tool stack to override
> STIBP independent of IBRSB.
>
> Jan
>
> x86: correct "further CPUID handling adjustments"
>
> Commit 62b187969 "x86: further CPUID handling adjustments" went too far,
> breaking feature levelling of DomU-s. Restrict the PV overrides to just
> Dom0 and undo the HVM overrides.

Why?  PV overrides (for dom0 or domU) are equally wrong.

~Andrew

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

Re: [PATCH v2 for-4.7/4.8] x86: Fix "x86: further CPUID handling adjustments"

[Jan Beulich]

>>> On 18.05.18 at 14:21, <andrew.cooper3@citrix.com> wrote:
> On 17/05/18 13:23, Jan Beulich wrote:
>>>>> On 16.05.18 at 19:27, <andrew.cooper3@citrix.com> wrote:
>>> c/s 62b187969 "x86: further CPUID handling adjustments" make some adjustments.
>>> However, it breaks levelling of guests, making it impossible for the toolstack
>>> to hide STIBP or IBPB from guests on hardware with up-to-date microcode.
>>>
>>> The dom0 issue referenced in the commit message was fixed by the hunk
>>> adjusting the zeroing alone.  STIBP and IBPB don't need (and indeed, must not
>>> be for levelling purposes) OR'd into the leaf.
>>>
>>> One final item which was missed in backport was the need to ignore the
>>> toolstack choice of STIBP, and set it equal to IBRSB.  This needs doing after
>>> the mask has been applied.
>>>
>>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
>> What about the patch below instead? This then allows the tool stack to override
>> STIBP independent of IBRSB.
>>
>> Jan
>>
>> x86: correct "further CPUID handling adjustments"
>>
>> Commit 62b187969 "x86: further CPUID handling adjustments" went too far,
>> breaking feature levelling of DomU-s. Restrict the PV overrides to just
>> Dom0 and undo the HVM overrides.
> 
> Why?  PV overrides (for dom0 or domU) are equally wrong.

Why do you talk about DomU? Those overrides are being removed by the
proposed patch. For Dom0, otoh, there's no other way to do the "IBRSB
implies both STIBP and IBPB" override we otherwise advertise to the tool
stack via the PV and HVM feature sets. And I can't see why we wouldn't
want to treat Dom0 equally in this regard. Even on master I think we only
do the STIBP part of the above, while the IBPB part is only done for the
feature sets.

Furthermore, as indicated before, I'm not convinced overriding STIBP
when the tool stack may have intentionally masked it (as your patch
does, and as I understand is the effect of

    /*
     * 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;

in master) is the right thing to do. Advertising it in the feature set should
be all that's needed.

Jan



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

Re: [PATCH v2 for-4.7/4.8] x86: Fix "x86: further CPUID handling adjustments"

[Andrew Cooper]

On 18/05/18 13:37, Jan Beulich wrote:
>>>> On 18.05.18 at 14:21, <andrew.cooper3@citrix.com> wrote:
>> On 17/05/18 13:23, Jan Beulich wrote:
>>>>>> On 16.05.18 at 19:27, <andrew.cooper3@citrix.com> wrote:
>>>> c/s 62b187969 "x86: further CPUID handling adjustments" make some adjustments.
>>>> However, it breaks levelling of guests, making it impossible for the toolstack
>>>> to hide STIBP or IBPB from guests on hardware with up-to-date microcode.
>>>>
>>>> The dom0 issue referenced in the commit message was fixed by the hunk
>>>> adjusting the zeroing alone.  STIBP and IBPB don't need (and indeed, must not
>>>> be for levelling purposes) OR'd into the leaf.
>>>>
>>>> One final item which was missed in backport was the need to ignore the
>>>> toolstack choice of STIBP, and set it equal to IBRSB.  This needs doing after
>>>> the mask has been applied.
>>>>
>>>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
>>> What about the patch below instead? This then allows the tool stack to override
>>> STIBP independent of IBRSB.
>>>
>>> Jan
>>>
>>> x86: correct "further CPUID handling adjustments"
>>>
>>> Commit 62b187969 "x86: further CPUID handling adjustments" went too far,
>>> breaking feature levelling of DomU-s. Restrict the PV overrides to just
>>> Dom0 and undo the HVM overrides.
>> Why?  PV overrides (for dom0 or domU) are equally wrong.
> Why do you talk about DomU? Those overrides are being removed by the
> proposed patch. For Dom0, otoh, there's no other way to do the "IBRSB
> implies both STIBP and IBPB" override we otherwise advertise to the tool
> stack via the PV and HVM feature sets. And I can't see why we wouldn't
> want to treat Dom0 equally in this regard. Even on master I think we only
> do the STIBP part of the above, while the IBPB part is only done for the
> feature sets.
>
> Furthermore, as indicated before, I'm not convinced overriding STIBP
> when the tool stack may have intentionally masked it (as your patch
> does, and as I understand is the effect of
>
>     /*
>      * 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;
>
> in master) is the right thing to do.

I'm not sure how to answer this than simply with "yes it is".  We do not
tolerate the toolstack creating invalid configurations, and seeing STIBP
without IBRSB is invalid.

Until the Xen/toolstack CPUID handling improvements get posted, Xen's
only recourse to bad toolstack configuration is to correct it, and we do
this to any CPUID information we don't like.  In the future, we will
fail a toolstack hypercall which passes bad configuration.

The featureset side of consistency is handled by
guest_common_feature_adjustments()

/*

 * If IBRS is offered to the guest, unconditionally offer STIBP.  It is a

 * nop on non-HT hardware, and has this behaviour to make heterogeneous

 * setups easier to manage.

 */

if ( test_bit(X86_FEATURE_IBRSB, fs) )

    __set_bit(X86_FEATURE_STIBP, fs);


which, in combination with the featureset dependency logic, ensures that
the max featuresets see a consistent view for these features.

The IBRSB implies IBRS corner case is more problematic.  Before the
cpuid_policy changes, dom0 saw a wildly different set of CPUID values,
and even now, still sees an increased set.  I'm not sure its worth
trying to fix on the older branches, because there are much larger holes
with CPUID handling.

~Andrew

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

Re: [PATCH v2 for-4.7/4.8] x86: Fix "x86: further CPUID handling adjustments"

[Jan Beulich]

>>> On 18.05.18 at 15:23, <andrew.cooper3@citrix.com> wrote:
> On 18/05/18 13:37, Jan Beulich wrote:
>>>>> On 18.05.18 at 14:21, <andrew.cooper3@citrix.com> wrote:
>>> On 17/05/18 13:23, Jan Beulich wrote:
>>>>>>> On 16.05.18 at 19:27, <andrew.cooper3@citrix.com> wrote:
>>>>> c/s 62b187969 "x86: further CPUID handling adjustments" make some adjustments.
>>>>> However, it breaks levelling of guests, making it impossible for the toolstack
>>>>> to hide STIBP or IBPB from guests on hardware with up-to-date microcode.
>>>>>
>>>>> The dom0 issue referenced in the commit message was fixed by the hunk
>>>>> adjusting the zeroing alone.  STIBP and IBPB don't need (and indeed, must not
>>>>> be for levelling purposes) OR'd into the leaf.
>>>>>
>>>>> One final item which was missed in backport was the need to ignore the
>>>>> toolstack choice of STIBP, and set it equal to IBRSB.  This needs doing after
>>>>> the mask has been applied.
>>>>>
>>>>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
>>>> What about the patch below instead? This then allows the tool stack to override
>>>> STIBP independent of IBRSB.
>>>>
>>>> Jan
>>>>
>>>> x86: correct "further CPUID handling adjustments"
>>>>
>>>> Commit 62b187969 "x86: further CPUID handling adjustments" went too far,
>>>> breaking feature levelling of DomU-s. Restrict the PV overrides to just
>>>> Dom0 and undo the HVM overrides.
>>> Why?  PV overrides (for dom0 or domU) are equally wrong.
>> Why do you talk about DomU? Those overrides are being removed by the
>> proposed patch. For Dom0, otoh, there's no other way to do the "IBRSB
>> implies both STIBP and IBPB" override we otherwise advertise to the tool
>> stack via the PV and HVM feature sets. And I can't see why we wouldn't
>> want to treat Dom0 equally in this regard. Even on master I think we only
>> do the STIBP part of the above, while the IBPB part is only done for the
>> feature sets.
>>
>> Furthermore, as indicated before, I'm not convinced overriding STIBP
>> when the tool stack may have intentionally masked it (as your patch
>> does, and as I understand is the effect of
>>
>>     /*
>>      * 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;
>>
>> in master) is the right thing to do.
> 
> I'm not sure how to answer this than simply with "yes it is".  We do not
> tolerate the toolstack creating invalid configurations, and seeing STIBP
> without IBRSB is invalid.

But I'm talking about the opposite: The tool stack clearing STIBP but
keeping IBRSB set. The invalid configuration you mention can only
happen if someone enabled STIBP in the guest config, and if there
really was no checking anywhere (after all the feature set reported
to the tool stack is sane, as you explain further down). But how would
that be different from someone enabling e.g. AVX in the guest config
when the underlying hardware doesn't have AVX?

> Until the Xen/toolstack CPUID handling improvements get posted, Xen's
> only recourse to bad toolstack configuration is to correct it, and we do
> this to any CPUID information we don't like.  In the future, we will
> fail a toolstack hypercall which passes bad configuration.
> 
> The featureset side of consistency is handled by
> guest_common_feature_adjustments()
> 
> /*
>  * If IBRS is offered to the guest, unconditionally offer STIBP.  It is a
>  * nop on non-HT hardware, and has this behaviour to make heterogeneous
>  * setups easier to manage.
>  */
> 
> if ( test_bit(X86_FEATURE_IBRSB, fs) )
>     __set_bit(X86_FEATURE_STIBP, fs);
> 
> which, in combination with the featureset dependency logic, ensures that
> the max featuresets see a consistent view for these features.

Exactly.

> The IBRSB implies IBRS corner case is more problematic.  Before the

I assume you mean IBPB instead of IBRS here? Iirc we don't have code
for the separate AMD side IBRS (and their form of STIBP) just yet.

> cpuid_policy changes, dom0 saw a wildly different set of CPUID values,
> and even now, still sees an increased set.  I'm not sure its worth
> trying to fix on the older branches, because there are much larger holes
> with CPUID handling.

We shouldn't make older versions behave artificially inconsistent with
master. We do surface IBPB to Dom0 on Intel hardware with IBRSB there.
Hence unless it's technically impossible we should do so in backports as
well.

I would actually go a step further and say that "cpuid=no-ibrsb" should
still surface IBPB to guests (Xen itself perhaps ought to honor the request
and refrain from issuing barriers, albeit that effect could also be achieved
by "spec-ctrl=no-ibpb").

Jan


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