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

[Paul Durrant]

> -Original Message-
> From: Andrew Cooper [mailto:am...@hermes.cam.ac.uk] On Behalf Of
> Andrew Cooper
> Sent: 23 January 2017 09:15
> To: Paul Durrant ; xen-de...@lists.xenproject.org
> Cc: Ian Jackson ; Jennifer Herbert
> ; Daniel De Graaf ;
> Wei Liu ; Jan Beulich 
> Subject: Re: [PATCH v4 1/8] public / x86: Introduce __HYPERCALL_dm_op...
> 
> On 20/01/2017 15:02, Paul Durrant wrote:
> >
> >>> +if ( !rc &&
> >>> + !copy_buf_to_guest(bufs, nr_bufs, 0, , 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.
> 
> Extraneous writebacks to PV guests are fairly cheep, but is is certainly
> not the case for HVM guests.  A writeback to HVM requires a least one
> guest pagetable walk (which itself most likely includes an EPT/NPT walk).
> 
> From a correctness point of view, it is reasonable for an implementation
> which expects a hypercall datastructure to be read only, to put said
> structure in read-only memory.  The PKRU feature in particular makes it
> very easy to set something up, then switch it from RW to RO for use.
> Such an implementation should have the hypercall fail with a spurious
> -EFAULT after it has otherwise completed successfully.

 Ok, I'll re-work the code yet again.

  Paul

> 
> ~Andrew

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

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

[Andrew Cooper]

On 20/01/2017 15:02, Paul Durrant wrote:
>
>>> +if ( !rc &&
>>> + !copy_buf_to_guest(bufs, nr_bufs, 0, , 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.

Extraneous writebacks to PV guests are fairly cheep, but is is certainly
not the case for HVM guests.  A writeback to HVM requires a least one
guest pagetable walk (which itself most likely includes an EPT/NPT walk).

From a correctness point of view, it is reasonable for an implementation
which expects a hypercall datastructure to be read only, to put said
structure in read-only memory.  The PKRU feature in particular makes it
very easy to set something up, then switch it from RW to RO for use. 
Such an implementation should have the hypercall fail with a spurious
-EFAULT after it has otherwise completed successfully.

~Andrew

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

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

[Paul Durrant]

> -Original Message-
> From: Xen-devel [mailto:xen-devel-boun...@lists.xen.org] On Behalf Of Jan
> Beulich
> Sent: 20 January 2017 16:38
> To: Paul Durrant <paul.durr...@citrix.com>
> Cc: Wei Liu <wei.l...@citrix.com>; Andrew Cooper
> <andrew.coop...@citrix.com>; Jennifer Herbert
> <jennifer.herb...@citrix.com>; Ian Jackson <ian.jack...@citrix.com>; xen-
> de...@lists.xenproject.org; Daniel De Graaf <dgde...@tycho.nsa.gov>
> Subject: Re: [Xen-devel] [PATCH v4 1/8] public / x86: Introduce
> __HYPERCALL_dm_op...
> 
> >>> On 20.01.17 at 17:20, <paul.durr...@citrix.com> wrote:
> >> From: Jan Beulich [mailto:jbeul...@suse.com]
> >> Sent: 20 January 2017 16:18
> >> >>> On 17.01.17 at 18:29, <paul.durr...@citrix.com> wrote:
> >> > +#ifndef __XEN_PUBLIC_HVM_DM_OP_H__
> >> > +#define __XEN_PUBLIC_HVM_DM_OP_H__
> >> > +
> >> > +#if defined(__XEN__) || defined(__XEN_TOOLS__)
> >> > +
> >> > +#include "../xen.h"
> >> > +
> >> > +#define XEN_DMOP_invalid 0
> >>
> >> Do we actually need this, btw?
> >>
> >
> > Not really, I just prefer to have 0 be an invalid sub-op and #defining it
> > this way makes that obvious.
> 
> Well, as soon as there are subops 1, 2, 3, ... it'll be obvious too.
> 

Ok. I'll take it out then.

  Paul

> Jan
> 
> 
> ___
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> https://lists.xen.org/xen-devel
___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

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

[Jan Beulich]

>>> On 20.01.17 at 17:20,  wrote:
>> From: Jan Beulich [mailto:jbeul...@suse.com]
>> Sent: 20 January 2017 16:18
>> >>> On 17.01.17 at 18:29,  wrote:
>> > +#ifndef __XEN_PUBLIC_HVM_DM_OP_H__
>> > +#define __XEN_PUBLIC_HVM_DM_OP_H__
>> > +
>> > +#if defined(__XEN__) || defined(__XEN_TOOLS__)
>> > +
>> > +#include "../xen.h"
>> > +
>> > +#define XEN_DMOP_invalid 0
>> 
>> Do we actually need this, btw?
>> 
> 
> Not really, I just prefer to have 0 be an invalid sub-op and #defining it 
> this way makes that obvious.

Well, as soon as there are subops 1, 2, 3, ... it'll be obvious too.

Jan


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

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

[Jan Beulich]

>>> On 17.01.17 at 18:29,  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)
> +{
> +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);
> +}

Wouldn't it be better to require an exact input size here? The guest
providing a different amount is likely to indicate some version
mismatch, build issue, or what not.

> +#ifndef __XEN_PUBLIC_HVM_DM_OP_H__
> +#define __XEN_PUBLIC_HVM_DM_OP_H__
> +
> +#if defined(__XEN__) || defined(__XEN_TOOLS__)
> +
> +#include "../xen.h"
> +
> +#define XEN_DMOP_invalid 0

Do we actually need this, btw?

> +struct xen_dm_op {
> +uint32_t op;
> +};
> +
> +struct xen_dm_op_buf {
> +XEN_GUEST_HANDLE(void) h;
> +unsigned long size;

xen_ulong_t?

> --- a/xen/include/xlat.lst
> +++ b/xen/include/xlat.lst
> @@ -129,3 +129,4 @@
>  ?flask_setenforcexsm/flask_op.h
>  !flask_sid_context   xsm/flask_op.h
>  ?flask_transitionxsm/flask_op.h
> +!dm_op_buf   hvm/dm_op.h

Please don't break the (mostly) sorted sequence here (sorting is
done by header name first, for - I hope - obvious reasons).

Jan


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

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

[Paul Durrant]

> -Original Message-
> From: Jan Beulich [mailto:jbeul...@suse.com]
> Sent: 20 January 2017 16:18
> To: Paul Durrant 
> Cc: Andrew Cooper ; Ian Jackson
> ; Jennifer Herbert ;
> Wei Liu ; xen-de...@lists.xenproject.org; Daniel De
> Graaf 
> Subject: Re: [PATCH v4 1/8] public / x86: Introduce __HYPERCALL_dm_op...
> 
> >>> On 17.01.17 at 18:29,  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)
> > +{
> > +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);
> > +}
> 
> Wouldn't it be better to require an exact input size here? The guest
> providing a different amount is likely to indicate some version
> mismatch, build issue, or what not.
> 

