Re: [PATCH 13/13] x86/domctl: Implement XEN_DOMCTL_set_cpumsr_policy

["Jan Beulich" <JBeulich@suse.com>]

>>> On 03.07.18 at 22:55, <andrew.cooper3@citrix.com> wrote:
> From: Sergey Dyasli <sergey.dyasli@citrix.com>
> 
> This hypercall allows the toolstack to present one combined CPUID and MSR
> policy for a domain, which can be audited in one go by Xen, which is necessary
> for correctness of the auditing.
> 
> A stub x86_policies_are_compatible() function is introduced, although at
> present it will always fail the hypercall.
> 
> The hypercall ABI allows for update of individual CPUID or MSR entries, so
> begins by duplicating the existing policy (for which a helper is introduced),
> merging the toolstack data, then checking compatibility of the result.

This reads to me as if it was fine for the tool stack to supply only partial
data (or else there would be no need to merge anything). What's the
thinking behind this, rather than requiring complete sets of data to be
supplied?

> One awkard corner case is re-deserialising of the vcpu msrs.  The correct fix
> would be to allocate a buffer, copy the MSRs list, then deserialise from that,
> but trips the bounds checks in the copy_from_guest() helpers.  The compat XLAT
> are would work, but would require that we allocate it even for 64bit PV
> guests.



> --- a/xen/arch/x86/domctl.c
> +++ b/xen/arch/x86/domctl.c
> @@ -330,6 +330,71 @@ static int update_domain_cpuid_info(struct domain *d,
>      return 0;
>  }
>  
> +static int update_domain_cpumsr_policy(struct domain *d,
> +                                       xen_domctl_cpumsr_policy_t *xdpc)
> +{
> +    struct policy_group new = {};
> +    const struct policy_group *sys = is_pv_domain(d)
> +        ? &system_policies[XEN_SYSCTL_cpumsr_policy_pv_max]
> +        : &system_policies[XEN_SYSCTL_cpumsr_policy_hvm_max];
> +    struct vcpu *v = d->vcpu[0];
> +    int ret = -ENOMEM;
> +
> +    /* Initialise some help identifying auditing errors. */
> +    xdpc->err_leaf = xdpc->err_subleaf = XEN_CPUID_NO_SUBLEAF;
> +    xdpc->err_msr_idx = ~0;

I'm having trouble extracting information from the comment.

> +    /* Start with existing domain's policies */
> +    if ( !(new.cp = xmemdup(d->arch.cpuid)) ||
> +         !(new.dp = xmemdup(d->arch.msr)) ||
> +         !(new.vp = xmemdup(v->arch.msr)) )
> +        goto out;
> +
> +    /* Merge the toolstack provided data. */
> +    if ( (ret = x86_cpuid_copy_from_buffer(
> +              new.cp, xdpc->cpuid_policy, xdpc->nr_leaves,
> +              &xdpc->err_leaf, &xdpc->err_subleaf)) )
> +        goto out;
> +
> +    if ( (ret = x86_msr_copy_from_buffer(
> +              new.dp, new.vp,
> +              xdpc->msr_policy, xdpc->nr_msrs, &xdpc->err_msr_idx)) )
> +        goto out;
> +
> +    /* Audit the combined dataset. */
> +    ret = x86_policies_are_compatible(sys, &new);
> +    if ( ret )
> +        goto out;

I'm afraid I don't follow - where's the merging? All you do is copy the
first so many entries coming from libxc, and using the later so many
entries from the previous policies. How's that going to provide a
complete set, rather than e.g. some duplicate entries and some
missing ones?

> +    /*
> +     * Audit was successful.  Replace existing policies, leaving the old
> +     * policies to be freed.
> +     */
> +    SWAP(new.cp, d->arch.cpuid);
> +    SWAP(new.dp, d->arch.msr);
> +    SWAP(new.vp, v->arch.msr);
> +
> +    /* Merge the (now audited) vCPU MSRs into every other msr_vcpu_policy. */
> +    for ( ; v; v = v->next_in_list )

This open-coded almost-for_each_domain() doesn't look very nice.

> +    {
> +        /* XXX - Figure out how to avoid a TOCTOU race here.  XLAT area? */
> +        if ( (ret = x86_msr_copy_from_buffer(
> +                  NULL, v->arch.msr, xdpc->msr_policy, xdpc->nr_msrs, NULL)) )

Why can't you go from vCPU 0's v->arch.msr here, which is the copied-in
(and sanitized) representation already? Also, is it really a good idea to
assume all vCPU-s have the same policies?

> @@ -1570,6 +1635,28 @@ long arch_do_domctl(
>          domain_unpause(d);
>          break;
>  
> +    case XEN_DOMCTL_set_cpumsr_policy:
> +        if ( d == currd ||       /* no domain_pause() */
> +             d->max_vcpus == 0 ) /* No vcpus yet. */
> +        {
> +            ret = -EINVAL;
> +            break;
> +        }
> +
> +        domain_pause(d);
> +
> +        if ( d->creation_finished )
> +            ret = -EEXIST; /* No changing once the domain is running. */
> +        else
> +        {
> +            ret = update_domain_cpumsr_policy(d, &domctl->u.cpumsr_policy);
> +            if ( !ret ) /* Copy domctl->u.cpumsr_policy.err_* to guest. */
> +                copyback = true;

x86_cpuid_copy_from_buffer(), for example, sets the err_ fields
only when returning -ERANGE. Is the if() condition inverted?

> --- a/xen/include/public/domctl.h
> +++ b/xen/include/public/domctl.h
> @@ -648,6 +648,12 @@ struct xen_domctl_cpumsr_policy {
>                           * 'msr_domain_policy' */
>      XEN_GUEST_HANDLE_64(xen_cpuid_leaf_t) cpuid_policy; /* IN/OUT: */
>      XEN_GUEST_HANDLE_64(xen_msr_entry_t) msr_policy;    /* IN/OUT: */
> +    uint32_t err_leaf, err_subleaf; /* OUT, set_policy only.  If not ~0,
> +                                     * indicates the leaf/subleaf which
> +                                     * auditing objected to. */
> +    uint32_t err_msr_idx;           /* OUT, set_policy only.  If not ~0,
> +                                     * indicates the MSR idx which
> +                                     * auditing objected to. */

Explicit padding again please, with the handler checking it to be
zero.

> --- a/xen/include/xen/xmalloc.h
> +++ b/xen/include/xen/xmalloc.h
> @@ -13,6 +13,13 @@
>  #define xmalloc(_type) ((_type *)_xmalloc(sizeof(_type), __alignof__(_type)))
>  #define xzalloc(_type) ((_type *)_xzalloc(sizeof(_type), __alignof__(_type)))
>  
> +/* Allocate space for a typed object and copy an existing instance. */
> +#define xmemdup(ptr)                                    \
> +    ({  typeof(*ptr) *n_ = xmalloc(typeof(*ptr));       \
> +        if ( n_ )                                       \
> +            memcpy(n_, ptr, sizeof(*ptr));              \
> +        n_; })

Would be nice if this could handle input pointers to const-qualified types.
I vaguely recall having seen a solution to this recently, but I don't recall
where that was or how it looked like. Until then, may I suggest to use
void * instead, despite this opening the risk of type incompatibilities?

Jan


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