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

["Roger Pau Monné" <roger.pau@citrix.com>]

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)
>