Re: [PATCH v4 1/8] public / x86: Introduce __HYPERCALL_dm_op...

[Paul Durrant <Paul.Durrant@citrix.com>]

> -----Original Message-----
> From: Andrew Cooper
> Sent: 20 January 2017 14:35
> To: Paul Durrant <Paul.Durrant@citrix.com>; xen-devel@lists.xenproject.org
> Cc: Ian Jackson <Ian.Jackson@citrix.com>; Jennifer Herbert
> <jennifer.herbert@citrix.com>; Daniel De Graaf <dgdegra@tycho.nsa.gov>;
> Wei Liu <wei.liu2@citrix.com>; Jan Beulich <jbeulich@suse.com>
> Subject: Re: [PATCH v4 1/8] public / x86: Introduce __HYPERCALL_dm_op...
> 
> On 17/01/17 17:29, Paul Durrant wrote:
> > ...as a set of hypercalls to be used by a device model.
> >
> > As stated in the new docs/designs/dm_op.markdown:
> >
> > "The aim of DMOP is to prevent a compromised device model from
> > compromising domains other then the one it is associated with. (And is
> > therefore likely already compromised)."
> 
> "associated with" is a bit ambiguous, as a dm running in dom0 is
> associated with dom0.
> 
> How about "other than the one it is providing emulation for." ?
> 

Yes, that's clearer.

> > +The Design
> > +----------
> > +
> > +The privcmd driver implements a new restriction ioctl, which takes a
> domid
> > +parameter.  After that restriction ioctl is issued, the privcmd driver will
> > +permit only DMOP hypercalls, and only with the specified target domid.
> 
> The plan (iirc) is to implement dmop via a new ioctl() on /dev/xen/privcmd
> 
> At the point that restrict() is called, all unaudited operations on
> /dev/xen/privcmd should cease to function, including regular hypercalls.
> 

That makes sense, but I may not have been in the loop for that bit of planning. I'll certainly add the text though.

> > diff --git a/xen/arch/x86/hvm/dm.c b/xen/arch/x86/hvm/dm.c
> > new file mode 100644
> > index 0000000..f00bc2c
> > --- /dev/null
> > +++ b/xen/arch/x86/hvm/dm.c
> > @@ -0,0 +1,149 @@
> > +/*
> > + * Copyright (c) 2016 Citrix Systems Inc.
> > + *
> > + * This program is free software; you can redistribute it and/or modify it
> > + * under the terms and conditions of the GNU General Public License,
> > + * version 2, as published by the Free Software Foundation.
> > + *
> > + * This program is distributed in the hope it will be useful, but WITHOUT
> > + * ANY WARRANTY; without even the implied warranty of
> MERCHANTABILITY or
> > + * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public
> License for
> > + * more details.
> > + *
> > + * You should have received a copy of the GNU General Public License
> along with
> > + * this program; If not, see <http://www.gnu.org/licenses/>.
> > + */
> > +
> > +#include <xen/guest_access.h>
> > +#include <xen/hypercall.h>
> > +#include <xen/sched.h>
> > +
> > +#include <asm/hvm/ioreq.h>
> > +
> > +#include <xsm/xsm.h>
> > +
> > +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)
> > +{
> > +    size_t size = min_t(size_t, dst_size, bufs[idx].size);
> > +
> > +    return !copy_from_guest(dst, bufs[idx].h, 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)
> > +{
> > +    size_t size = min_t(size_t, bufs[idx].size, src_size);
> > +
> > +    return !copy_to_guest(bufs[idx].h, src, size);
> > +}
> > +
> > +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;
> > +    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)) )
> > +    {
> > +        rc = -EFAULT;
> > +        goto out;
> > +    }
> > +
> > +    switch ( op.op )
> > +    {
> > +    default:
> > +        rc = -EOPNOTSUPP;
> > +        break;
> > +    }
> > +
> > +    if ( !rc &&
> > +         !copy_buf_to_guest(bufs, nr_bufs, 0, &op, sizeof(op)) )
> 
> Do all ops need a copyback?  If they do, this is fine.  If not, it would
> be better to have a copyback boolean which subops set as necessary.

I can restrict copy-back using a boolean set for sub-ops that have 'out' params, or when there needs to be a continuation but I didn't really think it was worth the extra complexity.

> 
> > +        rc = -EFAULT;
> > +
> > + out:
> > +    rcu_unlock_domain(d);
> > +
> > +    return rc;
> > +}
> > +
> > +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;
> > +    unsigned int i;
> > +    int rc = -EFAULT;
> > +
> > +    nat = xzalloc_array(struct xen_dm_op_buf, nr_bufs);
> > +    if ( !nat )
> > +        return -ENOMEM;
> > +
> > +    for ( i = 0; i < nr_bufs; i++ )
> > +    {
> > +        struct compat_dm_op_buf cmp;
> > +
> > +        if ( copy_from_compat_offset(&cmp, bufs, i, 1) )
> > +            goto out;
> > +
> > +#define XLAT_dm_op_buf_HNDL_h(_d_, _s_) \
> > +        guest_from_compat_handle((_d_)->h, (_s_)->h)
> > +
> > +        XLAT_dm_op_buf(&nat[i], &cmp);
> > +
> > +#undef XLAT_dm_op_buf_HNDL_h
> > +    }
> > +
> > +    rc = dm_op(domid, nr_bufs, nat);
> > +
> > + out:
> > +    xfree(nat);
> > +
> > +    return rc;
> > +}
> > +
> > +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;
> > +    int rc;
> > +
> > +    nat = xzalloc_array(struct xen_dm_op_buf, nr_bufs);
> > +    if ( !nat )
> > +        return -ENOMEM;
> 
> nr_bufs is a guest-passed value at this point, and needs a boundary check.
> 
> The maximum number of bufs on all implemented ops is 2, isn't it?  I'd
> have a define somewhere, audit against the global max, and put the array
> on the stack to avoid the memory allocation.
> 

Yes, I'll define max value and put it on the stack as you suggest.

> Otherwise, LGTM.

Cool. Thanks,

  Paul

> 
> ~Andrew

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