[Xen-devel] [PATCH v5 3/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, using XS_RESTRICT, only implemented by oxenstored at present.

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 doesn't already know. However be aware that it
still means relaxing an hypervisor/guest interface. See below.

Add a maximum limit of physmap entries to save, so that the guest cannot
DOS the toolstack.

In the case of qemu-traditional, the state node is used to issue
commands to the device model, so avoid to change permissions.


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.


Is it dangerous to let the guest write values that later QEMU and libxl
read back? Let's dig a bit deeper into the code. QEMU and libxl use very
similar functions to parse the physmap entries. Let's start from libxl,
the function that does the job is
tools/libxl/libxl_dom.c:libxl__toolstack_save. It calls:

- libxl__xs_directory on /physmap
this is safe

- libxl__xs_read
if the path is wrong (nothing is there), it returns NULL, and it is
handled correctly

- strtoll on the values read
The values are under guest control but strtoll can handle bad formats.
strtoll always returns an unsigned long long. In case of errors, it can
return LLONG_MIN or LLONG_MAX.  libxl__toolstack_save doesn't check for
conversion errors, but it never uses the returned values anywhere.
That's OK.  tools/libxl/libxl_dom.c:libxl__toolstack_restore writes back
the values to xenstore.

So in case of errors:

1) libxl__toolstack_save could return early with an error, if the
xenstore paths are wrong (nothing is on xenstore)

2) libxl__toolstack_restore could write back to xenstore any unsigned
long long, including LLONG_MIN or LLONG_MAX

Either way we are fine.

From the QEMU side, the code is very similar and uses strtoll to parse
the entries, that can deal with bad input values. The values are used to
avoid memory allocations at restore time for memory that has already
been allocated (video memory). If the values are wrong, QEMU will
attempt another memory allocation, failing because the maxmem limit has
been reached. Either way we are fine.


Signed-off-by: Stefano Stabellini <stefano.stabellini@xxxxxxxxxxxxx>


---
Changes in v5:
- improve commit message with security details

Changes in v4:
- add error message in case count > MAX_PHYSMAP_ENTRIES
- add a note to xenstore-paths.markdown about the possible change in
privilege level
- only change permissions if xsrestrict is supported

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
---
 docs/misc/xenstore-paths.markdown |    7 +++++--
 tools/libxl/libxl_dm.c            |   12 +++++++++++-
 tools/libxl/libxl_dom.c           |    7 +++++++
 3 files changed, 23 insertions(+), 3 deletions(-)

diff --git a/docs/misc/xenstore-paths.markdown 
b/docs/misc/xenstore-paths.markdown
index 780f601..99e038b 100644
--- a/docs/misc/xenstore-paths.markdown
+++ b/docs/misc/xenstore-paths.markdown
@@ -306,10 +306,13 @@ A virtual network device backend. Described by
 
 A PV console backend. Described in [console.txt](console.txt)
 
-#### ~/device-model/$DOMID/* [INTERNAL]
+#### ~/device-model/$DOMID/* [w]
 
 Information relating to device models running in the domain. $DOMID is
-the target domain of the device model.
+the target domain of the device model. When the device model is running
+at the same privilege level of the guest domain, this path does not
+contain any sensitive information and becomes guest writeable. Otherwise
+it is INTERNAL.
 
 #### ~/libxl/disable_udev = ("1"|"0") []
 
diff --git a/tools/libxl/libxl_dm.c b/tools/libxl/libxl_dm.c
index 455b66c..99e2553 100644
--- a/tools/libxl/libxl_dm.c
+++ b/tools/libxl/libxl_dm.c
@@ -1529,6 +1529,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();
@@ -1571,7 +1572,16 @@ 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 &&
+        libxl__check_qemu_supported(gc, dm, "xsrestrict")) {
+        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..c8523da 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,11 @@ int libxl__toolstack_save(uint32_t domid, uint8_t **buf,
                 &num);
     count = num;
 
+    if (count > MAX_PHYSMAP_ENTRIES) {
+        LOG(ERROR, "Max physmap entries reached");
+        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