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

[Paul Durrant <xadimgnik@xxxxxxxxx>]

> -----Original Message-----
> From: Jan Beulich <jbeulich@xxxxxxxx>
> Sent: 29 April 2020 15:51
> To: Paul Durrant <paul@xxxxxxx>
> Cc: xen-devel@xxxxxxxxxxxxxxxxxxxx; Paul Durrant <pdurrant@xxxxxxxxxx>; 
> Daniel De Graaf
> <dgdegra@xxxxxxxxxxxxx>; Ian Jackson <ian.jackson@xxxxxxxxxxxxx>; Wei Liu 
> <wl@xxxxxxx>; Andrew Cooper
> <andrew.cooper3@xxxxxxxxxx>; George Dunlap <george.dunlap@xxxxxxxxxx>; Julien 
> Grall <julien@xxxxxxx>;
> Stefano Stabellini <sstabellini@xxxxxxxxxx>
> Subject: Re: [PATCH v2 2/5] xen/common/domctl: introduce 
> XEN_DOMCTL_get/setdomaincontext
> 
> On 07.04.2020 19:38, Paul Durrant wrote:
> > @@ -358,6 +359,113 @@ 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, const void *data, size_t len)
> > +{
> > +    struct domctl_context *c = priv;
> > +
> > +    if ( c->len + len < c->len )
> > +        return -EOVERFLOW;
> > +
> > +    c->len += len;
> > +
> > +    return 0;
> > +}
> > +
> > +static int save_data(void *priv, const 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_getdomaincontext *gdc)
> > +{
> > +    struct domctl_context c = { };
> 
> Please can you use ZERO_BLOCK_PTR or some such for the buffer
> field, such that errnoeous use of the field would not end up
> as a (PV-controllable) NULL deref. (Yes, it's a domctl, but
> still.) This being common code you also want to get things
> right for Arm, irrespective of whether the code will be dead
> there for now.
> 

Ok.

> > +    int rc;
> > +
> > +    if ( d == current->domain )
> > +        return -EPERM;
> > +
> > +    if ( guest_handle_is_null(gdc->buffer) ) /* query for buffer size */
> > +    {
> > +        if ( gdc->size )
> > +            return -EINVAL;
> > +
> > +        /* dry run to acquire buffer size */
> > +        rc = domain_save(d, accumulate_size, &c, true);
> > +        if ( rc )
> > +            return rc;
> > +
> > +        gdc->size = c.len;
> > +        return 0;
> > +    }
> > +
> > +    c.len = gdc->size;
> > +    c.buffer = xmalloc_bytes(c.len);
> 
> What sizes are we looking at here? It may be better to use vmalloc()
> right from the start.

Could be quite big so that seems reasonable.

> If not, I'd like to advocate for using
> xmalloc_array() instead of xmalloc_bytes() - see the almost-XSA
> commit cf38b4926e2b.
> 
> > +    if ( !c.buffer )
> > +        return -ENOMEM;
> > +
> > +    rc = domain_save(d, save_data, &c, false);
> > +
> > +    gdc->size = c.cur;
> > +    if ( !rc && copy_to_guest(gdc->buffer, c.buffer, gdc->size) )
> 
> As to my remark in patch 1 on the size field, applying to this size
> field too - copy_to_user{,_hvm}() don't support a 64-bit value (on
> y86 at least).

Ok.

> 
> > --- 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
> 
> I don't see you making any change making the interface backwards
> incompatible, hence no need for the bump.
> 

Ok.

> > @@ -1129,6 +1129,44 @@ 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.
> 
> This leaves open whether the size also gets updated in this latter
> case.

I'll clarify.

> 
> > + */
> > +struct xen_domctl_getdomaincontext {
> > +    uint64_t size;
> 
> If this is to remain 64-bits (with too large values suitably taken
> care of for all cases - see above), uint64_aligned_t please for
> consistency, if nothing else.

I've changed to uint32_t.

  Paul

> 
> Jan