Re: [RFC PATCH 10/10] Add VMware guest info access

[Matthew Daley <mattd@bugfuzz.com>]

On Fri, Dec 13, 2013 at 8:15 AM, Don Slutz <dslutz@verizon.com> wrote:
> From: Don Slutz <dslutz@verizon.com>
>
> Signed-off-by: Don Slutz <dslutz@verizon.com>
> ---
>  tools/libxc/xc_domain.c         | 112 ++++++++++++++++++++++++++++++
>  tools/libxc/xenctrl.h           |  24 +++++++
>  xen/arch/x86/hvm/hvm.c          | 148 ++++++++++++++++++++++++++++++++++++++++
>  xen/include/public/hvm/hvm_op.h |  18 +++++
>  4 files changed, 302 insertions(+)
>
> diff --git a/tools/libxc/xc_domain.c b/tools/libxc/xc_domain.c
> index 1ccafc5..0437c6f 100644
> --- a/tools/libxc/xc_domain.c
> +++ b/tools/libxc/xc_domain.c
> @@ -1246,6 +1246,118 @@ int xc_get_hvm_param(xc_interface *handle, domid_t dom, int param, unsigned long
>      return rc;
>  }
>
> +int xc_set_vmport_guest_info(xc_interface *handle,
> +                             domid_t dom,
> +                             unsigned int key_len,
> +                             char *key,
> +                             unsigned int val_len,
> +                             char *val)
> +{
> +    DECLARE_HYPERCALL;
> +    DECLARE_HYPERCALL_BUFFER(xen_hvm_vmport_guest_info_t, arg);
> +    int rc;
> +
> +    if ((key_len < 1) ||
> +        (key_len > VMPORT_GUEST_INFO_KEY_MAX) ||
> +        (val_len > VMPORT_GUEST_INFO_VAL_MAX)) {
> +        return -1;
> +    }
> +
> +    arg = xc_hypercall_buffer_alloc(handle, arg, sizeof(*arg));
> +    if ( arg == NULL )
> +        return -1;
> +
> +    hypercall.op     = __HYPERVISOR_hvm_op;
> +    hypercall.arg[0] = HVMOP_set_vmport_guest_info;
> +    hypercall.arg[1] = HYPERCALL_BUFFER_AS_ARG(arg);
> +    arg->domid = dom;
> +    arg->key_length = key_len;
> +    arg->value_length = val_len;
> +    memcpy(arg->data, key, key_len);
> +    memcpy(&arg->data[key_len], val, val_len);
> +    rc = do_xen_hypercall(handle, &hypercall);
> +    xc_hypercall_buffer_free(handle, arg);
> +    return rc;
> +}
> +
> +int xc_get_vmport_guest_info(xc_interface *handle,
> +                             domid_t dom,
> +                             unsigned int key_len,
> +                             char *key,
> +                             unsigned int val_max,
> +                             unsigned int *val_len,
> +                             char *val)
> +{
> +    DECLARE_HYPERCALL;
> +    DECLARE_HYPERCALL_BUFFER(xen_hvm_vmport_guest_info_t, arg);
> +    int rc;
> +
> +    if ((key_len < 1) ||
> +        (key_len > VMPORT_GUEST_INFO_KEY_MAX) )
> +        return -1;
> +
> +    arg = xc_hypercall_buffer_alloc(handle, arg, sizeof(*arg));
> +
> +    hypercall.op     = __HYPERVISOR_hvm_op;
> +    hypercall.arg[0] = HVMOP_get_vmport_guest_info;
> +    hypercall.arg[1] = HYPERCALL_BUFFER_AS_ARG(arg);
> +    arg->domid = dom;
> +    arg->key_length = key_len;
> +    arg->value_length = 0;
> +    *val_len = 0;
> +    memcpy(arg->data, key, key_len);
> +    rc = do_xen_hypercall(handle, &hypercall);
> +    if (rc == 0) {
> +        if (arg->value_length > val_max)
> +            arg->value_length = val_max;
> +        *val_len = arg->value_length;
> +        memcpy(val, &arg->data[key_len], arg->value_length);
> +    }
> +    xc_hypercall_buffer_free(handle, arg);
> +    return rc;
> +}
> +
> +int xc_fetch_vmport_guest_info(xc_interface *handle,
> +                               domid_t dom,
> +                               unsigned int idx,
> +                               unsigned int key_max,
> +                               unsigned int *key_len,
> +                               char *key,
> +                               unsigned int val_max,
> +                               unsigned int *val_len,
> +                               char *val)
> +{
> +    DECLARE_HYPERCALL;
> +    DECLARE_HYPERCALL_BUFFER(xen_hvm_vmport_guest_info_t, arg);
> +    int rc;
> +
> +    arg = xc_hypercall_buffer_alloc(handle, arg, sizeof(*arg));
> +
> +    hypercall.op     = __HYPERVISOR_hvm_op;
> +    hypercall.arg[0] = HVMOP_get_vmport_guest_info;
> +    hypercall.arg[1] = HYPERCALL_BUFFER_AS_ARG(arg);
> +    arg->domid = dom;
> +    arg->key_length = 0;
> +    arg->value_length = idx;
> +    *key_len = 0;
> +    *val_len = 0;
> +    rc = do_xen_hypercall(handle, &hypercall);
> +    if (rc == 0) {
> +        if (arg->key_length > key_max)
> +            arg->key_length = key_max;
> +        *key_len = arg->key_length;
> +        memcpy(key, arg->data, arg->key_length);
> +        if (arg->value_length > val_max)
> +            arg->value_length = val_max;
> +        *val_len = arg->value_length;
> +        memcpy(val,
> +               &arg->data[arg->key_length],
> +               arg->value_length);
> +    }
> +    xc_hypercall_buffer_free(handle, arg);
> +    return rc;
> +}
> +
>  int xc_domain_setdebugging(xc_interface *xch,
>                             uint32_t domid,
>                             unsigned int enable)
> diff --git a/tools/libxc/xenctrl.h b/tools/libxc/xenctrl.h
> index 6e58ebe..6b22b3b 100644
> --- a/tools/libxc/xenctrl.h
> +++ b/tools/libxc/xenctrl.h
> @@ -1774,6 +1774,30 @@ void xc_clear_last_error(xc_interface *xch);
>  int xc_set_hvm_param(xc_interface *handle, domid_t dom, int param, unsigned long value);
>  int xc_get_hvm_param(xc_interface *handle, domid_t dom, int param, unsigned long *value);
>
> +int xc_set_vmport_guest_info(xc_interface *handle,
> +                             domid_t dom,
> +                             unsigned int key_len,
> +                             char *key,
> +                             unsigned int val_len,
> +                             char *val);
> +int xc_get_vmport_guest_info(xc_interface *handle,
> +                             domid_t dom,
> +                             unsigned int key_len,
> +                             char *key,
> +                             unsigned int val_max,
> +                             unsigned int *val_len,
> +                             char *val);
> +int xc_fetch_vmport_guest_info(xc_interface *handle,
> +                               domid_t dom,
> +                               unsigned int idx,
> +                               unsigned int key_max,
> +                               unsigned int *key_len,
> +                               char *key,
> +                               unsigned int val_max,
> +                               unsigned int *val_len,
> +                               char *val);
> +
> +
>  /* HVM guest pass-through */
>  int xc_assign_device(xc_interface *xch,
>                       uint32_t domid,
> diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
> index a557272..c6f84fc 100644
> --- a/xen/arch/x86/hvm/hvm.c
> +++ b/xen/arch/x86/hvm/hvm.c
> @@ -4662,6 +4662,154 @@ long do_hvm_op(unsigned long op, XEN_GUEST_HANDLE_PARAM(void) arg)
>          break;
>      }
>
> +    case HVMOP_get_vmport_guest_info:
> +    case HVMOP_set_vmport_guest_info:
> +    {
> +        struct xen_hvm_vmport_guest_info a;
> +        struct domain *d;
> +        char *key = NULL;
> +        char *value = NULL;
> +        struct vmport_state *vs;
> +        int idx;
> +        vmport_guestinfo_t *add_slots[5];
> +        int num_slots = 0, num_free_slots = 0;
> +
> +        if ( copy_from_guest(&a, arg, 1) )
> +            return -EFAULT;
> +
> +        ASSERT(strlen("guestinfo.") == 10);
> +#if VMPORT_MAX_KEY_LEN + 10 != VMPORT_GUEST_INFO_KEY_MAX
> +#error Need to adjust VMPORT_MAX_KEY_LEN & VMPORT_GUEST_INFO_KEY_MAX
> +#endif
> +#if VMPORT_MAX_VAL_LEN != VMPORT_GUEST_INFO_VAL_MAX
> +#error Need to adjust VMPORT_MAX_VAL_LEN & VMPORT_GUEST_INFO_VAL_MAX
> +#endif

I don't think I quite understand the need for the
VMPORT_MAX_{KEY,VAL}_LEN macros as well as the
VMPORT_GUEST_INFO_{KEY,VAL}_MAX ones. IIUC VMPORT_MAX_{KEY,VAL}_LEN is
used for sizing the guestinfo data buffer on the Xen side, but
VMPORT_GUEST_INFO_{KEY,VAL}_MAX is used for the public subop hypercall
interface.

Is there a need for the "guestinfo." key prefix chopping? This is
being done in this new hypercall subop implemented by Xen, so
shouldn't there be no existing callers expecting this behaviour,
allowing it to be dropped?

> +        if ( a.key_length > strlen("guestinfo.") ) {
> +            if ( (unsigned long)a.key_length + (unsigned long)a.value_length > sizeof(a.data) )

Are the casts necessary? It looks like you're trying to avoid
overflow, but {key,value}_length are uint16's, which will be
automatically promoted to ints.

> +                return -EINVAL;
> +            if ( memcmp(a.data, "guestinfo.", strlen("guestinfo.")) == 0 ) {
> +                key = &a.data[strlen("guestinfo.")];
> +                a.key_length -= strlen("guestinfo.");
> +            } else {
> +                key = &a.data[0];
> +            }
> +            value = key + a.key_length;
> +        } else if (a.key_length > 0) {
> +            if ( (unsigned long)a.key_length + (unsigned long)a.value_length > sizeof(a.data) )
> +                return -EINVAL;
> +            key = &a.data[0];
> +            if ( a.key_length > VMPORT_MAX_KEY_LEN )
> +                return -EINVAL;
> +            if ( a.value_length > VMPORT_MAX_VAL_LEN )
> +                return -EINVAL;

You don't check these two maximum lengths in the a.key_length >
strlen("guestinfo.") case, where one could still provide a large key
and tiny value.

> +            value = key + a.key_length;
> +        } else if ( (a.key_length == 0) && (op == HVMOP_set_vmport_guest_info) ) {

I wouldn't bother checking for a.key_length == 0 explicitly here, it's
implied at this point. In addition, the check on op should be a
whitelist (op != ...), not a blacklist (op == ...).

> +            return -EINVAL;
> +        }

Perhaps these three cases can be shuffled around a bit. By making the
a.key_length > strlen("guestinfo.") check also include the memcmp(),
the two cases then become "key *is* prefixed with guestinfo." and "key
is not prefixed with guestinfo." instead of the current setup, which
has "key *could* be prefixed with guestinfo." and "key is not prefixed
with guestinfo.". This allows factoring out the "is not prefixed with
guestinfo." part into a single case. Before all this, the common
length checks can be done. Something like this:

if ( (op != HVMOP_get_vmport_guest_info && !a.key_length) ||
        a.key_length > VMPORT_MAX_KEY_LEN ||
        a.value_length > VMPORT_MAX_VAL_LEN )
        return -EINVAL;

key = a.data;
if ( a.key_length > strlen("guestinfo.") &&
        memcmp(a.data, "guestinfo.", strlen("guestinfo.") == 0 ) {
        key += strlen("guestinfo.");
        a.key_length -= strlen("guestinfo.");
}

value = key + a.key_length;

> +        d = rcu_lock_domain_by_any_id(a.domid);
> +        if ( d == NULL )
> +            return rc;
> +
> +        rc = -EINVAL;
> +        if ( !is_hvm_domain(d) )
> +            goto param_fail9;
> +
> +        rc = xsm_hvm_param(XSM_TARGET, d, op);
> +        if ( rc )
> +            goto param_fail9;
> +
> +        vs = d->arch.hvm_domain.vmport_data;
> +        if ((a.key_length == 0) && (a.value_length >= vs->used_guestinfo)) {
> +            rc = -E2BIG;
> +            goto param_fail9;
> +        }
> +        for (idx = 0; idx < vs->used_guestinfo; idx++) {
> +            if (vs->guestinfo[idx] &&
> +                (vs->guestinfo[idx]->key_len == 0))
> +                num_free_slots++;
> +        }
> +        if (num_free_slots < 5) {
> +            num_slots = 5 - num_free_slots;
> +            if (vs->used_guestinfo + num_slots > VMPORT_MAX_NUM_KEY)
> +                num_slots = VMPORT_MAX_NUM_KEY - vs->used_guestinfo;
> +            for (idx = 0; idx < num_slots; idx++)
> +                add_slots[idx] = xzalloc(vmport_guestinfo_t);
> +        }

Is it really worth the extra complexity to maintain allocated but
unused guestinfos? Can't they just be allocated on demand and freed
when unneeded? Speaking of which, is there a way to delete (and not
just empty out) a guestinfo? If not, would it be a good thing to have?

> +
> +        spin_lock(&d->arch.hvm_domain.vmport_lock);
> +
> +        for (idx = 0; idx < num_slots; idx++)
> +            vs->guestinfo[vs->used_guestinfo + idx] = add_slots[idx];
> +        vs->used_guestinfo += num_slots;
> +
> +        if ( op == HVMOP_set_vmport_guest_info )
> +        {
> +            int free_idx = -1;
> +
> +            for (idx = 0; idx < vs->used_guestinfo; idx++) {
> +                if (!vs->guestinfo[idx]) {
> +                    gdprintk(XENLOG_WARNING, "idx=%d not allocated, but used_guestinfo=%d\n",
> +                             idx, vs->used_guestinfo);
> +                } else if ((vs->guestinfo[idx]->key_len == a.key_length) &&
> +                           (memcmp(key,
> +                                   vs->guestinfo[idx]->key_data,
> +                                   vs->guestinfo[idx]->key_len) == 0)) {
> +                    vs->guestinfo[idx]->val_len = a.value_length;
> +                    memcpy(vs->guestinfo[idx]->val_data, value, a.value_length);
> +                    break;
> +                } else if ((vs->guestinfo[idx]->key_len == 0) &&
> +                           (free_idx == -1)) {
> +                    free_idx = idx;
> +                }
> +            }
> +            if (idx >= vs->used_guestinfo) {
> +                if (free_idx == -1) {
> +                    rc = -EBUSY;
> +                } else {
> +                    vs->guestinfo[free_idx]->key_len = a.key_length;
> +                    memcpy(vs->guestinfo[free_idx]->key_data, key, a.key_length);
> +                    vs->guestinfo[free_idx]->val_len = a.value_length;
> +                    memcpy(vs->guestinfo[free_idx]->val_data, value, a.value_length);
> +                }
> +            }
> +        }
> +        else
> +        {
> +            if (a.key_length == 0) {

The use of key_length being 0 in a "get" operation confused me here
(for example, I thought this was open to bad indexing if value_length
was out-of-range, but I see that you check it back up where you return
E2BIG).

Really, there are three subops being added here; one set and two gets
(looking up by key or index). Hence I think it might be a good idea to
explicitly define a third subop, say
HVMOP_get_by_idx_vmport_guest_info. You could then use a union for the
{get,set} subops which have differing arguments to the new get_by_idx
subop.

> +                idx = a.value_length;
> +                a.key_length = vs->guestinfo[idx]->key_len;
> +                memcpy(a.data, vs->guestinfo[idx]->key_data, a.key_length);
> +                a.value_length = vs->guestinfo[idx]->val_len;
> +                memcpy(&a.data[a.key_length],
> +                       vs->guestinfo[idx]->val_data,
> +                       a.value_length);
> +                rc = copy_to_guest(arg, &a, 1) ? -EFAULT : 0;
> +            } else {
> +                for (idx = 0; idx < vs->used_guestinfo; idx++) {
> +                    if ((vs->guestinfo[idx]->key_len == a.key_length) &&
> +                        (memcmp(key,
> +                                vs->guestinfo[idx]->key_data,
> +                                vs->guestinfo[idx]->key_len) == 0)) {
> +                        a.value_length = vs->guestinfo[idx]->val_len;
> +                        memcpy(value,
> +                               vs->guestinfo[idx]->val_data,
> +                               a.value_length);
> +                        rc = copy_to_guest(arg, &a, 1) ? -EFAULT : 0;
> +                        break;
> +                    }
> +                }
> +                if (idx >= vs->used_guestinfo) {
> +                    rc = -ENOENT;
> +                }
> +            }
> +        }
> +        spin_unlock(&d->arch.hvm_domain.vmport_lock);
> +
> +    param_fail9:
> +        rcu_unlock_domain(d);
> +        break;
> +    }
> +
>      default:
>      {
>          gdprintk(XENLOG_DEBUG, "Bad HVM op %ld.\n", op);
> diff --git a/xen/include/public/hvm/hvm_op.h b/xen/include/public/hvm/hvm_op.h
> index a9aab4b..a530903 100644
> --- a/xen/include/public/hvm/hvm_op.h
> +++ b/xen/include/public/hvm/hvm_op.h
> @@ -272,4 +272,22 @@ DEFINE_XEN_GUEST_HANDLE(xen_hvm_inject_msi_t);
>
>  #endif /* defined(__XEN__) || defined(__XEN_TOOLS__) */
>
> +/* Get/set vmport subcommands */
> +#define HVMOP_get_vmport_guest_info 17
> +#define HVMOP_set_vmport_guest_info 18
> +#define VMPORT_GUEST_INFO_KEY_MAX 40
> +#define VMPORT_GUEST_INFO_VAL_MAX 128
> +struct xen_hvm_vmport_guest_info {
> +    /* Domain to be accessed */
> +    domid_t   domid;
> +    /* key length */
> +    uint16_t   key_length;
> +    /* value length */
> +    uint16_t   value_length;
> +    /* key and value data */
> +    char      data[VMPORT_GUEST_INFO_KEY_MAX + VMPORT_GUEST_INFO_VAL_MAX];
> +};
> +typedef struct xen_hvm_vmport_guest_info xen_hvm_vmport_guest_info_t;
> +DEFINE_XEN_GUEST_HANDLE(xen_hvm_vmport_guest_info_t);
> +
>  #endif /* __XEN_PUBLIC_HVM_HVM_OP_H__ */
> --
> 1.8.4
>
>
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel