Re: [Xen-devel] [PATCH v7 1/8] public / x86: Introduce __HYPERCALL_dm_op...

[Paul Durrant <Paul.Durrant@xxxxxxxxxx>]

> -----Original Message-----
> From: Jan Beulich [mailto:JBeulich@xxxxxxxx]
> Sent: 24 January 2017 10:00
> To: Paul Durrant <Paul.Durrant@xxxxxxxxxx>
> Cc: Ian Jackson <Ian.Jackson@xxxxxxxxxx>; Jennifer Herbert
> <jennifer.herbert@xxxxxxxxxx>; xen-devel@xxxxxxxxxxxxxxxxxxxx
> Subject: Re: [PATCH v7 1/8] public / x86: Introduce __HYPERCALL_dm_op...
> 
> >>> On 23.01.17 at 14:59, <paul.durrant@xxxxxxxxxx> wrote:
> > +static bool copy_buf_from_guest(xen_dm_op_buf_t bufs[],
> > +                                unsigned int nr_bufs, void *dst,
> > +                                unsigned int idx, size_t dst_size)
> > +{
> > +    if ( dst_size != bufs[idx].size )
> > +        return false;
> > +
> > +    return !copy_from_guest(dst, bufs[idx].h, dst_size);
> > +}
> > +
> > +static bool copy_buf_to_guest(xen_dm_op_buf_t bufs[],
> > +                              unsigned int nr_bufs, unsigned int idx,
> > +                              void *src, size_t src_size)
> > +{
> > +    if ( bufs[idx].size != src_size )
> > +        return false;
> > +
> > +    return !copy_to_guest(bufs[idx].h, src, bufs[idx].size);
> > +}
> 
> What are the "nr_bufs" parameters good for in both of these?

Good point. I'm not sure what happened to the range check. I think it would 
still be good to have one.

> 
> > +static int dm_op(domid_t domid,
> > +                 unsigned int nr_bufs,
> > +                 xen_dm_op_buf_t bufs[])
> > +{
> > +    struct domain *d;
> > +    struct xen_dm_op op;
> > +    bool const_op = true;
> > +    long rc;
> > +
> > +    rc = rcu_lock_remote_domain_by_id(domid, &d);
> > +    if ( rc )
> > +        return rc;
> > +
> > +    if ( !has_hvm_container_domain(d) )
> > +        goto out;
> > +
> > +    rc = xsm_dm_op(XSM_DM_PRIV, d);
> > +    if ( rc )
> > +        goto out;
> > +
> > +    if ( !copy_buf_from_guest(bufs, nr_bufs, &op, 0, sizeof(op)) )
> 
> I'm afraid my request to have an exact size check in the copy
> functions was going a little too far for this particular instance: Just
> consider what would happen for a tool stack built with just this one
> patch in place, but run against a hypervisor with at least one more
> applied.

I was wondering about that. I assumed that you were expecting the hypervisor 
and libxc to be kept in lock-step.

> We can of course keep things the way they are here, but
> then we'll need a placeholder added to the structure right away
> (like is e.g. the case for domctl/sysctl). Every sub-structure should
> then be checked to not violate that constraint by a BUILD_BUG_ON()
> in its respective function (I'd prefer that over a single verification
> of the entire structure/union, as that would more clearly pinpoint
> a possible offender).
> 

Can I not revert things to the min size check that we had before. Surely that 
is preferable over the above complexity?

> > +    {
> > +        rc = -EFAULT;
> > +        goto out;
> > +    }
> > +
> > +    switch ( op.op )
> > +    {
> > +    default:
> > +        rc = -EOPNOTSUPP;
> > +        break;
> > +    }
> > +
> > +    if ( !rc &&
> > +         !const_op &&
> > +         !copy_buf_to_guest(bufs, nr_bufs, 0, &op, sizeof(op)) )
> > +        rc = -EFAULT;
> > +
> > + out:
> > +    rcu_unlock_domain(d);
> > +
> > +    return rc;
> > +}
> > +
> > +#define MAX_NR_BUFS 1
> > +
> > +int compat_dm_op(domid_t domid,
> > +                 unsigned int nr_bufs,
> > +                 COMPAT_HANDLE_PARAM(compat_dm_op_buf_t) bufs)
> > +{
> > +    struct xen_dm_op_buf nat[MAX_NR_BUFS];
> > +    unsigned int i;
> > +
> > +    if ( nr_bufs > MAX_NR_BUFS )
> > +        return -EINVAL;
> 
> E2BIG (to make it distinguishable)?
> 

Ok, if you'd prefer. I don't care either way.

> > +long do_dm_op(domid_t domid,
> > +              unsigned int nr_bufs,
> > +              XEN_GUEST_HANDLE_PARAM(xen_dm_op_buf_t) bufs)
> > +{
> > +    struct xen_dm_op_buf nat[MAX_NR_BUFS];
> > +
> > +    if ( nr_bufs > MAX_NR_BUFS )
> > +        return -EINVAL;
> 
> Same here then.
> 

Ok.

> > --- a/xen/include/xlat.lst
> > +++ b/xen/include/xlat.lst
> > @@ -56,6 +56,7 @@
> >  ?       grant_entry_header              grant_table.h
> >  ?  grant_entry_v2                  grant_table.h
> >  ?  gnttab_swap_grant_ref           grant_table.h
> > +!  dm_op_buf                       hvm/dm_op.h
> >  ?  vcpu_hvm_context                hvm/hvm_vcpu.h
> >  ?  vcpu_hvm_x86_32                 hvm/hvm_vcpu.h
> >  ?  vcpu_hvm_x86_64                 hvm/hvm_vcpu.h
> 
> While for this patch the addition here is sufficient, subsequent
> patches should add their sub-structures here with a leading ?,
> and you'd then need to invoke the resulting CHECK_* macros
> somewhere. I don't think we should leave those structures
> unchecked.
> 

OK, I can add this if you think it is necessary.

 Paul

> Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel