* [PATCH v2 0/2] x86/xstate: Fixes and improvements to xsetbv handling
@ 2018-07-18 17:20 Andrew Cooper
2018-07-18 17:20 ` [PATCH v2 1/2] x86/xstate: Use a guests CPUID policy, rather than allowing all features Andrew Cooper
2018-07-18 17:20 ` [PATCH v2 2/2] x86/xstate: Make errors in xstate calculations more obvious by crashing the domain Andrew Cooper
0 siblings, 2 replies; 11+ messages in thread
From: Andrew Cooper @ 2018-07-18 17:20 UTC (permalink / raw)
To: Xen-devel; +Cc: 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
^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH v2 1/2] x86/xstate: Use a guests CPUID policy, rather than allowing all features
2018-07-18 17:20 [PATCH v2 0/2] x86/xstate: Fixes and improvements to xsetbv handling Andrew Cooper
@ 2018-07-18 17:20 ` Andrew Cooper
2018-07-19 7:10 ` Jan Beulich
2018-07-18 17:20 ` [PATCH v2 2/2] x86/xstate: Make errors in xstate calculations more obvious by crashing the domain Andrew Cooper
1 sibling, 1 reply; 11+ messages in thread
From: Andrew Cooper @ 2018-07-18 17:20 UTC (permalink / raw)
To: Xen-devel; +Cc: Andrew Cooper, Jan Beulich
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
^ permalink raw reply related [flat|nested] 11+ messages in thread
* [PATCH v2 2/2] x86/xstate: Make errors in xstate calculations more obvious by crashing the domain
2018-07-18 17:20 [PATCH v2 0/2] x86/xstate: Fixes and improvements to xsetbv handling Andrew Cooper
2018-07-18 17:20 ` [PATCH v2 1/2] x86/xstate: Use a guests CPUID policy, rather than allowing all features Andrew Cooper
@ 2018-07-18 17:20 ` Andrew Cooper
2018-07-19 7:13 ` Jan Beulich
1 sibling, 1 reply; 11+ messages in thread
From: Andrew Cooper @ 2018-07-18 17:20 UTC (permalink / raw)
To: Xen-devel; +Cc: Andrew Cooper, Jan Beulich
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
^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH v2 1/2] x86/xstate: Use a guests CPUID policy, rather than allowing all features
2018-07-18 17:20 ` [PATCH v2 1/2] x86/xstate: Use a guests CPUID policy, rather than allowing all features Andrew Cooper
@ 2018-07-19 7:10 ` Jan Beulich
2018-07-19 7:59 ` Andrew Cooper
0 siblings, 1 reply; 11+ messages in thread
From: Jan Beulich @ 2018-07-19 7:10 UTC (permalink / raw)
To: Andrew Cooper; +Cc: Xen-devel
>>> 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
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2 2/2] x86/xstate: Make errors in xstate calculations more obvious by crashing the domain
2018-07-18 17:20 ` [PATCH v2 2/2] x86/xstate: Make errors in xstate calculations more obvious by crashing the domain Andrew Cooper
@ 2018-07-19 7:13 ` Jan Beulich
0 siblings, 0 replies; 11+ messages in thread
From: Jan Beulich @ 2018-07-19 7:13 UTC (permalink / raw)
To: Andrew Cooper; +Cc: Xen-devel
>>> 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
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2 1/2] x86/xstate: Use a guests CPUID policy, rather than allowing all features
2018-07-19 7:10 ` Jan Beulich
@ 2018-07-19 7:59 ` Andrew Cooper
2018-07-19 8:21 ` Jan Beulich
0 siblings, 1 reply; 11+ messages in thread
From: Andrew Cooper @ 2018-07-19 7:59 UTC (permalink / raw)
To: Jan Beulich; +Cc: Xen-devel
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
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2 1/2] x86/xstate: Use a guests CPUID policy, rather than allowing all features
2018-07-19 7:59 ` Andrew Cooper
@ 2018-07-19 8:21 ` Jan Beulich
2018-07-19 8:26 ` Andrew Cooper
0 siblings, 1 reply; 11+ messages in thread
From: Jan Beulich @ 2018-07-19 8:21 UTC (permalink / raw)
To: Andrew Cooper; +Cc: Xen-devel
>>> 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
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2 1/2] x86/xstate: Use a guests CPUID policy, rather than allowing all features
2018-07-19 8:21 ` Jan Beulich
@ 2018-07-19 8:26 ` Andrew Cooper
2018-07-19 8:34 ` Jan Beulich
0 siblings, 1 reply; 11+ messages in thread
From: Andrew Cooper @ 2018-07-19 8:26 UTC (permalink / raw)
To: Jan Beulich; +Cc: Xen-devel
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
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2 1/2] x86/xstate: Use a guests CPUID policy, rather than allowing all features
2018-07-19 8:26 ` Andrew Cooper
@ 2018-07-19 8:34 ` Jan Beulich
2018-07-19 8:59 ` Andrew Cooper
0 siblings, 1 reply; 11+ messages in thread
From: Jan Beulich @ 2018-07-19 8:34 UTC (permalink / raw)
To: Andrew Cooper; +Cc: Xen-devel
>>> 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
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2 1/2] x86/xstate: Use a guests CPUID policy, rather than allowing all features
2018-07-19 8:34 ` Jan Beulich
@ 2018-07-19 8:59 ` Andrew Cooper
2018-07-19 10:04 ` Jan Beulich
0 siblings, 1 reply; 11+ messages in thread
From: Andrew Cooper @ 2018-07-19 8:59 UTC (permalink / raw)
To: Jan Beulich; +Cc: Xen-devel
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
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2 1/2] x86/xstate: Use a guests CPUID policy, rather than allowing all features
2018-07-19 8:59 ` Andrew Cooper
@ 2018-07-19 10:04 ` Jan Beulich
0 siblings, 0 replies; 11+ messages in thread
From: Jan Beulich @ 2018-07-19 10:04 UTC (permalink / raw)
To: Andrew Cooper; +Cc: Xen-devel
>>> 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
^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2018-07-19 10:04 UTC | newest]
Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-07-18 17:20 [PATCH v2 0/2] x86/xstate: Fixes and improvements to xsetbv handling Andrew Cooper
2018-07-18 17:20 ` [PATCH v2 1/2] x86/xstate: Use a guests CPUID policy, rather than allowing all features Andrew Cooper
2018-07-19 7:10 ` Jan Beulich
2018-07-19 7:59 ` Andrew Cooper
2018-07-19 8:21 ` Jan Beulich
2018-07-19 8:26 ` Andrew Cooper
2018-07-19 8:34 ` Jan Beulich
2018-07-19 8:59 ` Andrew Cooper
2018-07-19 10:04 ` Jan Beulich
2018-07-18 17:20 ` [PATCH v2 2/2] x86/xstate: Make errors in xstate calculations more obvious by crashing the domain Andrew Cooper
2018-07-19 7:13 ` Jan Beulich
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.