From mboxrd@z Thu Jan 1 00:00:00 1970 From: Stefano Stabellini Subject: [PATCH v3 1/6] libxl: allow /local/domain/$LIBXL_TOOLSTACK_DOMID/device-model/$DOMID to be written by $DOMID Date: Wed, 10 Jun 2015 11:09:49 +0100 Message-ID: <1433930994-32527-1-git-send-email-stefano.stabellini@eu.citrix.com> References: Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: xen-devel@lists.xensource.com Cc: wei.liu2@citrix.com, Ian.Jackson@eu.citrix.com, Ian.Campbell@citrix.com, Stefano Stabellini List-Id: xen-devel@lists.xenproject.org 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. 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); + } else { + xs_mkdir(ctx->xsh, XBT_NULL, path); + } if (b_info->type == LIBXL_DOMAIN_TYPE_HVM && b_info->device_model_version diff --git a/tools/libxl/libxl_dom.c b/tools/libxl/libxl_dom.c index f408646..34b3c54 100644 --- a/tools/libxl/libxl_dom.c +++ b/tools/libxl/libxl_dom.c @@ -1677,6 +1677,8 @@ static inline char *physmap_path(libxl__gc *gc, uint32_t dm_domid, phys_offset, node); } +#define MAX_PHYSMAP_ENTRIES 12 + int libxl__toolstack_save(uint32_t domid, uint8_t **buf, uint32_t *len, void *dss_void) { @@ -1698,6 +1700,9 @@ int libxl__toolstack_save(uint32_t domid, uint8_t **buf, &num); count = num; + if (count > MAX_PHYSMAP_ENTRIES) + return -1; + *len = sizeof(version) + sizeof(count); *buf = calloc(1, *len); ptr = *buf; -- 1.7.10.4