From mboxrd@z Thu Jan 1 00:00:00 1970 From: Stefano Stabellini Subject: Re: [PATCH v3 1/6] libxl: allow /local/domain/$LIBXL_TOOLSTACK_DOMID/device-model/$DOMID to be written by $DOMID Date: Mon, 29 Jun 2015 18:50:21 +0100 Message-ID: References: <1433930994-32527-1-git-send-email-stefano.stabellini@eu.citrix.com> <20150616145227.GB1813@zion.uk.xensource.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <20150616145227.GB1813@zion.uk.xensource.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Wei Liu Cc: xen-devel@lists.xensource.com, Ian.Jackson@eu.citrix.com, Ian.Campbell@citrix.com, Stefano Stabellini List-Id: xen-devel@lists.xenproject.org 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 > > > > > > --- > > > > 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!