Re: [Xen-devel] [PATCH v3 1/6] libxl: allow /local/domain/$LIBXL_TOOLSTACK_DOMID/device-model/$DOMID to be written by $DOMID

[Wei Liu <wei.liu2@xxxxxxxxxx>]

On Wed, Jun 10, 2015 at 11:09:49AM +0100, Stefano Stabellini wrote:
> The device model is going to restrict its xenstore connection to $DOMID
> level. Let qemu-xen access
> /local/domain/$LIBXL_TOOLSTACK_DOMID/device-model/$DOMID, as it is
> required by QEMU to read/write the physmap. It doesn't contain any
> information the guest is not already fully aware of.
> 
> Add a maximum limit of physmap entries to save, so that the guest cannot
> DOS the toolstack.
> 
> Information under
> /local/domain/$LIBXL_TOOLSTACK_DOMID/device-model/$DOMID include:
> 
> - the device model status, which is updated by the device model before
> the guest is unpaused, then ignored by both QEMU and libxl
> 
> - the guest physmap, which contains the memory layout of the guest. The
> guest is already in possession of this information. A malicious guest
> could modify the physmap entries causing QEMU to read wrong information
> at restore time. QEMU would populate its XenPhysmap list with wrong
> entries that wouldn't match its own device emulation addresses, leading
> to a failure in restoring the domain. But it could not compromise the
> security of the system because the addresses are only used for checks
> against QEMU's addresses.
> 

I will leave this to Ian. FWIW they look reasonable to me.

> 
> In the case of qemu-traditional, the state node is used to issue
> commands to the device model, so avoid to change permissions in that
> case.
> 
> Signed-off-by: Stefano Stabellini <stefano.stabellini@xxxxxxxxxxxxx>
> 
> 
> ---
> 
> In this version I added a limit to the number of xenstore entries to
> save, because with this patch they become guest writeable. The guest
> could fill xenstore with entries that libxl would need to save during
> domain migration.
> 
> I didn't worry about the guest filling up xenstore itself, because the
> guest could still do it under /local/domain/$DOMID/data or any of the
> other nodes owned by the guest. Xenstore needs to protect itself against
> this anyway.
> 
> ---
> 
> Changes in v3:
> - use LIBXL_TOOLSTACK_DOMID instead of 0 in the commit message
> - update commit message with more info on why it is safe
> - add a limit on the number of physmap entries to save and restore
> 
> Changes in v2:
> - fix permissions to actually do what intended
> - use LIBXL_TOOLSTACK_DOMID instead of 0
> ---
>  tools/libxl/libxl_dm.c  |   11 ++++++++++-
>  tools/libxl/libxl_dom.c |    5 +++++
>  2 files changed, 15 insertions(+), 1 deletion(-)
> 
> diff --git a/tools/libxl/libxl_dm.c b/tools/libxl/libxl_dm.c
> index 79a9a22..2809ba0 100644
> --- a/tools/libxl/libxl_dm.c
> +++ b/tools/libxl/libxl_dm.c
> @@ -1461,6 +1461,7 @@ void libxl__spawn_local_dm(libxl__egc *egc, 
> libxl__dm_spawn_state *dmss)
>      char **pass_stuff;
>      const char *dm;
>      int dm_state_fd = -1;
> +    struct xs_permissions rwperm[2];
>  
>      if (libxl_defbool_val(b_info->device_model_stubdomain)) {
>          abort();
> @@ -1503,7 +1504,15 @@ void libxl__spawn_local_dm(libxl__egc *egc, 
> libxl__dm_spawn_state *dmss)
>      }
>  
>      path = libxl__device_model_xs_path(gc, LIBXL_TOOLSTACK_DOMID, domid, "");
> -    xs_mkdir(ctx->xsh, XBT_NULL, path);
> +    if (b_info->device_model_version == LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN) 
> {
> +        rwperm[0].id = LIBXL_TOOLSTACK_DOMID;
> +        rwperm[0].perms = XS_PERM_NONE;
> +        rwperm[1].id = domid;
> +        rwperm[1].perms = XS_PERM_WRITE;
> +        libxl__xs_mkdir(gc, XBT_NULL, path, rwperm, 2);

This now grants write permission to $domid regardless whether QEMU will
be actually started with xsrestrict option. Can you move this patch
later in this series after having checked QEMU supported xsrestrict and
only grant access when QEMU is capable of xsrestrict?

Wei.

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