Re: [Xen-devel] [PATCH 1/2] libxl: do not assume Dom0 backend while listing disks and nics

[Ian Campbell <Ian.Campbell@xxxxxxxxxx>]

On Sun, 2013-04-28 at 00:12 +0100, 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.
> 
> Signed-off-by: Marek Marczykowski <marmarek@xxxxxxxxxxxxxxxxxxxxxx>
> ---
>  tools/libxl/libxl.c | 71 
> ++++++++++++++++++++++++++++++++++++++---------------
>  1 file changed, 51 insertions(+), 20 deletions(-)
> 
> diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
> index c386004..e2c678a 100644
> --- a/tools/libxl/libxl.c
> +++ b/tools/libxl/libxl.c
> @@ -2308,7 +2308,7 @@ int libxl_vdev_to_device_disk(libxl_ctx *ctx, uint32_t 
> domid,
>                                const char *vdev, libxl_device_disk *disk)
>  {
>      GC_INIT(ctx);
> -    char *dompath, *path;
> +    char *dompath, *path, *backend_domid;
>      int devid = libxl__device_disk_dev_number(vdev, NULL, NULL);
>      int rc = ERROR_FAIL;
>  
> @@ -2328,6 +2328,13 @@ int libxl_vdev_to_device_disk(libxl_ctx *ctx, uint32_t 
> domid,
>          goto out;
>  
>      rc = libxl__device_disk_from_xs_be(gc, path, disk);
> +
> +    backend_domid = libxl__xs_read(gc, XBT_NULL,
> +            libxl__sprintf(gc, "%s/device/vbd/%d/backend-id",
> +                dompath, devid));
> +    if (backend_domid)
> +        disk->backend_domid = atoi(backend_domid);

I think this should be folded into ..._from_xs_be, either by parsing the
path argument or by doing the appropriate lookup via the frontend-path
node inside the function. Since I guess this will be common to both NIC
and disk perhaps a helper for from_xs_be to call would be appropriate?

In general we prefer to avoid relying on frontend owned state (makes it
easier to reason about the safety if we just don't do it, although
obviously we sometimes have to, and this may be one of those cases).

>  out:
>      GC_FREE;
>      return rc;
> @@ -2340,16 +2347,16 @@ 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);
> +    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);

Can't really avoid relying on the f.e. path here.

>      if (dir) {
>          libxl_device_disk *tmp;
>          tmp = realloc(*disks, sizeof (libxl_device_disk) * (*ndisks + n));
> @@ -2359,11 +2366,20 @@ 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)))
> +            const char *be_domid;
> +            const char *be_path;
> +            be_path = libxl__xs_read(gc, XBT_NULL,
> +                    libxl__sprintf(gc, "%s/%s/backend", fe_path, *dir));
> +            if (!be_path)
> +                return ERROR_FAIL;
> +            if ((rc=libxl__device_disk_from_xs_be(gc, be_path, pdisk)))
>                  goto out;
> -            pdisk->backend_domid = 0;
> +            be_domid = libxl__xs_read(gc, XBT_NULL,
> +                    libxl__sprintf(gc, "%s/%s/backend-id", fe_path, *dir));
> +            if (be_domid)
> +                pdisk->backend_domid = atoi(be_domid);
> +            else
> +                pdisk->backend_domid = LIBXL_TOOLSTACK_DOMID;

If you push this down into from_xs_be then you avoid duplicating this
logic.

NB have you checked that from_xs_be is robust against a potentially
malicious backend path, since the frontend now controls where it goes
and could redirect it to a directory which it controls?

Simple things like allowing for missing keys which "must" be there. I
wonder if an owner + permissions check on the backend directory would be
a good idea, i.e. parse the path to get the backend id and insist that
it owns the directory?

>              *ndisks += 1;
>          }
>      }
> @@ -2991,7 +3007,7 @@ int libxl_devid_to_device_nic(libxl_ctx *ctx, uint32_t 
> domid,
>                                int devid, libxl_device_nic *nic)

Everything I said about disks applies to the NIC case too.

Ian.



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