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

[Stefano Stabellini <stefano.stabellini@xxxxxxxxxxxxx>]

On Tue, 16 Jun 2015, Wei Liu wrote:
> 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?

That's a great idea, I'll do that!

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