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

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

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." ?

> +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.

> 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.

> +        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.

Otherwise, LGTM.

~Andrew

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