[PATCH v2 0/2] x86/xstate: Fixes and improvements to xsetbv handling

[PATCH v2 0/2] x86/xstate: Fixes and improvements to xsetbv handling

[Andrew Cooper]

Andrew Cooper (2):
  x86/xstate: Use a guests CPUID policy, rather than allowing all features
  x86/xstate: Make errors in xstate calculations more obvious by crashing the domain

 xen/arch/x86/domctl.c        |  2 +-
 xen/arch/x86/hvm/hvm.c       |  2 +-
 xen/arch/x86/xstate.c        | 30 +++++++++++++++++++++++++-----
 xen/include/asm-x86/xstate.h |  5 +++--
 4 files changed, 30 insertions(+), 9 deletions(-)

-- 
2.1.4


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

[PATCH v2 1/2] x86/xstate: Use a guests CPUID policy, rather than allowing all features

[Andrew Cooper]

It turns out that Xen has never enforced that a domain remain within the
xstate features advertised in CPUID.

The check of new_bv against xfeature_mask ensures that a domain stays within
the set of features that Xen has enabled in hardware (and therefore isn't a
security problem), but this does means that attempts to level a guest for
migration safety might not be effective if the guest ignores CPUID.

Check the CPUID policy in validate_xstate() (for incoming migration) and in
handle_xsetbv() (for guest XSETBV instructions).  This subsumes the PKRU check
for PV guests in handle_xsetbv() (and also demonstrates that I should have
spotted this problem while reviewing c/s fbf9971241f).

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

v2:
 * Leave valid_xcr0() alone and duplicate the checks in validate_xstate() and
   handle_xsetbv().

Backporting notes: This is safe in the restore case, but only back as far as
the introduction of cpuid_policy infrastructure.  Before then, a restore
boolean needs to be plumbed down as well, and used to select between the
hardware maximum value and calls to {hvm,pv}_cpuid() to find the
toolstack-chosen level.
---
 xen/arch/x86/domctl.c        |  2 +-
 xen/arch/x86/hvm/hvm.c       |  2 +-
 xen/arch/x86/xstate.c        | 17 +++++++++++------
 xen/include/asm-x86/xstate.h |  5 +++--
 4 files changed, 16 insertions(+), 10 deletions(-)

