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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 18.07.18 at 19:20, <andrew.cooper3@xxxxxxxxxx> 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@xxxxxxxxxx>

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@xxxxxxxx>

Jan



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