Ok.

> > +#ifndef __XEN_PUBLIC_HVM_DM_OP_H__
> > +#define __XEN_PUBLIC_HVM_DM_OP_H__
> > +
> > +#if defined(__XEN__) || defined(__XEN_TOOLS__)
> > +
> > +#include "../xen.h"
> > +
> > +#define XEN_DMOP_invalid 0
> 
> Do we actually need this, btw?
> 

Not really, I just prefer to have 0 be an invalid sub-op and #defining it this 
way makes that obvious.

> > +struct xen_dm_op {
> > +uint32_t op;
> > +};
> > +
> > +struct xen_dm_op_buf {
> > +XEN_GUEST_HANDLE(void) h;
> > +unsigned long size;
> 
> xen_ulong_t?
> 

If that's preferable.

> > --- a/xen/include/xlat.lst
> > +++ b/xen/include/xlat.lst
> > @@ -129,3 +129,4 @@
> >  ?  flask_setenforcexsm/flask_op.h
> >  !  flask_sid_context   xsm/flask_op.h
> >  ?  flask_transitionxsm/flask_op.h
> > +!  dm_op_buf   hvm/dm_op.h
> 
> Please don't break the (mostly) sorted sequence here (sorting is
> done by header name first, for - I hope - obvious reasons).
> 

Ok.

> Jan


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

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

[Wei Liu]