diff --git a/xen/arch/x86/domctl.c b/xen/arch/x86/domctl.c
index b973629..0423a0c 100644
--- a/xen/arch/x86/domctl.c
+++ b/xen/arch/x86/domctl.c
@@ -1170,7 +1170,7 @@ long arch_do_domctl(
             if ( _xcr0_accum )
             {
                 if ( evc->size >= PV_XSAVE_HDR_SIZE + XSTATE_AREA_MIN_SIZE )
-                    ret = validate_xstate(_xcr0, _xcr0_accum,
+                    ret = validate_xstate(d, _xcr0, _xcr0_accum,
                                           &_xsave_area->xsave_hdr);
             }
             else if ( !_xcr0 )
diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
index f9408e1..d57a942 100644
--- a/xen/arch/x86/hvm/hvm.c
+++ b/xen/arch/x86/hvm/hvm.c
@@ -1254,7 +1254,7 @@ static int hvm_load_cpu_xsave_states(struct domain *d, hvm_domain_context_t *h)
     ctxt = (struct hvm_hw_cpu_xsave *)&h->data[h->cur];
     h->cur += desc->length;
 
-    err = validate_xstate(ctxt->xcr0, ctxt->xcr0_accum,
+    err = validate_xstate(d, ctxt->xcr0, ctxt->xcr0_accum,
                           (const void *)&ctxt->save_area.xsave_hdr);
     if ( err )
     {
diff --git a/xen/arch/x86/xstate.c b/xen/arch/x86/xstate.c
index b4aea4b..1fbb087 100644
--- a/xen/arch/x86/xstate.c
+++ b/xen/arch/x86/xstate.c
@@ -670,12 +670,17 @@ static bool valid_xcr0(u64 xcr0)
     return !(xcr0 & X86_XCR0_BNDREGS) == !(xcr0 & X86_XCR0_BNDCSR);
 }
 
-int validate_xstate(u64 xcr0, u64 xcr0_accum, const struct xsave_hdr *hdr)
+int validate_xstate(const struct domain *d, uint64_t xcr0, uint64_t xcr0_accum,
+                    const struct xsave_hdr *hdr)
 {
+    const struct cpuid_policy *cp = d->arch.cpuid;
+    uint64_t xcr0_max =
+        ((uint64_t)cp->xstate.xcr0_high << 32) | cp->xstate.xcr0_low;
     unsigned int i;
 
     if ( (hdr->xstate_bv & ~xcr0_accum) ||
          (xcr0 & ~xcr0_accum) ||
+         (xcr0_accum & ~xcr0_max) ||
          !valid_xcr0(xcr0) ||
          !valid_xcr0(xcr0_accum) )
         return -EINVAL;
@@ -694,18 +699,18 @@ int validate_xstate(u64 xcr0, u64 xcr0_accum, const struct xsave_hdr *hdr)
 int handle_xsetbv(u32 index, u64 new_bv)
 {
     struct vcpu *curr = current;
+    const struct cpuid_policy *cp = curr->domain->arch.cpuid;
+    uint64_t xcr0_max =
+        ((uint64_t)cp->xstate.xcr0_high << 32) | cp->xstate.xcr0_low;
     u64 mask;
 
     if ( index != XCR_XFEATURE_ENABLED_MASK )
         return -EOPNOTSUPP;
 
-    if ( (new_bv & ~xfeature_mask) || !valid_xcr0(new_bv) )
+    if ( (new_bv & ~xcr0_max) ||
+         (new_bv & ~xfeature_mask) || !valid_xcr0(new_bv) )
         return -EINVAL;
 
-    /* XCR0.PKRU is disabled on PV mode. */
-    if ( is_pv_vcpu(curr) && (new_bv & X86_XCR0_PKRU) )
-        return -EOPNOTSUPP;
-
     if ( !set_xcr0(new_bv) )
         return -EFAULT;
 
diff --git a/xen/include/asm-x86/xstate.h b/xen/include/asm-x86/xstate.h
index 86a4a1f..47f602b 100644
--- a/xen/include/asm-x86/xstate.h
+++ b/xen/include/asm-x86/xstate.h
@@ -97,8 +97,9 @@ void xsave(struct vcpu *v, uint64_t mask);
 void xrstor(struct vcpu *v, uint64_t mask);
 void xstate_set_init(uint64_t mask);
 bool xsave_enabled(const struct vcpu *v);
-int __must_check validate_xstate(u64 xcr0, u64 xcr0_accum,
-                                 const struct xsave_hdr *);
+int __must_check validate_xstate(const struct domain *d,
+                                 uint64_t xcr0, uint64_t xcr0_accum,
+                                 const struct xsave_hdr *hdr);
 int __must_check handle_xsetbv(u32 index, u64 new_bv);
 void expand_xsave_states(struct vcpu *v, void *dest, unsigned int size);
 void compress_xsave_states(struct vcpu *v, const void *src, unsigned int size);
-- 
2.1.4


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

[PATCH v2 2/2] x86/xstate: Make errors in xstate calculations more obvious by crashing the domain

[Andrew Cooper]

If new_bv which exceeds xfeature_mask, then something is broken with the CPUID
policy derivation or auditing logic.  If hardware rejects new_bv, then
something is broken with Xen's xstate logic.

In both cases, crash the domain with an obvious error message, to help
highlight the issues.

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

v2:
 * Rebase over changes to patch 1.
---
 xen/arch/x86/xstate.c | 19 +++++++++++++++++--
 1 file changed, 17 insertions(+), 2 deletions(-)

diff --git a/xen/arch/x86/xstate.c b/xen/arch/x86/xstate.c
index 1fbb087..fe25624 100644
--- a/xen/arch/x86/xstate.c
+++ b/xen/arch/x86/xstate.c
@@ -707,12 +707,27 @@ int handle_xsetbv(u32 index, u64 new_bv)
     if ( index != XCR_XFEATURE_ENABLED_MASK )
         return -EOPNOTSUPP;
 
-    if ( (new_bv & ~xcr0_max) ||
-         (new_bv & ~xfeature_mask) || !valid_xcr0(new_bv) )
+    if ( (new_bv & ~xcr0_max) || !valid_xcr0(new_bv) )
         return -EINVAL;
 
+    if ( new_bv & ~xfeature_mask )
+    {
+        gprintk(XENLOG_ERR,
+                "new_bv %016" PRIx64 " exceeds hardware max %016" PRIx64 "\n",
+                new_bv, xfeature_mask);
+        domain_crash(curr->domain);
+
+        return -EINVAL;
+    }
+
     if ( !set_xcr0(new_bv) )
+    {
+        gprintk(XENLOG_ERR, "new_bv %016" PRIx64 " rejected by hardware\n",
+                new_bv);
+        domain_crash(curr->domain);
+
         return -EFAULT;
+    }
 
     mask = new_bv & ~curr->arch.xcr0_accum;
     curr->arch.xcr0 = new_bv;
-- 
2.1.4


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

Re: [PATCH v2 1/2] x86/xstate: Use a guests CPUID policy, rather than allowing all features

[Jan Beulich]

>>> On 18.07.18 at 19:20, <andrew.cooper3@citrix.com> wrote:
> It turns out that Xen has never enforced that a domain remain within the
> xstate features advertised in CPUID.
> 
> The check of new_bv against xfeature_mask ensures that a domain stays within
> the set of features that Xen has enabled in hardware (and therefore isn't a
> security problem), but this does means that attempts to level a guest for
> migration safety might not be effective if the guest ignores CPUID.
> 
> Check the CPUID policy in validate_xstate() (for incoming migration) and in
> handle_xsetbv() (for guest XSETBV instructions).  This subsumes the PKRU check
> for PV guests in handle_xsetbv() (and also demonstrates that I should have
> spotted this problem while reviewing c/s fbf9971241f).
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

Would be nice if you mentioned the ordering / max vs default policy
aspect here, as this remains as a latent issue for now.

> --- a/xen/arch/x86/hvm/hvm.c
> +++ b/xen/arch/x86/hvm/hvm.c
> @@ -1254,7 +1254,7 @@ static int hvm_load_cpu_xsave_states(struct domain *d, hvm_domain_context_t *h)
>      ctxt = (struct hvm_hw_cpu_xsave *)&h->data[h->cur];
>      h->cur += desc->length;
>  
> -    err = validate_xstate(ctxt->xcr0, ctxt->xcr0_accum,
> +    err = validate_xstate(d, ctxt->xcr0, ctxt->xcr0_accum,
>                            (const void *)&ctxt->save_area.xsave_hdr);

Considering the arch_do_domctl() invocation gets away without the cast,
could I talk you into dropping the cast here while touching this anyway?

> --- a/xen/arch/x86/xstate.c
> +++ b/xen/arch/x86/xstate.c
> @@ -670,12 +670,17 @@ static bool valid_xcr0(u64 xcr0)
>      return !(xcr0 & X86_XCR0_BNDREGS) == !(xcr0 & X86_XCR0_BNDCSR);
>  }
>  
> -int validate_xstate(u64 xcr0, u64 xcr0_accum, const struct xsave_hdr *hdr)
> +int validate_xstate(const struct domain *d, uint64_t xcr0, uint64_t xcr0_accum,
> +                    const struct xsave_hdr *hdr)
>  {
> +    const struct cpuid_policy *cp = d->arch.cpuid;
> +    uint64_t xcr0_max =
> +        ((uint64_t)cp->xstate.xcr0_high << 32) | cp->xstate.xcr0_low;
>      unsigned int i;
>  
>      if ( (hdr->xstate_bv & ~xcr0_accum) ||
>           (xcr0 & ~xcr0_accum) ||
> +         (xcr0_accum & ~xcr0_max) ||
>           !valid_xcr0(xcr0) ||
>           !valid_xcr0(xcr0_accum) )
>          return -EINVAL;

Quite a bit easier to follow than v1, thanks.

> @@ -694,18 +699,18 @@ int validate_xstate(u64 xcr0, u64 xcr0_accum, const struct xsave_hdr *hdr)
>  int handle_xsetbv(u32 index, u64 new_bv)
>  {
>      struct vcpu *curr = current;
> +    const struct cpuid_policy *cp = curr->domain->arch.cpuid;
> +    uint64_t xcr0_max =
> +        ((uint64_t)cp->xstate.xcr0_high << 32) | cp->xstate.xcr0_low;
>      u64 mask;
>  
>      if ( index != XCR_XFEATURE_ENABLED_MASK )
>          return -EOPNOTSUPP;
>  
> -    if ( (new_bv & ~xfeature_mask) || !valid_xcr0(new_bv) )
> +    if ( (new_bv & ~xcr0_max) ||
> +         (new_bv & ~xfeature_mask) || !valid_xcr0(new_bv) )
>          return -EINVAL;

xcr0_max ought to have no bits set which aren't set in xfeature_mask.
Therefore I'd like to suggest

    ASSERT(!(xcr0_max & ~xfeature_mask));
    if ( (new_bv & ~xcr0_max) || !valid_xcr0(new_bv) )
         return -EINVAL;

If you agree, then with the change feel free to add
Reviewed-by: Jan Beulich <jbeulich@suse.com>

Jan



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

Re: [PATCH v2 2/2] x86/xstate: Make errors in xstate calculations more obvious by crashing the domain

[Jan Beulich]

>>> On 18.07.18 at 19:20, <andrew.cooper3@citrix.com> wrote:
> --- a/xen/arch/x86/xstate.c
> +++ b/xen/arch/x86/xstate.c
> @@ -707,12 +707,27 @@ int handle_xsetbv(u32 index, u64 new_bv)
>      if ( index != XCR_XFEATURE_ENABLED_MASK )
>          return -EOPNOTSUPP;
>  
> -    if ( (new_bv & ~xcr0_max) ||
> -         (new_bv & ~xfeature_mask) || !valid_xcr0(new_bv) )
> +    if ( (new_bv & ~xcr0_max) || !valid_xcr0(new_bv) )
>          return -EINVAL;
>  
> +    if ( new_bv & ~xfeature_mask )
> +    {
> +        gprintk(XENLOG_ERR,
> +                "new_bv %016" PRIx64 " exceeds hardware max %016" PRIx64 "\n",
> +                new_bv, xfeature_mask);
> +        domain_crash(curr->domain);
> +
> +        return -EINVAL;
> +    }
> +

The suggested change to patch 1 will render this addition pointless.

>      if ( !set_xcr0(new_bv) )
> +    {
> +        gprintk(XENLOG_ERR, "new_bv %016" PRIx64 " rejected by hardware\n",
> +                new_bv);
> +        domain_crash(curr->domain);
> +
>          return -EFAULT;
> +    }
>  
>      mask = new_bv & ~curr->arch.xcr0_accum;
>      curr->arch.xcr0 = new_bv;

With just this part left
Acked-by: Jan Beulich <jbeulich@suse.com>

Jan



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

Re: [PATCH v2 1/2] x86/xstate: Use a guests CPUID policy, rather than allowing all features

[Andrew Cooper]

On 19/07/2018 08:10, Jan Beulich wrote:
>
>> @@ -694,18 +699,18 @@ int validate_xstate(u64 xcr0, u64 xcr0_accum, const struct xsave_hdr *hdr)
>>  int handle_xsetbv(u32 index, u64 new_bv)
>>  {
>>      struct vcpu *curr = current;
>> +    const struct cpuid_policy *cp = curr->domain->arch.cpuid;
>> +    uint64_t xcr0_max =
>> +        ((uint64_t)cp->xstate.xcr0_high << 32) | cp->xstate.xcr0_low;
>>      u64 mask;
>>  
>>      if ( index != XCR_XFEATURE_ENABLED_MASK )
>>          return -EOPNOTSUPP;
>>  
>> -    if ( (new_bv & ~xfeature_mask) || !valid_xcr0(new_bv) )
>> +    if ( (new_bv & ~xcr0_max) ||
>> +         (new_bv & ~xfeature_mask) || !valid_xcr0(new_bv) )
>>          return -EINVAL;
> xcr0_max ought to have no bits set which aren't set in xfeature_mask.
> Therefore I'd like to suggest
>
>     ASSERT(!(xcr0_max & ~xfeature_mask));
>     if ( (new_bv & ~xcr0_max) || !valid_xcr0(new_bv) )
>          return -EINVAL;
>
> If you agree, then with the change feel free to add
> Reviewed-by: Jan Beulich <jbeulich@suse.com>

Yes - we could make that assertion, but I deliberately opted for the
code in patch 2 instead.

If that assertion were to be violated, we'd have a security issue (using
xstate available in hardware, but unknown to Xen) which would go
unnoticed, and at the very best, just be a state leak between vcpus.

I'm open to rearranging the code, but one way or another, the check
should remain in a release build for robustness.  Alternatively, we
could perhaps and xfeature_mask with new_bv which would retain the
safety, but be far less obvious in the failure case on release builds,
most likely resulting in a guest userspace crash.

~Andrew

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

Re: [PATCH v2 1/2] x86/xstate: Use a guests CPUID policy, rather than allowing all features

[Jan Beulich]

>>> On 19.07.18 at 09:59, <andrew.cooper3@citrix.com> wrote:
> On 19/07/2018 08:10, Jan Beulich wrote:
>>
>>> @@ -694,18 +699,18 @@ int validate_xstate(u64 xcr0, u64 xcr0_accum, const struct xsave_hdr *hdr)
>>>  int handle_xsetbv(u32 index, u64 new_bv)
>>>  {
>>>      struct vcpu *curr = current;
>>> +    const struct cpuid_policy *cp = curr->domain->arch.cpuid;
>>> +    uint64_t xcr0_max =
>>> +        ((uint64_t)cp->xstate.xcr0_high << 32) | cp->xstate.xcr0_low;
>>>      u64 mask;
>>>  
>>>      if ( index != XCR_XFEATURE_ENABLED_MASK )
>>>          return -EOPNOTSUPP;
>>>  
>>> -    if ( (new_bv & ~xfeature_mask) || !valid_xcr0(new_bv) )
>>> +    if ( (new_bv & ~xcr0_max) ||
>>> +         (new_bv & ~xfeature_mask) || !valid_xcr0(new_bv) )
>>>          return -EINVAL;
>> xcr0_max ought to have no bits set which aren't set in xfeature_mask.
>> Therefore I'd like to suggest
>>
>>     ASSERT(!(xcr0_max & ~xfeature_mask));
>>     if ( (new_bv & ~xcr0_max) || !valid_xcr0(new_bv) )
>>          return -EINVAL;
>>
>> If you agree, then with the change feel free to add
>> Reviewed-by: Jan Beulich <jbeulich@suse.com>
> 
> Yes - we could make that assertion, but I deliberately opted for the
> code in patch 2 instead.
> 
> If that assertion were to be violated, we'd have a security issue (using
> xstate available in hardware, but unknown to Xen) which would go
> unnoticed, and at the very best, just be a state leak between vcpus.
> 
> I'm open to rearranging the code, but one way or another, the check
> should remain in a release build for robustness.

Well, okay, how about you do as suggested above in this patch, and
then replace / amend the ASSERT() in the next one?

Jan



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

Re: [PATCH v2 1/2] x86/xstate: Use a guests CPUID policy, rather than allowing all features

[Andrew Cooper]

On 19/07/2018 09:21, Jan Beulich wrote:
>>>> On 19.07.18 at 09:59, <andrew.cooper3@citrix.com> wrote:
>> On 19/07/2018 08:10, Jan Beulich wrote:
>>>> @@ -694,18 +699,18 @@ int validate_xstate(u64 xcr0, u64 xcr0_accum, const struct xsave_hdr *hdr)
>>>>  int handle_xsetbv(u32 index, u64 new_bv)
>>>>  {
>>>>      struct vcpu *curr = current;
>>>> +    const struct cpuid_policy *cp = curr->domain->arch.cpuid;
>>>> +    uint64_t xcr0_max =
>>>> +        ((uint64_t)cp->xstate.xcr0_high << 32) | cp->xstate.xcr0_low;
>>>>      u64 mask;
>>>>  
>>>>      if ( index != XCR_XFEATURE_ENABLED_MASK )
>>>>          return -EOPNOTSUPP;
>>>>  
>>>> -    if ( (new_bv & ~xfeature_mask) || !valid_xcr0(new_bv) )
>>>> +    if ( (new_bv & ~xcr0_max) ||
>>>> +         (new_bv & ~xfeature_mask) || !valid_xcr0(new_bv) )
>>>>          return -EINVAL;
>>> xcr0_max ought to have no bits set which aren't set in xfeature_mask.
>>> Therefore I'd like to suggest
>>>
>>>     ASSERT(!(xcr0_max & ~xfeature_mask));
>>>     if ( (new_bv & ~xcr0_max) || !valid_xcr0(new_bv) )
>>>          return -EINVAL;
>>>
>>> If you agree, then with the change feel free to add
>>> Reviewed-by: Jan Beulich <jbeulich@suse.com>
>> Yes - we could make that assertion, but I deliberately opted for the
>> code in patch 2 instead.
>>
>> If that assertion were to be violated, we'd have a security issue (using
>> xstate available in hardware, but unknown to Xen) which would go
>> unnoticed, and at the very best, just be a state leak between vcpus.
>>
>> I'm open to rearranging the code, but one way or another, the check
>> should remain in a release build for robustness.
> Well, okay, how about you do as suggested above in this patch, and
> then replace / amend the ASSERT() in the next one?

Why?  From a bisection point of view that's strictly worse that the
order of changes presented here, even if it is a condition we don't
expect to hit, and its unnecessary work as the end result is still going
to remain the same.

~Andrew

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

Re: [PATCH v2 1/2] x86/xstate: Use a guests CPUID policy, rather than allowing all features

[Jan Beulich]

>>> On 19.07.18 at 10:26, <andrew.cooper3@citrix.com> wrote:
> On 19/07/2018 09:21, Jan Beulich wrote:
>>>>> On 19.07.18 at 09:59, <andrew.cooper3@citrix.com> wrote:
>>> On 19/07/2018 08:10, Jan Beulich wrote:
>>>>> @@ -694,18 +699,18 @@ int validate_xstate(u64 xcr0, u64 xcr0_accum, const 
> struct xsave_hdr *hdr)
>>>>>  int handle_xsetbv(u32 index, u64 new_bv)
>>>>>  {
>>>>>      struct vcpu *curr = current;
>>>>> +    const struct cpuid_policy *cp = curr->domain->arch.cpuid;
>>>>> +    uint64_t xcr0_max =
>>>>> +        ((uint64_t)cp->xstate.xcr0_high << 32) | cp->xstate.xcr0_low;
>>>>>      u64 mask;
>>>>>  
>>>>>      if ( index != XCR_XFEATURE_ENABLED_MASK )
>>>>>          return -EOPNOTSUPP;
>>>>>  
>>>>> -    if ( (new_bv & ~xfeature_mask) || !valid_xcr0(new_bv) )
>>>>> +    if ( (new_bv & ~xcr0_max) ||
>>>>> +         (new_bv & ~xfeature_mask) || !valid_xcr0(new_bv) )
>>>>>          return -EINVAL;
>>>> xcr0_max ought to have no bits set which aren't set in xfeature_mask.
>>>> Therefore I'd like to suggest
>>>>
>>>>     ASSERT(!(xcr0_max & ~xfeature_mask));
>>>>     if ( (new_bv & ~xcr0_max) || !valid_xcr0(new_bv) )
>>>>          return -EINVAL;
>>>>
>>>> If you agree, then with the change feel free to add
>>>> Reviewed-by: Jan Beulich <jbeulich@suse.com>
>>> Yes - we could make that assertion, but I deliberately opted for the
>>> code in patch 2 instead.
>>>
>>> If that assertion were to be violated, we'd have a security issue (using
>>> xstate available in hardware, but unknown to Xen) which would go
>>> unnoticed, and at the very best, just be a state leak between vcpus.
>>>
>>> I'm open to rearranging the code, but one way or another, the check
>>> should remain in a release build for robustness.
>> Well, okay, how about you do as suggested above in this patch, and
>> then replace / amend the ASSERT() in the next one?
> 
> Why?  From a bisection point of view that's strictly worse that the
> order of changes presented here, even if it is a condition we don't
> expect to hit, and its unnecessary work as the end result is still going
> to remain the same.

The end result is the same, yes, so it doesn't matter all that much.
But

    if ( (new_bv & ~xcr0_max) ||
         (new_bv & ~xfeature_mask) || !valid_xcr0(new_bv) )

is still not making obvious that there's actually redundancy there
here, while

    ASSERT(!(xcr0_max & ~xfeature_mask));
    if ( (new_bv & ~xcr0_max) || !valid_xcr0(new_bv) )

does. So yes, it's minor enough that you may feel free to ignore
my comments and put in the patches as they are.

Jan



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

Re: [PATCH v2 1/2] x86/xstate: Use a guests CPUID policy, rather than allowing all features

[Andrew Cooper]

On 19/07/2018 09:34, Jan Beulich wrote:
>>>> On 19.07.18 at 10:26, <andrew.cooper3@citrix.com> wrote:
>> On 19/07/2018 09:21, Jan Beulich wrote:
>>>>>> On 19.07.18 at 09:59, <andrew.cooper3@citrix.com> wrote:
>>>> On 19/07/2018 08:10, Jan Beulich wrote:
>>>>>> @@ -694,18 +699,18 @@ int validate_xstate(u64 xcr0, u64 xcr0_accum, const 
>> struct xsave_hdr *hdr)
>>>>>>  int handle_xsetbv(u32 index, u64 new_bv)
>>>>>>  {
>>>>>>      struct vcpu *curr = current;
>>>>>> +    const struct cpuid_policy *cp = curr->domain->arch.cpuid;
>>>>>> +    uint64_t xcr0_max =
>>>>>> +        ((uint64_t)cp->xstate.xcr0_high << 32) | cp->xstate.xcr0_low;
>>>>>>      u64 mask;
>>>>>>  
>>>>>>      if ( index != XCR_XFEATURE_ENABLED_MASK )
>>>>>>          return -EOPNOTSUPP;
>>>>>>  
>>>>>> -    if ( (new_bv & ~xfeature_mask) || !valid_xcr0(new_bv) )
>>>>>> +    if ( (new_bv & ~xcr0_max) ||
>>>>>> +         (new_bv & ~xfeature_mask) || !valid_xcr0(new_bv) )
>>>>>>          return -EINVAL;
>>>>> xcr0_max ought to have no bits set which aren't set in xfeature_mask.
>>>>> Therefore I'd like to suggest
>>>>>
>>>>>     ASSERT(!(xcr0_max & ~xfeature_mask));
>>>>>     if ( (new_bv & ~xcr0_max) || !valid_xcr0(new_bv) )
>>>>>          return -EINVAL;
>>>>>
>>>>> If you agree, then with the change feel free to add
>>>>> Reviewed-by: Jan Beulich <jbeulich@suse.com>
>>>> Yes - we could make that assertion, but I deliberately opted for the
>>>> code in patch 2 instead.
>>>>
>>>> If that assertion were to be violated, we'd have a security issue (using
>>>> xstate available in hardware, but unknown to Xen) which would go
>>>> unnoticed, and at the very best, just be a state leak between vcpus.
>>>>
>>>> I'm open to rearranging the code, but one way or another, the check
>>>> should remain in a release build for robustness.
>>> Well, okay, how about you do as suggested above in this patch, and
>>> then replace / amend the ASSERT() in the next one?
>> Why?  From a bisection point of view that's strictly worse that the
>> order of changes presented here, even if it is a condition we don't
>> expect to hit, and its unnecessary work as the end result is still going
>> to remain the same.
> The end result is the same, yes, so it doesn't matter all that much.
> But
>
>     if ( (new_bv & ~xcr0_max) ||
>          (new_bv & ~xfeature_mask) || !valid_xcr0(new_bv) )
>
> is still not making obvious that there's actually redundancy there
> here, while
>
>     ASSERT(!(xcr0_max & ~xfeature_mask));
>     if ( (new_bv & ~xcr0_max) || !valid_xcr0(new_bv) )
>
> does. So yes, it's minor enough that you may feel free to ignore
> my comments and put in the patches as they are.

I can tweak patch 2 to follow this layout rather than the current, and
can leave some comments and an ASSERT_UNREACHABLE() after the printk()
if you'd like?

~Andrew

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

Re: [PATCH v2 1/2] x86/xstate: Use a guests CPUID policy, rather than allowing all features

[Jan Beulich]

>>> On 19.07.18 at 10:59, <andrew.cooper3@citrix.com> wrote:
> On 19/07/2018 09:34, Jan Beulich wrote:
>>>>> On 19.07.18 at 10:26, <andrew.cooper3@citrix.com> wrote:
>>> On 19/07/2018 09:21, Jan Beulich wrote:
>>>>>>> On 19.07.18 at 09:59, <andrew.cooper3@citrix.com> wrote:
>>>>> On 19/07/2018 08:10, Jan Beulich wrote:
>>>>>>> @@ -694,18 +699,18 @@ int validate_xstate(u64 xcr0, u64 xcr0_accum, const 
>>> struct xsave_hdr *hdr)
>>>>>>>  int handle_xsetbv(u32 index, u64 new_bv)
>>>>>>>  {
>>>>>>>      struct vcpu *curr = current;
>>>>>>> +    const struct cpuid_policy *cp = curr->domain->arch.cpuid;
>>>>>>> +    uint64_t xcr0_max =
>>>>>>> +        ((uint64_t)cp->xstate.xcr0_high << 32) | cp->xstate.xcr0_low;
>>>>>>>      u64 mask;
>>>>>>>  
>>>>>>>      if ( index != XCR_XFEATURE_ENABLED_MASK )
>>>>>>>          return -EOPNOTSUPP;
>>>>>>>  
>>>>>>> -    if ( (new_bv & ~xfeature_mask) || !valid_xcr0(new_bv) )
>>>>>>> +    if ( (new_bv & ~xcr0_max) ||
>>>>>>> +         (new_bv & ~xfeature_mask) || !valid_xcr0(new_bv) )
>>>>>>>          return -EINVAL;
>>>>>> xcr0_max ought to have no bits set which aren't set in xfeature_mask.
>>>>>> Therefore I'd like to suggest
>>>>>>
>>>>>>     ASSERT(!(xcr0_max & ~xfeature_mask));
>>>>>>     if ( (new_bv & ~xcr0_max) || !valid_xcr0(new_bv) )
>>>>>>          return -EINVAL;
>>>>>>
>>>>>> If you agree, then with the change feel free to add
>>>>>> Reviewed-by: Jan Beulich <jbeulich@suse.com>
>>>>> Yes - we could make that assertion, but I deliberately opted for the
>>>>> code in patch 2 instead.
>>>>>
>>>>> If that assertion were to be violated, we'd have a security issue (using
>>>>> xstate available in hardware, but unknown to Xen) which would go
>>>>> unnoticed, and at the very best, just be a state leak between vcpus.
>>>>>
>>>>> I'm open to rearranging the code, but one way or another, the check
>>>>> should remain in a release build for robustness.
>>>> Well, okay, how about you do as suggested above in this patch, and
>>>> then replace / amend the ASSERT() in the next one?
>>> Why?  From a bisection point of view that's strictly worse that the
>>> order of changes presented here, even if it is a condition we don't
>>> expect to hit, and its unnecessary work as the end result is still going
>>> to remain the same.
>> The end result is the same, yes, so it doesn't matter all that much.
>> But
>>
>>     if ( (new_bv & ~xcr0_max) ||
>>          (new_bv & ~xfeature_mask) || !valid_xcr0(new_bv) )
>>
>> is still not making obvious that there's actually redundancy there
>> here, while
>>
>>     ASSERT(!(xcr0_max & ~xfeature_mask));
>>     if ( (new_bv & ~xcr0_max) || !valid_xcr0(new_bv) )
>>
>> does. So yes, it's minor enough that you may feel free to ignore
>> my comments and put in the patches as they are.
> 
> I can tweak patch 2 to follow this layout rather than the current, and
> can leave some comments and an ASSERT_UNREACHABLE() after the printk()
> if you'd like?

Layout - perhaps yes. ASSERT_UNREACHABLE() - I don't see a need,
as the domain_crash() ought to be prominent enough.

Jan



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