[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>]

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 <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);
+    } 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


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