On Fri, Jan 20, 2017 at 03:59:22PM +, Paul Durrant wrote:
> > -Original Message-
> [snip]
> > > +
> > > +va_start(args, nr_bufs);
> > > +for (idx = 0; idx < nr_bufs; idx++)
> > 
> > Coding style.
> 
> Ah, yes.
> 
> > 
> > > +
> > > +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(, 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([i], );
> > > +
> > > +#undef XLAT_dm_op_buf_HNDL_h
> > > +}
> > > +
> > > +rc = dm_op(domid, nr_bufs, nat);
> > > +
> > 
> > Need to copy back to the original places with copy_to_compat?
> > 
> 
> No. It's only the array itself that is subject to compat translation and that 
> is not copied back.
> 

Ah, you're right. The array contains only a bunch of guest handles so no
copy back is required.

Wei.

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

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

[Paul Durrant]

> -Original Message-
[snip]
> > +
> > +va_start(args, nr_bufs);
> > +for (idx = 0; idx < nr_bufs; idx++)
> 
> Coding style.

Ah, yes.

> 
> > +
> > +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(, 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([i], );
> > +
> > +#undef XLAT_dm_op_buf_HNDL_h
> > +}
> > +
> > +rc = dm_op(domid, nr_bufs, nat);
> > +
> 
> Need to copy back to the original places with copy_to_compat?
> 

No. It's only the array itself that is subject to compat translation and that 
is not copied back.

  Paul

> Wei.

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

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

[Wei Liu]

On Tue, Jan 17, 2017 at 05:29:49PM +, 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)."
> 
> See that file for further information.
> 
> This patch simply adds the boilerplate for the hypercall.
> 
> Signed-off-by: Paul Durrant 
> Suggested-by: Ian Jackson 
> Suggested-by: Jennifer Herbert 
> ---
> Cc: Ian Jackson 
> Cc: Jennifer Herbert 
> Cc: Daniel De Graaf 
> Cc: Wei Liu 
> Cc: Jan Beulich 
> Cc: Andrew Cooper 
> 
> v4:
> - Change XEN_GUEST_HANDLE_64 to XEN_GUEST_HANDLE in struct xen_dm_op_buf
>   and add the necessary compat code. Drop Jan's R-b since the patch has
>   been fundamentally modified.
> 
> v3:
> - Re-written large portions of dmop.markdown to remove references to
>   previous proposals and make it a standalone design doc.
> 
> v2:
> - Addressed several comments from Jan.
> - Removed modification of __XEN_LATEST_INTERFACE_VERSION__ as it is not
>   needed in this patch.
> ---
>  docs/designs/dmop.markdown| 165 
> ++
>  tools/flask/policy/modules/xen.if |   2 +-
>  tools/libxc/include/xenctrl.h |   1 +
>  tools/libxc/xc_private.c  |  70 
>  tools/libxc/xc_private.h  |   2 +
>  xen/arch/x86/hvm/Makefile |   1 +
>  xen/arch/x86/hvm/dm.c | 149 ++
>  xen/arch/x86/hvm/hvm.c|   1 +
>  xen/arch/x86/hypercall.c  |   2 +
>  xen/include/Makefile  |   1 +
>  xen/include/public/hvm/dm_op.h|  71 
>  xen/include/public/xen.h  |   1 +
>  xen/include/xen/hypercall.h   |  15 
>  xen/include/xlat.lst  |   1 +
>  xen/include/xsm/dummy.h   |   6 ++
>  xen/include/xsm/xsm.h |   6 ++
>  xen/xsm/flask/hooks.c |   7 ++
>  17 files changed, 500 insertions(+), 1 deletion(-)
>  create mode 100644 docs/designs/dmop.markdown
>  create mode 100644 xen/arch/x86/hvm/dm.c
>  create mode 100644 xen/include/public/hvm/dm_op.h
> 
> diff --git a/docs/designs/dmop.markdown b/docs/designs/dmop.markdown
> new file mode 100644
> index 000..9f2f0d4
> --- /dev/null
> +++ b/docs/designs/dmop.markdown
> @@ -0,0 +1,165 @@
> +DMOP
> +
> +
> +Introduction
> +
> +
> +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).
> +
> +The problem occurs when you a device model issues an hypercall that
> +includes references to user memory other than the operation structure
> +itself, such as with Track dirty VRAM (as used in VGA emulation).
> +Is this case, the address of this other user memory needs to be vetted,
> +to ensure it is not within restricted address ranges, such as kernel
> +memory. The real problem comes down to how you would vet this address -
> +the idea place to do this is within the privcmd driver, without privcmd
> +having to have specific knowledge of the hypercall's semantics.
> +
> +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.
> +
> +A DMOP hypercall consists of an array of buffers and lengths, with the
> +first one containing the specific DMOP parameters. These can then reference
> +further buffers from within in the array. Since the only user buffers
> +passed are that found with that array, they can all can be audited by
> +privcmd.
> +
> +The following code illustrates this idea:
> +
> +struct xen_dm_op {
> +uint32_t op;
> +};
> +
> +struct xen_dm_op_buf {
> +XEN_GUEST_HANDLE(void) h;
> +unsigned long size;
> +};
> +typedef struct xen_dm_op_buf xen_dm_op_buf_t;
> +
> +enum neg_errnoval
> +HYPERVISOR_dm_op(domid_t domid,
> + xen_dm_op_buf_t bufs[],
> + unsigned int nr_bufs)
> +
> +@domid is the domain the hypercall operates on.
> +@bufs points to an array of buffers where @bufs[0] contains a struct
> +dm_op, describing the specific device model operation and its parameters.
> +@bufs[1..] may be referenced in the parameters for the purposes of
> +passing extra information to or from the domain.
> +@nr_bufs is the number of buffers in the @bufs array.
> +
> +It is forbidden for the above struct (xen_dm_op) to contain any guest
> +handles. If they are needed, they should instead be in
> 

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

