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

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 12/01/17 14:58, 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 <paul.durrant@xxxxxxxxxx>
> Suggested-by: Ian Jackson <ian.jackson@xxxxxxxxxx>
> Suggested-by: Jennifer Herbert <jennifer.herbert@xxxxxxxxxx>
> ---
> Cc: Ian Jackson <ian.jackson@xxxxxxxxxx>
> Cc: Jennifer Herbert <jennifer.herbert@xxxxxxxxxx>
> Cc: Daniel De Graaf <dgdegra@xxxxxxxxxxxxx>
> Cc: Wei Liu <wei.liu2@xxxxxxxxxx>
> Reviewed-by: Jan Beulich <jbeulich@xxxxxxxx>
> Cc: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
>
> 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        | 158 
> ++++++++++++++++++++++++++++++++++++++
>  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             | 118 ++++++++++++++++++++++++++++
>  xen/arch/x86/hvm/hvm.c            |   1 +
>  xen/arch/x86/hypercall.c          |   2 +
>  xen/include/public/hvm/dm_op.h    |  71 +++++++++++++++++
>  xen/include/public/xen.h          |   1 +
>  xen/include/xen/hypercall.h       |   7 ++
>  xen/include/xsm/dummy.h           |   6 ++
>  xen/include/xsm/xsm.h             |   6 ++
>  xen/xsm/flask/hooks.c             |   7 ++
>  15 files changed, 452 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 0000000..2a4bd16
> --- /dev/null
> +++ b/docs/designs/dmop.markdown
> @@ -0,0 +1,158 @@
> +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_64(void) h;
> +    uint32_t size;
> +};

Sorry to quibble, but there is a problem here which has only just
occurred to me.  This ABI isn't futureproof, and has padding at the end
which affects how the array is layed out.

The userspace side should be

struct xen_dm_op_buf {
    void *h;
    size_t size;
}

which will work sensibly for 32bit and 64bit userspace, and futureproof
(for when 128bit turns up).  Its size is also a power of two which
avoids alignment issues in the array.

The kernel already has to parse this structure anyway, and will know the
bitness of its userspace process.  We could easily (at this point)
require the kernel to turn it into the kernels bitness for forwarding on
to Xen, which covers the 32bit userspace under a 64bit kernel problem,
in a way which won't break the hypercall ABI when 128bit comes along.

~Andrew

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