RE: [PATCH 2/5] xen/common/domctl: introduce XEN_DOMCTL_get/setdomaincontext

[Paul Durrant <xadimgnik@gmail.com>]

> -----Original Message-----
> From: Julien Grall <julien@xen.org>
> Sent: 01 April 2020 14:42
> To: Paul Durrant <paul@xen.org>; xen-devel@lists.xenproject.org
> Cc: Daniel De Graaf <dgdegra@tycho.nsa.gov>; Ian Jackson <ian.jackson@eu.citrix.com>; Wei Liu
> <wl@xen.org>; Andrew Cooper <andrew.cooper3@citrix.com>; George Dunlap <george.dunlap@citrix.com>; Jan
> Beulich <jbeulich@suse.com>; Stefano Stabellini <sstabellini@kernel.org>
> Subject: Re: [PATCH 2/5] xen/common/domctl: introduce XEN_DOMCTL_get/setdomaincontext
> 
> Hi Paul,
> 
> On 27/03/2020 18:50, Paul Durrant wrote:
> > These domctls provide a mechanism to get and set domain context from
> > the toolstack.
> >
> > Signed-off-by: Paul Durrant <paul@xen.org>
> > ---
> > Cc: Daniel De Graaf <dgdegra@tycho.nsa.gov>
> > Cc: Ian Jackson <ian.jackson@eu.citrix.com>
> > Cc: Wei Liu <wl@xen.org>
> > Cc: Andrew Cooper <andrew.cooper3@citrix.com>
> > Cc: George Dunlap <george.dunlap@citrix.com>
> > Cc: Jan Beulich <jbeulich@suse.com>
> > Cc: Julien Grall <julien@xen.org>
> > Cc: Stefano Stabellini <sstabellini@kernel.org>
> > ---
> >   tools/flask/policy/modules/xen.if   |   4 +-
> >   tools/libxc/include/xenctrl.h       |  11 +++
> >   tools/libxc/xc_domain.c             |  52 +++++++++++++
> >   xen/common/domctl.c                 | 115 ++++++++++++++++++++++++++++
> >   xen/include/public/domctl.h         |  41 +++++++++-
> >   xen/xsm/flask/hooks.c               |   6 ++
> >   xen/xsm/flask/policy/access_vectors |   4 +
> >   7 files changed, 230 insertions(+), 3 deletions(-)
> >
> > diff --git a/tools/flask/policy/modules/xen.if b/tools/flask/policy/modules/xen.if
> > index 8eb2293a52..2bc9db4f64 100644
> > --- a/tools/flask/policy/modules/xen.if
> > +++ b/tools/flask/policy/modules/xen.if
> > @@ -53,7 +53,7 @@ define(`create_domain_common', `
> >   	allow $1 $2:domain2 { set_cpu_policy settsc setscheduler setclaim
> >   			set_vnumainfo get_vnumainfo cacheflush
> >   			psr_cmt_op psr_alloc soft_reset
> > -			resource_map get_cpu_policy };
> > +			resource_map get_cpu_policy setcontext };
> >   	allow $1 $2:security check_context;
> >   	allow $1 $2:shadow enable;
> >   	allow $1 $2:mmu { map_read map_write adjust memorymap physmap pinpage mmuext_op updatemp };
> > @@ -97,7 +97,7 @@ define(`migrate_domain_out', `
> >   	allow $1 $2:hvm { gethvmc getparam };
> >   	allow $1 $2:mmu { stat pageinfo map_read };
> >   	allow $1 $2:domain { getaddrsize getvcpucontext pause destroy };
> > -	allow $1 $2:domain2 gettsc;
> > +	allow $1 $2:domain2 { gettsc getcontext };
> >   	allow $1 $2:shadow { enable disable logdirty };
> >   ')
> >
> > diff --git a/tools/libxc/include/xenctrl.h b/tools/libxc/include/xenctrl.h
> > index fc6e57a1a0..5c0d0d27a4 100644
> > --- a/tools/libxc/include/xenctrl.h
> > +++ b/tools/libxc/include/xenctrl.h
> > @@ -867,6 +867,17 @@ int xc_domain_hvm_setcontext(xc_interface *xch,
> >                                uint8_t *hvm_ctxt,
> >                                uint32_t size);
> >
> > +int xc_domain_getcontext(xc_interface *xch,
> > +                         uint32_t domid,
> > +                         uint32_t mask,
> > +                         uint8_t *ctxt_buf,
> 
> Why do you use uint8_t rather than void here?
> 
> > +                         uint32_t size);
> > +int xc_domain_setcontext(xc_interface *xch,
> > +                         uint32_t domid,
> > +                         uint32_t mask,
> > +                         uint8_t *ctxt_buf,
> > +                         uint32_t size);
> > +
> >   /**
> >    * This function will return guest IO ABI protocol
> >    *
> > diff --git a/tools/libxc/xc_domain.c b/tools/libxc/xc_domain.c
> > index 71829c2bce..15bcf671fc 100644
> > --- a/tools/libxc/xc_domain.c
> > +++ b/tools/libxc/xc_domain.c
> > @@ -537,6 +537,58 @@ int xc_domain_hvm_setcontext(xc_interface *xch,
> >       return ret;
> >   }
> >
> > +int xc_domain_getcontext(xc_interface *xch,
> > +                         uint32_t domid,
> > +                         uint32_t mask,
> > +                         uint8_t *ctxt_buf,
> > +                         uint32_t size)
> > +{
> > +    int ret;
> > +    DECLARE_DOMCTL;
> > +    DECLARE_HYPERCALL_BOUNCE(ctxt_buf, size, XC_HYPERCALL_BUFFER_BOUNCE_OUT);
> > +
> > +    if ( xc_hypercall_bounce_pre(xch, ctxt_buf) )
> > +        return -1;
> > +
> > +    domctl.cmd = XEN_DOMCTL_getdomaincontext;
> > +    domctl.domain = domid;
> > +    domctl.u.domaincontext.mask = mask;
> > +    domctl.u.domaincontext.size = size;
> > +    set_xen_guest_handle(domctl.u.domaincontext.buffer, ctxt_buf);
> > +
> > +    ret = do_domctl(xch, &domctl);
> > +
> > +    xc_hypercall_bounce_post(xch, ctxt_buf);
> > +
> > +    return !ret ? domctl.u.domaincontext.size : -1;
> 
> Looking at the domctl interface, size would be a 32-bit unsigned value.
> However the return is a 32-bit signed value. This is a call for trouble
> if the size is too big.
> 
> > +}
> > +
> > +int xc_domain_setcontext(xc_interface *xch,
> > +                         uint32_t domid,
> > +                         uint32_t mask,
> > +                         uint8_t *ctxt_buf,
> 
> The context is not meant to be modified. So this should really be const.
> 

All of the above were basically inherited via cut'n'paste from the HVM save code. I'll re-work it.

> > +                         uint32_t size)
> > +{
> > +    int ret;
> > +    DECLARE_DOMCTL;
> > +    DECLARE_HYPERCALL_BOUNCE(ctxt_buf, size, XC_HYPERCALL_BUFFER_BOUNCE_IN);
> > +
> > +    if ( xc_hypercall_bounce_pre(xch, ctxt_buf) )
> > +        return -1;
> > +
> > +    domctl.cmd = XEN_DOMCTL_setdomaincontext;
> > +    domctl.domain = domid;
> > +    domctl.u.domaincontext.mask = mask;
> > +    domctl.u.domaincontext.size = size;
> > +    set_xen_guest_handle(domctl.u.domaincontext.buffer, ctxt_buf);
> > +
> > +    ret = do_domctl(xch, &domctl);
> > +
> > +    xc_hypercall_bounce_post(xch, ctxt_buf);
> > +
> > +    return ret;
> > +}
> > +
> >   int xc_vcpu_getcontext(xc_interface *xch,
> >                          uint32_t domid,
> >                          uint32_t vcpu,
> > diff --git a/xen/common/domctl.c b/xen/common/domctl.c
> > index a69b3b59a8..9c39519b04 100644
> > --- a/xen/common/domctl.c
> > +++ b/xen/common/domctl.c
> > @@ -25,6 +25,7 @@
> >   #include <xen/hypercall.h>
> >   #include <xen/vm_event.h>
> >   #include <xen/monitor.h>
> > +#include <xen/save.h>
> >   #include <asm/current.h>
> >   #include <asm/irq.h>
> >   #include <asm/page.h>
> > @@ -358,6 +359,111 @@ static struct vnuma_info *vnuma_init(const struct xen_domctl_vnuma *uinfo,
> >       return ERR_PTR(ret);
> >   }
> >
> > +struct domctl_context
> > +{
> > +    void *buffer;
> > +    size_t len;
> > +    size_t cur;
> > +};
> > +
> > +static int accumulate_size(void *priv, void *data, size_t len)
> > +{
> > +    struct domctl_context *c = priv;
> > +
> > +    c->len += len;
> 
> What does prevent overflow?
> 

Good point. That needs to be checked.

> > +
> > +    return 0;
> > +}
> > +
> > +static int save_data(void *priv, void *data, size_t len)
> > +{
> > +    struct domctl_context *c = priv;
> > +
> > +    if ( c->len - c->cur < len )
> > +        return -ENOSPC;
> > +
> > +    memcpy(c->buffer + c->cur, data, len);
> > +    c->cur += len;
> > +
> > +    return 0;
> > +}
> > +
> > +static int getdomaincontext(struct domain *d,
> > +                            struct xen_domctl_domaincontext *dc)
> > +{
> > +    struct domctl_context c = { };
> > +    int rc;
> > +
> > +    if ( d == current->domain )
> > +        return -EPERM;
> > +
> > +    if ( guest_handle_is_null(dc->buffer) ) /* query for buffer size */
> > +
> > +    {
> > +        if ( dc->size )
> > +            return -EINVAL;
> > +
> > +        /* dry run to acquire buffer size */
> > +        rc = domain_save(d, accumulate_size, &c, dc->mask, true);
> > +        if ( rc )
> > +            return rc;
> > +
> > +        dc->size = c.len;
> 
> len is a size_t (so 64-bit on 64-bit arch) value, but size is 32-bit. So
> we return an error if the stream is going to be bigger than 4G?
> 

I'd hope a 4G stream is unlikely but I'll expand size to 64 bits.

> > +        return 0;
> > +    }
> > +
> > +    c.len = dc->size;
> > +    c.buffer = xmalloc_bytes(c.len);
> > +    if ( !c.buffer )
> > +        return -ENOMEM;
> > +
> > +    rc = domain_save(d, save_data, &c, dc->mask, false);
> > +
> > +    dc->size = c.cur;
> > +    if ( !rc && copy_to_guest(dc->buffer, c.buffer, dc->size) )
> > +        rc = -EFAULT;
> > +
> > +    xfree(c.buffer);
> > +
> > +    return rc;
> > +}
> > +
> > +static int load_data(void *priv, void *data, size_t len)
> > +{
> > +    struct domctl_context *c = priv;
> > +
> > +    if ( c->len - c->cur < len )
> > +        return -ENODATA;
> > +
> > +    if ( data )
> > +        memcpy(data, c->buffer + c->cur, len);
> > +
> > +    c->cur += len;
> > +
> > +    return 0;
> > +}
> > +
> > +static int setdomaincontext(struct domain *d,
> > +                            const struct xen_domctl_domaincontext *dc)
> > +{
> > +    struct domctl_context c = { .len = dc->size };
> > +    int rc;
> > +
> > +    if ( d == current->domain )
> > +        return -EPERM;
> > +
> > +    c.buffer = xmalloc_bytes(c.len);
> > +    if ( !c.buffer )
> > +        return -ENOMEM;
> > +
> > +    rc = !copy_from_guest(c.buffer, dc->buffer, c.len) ?
> > +        domain_load(d, load_data, &c, dc->mask) : -EFAULT;
> > +
> > +    xfree(c.buffer);
> > +
> > +    return rc;
> > +}
> > +
> >   long do_domctl(XEN_GUEST_HANDLE_PARAM(xen_domctl_t) u_domctl)
> >   {
> >       long ret = 0;
> > @@ -942,6 +1048,15 @@ long do_domctl(XEN_GUEST_HANDLE_PARAM(xen_domctl_t) u_domctl)
> >               copyback = 1;
> >           break;
> >
> > +    case XEN_DOMCTL_getdomaincontext:
> > +        ret = getdomaincontext(d, &op->u.domaincontext);
> > +        copyback = !ret;
> > +        break;
> > +
> > +    case XEN_DOMCTL_setdomaincontext:
> > +        ret = setdomaincontext(d, &op->u.domaincontext);
> > +        break;
> > +
> >       default:
> >           ret = arch_do_domctl(op, d, u_domctl);
> >           break;
> > diff --git a/xen/include/public/domctl.h b/xen/include/public/domctl.h
> > index 1ad34c35eb..24ed6852cf 100644
> > --- a/xen/include/public/domctl.h
> > +++ b/xen/include/public/domctl.h
> > @@ -38,7 +38,7 @@
> >   #include "hvm/save.h"
> >   #include "memory.h"
> >
> > -#define XEN_DOMCTL_INTERFACE_VERSION 0x00000012
> > +#define XEN_DOMCTL_INTERFACE_VERSION 0x00000013
> >
> >   /*
> >    * NB. xen_domctl.domain is an IN/OUT parameter for this operation.
> > @@ -1129,6 +1129,42 @@ struct xen_domctl_vuart_op {
> >                                    */
> >   };
> >
> > +/*
> > + * Get/Set domain PV context. The same struct xen_domctl_domaincontext
> > + * is used for both commands but with slightly different field semantics
> > + * as follows:
> > + *
> > + * XEN_DOMCTL_getdomaincontext
> > + * ---------------------------
> > + *
> > + * buffer (IN):   The buffer into which the context data should be
> > + *                copied, or NULL to query the buffer size that should
> > + *                be allocated.
> > + * size (IN/OUT): If 'buffer' is NULL then the value passed in must be
> > + *                zero, and the value passed out will be the size of the
> > + *                buffer to allocate.
> > + *                If 'buffer' is non-NULL then the value passed in must
> > + *                be the size of the buffer into which data may be copied.
> > + * mask (IN):     See comment on domain_save/load() in xen/save.h.
> > + *
> > + * XEN_DOMCTL_setdomaincontext
> > + * ---------------------------
> > + *
> > + * buffer (IN):   The buffer from which the context data should be
> > + *                copied.
> > + * size (IN):     The size of the buffer from which data may be copied.
> > + *                This data must include DOMAIN_SAVE_CODE_HEADER at the
> > + *                start and terminate with a DOMAIN_SAVE_CODE_END record.
> > + *                Any data beyond the DOMAIN_SAVE_CODE_END record will be
> > + *                ignored.
> > + * mask (IN):     See comment on domain_save/load() in xen/save.h.
> > + */
> > +struct xen_domctl_domaincontext {
> > +    uint32_t size;
> > +    uint32_t mask;
> > +    XEN_GUEST_HANDLE_64(uint8) buffer;
> 
> Should not it be a void handle?
>

Perhaps.
 
> Also, in the case of XEN_DOMCTL_setdomaincontext, the buffer is not
> meant to be modified by the hypervisor. So I would rather introduce two
> separate structure so we can enforce the const.
> 

Can handles be meaningfully const?

  Paul