[Paul Durrant]

> -Original Message-
> From: Andrew Cooper
> Sent: 20 January 2017 14:35
> To: Paul Durrant ; xen-de...@lists.xenproject.org
> Cc: Ian Jackson ; Jennifer Herbert
> ; Daniel De Graaf ;
> Wei Liu ; Jan Beulich 
> 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 000..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 .
> > + */
> > +
> > +#include 
> > +#include 
> > +#include 
> > +
> > +#include 
> > +
> > +#include 
> > +
> > +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, );
> > +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, , 0, sizeof(op)) )
> > +{
> > +rc = -EFAULT;
> > +goto out;
> > +}
> > +
> > +switch ( op.op )
> > +{
> > +default:
> > +rc = -EOPNOTSUPP;
> > +break;
> > +}
> > +
> > +if ( !rc &&
> > + !copy_buf_to_guest(bufs, nr_bufs, 0, , 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; 

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

[Andrew Cooper]

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 000..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 .
> + */
> +
> +#include 
> +#include 
> +#include 
> +
> +#include 
> +
> +#include 
> +
> +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, );
> +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, , 0, sizeof(op)) )
> +{
> +rc = -EFAULT;
> +goto out;
> +}
> +
> +switch ( op.op )
> +{
> +default:
> +rc = -EOPNOTSUPP;
> +break;
> +}
> +
> +if ( !rc &&
> + !copy_buf_to_guest(bufs, nr_bufs, 0, , 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(, 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([i], );
> +
> +#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

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

[Paul Durrant]

> -Original Message-
> From: Paul Durrant [mailto:paul.durr...@citrix.com]
> Sent: 17 January 2017 17:30
> To: xen-de...@lists.xenproject.org
> Cc: Paul Durrant ; Ian Jackson
> ; Jennifer Herbert ;
> Daniel De Graaf ; Wei Liu ;
> Jan Beulich ; Andrew Cooper
> 
> Subject: [PATCH v4 1/8] public / x86: Introduce __HYPERCALL_dm_op...
> 
> ...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)."
> 
> See that file for further information.
> 
> This patch simply adds the boilerplate for the hypercall.
> 
> Signed-off-by: Paul Durrant 
> Suggested-by: Ian Jackson 
> Suggested-by: Jennifer Herbert 
> ---
> Cc: Ian Jackson 
> Cc: Jennifer Herbert 
> Cc: Daniel De Graaf 
> Cc: Wei Liu 
> Cc: Jan Beulich 
> Cc: Andrew Cooper 

Andrew,

  Can you confirm that you are happy with the ABI as it now stands with this 
patch?

  Paul

> 
> v4:
> - Change XEN_GUEST_HANDLE_64 to XEN_GUEST_HANDLE in struct
> xen_dm_op_buf
>   and add the necessary compat code. Drop Jan's R-b since the patch has
>   been fundamentally modified.
> 
> v3:
> - Re-written large portions of dmop.markdown to remove references to
>   previous proposals and make it a standalone design doc.
> 
> v2:
> - Addressed several comments from Jan.
> - Removed modification of __XEN_LATEST_INTERFACE_VERSION__ as it is
> not
>   needed in this patch.
> ---
>  docs/designs/dmop.markdown| 165
> ++
>  tools/flask/policy/modules/xen.if |   2 +-
>  tools/libxc/include/xenctrl.h |   1 +
>  tools/libxc/xc_private.c  |  70 
>  tools/libxc/xc_private.h  |   2 +
>  xen/arch/x86/hvm/Makefile |   1 +
>  xen/arch/x86/hvm/dm.c | 149
> ++
>  xen/arch/x86/hvm/hvm.c|   1 +
>  xen/arch/x86/hypercall.c  |   2 +
>  xen/include/Makefile  |   1 +
>  xen/include/public/hvm/dm_op.h|  71 
>  xen/include/public/xen.h  |   1 +
>  xen/include/xen/hypercall.h   |  15 
>  xen/include/xlat.lst  |   1 +
>  xen/include/xsm/dummy.h   |   6 ++
>  xen/include/xsm/xsm.h |   6 ++
>  xen/xsm/flask/hooks.c |   7 ++
>  17 files changed, 500 insertions(+), 1 deletion(-)
>  create mode 100644 docs/designs/dmop.markdown
>  create mode 100644 xen/arch/x86/hvm/dm.c
>  create mode 100644 xen/include/public/hvm/dm_op.h
> 
> diff --git a/docs/designs/dmop.markdown b/docs/designs/dmop.markdown
> new file mode 100644
> index 000..9f2f0d4
> --- /dev/null
> +++ b/docs/designs/dmop.markdown
> @@ -0,0 +1,165 @@
> +DMOP
> +
> +
> +Introduction
> +
> +
> +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).
> +
> +The problem occurs when you a device model issues an hypercall that
> +includes references to user memory other than the operation structure
> +itself, such as with Track dirty VRAM (as used in VGA emulation).
> +Is this case, the address of this other user memory needs to be vetted,
> +to ensure it is not within restricted address ranges, such as kernel
> +memory. The real problem comes down to how you would vet this address
> -
> +the idea place to do this is within the privcmd driver, without privcmd
> +having to have specific knowledge of the hypercall's semantics.
> +
> +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.
> +
> +A DMOP hypercall consists of an array of buffers and lengths, with the
> +first one containing the specific DMOP parameters. These can then
> reference
> +further buffers from within in the array. Since the only user buffers
> +passed are that found with that array, they can all can be audited by
> +privcmd.
> +
> +The following code illustrates this idea:
> +
> +struct xen_dm_op {
> +uint32_t op;
> +};
> +
> +struct xen_dm_op_buf {
> +XEN_GUEST_HANDLE(void) h;
> +unsigned long size;
> +};
> +typedef struct xen_dm_op_buf xen_dm_op_buf_t;
> +
> +enum neg_errnoval
> +HYPERVISOR_dm_op(domid_t domid,
> + xen_dm_op_buf_t bufs[],
> + 

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

[Daniel De Graaf]

On 01/17/2017 12:29 PM, 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)."

See that file for further information.

This patch simply adds the boilerplate for the hypercall.

Signed-off-by: Paul Durrant 
Suggested-by: Ian Jackson 
Suggested-by: Jennifer Herbert 


Acked-by: Daniel De Graaf 

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

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

[Paul Durrant]

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

See that file for further information.

This patch simply adds the boilerplate for the hypercall.

Signed-off-by: Paul Durrant 
Suggested-by: Ian Jackson 
Suggested-by: Jennifer Herbert 
---
Cc: Ian Jackson 
Cc: Jennifer Herbert 
Cc: Daniel De Graaf 
Cc: Wei Liu 
Cc: Jan Beulich 
Cc: Andrew Cooper 

v4:
- Change XEN_GUEST_HANDLE_64 to XEN_GUEST_HANDLE in struct xen_dm_op_buf
  and add the necessary compat code. Drop Jan's R-b since the patch has
  been fundamentally modified.

v3:
- Re-written large portions of dmop.markdown to remove references to
  previous proposals and make it a standalone design doc.

v2:
- Addressed several comments from Jan.
- Removed modification of __XEN_LATEST_INTERFACE_VERSION__ as it is not
  needed in this patch.
---
 docs/designs/dmop.markdown| 165 ++
 tools/flask/policy/modules/xen.if |   2 +-
 tools/libxc/include/xenctrl.h |   1 +
 tools/libxc/xc_private.c  |  70 
 tools/libxc/xc_private.h  |   2 +
 xen/arch/x86/hvm/Makefile |   1 +
 xen/arch/x86/hvm/dm.c | 149 ++
 xen/arch/x86/hvm/hvm.c|   1 +
 xen/arch/x86/hypercall.c  |   2 +
 xen/include/Makefile  |   1 +
 xen/include/public/hvm/dm_op.h|  71 
 xen/include/public/xen.h  |   1 +
 xen/include/xen/hypercall.h   |  15 
 xen/include/xlat.lst  |   1 +
 xen/include/xsm/dummy.h   |   6 ++
 xen/include/xsm/xsm.h |   6 ++
 xen/xsm/flask/hooks.c |   7 ++
 17 files changed, 500 insertions(+), 1 deletion(-)
 create mode 100644 docs/designs/dmop.markdown
 create mode 100644 xen/arch/x86/hvm/dm.c
 create mode 100644 xen/include/public/hvm/dm_op.h

diff --git a/docs/designs/dmop.markdown b/docs/designs/dmop.markdown
new file mode 100644
index 000..9f2f0d4
--- /dev/null
+++ b/docs/designs/dmop.markdown
@@ -0,0 +1,165 @@
+DMOP
+
+
+Introduction
+
+
+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).
+
+The problem occurs when you a device model issues an hypercall that
+includes references to user memory other than the operation structure
+itself, such as with Track dirty VRAM (as used in VGA emulation).
+Is this case, the address of this other user memory needs to be vetted,
+to ensure it is not within restricted address ranges, such as kernel
+memory. The real problem comes down to how you would vet this address -
+the idea place to do this is within the privcmd driver, without privcmd
+having to have specific knowledge of the hypercall's semantics.
+
+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.
+
+A DMOP hypercall consists of an array of buffers and lengths, with the
+first one containing the specific DMOP parameters. These can then reference
+further buffers from within in the array. Since the only user buffers
+passed are that found with that array, they can all can be audited by
+privcmd.
+
+The following code illustrates this idea:
+
+struct xen_dm_op {
+uint32_t op;
+};
+
+struct xen_dm_op_buf {
+XEN_GUEST_HANDLE(void) h;
+unsigned long size;
+};
+typedef struct xen_dm_op_buf xen_dm_op_buf_t;
+
+enum neg_errnoval
+HYPERVISOR_dm_op(domid_t domid,
+ xen_dm_op_buf_t bufs[],
+ unsigned int nr_bufs)
+
+@domid is the domain the hypercall operates on.
+@bufs points to an array of buffers where @bufs[0] contains a struct
+dm_op, describing the specific device model operation and its parameters.
+@bufs[1..] may be referenced in the parameters for the purposes of
+passing extra information to or from the domain.
+@nr_bufs is the number of buffers in the @bufs array.
+
+It is forbidden for the above struct (xen_dm_op) to contain any guest
+handles. If they are needed, they should instead be in
+HYPERVISOR_dm_op->bufs.
+
+Validation by privcmd driver
+
+
+If the privcmd driver has been restricted to specific domain (using a
+ new ioctl), when it received an op, it will:
+
+1. Check hypercall is DMOP.
+
+2. Check domid == restricted domid.
+
+3. For each @nr_bufs in @bufs: Check @h and @size give