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

[PATCH v3 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        | 37 +++++++++++++++++++++++++++++++------
 xen/include/asm-x86/xstate.h |  5 +++--
 4 files changed, 36 insertions(+), 10 deletions(-)

-- 
2.1.4


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

[PATCH v3 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).

For migration, this is correct despite the current (mis)ordering of data
because d->arch.cpuid is the applicable max policy.

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().
v3:
 * Note the migration safety in the commit message.

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 4ed24a4..1816faa 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

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

[Jan Beulich]

>>> On 19.07.18 at 13:44, <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).
> 
> For migration, this is correct despite the current (mis)ordering of data
> because d->arch.cpuid is the applicable max policy.
> 
> 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().
> v3:
>  * Note the migration safety in the commit message.
> 
> 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.

While trying to determine the exact boundary here (looks like it's
between 4.7 and 4.8, in which case the remark is relevant only for
people maintaining releases no longer fully XenProject maintained)
I've become confused by the reference to {hvm,pv}_cpuid() above:
Is this simply an ordering concern? If so, the bounding the two
functions do would need to be replicated (or better shared) I think,
if the sole reason for otherwise using the HW maximum is that
d->arch.cpuids[] isn't populated yet.

Jan



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

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

[Andrew Cooper]

On 27/07/18 10:28, Jan Beulich wrote:
>>>> On 19.07.18 at 13:44, <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).
>>
>> For migration, this is correct despite the current (mis)ordering of data
>> because d->arch.cpuid is the applicable max policy.
>>
>> 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().
>> v3:
>>  * Note the migration safety in the commit message.
>>
>> 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.
> While trying to determine the exact boundary here (looks like it's
> between 4.7 and 4.8, in which case the remark is relevant only for
> people maintaining releases no longer fully XenProject maintained)
> I've become confused by the reference to {hvm,pv}_cpuid() above:
> Is this simply an ordering concern? If so, the bounding the two
> functions do would need to be replicated (or better shared) I think,
> if the sole reason for otherwise using the HW maximum is that
> d->arch.cpuids[] isn't populated yet.

Looking over things, 4.9 is fine because that was when cpuid_policy was
introduced.

Before 4.9, the calls to {hvm,pv}_cpuid() are needed to because the
information can't be read directly out of d->arch.cpuids[].  The restore
boolean is needed because this array will be empty at the time it is
accessed on the restore path.

~Andrew

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

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

[Jan Beulich]

>>> On 27.07.18 at 11:37, <andrew.cooper3@citrix.com> wrote:
> On 27/07/18 10:28, Jan Beulich wrote:
>>>>> On 19.07.18 at 13:44, <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).
>>>
>>> For migration, this is correct despite the current (mis)ordering of data
>>> because d->arch.cpuid is the applicable max policy.
>>>
>>> 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().
>>> v3:
>>>  * Note the migration safety in the commit message.
>>>
>>> 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.
>> While trying to determine the exact boundary here (looks like it's
>> between 4.7 and 4.8, in which case the remark is relevant only for
>> people maintaining releases no longer fully XenProject maintained)
>> I've become confused by the reference to {hvm,pv}_cpuid() above:
>> Is this simply an ordering concern? If so, the bounding the two
>> functions do would need to be replicated (or better shared) I think,
>> if the sole reason for otherwise using the HW maximum is that
>> d->arch.cpuids[] isn't populated yet.
> 
> Looking over things, 4.9 is fine because that was when cpuid_policy was
> introduced.
> 
> Before 4.9, the calls to {hvm,pv}_cpuid() are needed to because the
> information can't be read directly out of d->arch.cpuids[].  The restore
> boolean is needed because this array will be empty at the time it is
> accessed on the restore path.

If at restore time we were to not obey to the restrictions {hvm,pv}_cpuid()
enforce, there's imo no point doing any checks at all - the check against
the HW maximum exists already anyway.

Jan



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

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

[Jan Beulich]

>>> On 27.07.18 at 11:37, <andrew.cooper3@citrix.com> wrote:
> Before 4.9, the calls to {hvm,pv}_cpuid() are needed to because the
> information can't be read directly out of d->arch.cpuids[].  The restore
> boolean is needed because this array will be empty at the time it is
> accessed on the restore path.

Would you mind looking over the 4.8 backport below, where I think I've
got away without such a boolean?

Thanks, Jan

--- a/xen/arch/x86/domctl.c
+++ b/xen/arch/x86/domctl.c
@@ -1177,7 +1177,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 )
--- a/xen/arch/x86/hvm/hvm.c
+++ b/xen/arch/x86/hvm/hvm.c
@@ -1356,7 +1356,7 @@ static int hvm_load_cpu_xsave_states(str
     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 )
     {
--- a/xen/arch/x86/xstate.c
+++ b/xen/arch/x86/xstate.c
@@ -652,12 +652,47 @@ static bool_t valid_xcr0(u64 xcr0)
     return !(xcr0 & XSTATE_BNDREGS) == !(xcr0 & XSTATE_BNDCSR);
 }
 
-int validate_xstate(u64 xcr0, u64 xcr0_accum, const struct xsave_hdr *hdr)
+static uint64_t guest_xcr0_max(const struct domain *d)
 {
+    if ( has_hvm_container_domain(d) )
+    {
+        uint32_t eax, ecx = 0, edx;
+
+        hvm_cpuid(XSTATE_CPUID, &eax, NULL, &ecx, &edx);
+
+        return ((uint64_t)edx << 32) | eax;
+    }
+    else
+    {
+        struct cpu_user_regs regs = { };
+
+        regs._eax = XSTATE_CPUID;
+        regs._ecx = 0;
+        pv_cpuid(&regs);
+
+        return (regs.rdx << 32) | regs._eax;
+    }
+}
+
+int validate_xstate(const struct domain *d, uint64_t xcr0, uint64_t xcr0_accum,
+                    const struct xsave_hdr *hdr)
+{
+    uint64_t xcr0_max;
     unsigned int i;
 
+    if ( d == current->domain )
+        xcr0_max = guest_xcr0_max(d);
+    else
+    {
+        xcr0_max = xfeature_mask;
+        if ( !has_hvm_container_domain(d) )
+            xcr0_max &= ~(XSTATE_BNDREGS | XSTATE_BNDCSR |
+                          XSTATE_PKRU | XSTATE_LWP);
+    }
+
     if ( (hdr->xstate_bv & ~xcr0_accum) ||
          (xcr0 & ~xcr0_accum) ||
+         (xcr0_accum & ~xcr0_max) ||
          !valid_xcr0(xcr0) ||
          !valid_xcr0(xcr0_accum) )
         return -EINVAL;
@@ -676,18 +711,16 @@ int validate_xstate(u64 xcr0, u64 xcr0_a
 int handle_xsetbv(u32 index, u64 new_bv)
 {
     struct vcpu *curr = current;
+    uint64_t xcr0_max = guest_xcr0_max(curr->domain);
     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 & XSTATE_PKRU) )
-        return -EOPNOTSUPP;
-
     if ( !set_xcr0(new_bv) )
         return -EFAULT;
 
--- a/xen/include/asm-x86/xstate.h
+++ b/xen/include/asm-x86/xstate.h
@@ -107,8 +107,9 @@ uint64_t get_msr_xss(void);
 void xsave(struct vcpu *v, uint64_t mask);
 void xrstor(struct vcpu *v, uint64_t mask);
 bool_t 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);



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

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

[Andrew Cooper]

If xcr0_max 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.
v3:
 * Check xcr0_max against xfeature_mask, rather than new_bv.
 * Additional comments explaining what is going on.
---
 xen/arch/x86/xstate.c | 26 +++++++++++++++++++++++---
 1 file changed, 23 insertions(+), 3 deletions(-)

