| Message ID | 20200327185012.1795-3-paul@xen.org (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | domain context infrastructure | expand |
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. > + 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? > + > + 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? > + 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? 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. > +}; > + > struct xen_domctl { > uint32_t cmd; > #define XEN_DOMCTL_createdomain 1 > @@ -1210,6 +1246,8 @@ struct xen_domctl { > #define XEN_DOMCTL_vuart_op 81 > #define XEN_DOMCTL_get_cpu_policy 82 > #define XEN_DOMCTL_set_cpu_policy 83 > +#define XEN_DOMCTL_getdomaincontext 84 > +#define XEN_DOMCTL_setdomaincontext 85 > #define XEN_DOMCTL_gdbsx_guestmemio 1000 > #define XEN_DOMCTL_gdbsx_pausevcpu 1001 > #define XEN_DOMCTL_gdbsx_unpausevcpu 1002 > @@ -1270,6 +1308,7 @@ struct xen_domctl { > struct xen_domctl_monitor_op monitor_op; > struct xen_domctl_psr_alloc psr_alloc; > struct xen_domctl_vuart_op vuart_op; > + struct xen_domctl_domaincontext domaincontext; > uint8_t pad[128]; > } u; > }; > diff --git a/xen/xsm/flask/hooks.c b/xen/xsm/flask/hooks.c > index 8af8602b46..d94d0fc125 100644 > --- a/xen/xsm/flask/hooks.c > +++ b/xen/xsm/flask/hooks.c > @@ -744,6 +744,12 @@ static int flask_domctl(struct domain *d, int cmd) > case XEN_DOMCTL_get_cpu_policy: > return current_has_perm(d, SECCLASS_DOMAIN2, DOMAIN2__GET_CPU_POLICY); > > + case XEN_DOMCTL_setdomaincontext: > + return current_has_perm(d, SECCLASS_DOMAIN2, DOMAIN2__SETCONTEXT); > + > + case XEN_DOMCTL_getdomaincontext: > + return current_has_perm(d, SECCLASS_DOMAIN2, DOMAIN2__GETCONTEXT); > + > default: > return avc_unknown_permission("domctl", cmd); > } > diff --git a/xen/xsm/flask/policy/access_vectors b/xen/xsm/flask/policy/access_vectors > index c055c14c26..fccfb9de82 100644 > --- a/xen/xsm/flask/policy/access_vectors > +++ b/xen/xsm/flask/policy/access_vectors > @@ -245,6 +245,10 @@ class domain2 > resource_map > # XEN_DOMCTL_get_cpu_policy > get_cpu_policy > +# XEN_DOMCTL_setdomaincontext > + setcontext > +# XEN_DOMCTL_getdomaincontext > + getcontext > } > > # Similar to class domain, but primarily contains domctls related to HVM domains > Cheers,
On 27/03/2020 18:50, Paul Durrant wrote: > + * 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; I actually forgot to comment on the mask. We are restricting ourself to 32 different type of records. I don't think this is going to fly very far. But what is the expected use of 'mask'. Are we expecting the toolstack to say which records should be saved/restored? Cheers,
> -----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
On 06.04.2020 11:07, Paul Durrant wrote: >> -----Original Message----- >> From: Julien Grall <julien@xen.org> >> Sent: 01 April 2020 14:42 >> >> On 27/03/2020 18:50, Paul Durrant wrote: >> 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? Yes, see e.g. Julien's recent patch to force honoring const-ness in some cases where it wasn't enforced so far. Luckily there look to not have crept in any violators. Jan
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, + 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; +} + +int xc_domain_setcontext(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_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; + + 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; + 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; +}; + struct xen_domctl { uint32_t cmd; #define XEN_DOMCTL_createdomain 1 @@ -1210,6 +1246,8 @@ struct xen_domctl { #define XEN_DOMCTL_vuart_op 81 #define XEN_DOMCTL_get_cpu_policy 82 #define XEN_DOMCTL_set_cpu_policy 83 +#define XEN_DOMCTL_getdomaincontext 84 +#define XEN_DOMCTL_setdomaincontext 85 #define XEN_DOMCTL_gdbsx_guestmemio 1000 #define XEN_DOMCTL_gdbsx_pausevcpu 1001 #define XEN_DOMCTL_gdbsx_unpausevcpu 1002 @@ -1270,6 +1308,7 @@ struct xen_domctl { struct xen_domctl_monitor_op monitor_op; struct xen_domctl_psr_alloc psr_alloc; struct xen_domctl_vuart_op vuart_op; + struct xen_domctl_domaincontext domaincontext; uint8_t pad[128]; } u; }; diff --git a/xen/xsm/flask/hooks.c b/xen/xsm/flask/hooks.c index 8af8602b46..d94d0fc125 100644 --- a/xen/xsm/flask/hooks.c +++ b/xen/xsm/flask/hooks.c @@ -744,6 +744,12 @@ static int flask_domctl(struct domain *d, int cmd) case XEN_DOMCTL_get_cpu_policy: return current_has_perm(d, SECCLASS_DOMAIN2, DOMAIN2__GET_CPU_POLICY); + case XEN_DOMCTL_setdomaincontext: + return current_has_perm(d, SECCLASS_DOMAIN2, DOMAIN2__SETCONTEXT); + + case XEN_DOMCTL_getdomaincontext: + return current_has_perm(d, SECCLASS_DOMAIN2, DOMAIN2__GETCONTEXT); + default: return avc_unknown_permission("domctl", cmd); } diff --git a/xen/xsm/flask/policy/access_vectors b/xen/xsm/flask/policy/access_vectors index c055c14c26..fccfb9de82 100644 --- a/xen/xsm/flask/policy/access_vectors +++ b/xen/xsm/flask/policy/access_vectors @@ -245,6 +245,10 @@ class domain2 resource_map # XEN_DOMCTL_get_cpu_policy get_cpu_policy +# XEN_DOMCTL_setdomaincontext + setcontext +# XEN_DOMCTL_getdomaincontext + getcontext } # Similar to class domain, but primarily contains domctls related to HVM domains
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(-)