[PATCH v2 0/3] Get devices list from frontend xenstore entries

[PATCH v2 0/3] Get devices list from frontend xenstore entries

[Marek Marczykowski]

To cover the case of non-dom0 backed devices. Version 2 enchanced based
on suggestions from IanC and Roger.
Also added one additional patch (3/3), which cover the same case for
vtpm - it already had non-dom0 backend support, so just clean up the
code.

Marek Marczykowski (3):
  libxl: add libxl__xs_get_permissions helper
  libxl: do not assume Dom0 backend while listing disks and nics
  libxl: reduce code duplication in libxl_device_vtpm_list

 tools/libxl/libxl.c          | 181 +++++++++++++++++++++++++++++++++----------
 tools/libxl/libxl_internal.h |   2 +
 tools/libxl/libxl_xshelp.c   |  11 +++
 3 files changed, 151 insertions(+), 43 deletions(-)

-- 
1.8.1.4

[PATCH v2 1/3] libxl: add libxl__xs_get_permissions helper

[Marek Marczykowski]

Signed-off-by: Marek Marczykowski <marmarek@invisiblethingslab.com>
---
 tools/libxl/libxl_internal.h |  2 ++
 tools/libxl/libxl_xshelp.c   | 11 +++++++++++
 2 files changed, 13 insertions(+)

diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
index 573e3d1..2e9d9b5 100644
--- a/tools/libxl/libxl_internal.h
+++ b/tools/libxl/libxl_internal.h
@@ -533,6 +533,8 @@ _hidden char *libxl__xs_get_dompath(libxl__gc *gc, uint32_t domid);
 
 _hidden char *libxl__xs_read(libxl__gc *gc, xs_transaction_t t,
                              const char *path);
+_hidden struct xs_permissions * libxl__xs_get_permissions(libxl__gc *gc,
+                xs_transaction_t t, const char *path, unsigned int *num);
 _hidden char **libxl__xs_directory(libxl__gc *gc, xs_transaction_t t,
                                    const char *path, unsigned int *nb);
    /* On error: returns NULL, sets errno (no logging) */
diff --git a/tools/libxl/libxl_xshelp.c b/tools/libxl/libxl_xshelp.c
index 52af484..48960de 100644
--- a/tools/libxl/libxl_xshelp.c
+++ b/tools/libxl/libxl_xshelp.c
@@ -115,6 +115,17 @@ char * libxl__xs_read(libxl__gc *gc, xs_transaction_t t, const char *path)
     return ptr;
 }
 