diff --git a/xen/arch/x86/xstate.c b/xen/arch/x86/xstate.c
index 1fbb087..c8197d2 100644
--- a/xen/arch/x86/xstate.c
+++ b/xen/arch/x86/xstate.c
@@ -707,12 +707,32 @@ 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) )
+    /*
+     * The CPUID logic shouldn't be able to hand out an XCR0 exceeding Xen's
+     * maximum features, but keep the check for robustness.
+     */
+    if ( unlikely(xcr0_max & ~xfeature_mask) )
+    {
+        gprintk(XENLOG_ERR,
+                "xcr0_max %016" PRIx64 " exceeds hardware max %016" PRIx64 "\n",
+                new_bv, xfeature_mask);
+        domain_crash(curr->domain);
+
+        return -EINVAL;
+    }
+
+    if ( (new_bv & ~xcr0_max) || !valid_xcr0(new_bv) )
         return -EINVAL;
 
-    if ( !set_xcr0(new_bv) )
+    /* By this point, new_bv really should be accepted by hardware. */
+    if ( unlikely(!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 v3 2/2] x86/xstate: Make errors in xstate calculations more obvious by crashing the domain

[Jan Beulich]

>>> On 19.07.18 at 13:44, <andrew.cooper3@citrix.com> wrote:
> --- a/xen/arch/x86/xstate.c
> +++ b/xen/arch/x86/xstate.c
> @@ -707,12 +707,32 @@ 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) )
> +    /*
> +     * The CPUID logic shouldn't be able to hand out an XCR0 exceeding Xen's
> +     * maximum features, but keep the check for robustness.
> +     */
> +    if ( unlikely(xcr0_max & ~xfeature_mask) )
> +    {
> +        gprintk(XENLOG_ERR,
> +                "xcr0_max %016" PRIx64 " exceeds hardware max %016" PRIx64 "\n",
> +                new_bv, xfeature_mask);

I've only noticed this while doing the 4.8 backport: I don't think you've
meant to log new_bv here, rather than xcr0_max.

Jan



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

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

[Andrew Cooper]

On 27/07/18 14:24, Jan Beulich wrote:
>>>> On 19.07.18 at 13:44, <andrew.cooper3@citrix.com> wrote:
>> --- a/xen/arch/x86/xstate.c
>> +++ b/xen/arch/x86/xstate.c
>> @@ -707,12 +707,32 @@ 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) )
>> +    /*
>> +     * The CPUID logic shouldn't be able to hand out an XCR0 exceeding Xen's
>> +     * maximum features, but keep the check for robustness.
>> +     */
>> +    if ( unlikely(xcr0_max & ~xfeature_mask) )
>> +    {
>> +        gprintk(XENLOG_ERR,
>> +                "xcr0_max %016" PRIx64 " exceeds hardware max %016" PRIx64 "\n",
>> +                new_bv, xfeature_mask);
> I've only noticed this while doing the 4.8 backport: I don't think you've
> meant to log new_bv here, rather than xcr0_max.

Oops - you're completely correct.

That is an oversight from rearranging the logic.

~Andrew

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

[PATCH] x86/xstate: correct logging in handle_xsetbv()

[Jan Beulich]

Correct a disagreement between text and logged value.

Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
NB: I've already corrected this in the backports.

--- a/xen/arch/x86/xstate.c
+++ b/xen/arch/x86/xstate.c
@@ -715,7 +715,7 @@ int handle_xsetbv(u32 index, u64 new_bv)
     {
         gprintk(XENLOG_ERR,
                 "xcr0_max %016" PRIx64 " exceeds hardware max %016" PRIx64 "\n",
-                new_bv, xfeature_mask);
+                xcr0_max, xfeature_mask);
         domain_crash(curr->domain);
 
         return -EINVAL;



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

Re: [PATCH] x86/xstate: correct logging in handle_xsetbv()

[Andrew Cooper]

On 31/07/18 11:37, Jan Beulich wrote:
> Correct a disagreement between text and logged value.
>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Acked-by: Andrew Cooper <andrew.cooper3@citrix.com>

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

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

[Jan Beulich]

>>> Andrew Cooper <andrew.cooper3@citrix.com> 07/19/18 1:46 PM >>>
>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

 Reviewed-by: Jan Beulich <jbeulich@suse.com>


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