+struct xs_permissions * libxl__xs_get_permissions(libxl__gc *gc,
+        xs_transaction_t t, const char *path, unsigned int *num)
+{
+    libxl_ctx *ctx = libxl__gc_owner(gc);
+    struct xs_permissions *ptr;
+
+    ptr = xs_get_permissions(ctx->xsh, t, path, num);
+    libxl__ptr_add(gc, ptr);
+    return ptr;
+}
+
 char *libxl__xs_get_dompath(libxl__gc *gc, uint32_t domid)
 {
     libxl_ctx *ctx = libxl__gc_owner(gc);
-- 
1.8.1.4

Re: [PATCH v2 1/3] libxl: add libxl__xs_get_permissions helper

[Roger Pau Monné]

On 08/05/13 05:39, Marek Marczykowski wrote:
> 
> Signed-off-by: Marek Marczykowski <marmarek@invisiblethingslab.com>

Acked-by: Roger Pau Monné <roger.pau@citrix.com>

> ---
>  tools/libxl/libxl_internal.h |  2 ++
>  tools/libxl/libxl_xshelp.c   | 11 +++++++++++
>  2 files changed, 13 insertions(+)
> 
> diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
> index 573e3d1..2e9d9b5 100644
> --- a/tools/libxl/libxl_internal.h
> +++ b/tools/libxl/libxl_internal.h
> @@ -533,6 +533,8 @@ _hidden char *libxl__xs_get_dompath(libxl__gc *gc, uint32_t domid);
>  
>  _hidden char *libxl__xs_read(libxl__gc *gc, xs_transaction_t t,
>                               const char *path);
> +_hidden struct xs_permissions * libxl__xs_get_permissions(libxl__gc *gc,
> +                xs_transaction_t t, const char *path, unsigned int *num);
>  _hidden char **libxl__xs_directory(libxl__gc *gc, xs_transaction_t t,
>                                     const char *path, unsigned int *nb);
>     /* On error: returns NULL, sets errno (no logging) */
> diff --git a/tools/libxl/libxl_xshelp.c b/tools/libxl/libxl_xshelp.c
> index 52af484..48960de 100644
> --- a/tools/libxl/libxl_xshelp.c
> +++ b/tools/libxl/libxl_xshelp.c
> @@ -115,6 +115,17 @@ char * libxl__xs_read(libxl__gc *gc, xs_transaction_t t, const char *path)
>      return ptr;
>  }
>  
> +struct xs_permissions * libxl__xs_get_permissions(libxl__gc *gc,
> +        xs_transaction_t t, const char *path, unsigned int *num)
> +{
> +    libxl_ctx *ctx = libxl__gc_owner(gc);
> +    struct xs_permissions *ptr;
> +
> +    ptr = xs_get_permissions(ctx->xsh, t, path, num);
> +    libxl__ptr_add(gc, ptr);
> +    return ptr;
> +}
> +
>  char *libxl__xs_get_dompath(libxl__gc *gc, uint32_t domid)
>  {
>      libxl_ctx *ctx = libxl__gc_owner(gc);
> 

[PATCH v2 2/3] libxl: do not assume Dom0 backend while listing disks and nics

[Marek Marczykowski]

One more place where code assumed that all backends are in dom0. List
devices in domain device/ tree, instead of backend/ of dom0.
Additionally fix libxl_devid_to_device_{nic,disk} to fill backend_domid
properly.

Changes in v2:
 - restructure *_from_xs_be to *_from_xs_fe, which take care of getting
   backend path and backend domid (with libxl__get_backend_from_xs_fe
   helper)
 - sanity checks on frontend-controlled entries

Signed-off-by: Marek Marczykowski <marmarek@invisiblethingslab.com>
---
 tools/libxl/libxl.c | 162 ++++++++++++++++++++++++++++++++++++++++------------
 1 file changed, 127 insertions(+), 35 deletions(-)

diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
index 5b9a3df..cb4a2a8 100644
--- a/tools/libxl/libxl.c
+++ b/tools/libxl/libxl.c
@@ -1739,6 +1739,83 @@ static int libxl__resolve_domid(libxl__gc *gc, const char *name,
     return libxl_domain_qualifier_to_domid(CTX, name, domid);
 }
 
+/* Get backend path from frontend, with some sanity checks:
+ *  - if both points to each other
+ *  - if backend is owned by backend domid
+ *
+ * Returns backend domid or -1 on error. If no error reported, be_path will be
+ * filled with backend path.
+ */
+static int libxl__get_backend_from_xs_fe(libxl__gc *gc, const char *fe_path,
+        const char **be_path_out)
+{
+    int be_domid;
+    const char *be_domid_str;
+    const char *be_path;
+    const char *fe_path_from_be;
+    char *be_dompath;
+    char *be_base_path;
+    struct xs_permissions *be_perms;
+    unsigned int be_perms_count;
+
+    if (libxl__xs_read_checked(gc, XBT_NULL,
+                GCSPRINTF("%s/backend-id", fe_path), &be_domid_str)) {
+        LOG(ERROR, "Missing xenstore node %s/backend-id", fe_path);
+        return -1;
+    }
+    be_domid = strtoul(be_domid_str, NULL, 10);
+
+    if (libxl__xs_read_checked(gc, XBT_NULL,
+                GCSPRINTF("%s/backend", fe_path), &be_path)) {
+        LOG(ERROR, "Missing xenstore node %s/backend", fe_path);
+        return -1;
+    }
+
+    if (!(be_dompath = libxl__xs_get_dompath(gc, be_domid))) {
+        LOG(ERROR, "Failed to get dompath for domain %d", be_domid);
+        return -1;
+    }
+
+    be_base_path = GCSPRINTF("%s/backend/", be_dompath);
+    if (strncmp(be_path, be_base_path, strlen(be_base_path))) {
+        /* possible malicious frontend action */
+        LOG(ERROR, "Backend xenstore path %s not withing %s",
+                be_path,
+                be_base_path);
+        return -1;
+    }
+
+    be_perms = libxl__xs_get_permissions(gc, XBT_NULL, be_path, &be_perms_count);
+    if (!be_perms) {
+        LOG(ERROR, "Failed to get %s path permissions", be_path);
+        return -1;
+    }
+
+    if (be_perms[0].id != be_domid) {
+        /* possible malicious frontend action */
+        /* perhaps LIBXL_TOOLSTACK_DOMID should be also allowed? */
+        LOG(ERROR, "Backend path %s not owned by backend domain (owner: %d)", be_path, be_perms[0].id);
+        return -1;
+    }
+
+    if (libxl__xs_read_checked(gc, XBT_NULL,
+                GCSPRINTF("%s/frontend", be_path), &fe_path_from_be)) {
+        LOG(ERROR, "Missing xenstore node %s/frontend", be_path);
+        return -1;
+    }
+
+    if (strcmp(fe_path, fe_path_from_be)) {
+        /* possible malicious frontend action */
+        LOG(ERROR, "Frontend path from backend (%s) doesn't match original "
+                "frontend path (%s)", fe_path_from_be, fe_path);
+        return -1;
+    }
+
+    *be_path_out = be_path;
+
+    return be_domid;
+}
+
 /******************************************************************************/
 int libxl__device_vtpm_setdefault(libxl__gc *gc, libxl_device_vtpm *vtpm)
 {
@@ -2235,16 +2312,24 @@ void libxl__device_disk_add(libxl__egc *egc, uint32_t domid,
     device_disk_add(egc, domid, disk, aodev, NULL, NULL);
 }
 
-static int libxl__device_disk_from_xs_be(libxl__gc *gc,
-                                         const char *be_path,
+static int libxl__device_disk_from_xs_fe(libxl__gc *gc,
+                                         const char *fe_path,
                                          libxl_device_disk *disk)
 {
     libxl_ctx *ctx = libxl__gc_owner(gc);
     unsigned int len;
+    const char *be_path;
+    int be_domid;
     char *tmp;
 
     libxl_device_disk_init(disk);
 
+    be_domid = libxl__get_backend_from_xs_fe(gc, fe_path, &be_path);
+    if (be_domid < 0)
+        goto cleanup;
+
+    disk->backend_domid = be_domid;
+
     /* "params" may not be present; but everything else must be. */
     tmp = xs_read(ctx->xsh, XBT_NULL,
                   libxl__sprintf(gc, "%s/params", be_path), &len);
@@ -2322,13 +2407,13 @@ int libxl_vdev_to_device_disk(libxl_ctx *ctx, uint32_t domid,
     if (!dompath) {
         goto out;
     }
-    path = libxl__xs_read(gc, XBT_NULL,
-                          libxl__sprintf(gc, "%s/device/vbd/%d/backend",
-                                         dompath, devid));
+    path = libxl__sprintf(gc, "%s/device/vbd/%d",
+            dompath, devid);
     if (!path)
         goto out;
 
-    rc = libxl__device_disk_from_xs_be(gc, path, disk);
+    rc = libxl__device_disk_from_xs_fe(gc, path, disk);
+
 out:
     GC_FREE;
     return rc;
@@ -2341,17 +2426,17 @@ static int libxl__append_disk_list_of_type(libxl__gc *gc,
                                            libxl_device_disk **disks,
                                            int *ndisks)
 {
-    char *be_path = NULL;
+    char *fe_path = NULL;
     char **dir = NULL;
     unsigned int n = 0;
     libxl_device_disk *pdisk = NULL, *pdisk_end = NULL;
     int rc=0;
     int initial_disks = *ndisks;
 
-    be_path = libxl__sprintf(gc, "%s/backend/%s/%d",
-                             libxl__xs_get_dompath(gc, 0), type, domid);
-    dir = libxl__xs_directory(gc, XBT_NULL, be_path, &n);
-    if (dir) {
+    fe_path = libxl__sprintf(gc, "%s/device/%s",
+                             libxl__xs_get_dompath(gc, domid), type);
+    dir = libxl__xs_directory(gc, XBT_NULL, fe_path, &n);
+    if (dir && n) {
         libxl_device_disk *tmp;
         tmp = realloc(*disks, sizeof (libxl_device_disk) * (*ndisks + n));
         if (tmp == NULL)
@@ -2360,11 +2445,10 @@ static int libxl__append_disk_list_of_type(libxl__gc *gc,
         pdisk = *disks + initial_disks;
         pdisk_end = *disks + initial_disks + n;
         for (; pdisk < pdisk_end; pdisk++, dir++) {
-            const char *p;
-            p = libxl__sprintf(gc, "%s/%s", be_path, *dir);
-            if ((rc=libxl__device_disk_from_xs_be(gc, p, pdisk)))
+            rc = libxl__device_disk_from_xs_fe(gc,
+                    GCSPRINTF("%s/%s", fe_path, *dir), pdisk);
+            if (rc)
                 goto out;
-            pdisk->backend_domid = 0;
             *ndisks += 1;
         }
     }
@@ -2949,17 +3033,25 @@ out:
     return;
 }
 
-static void libxl__device_nic_from_xs_be(libxl__gc *gc,
-                                         const char *be_path,
+static int libxl__device_nic_from_xs_fe(libxl__gc *gc,
+                                         const char *fe_path,
                                          libxl_device_nic *nic)
 {
     libxl_ctx *ctx = libxl__gc_owner(gc);
     unsigned int len;
+    const char *be_path;
+    int be_domid;
     char *tmp;
     int rc;
 
+    be_domid = libxl__get_backend_from_xs_fe(gc, fe_path, &be_path);
+    if (be_domid < 0)
+        return ERROR_FAIL;
+
     libxl_device_nic_init(nic);
 
+    nic->backend_domid = be_domid;
+
     tmp = xs_read(ctx->xsh, XBT_NULL,
                   libxl__sprintf(gc, "%s/handle", be_path), &len);
     if ( tmp )
@@ -2988,6 +3080,8 @@ static void libxl__device_nic_from_xs_be(libxl__gc *gc,
     nic->nictype = LIBXL_NIC_TYPE_VIF;
     nic->model = NULL; /* XXX Only for TYPE_IOEMU */
     nic->ifname = NULL; /* XXX Only for TYPE_IOEMU */
+
+    return 0;
 }
 
 int libxl_devid_to_device_nic(libxl_ctx *ctx, uint32_t domid,
@@ -3002,15 +3096,11 @@ int libxl_devid_to_device_nic(libxl_ctx *ctx, uint32_t domid,
     if (!dompath)
         goto out;
 
-    path = libxl__xs_read(gc, XBT_NULL,
-                          libxl__sprintf(gc, "%s/device/vif/%d/backend",
-                                         dompath, devid));
+    path = libxl__sprintf(gc, "%s/device/vif/%d", dompath, devid);
     if (!path)
         goto out;
 
-    libxl__device_nic_from_xs_be(gc, path, nic);
-
-    rc = 0;
+    rc = libxl__device_nic_from_xs_fe(gc, path, nic);
 out:
     GC_FREE;
     return rc;
@@ -3022,31 +3112,33 @@ static int libxl__append_nic_list_of_type(libxl__gc *gc,
                                            libxl_device_nic **nics,
                                            int *nnics)
 {
-    char *be_path = NULL;
+    char *fe_path = NULL;
     char **dir = NULL;
     unsigned int n = 0;
     libxl_device_nic *pnic = NULL, *pnic_end = NULL;
+    int rc = 0;
 
-    be_path = libxl__sprintf(gc, "%s/backend/%s/%d",
-                             libxl__xs_get_dompath(gc, 0), type, domid);
-    dir = libxl__xs_directory(gc, XBT_NULL, be_path, &n);
-    if (dir) {
+    fe_path = libxl__sprintf(gc, "%s/device/%s",
+                             libxl__xs_get_dompath(gc, domid), type);
+    dir = libxl__xs_directory(gc, XBT_NULL, fe_path, &n);
+    if (dir && n) {
         libxl_device_nic *tmp;
         tmp = realloc(*nics, sizeof (libxl_device_nic) * (*nnics + n));
         if (tmp == NULL)
             return ERROR_NOMEM;
         *nics = tmp;
         pnic = *nics + *nnics;
-        *nnics += n;
-        pnic_end = *nics + *nnics;
+        pnic_end = *nics + *nnics + n;
         for (; pnic < pnic_end; pnic++, dir++) {
-            const char *p;
-            p = libxl__sprintf(gc, "%s/%s", be_path, *dir);
-            libxl__device_nic_from_xs_be(gc, p, pnic);
-            pnic->backend_domid = 0;
+            rc = libxl__device_nic_from_xs_fe(gc,
+                    GCSPRINTF("%s/%s", fe_path, *dir), pnic);
+            if (rc)
+                goto out;
+            *nnics += 1;
         }
     }
-    return 0;
+out:
+    return rc;
 }
 
 libxl_device_nic *libxl_device_nic_list(libxl_ctx *ctx, uint32_t domid, int *num)
-- 
1.8.1.4

Re: [PATCH v2 2/3] libxl: do not assume Dom0 backend while listing disks and nics

[Roger Pau Monné]

On 08/05/13 05:39, Marek Marczykowski wrote:
> One more place where code assumed that all backends are in dom0. List
> devices in domain device/ tree, instead of backend/ of dom0.
> Additionally fix libxl_devid_to_device_{nic,disk} to fill backend_domid
> properly.
> 
> Changes in v2:
>  - restructure *_from_xs_be to *_from_xs_fe, which take care of getting
>    backend path and backend domid (with libxl__get_backend_from_xs_fe
>    helper)
>  - sanity checks on frontend-controlled entries
> 
> Signed-off-by: Marek Marczykowski <marmarek@invisiblethingslab.com>
> ---
>  tools/libxl/libxl.c | 162 ++++++++++++++++++++++++++++++++++++++++------------
>  1 file changed, 127 insertions(+), 35 deletions(-)
> 
> diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
> index 5b9a3df..cb4a2a8 100644
> --- a/tools/libxl/libxl.c
> +++ b/tools/libxl/libxl.c
> @@ -1739,6 +1739,83 @@ static int libxl__resolve_domid(libxl__gc *gc, const char *name,
>      return libxl_domain_qualifier_to_domid(CTX, name, domid);
>  }
>  
> +/* Get backend path from frontend, with some sanity checks:
> + *  - if both points to each other
> + *  - if backend is owned by backend domid
> + *
> + * Returns backend domid or -1 on error. If no error reported, be_path will be
> + * filled with backend path.
> + */
> +static int libxl__get_backend_from_xs_fe(libxl__gc *gc, const char *fe_path,
> +        const char **be_path_out)
> +{
> +    int be_domid;
> +    const char *be_domid_str;
> +    const char *be_path;
> +    const char *fe_path_from_be;
> +    char *be_dompath;
> +    char *be_base_path;
> +    struct xs_permissions *be_perms;
> +    unsigned int be_perms_count;
> +
> +    if (libxl__xs_read_checked(gc, XBT_NULL,
> +                GCSPRINTF("%s/backend-id", fe_path), &be_domid_str)) {
> +        LOG(ERROR, "Missing xenstore node %s/backend-id", fe_path);
> +        return -1;

It will be better to propagate the error returned from
libxl__xs_read_checked:

rc = libxl__xs_read_checked(gc, XBT_NULL,
               GCSPRINTF("%s/backend-id", fe_path), &be_domid_str);
if (rc) {
    LOG(ERROR, "Missing xenstore node %s/backend-id", fe_path);
    return rc;
}

Or better, have a fail label that could be used when an error is detected:

rc = libxl__xs_read_checked(gc, XBT_NULL,
               GCSPRINTF("%s/backend-id", fe_path), &be_domid_str);
if (rc) {
    LOG(ERROR, "Missing xenstore node %s/backend-id", fe_path);
    goto fail;
}

...

    return be_domid;

fail:
    assert(rc);
    return rc;

Also, the -1s below should be replaced with ERROR_FAIL or some more
appropriate error value.

> +    }
> +    be_domid = strtoul(be_domid_str, NULL, 10);
> +
> +    if (libxl__xs_read_checked(gc, XBT_NULL,
> +                GCSPRINTF("%s/backend", fe_path), &be_path)) {
> +        LOG(ERROR, "Missing xenstore node %s/backend", fe_path);
> +        return -1;
> +    }
> +
> +    if (!(be_dompath = libxl__xs_get_dompath(gc, be_domid))) {
> +        LOG(ERROR, "Failed to get dompath for domain %d", be_domid);
> +        return -1;
> +    }
> +
> +    be_base_path = GCSPRINTF("%s/backend/", be_dompath);
> +    if (strncmp(be_path, be_base_path, strlen(be_base_path))) {
> +        /* possible malicious frontend action */
> +        LOG(ERROR, "Backend xenstore path %s not withing %s",
> +                be_path,
> +                be_base_path);
> +        return -1;
> +    }
> +
> +    be_perms = libxl__xs_get_permissions(gc, XBT_NULL, be_path, &be_perms_count);
> +    if (!be_perms) {
> +        LOG(ERROR, "Failed to get %s path permissions", be_path);
> +        return -1;
> +    }
> +
> +    if (be_perms[0].id != be_domid) {
> +        /* possible malicious frontend action */
> +        /* perhaps LIBXL_TOOLSTACK_DOMID should be also allowed? */
> +        LOG(ERROR, "Backend path %s not owned by backend domain (owner: %d)", be_path, be_perms[0].id);
> +        return -1;
> +    }
> +
> +    if (libxl__xs_read_checked(gc, XBT_NULL,
> +                GCSPRINTF("%s/frontend", be_path), &fe_path_from_be)) {
> +        LOG(ERROR, "Missing xenstore node %s/frontend", be_path);
> +        return -1;
> +    }
> +
> +    if (strcmp(fe_path, fe_path_from_be)) {
> +        /* possible malicious frontend action */
> +        LOG(ERROR, "Frontend path from backend (%s) doesn't match original "
> +                "frontend path (%s)", fe_path_from_be, fe_path);
> +        return -1;
> +    }
> +
> +    *be_path_out = be_path;
> +
> +    return be_domid;
> +}
> +
>  /******************************************************************************/
>  int libxl__device_vtpm_setdefault(libxl__gc *gc, libxl_device_vtpm *vtpm)
>  {
> @@ -2235,16 +2312,24 @@ void libxl__device_disk_add(libxl__egc *egc, uint32_t domid,
>      device_disk_add(egc, domid, disk, aodev, NULL, NULL);
>  }
>  
> -static int libxl__device_disk_from_xs_be(libxl__gc *gc,
> -                                         const char *be_path,
> +static int libxl__device_disk_from_xs_fe(libxl__gc *gc,
> +                                         const char *fe_path,
>                                           libxl_device_disk *disk)
>  {
>      libxl_ctx *ctx = libxl__gc_owner(gc);
>      unsigned int len;
> +    const char *be_path;
> +    int be_domid;
>      char *tmp;
>  
>      libxl_device_disk_init(disk);
>  
> +    be_domid = libxl__get_backend_from_xs_fe(gc, fe_path, &be_path);
> +    if (be_domid < 0)
> +        goto cleanup;
> +
> +    disk->backend_domid = be_domid;
> +
>      /* "params" may not be present; but everything else must be. */
>      tmp = xs_read(ctx->xsh, XBT_NULL,
>                    libxl__sprintf(gc, "%s/params", be_path), &len);
> @@ -2322,13 +2407,13 @@ int libxl_vdev_to_device_disk(libxl_ctx *ctx, uint32_t domid,
>      if (!dompath) {
>          goto out;
>      }
> -    path = libxl__xs_read(gc, XBT_NULL,
> -                          libxl__sprintf(gc, "%s/device/vbd/%d/backend",
> -                                         dompath, devid));
> +    path = libxl__sprintf(gc, "%s/device/vbd/%d",
> +            dompath, devid);

GCSPRINTF

>      if (!path)
>          goto out;
>  
> -    rc = libxl__device_disk_from_xs_be(gc, path, disk);
> +    rc = libxl__device_disk_from_xs_fe(gc, path, disk);
> +
>  out:
>      GC_FREE;
>      return rc;
> @@ -2341,17 +2426,17 @@ static int libxl__append_disk_list_of_type(libxl__gc *gc,
>                                             libxl_device_disk **disks,
>                                             int *ndisks)
>  {
> -    char *be_path = NULL;
> +    char *fe_path = NULL;
>      char **dir = NULL;
>      unsigned int n = 0;
>      libxl_device_disk *pdisk = NULL, *pdisk_end = NULL;
>      int rc=0;
>      int initial_disks = *ndisks;
>  
> -    be_path = libxl__sprintf(gc, "%s/backend/%s/%d",
> -                             libxl__xs_get_dompath(gc, 0), type, domid);
> -    dir = libxl__xs_directory(gc, XBT_NULL, be_path, &n);
> -    if (dir) {
> +    fe_path = libxl__sprintf(gc, "%s/device/%s",
> +                             libxl__xs_get_dompath(gc, domid), type);

GCSPRINTF

> +    dir = libxl__xs_directory(gc, XBT_NULL, fe_path, &n);
> +    if (dir && n) {
>          libxl_device_disk *tmp;
>          tmp = realloc(*disks, sizeof (libxl_device_disk) * (*ndisks + n));
>          if (tmp == NULL)
> @@ -2360,11 +2445,10 @@ static int libxl__append_disk_list_of_type(libxl__gc *gc,
>          pdisk = *disks + initial_disks;
>          pdisk_end = *disks + initial_disks + n;
>          for (; pdisk < pdisk_end; pdisk++, dir++) {
> -            const char *p;
> -            p = libxl__sprintf(gc, "%s/%s", be_path, *dir);
> -            if ((rc=libxl__device_disk_from_xs_be(gc, p, pdisk)))
> +            rc = libxl__device_disk_from_xs_fe(gc,
> +                    GCSPRINTF("%s/%s", fe_path, *dir), pdisk);
> +            if (rc)
>                  goto out;
> -            pdisk->backend_domid = 0;
>              *ndisks += 1;
>          }
>      }
> @@ -2949,17 +3033,25 @@ out:
>      return;
>  }
>  
> -static void libxl__device_nic_from_xs_be(libxl__gc *gc,
> -                                         const char *be_path,
> +static int libxl__device_nic_from_xs_fe(libxl__gc *gc,
> +                                         const char *fe_path,
>                                           libxl_device_nic *nic)
>  {
>      libxl_ctx *ctx = libxl__gc_owner(gc);
>      unsigned int len;
> +    const char *be_path;
> +    int be_domid;
>      char *tmp;
>      int rc;
>  
> +    be_domid = libxl__get_backend_from_xs_fe(gc, fe_path, &be_path);
> +    if (be_domid < 0)
> +        return ERROR_FAIL;
> +
>      libxl_device_nic_init(nic);
>  
> +    nic->backend_domid = be_domid;
> +
>      tmp = xs_read(ctx->xsh, XBT_NULL,
>                    libxl__sprintf(gc, "%s/handle", be_path), &len);
>      if ( tmp )
> @@ -2988,6 +3080,8 @@ static void libxl__device_nic_from_xs_be(libxl__gc *gc,
>      nic->nictype = LIBXL_NIC_TYPE_VIF;
>      nic->model = NULL; /* XXX Only for TYPE_IOEMU */
>      nic->ifname = NULL; /* XXX Only for TYPE_IOEMU */
> +
> +    return 0;
>  }
>  
>  int libxl_devid_to_device_nic(libxl_ctx *ctx, uint32_t domid,
> @@ -3002,15 +3096,11 @@ int libxl_devid_to_device_nic(libxl_ctx *ctx, uint32_t domid,
>      if (!dompath)
>          goto out;
>  
> -    path = libxl__xs_read(gc, XBT_NULL,
> -                          libxl__sprintf(gc, "%s/device/vif/%d/backend",
> -                                         dompath, devid));
> +    path = libxl__sprintf(gc, "%s/device/vif/%d", dompath, devid);

GCSPRINTF

>      if (!path)
>          goto out;
>  
> -    libxl__device_nic_from_xs_be(gc, path, nic);
> -
> -    rc = 0;
> +    rc = libxl__device_nic_from_xs_fe(gc, path, nic);
>  out:
>      GC_FREE;
>      return rc;
> @@ -3022,31 +3112,33 @@ static int libxl__append_nic_list_of_type(libxl__gc *gc,
>                                             libxl_device_nic **nics,
>                                             int *nnics)
>  {
> -    char *be_path = NULL;
> +    char *fe_path = NULL;
>      char **dir = NULL;
>      unsigned int n = 0;
>      libxl_device_nic *pnic = NULL, *pnic_end = NULL;
> +    int rc = 0;
>  
> -    be_path = libxl__sprintf(gc, "%s/backend/%s/%d",
> -                             libxl__xs_get_dompath(gc, 0), type, domid);
> -    dir = libxl__xs_directory(gc, XBT_NULL, be_path, &n);
> -    if (dir) {
> +    fe_path = libxl__sprintf(gc, "%s/device/%s",
> +                             libxl__xs_get_dompath(gc, domid), type);

GCSPRINTF

> +    dir = libxl__xs_directory(gc, XBT_NULL, fe_path, &n);
> +    if (dir && n) {
>          libxl_device_nic *tmp;
>          tmp = realloc(*nics, sizeof (libxl_device_nic) * (*nnics + n));
>          if (tmp == NULL)
>              return ERROR_NOMEM;
>          *nics = tmp;
>          pnic = *nics + *nnics;
> -        *nnics += n;
> -        pnic_end = *nics + *nnics;
> +        pnic_end = *nics + *nnics + n;
>          for (; pnic < pnic_end; pnic++, dir++) {
> -            const char *p;
> -            p = libxl__sprintf(gc, "%s/%s", be_path, *dir);
> -            libxl__device_nic_from_xs_be(gc, p, pnic);
> -            pnic->backend_domid = 0;
> +            rc = libxl__device_nic_from_xs_fe(gc,
> +                    GCSPRINTF("%s/%s", fe_path, *dir), pnic);
> +            if (rc)
> +                goto out;
> +            *nnics += 1;
>          }
>      }
> -    return 0;
> +out:
> +    return rc;
>  }
>  
>  libxl_device_nic *libxl_device_nic_list(libxl_ctx *ctx, uint32_t domid, int *num)
> 

[PATCH v2 3/3] libxl: reduce code duplication in libxl_device_vtpm_list

[Marek Marczykowski]

Use recently introduced helper libxl__get_backend_from_xs_fe to handle
backend path and domid. This also improve security as the helper
performs various checks on frontend controlled entries.

Signed-off-by: Marek Marczykowski <marmarek@invisiblethingslab.com>
---
 tools/libxl/libxl.c | 19 +++++++++++--------
 1 file changed, 11 insertions(+), 8 deletions(-)

diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
index cb4a2a8..1d53cac 100644
--- a/tools/libxl/libxl.c
+++ b/tools/libxl/libxl.c
@@ -1917,24 +1917,27 @@ libxl_device_vtpm *libxl_device_vtpm_list(libxl_ctx *ctx, uint32_t domid, int *n
 
     fe_path = libxl__sprintf(gc, "%s/device/vtpm", libxl__xs_get_dompath(gc, domid));
     dir = libxl__xs_directory(gc, XBT_NULL, fe_path, &ndirs);
-    if(dir) {
+    if(dir && ndirs) {
        vtpms = malloc(sizeof(*vtpms) * ndirs);
        libxl_device_vtpm* vtpm;
        libxl_device_vtpm* end = vtpms + ndirs;
        for(vtpm = vtpms; vtpm < end; ++vtpm, ++dir) {
           char* tmp;
-          const char* be_path = libxl__xs_read(gc, XBT_NULL,
-                GCSPRINTF("%s/%s/backend",
-                   fe_path, *dir));
+          const char* be_path;
+          int be_domid;
+
+          be_domid = libxl__get_backend_from_xs_fe(gc,
+                  GCSPRINTF("%s/%s", fe_path, *dir), &be_path);
+          if (be_domid < 0) {
+              free(vtpms);
+              return NULL;
+          }
 
           libxl_device_vtpm_init(vtpm);
 
           vtpm->devid = atoi(*dir);
 
-          tmp = libxl__xs_read(gc, XBT_NULL,
-                GCSPRINTF("%s/%s/backend-id",
-                   fe_path, *dir));
-          vtpm->backend_domid = atoi(tmp);
+          vtpm->backend_domid = be_domid;
 
           tmp = libxl__xs_read(gc, XBT_NULL, GCSPRINTF("%s/uuid", be_path));
           if (tmp) {
-- 
1.8.1.4

Re: [PATCH v2 0/3] Get devices list from frontend xenstore entries

[Ian Campbell]

On Wed, 2013-05-08 at 05:35 +0100, Marek Marczykowski wrote:
> To cover the case of non-dom0 backed devices. Version 2 enchanced based
> on suggestions from IanC and Roger.
> Also added one additional patch (3/3), which cover the same case for
> vtpm - it already had non-dom0 backend support, so just clean up the
> code.

Either this is 4.4 material or it needs an argument making for a freeze
exception, George CCd.

> 
> Marek Marczykowski (3):
>   libxl: add libxl__xs_get_permissions helper
>   libxl: do not assume Dom0 backend while listing disks and nics
>   libxl: reduce code duplication in libxl_device_vtpm_list
> 
>  tools/libxl/libxl.c          | 181 +++++++++++++++++++++++++++++++++----------
>  tools/libxl/libxl_internal.h |   2 +
>  tools/libxl/libxl_xshelp.c   |  11 +++
>  3 files changed, 151 insertions(+), 43 deletions(-)
>