[PATCH 0/2] Two unrelated fixes to libxl

[PATCH 0/2] Two unrelated fixes to libxl

[Marek Marczykowski]

1. One more patch to fix driver domains. This time hardcoded dom0.  With each
major Xen update, I need to fix a bunch of hardcoded dom0 as backend domain, or
some other code depending on backend in dom0 - this is really annoying.
Perhaps some regression tests for driver domains would be helpful?

2. Fix speling of "backend-id". I'm not sure about this - haven't checked vtpm
drivers, but hope this is really fix.

Marek Marczykowski (2):
  libxl: do not assume Dom0 backend while listing disks and nics
  libxl: fix spelling of "backend-id" for vtpm

 tools/libxl/libxl.c | 73 ++++++++++++++++++++++++++++++++++++++---------------
 1 file changed, 52 insertions(+), 21 deletions(-)

-- 
1.8.1.4

[PATCH 1/2] 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.

Signed-off-by: Marek Marczykowski <marmarek@invisiblethingslab.com>
---
 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);
+
 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);
     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;
             *ndisks += 1;
         }
     }
@@ -2991,7 +3007,7 @@ int libxl_devid_to_device_nic(libxl_ctx *ctx, uint32_t domid,
                               int devid, libxl_device_nic *nic)
 {
     GC_INIT(ctx);
-    char *dompath, *path;
+    char *dompath, *path, *backend_domid;
     int rc = ERROR_FAIL;
 
     libxl_device_nic_init(nic);
@@ -3007,6 +3023,12 @@ int libxl_devid_to_device_nic(libxl_ctx *ctx, uint32_t domid,
 
     libxl__device_nic_from_xs_be(gc, path, nic);
 
+    backend_domid = libxl__xs_read(gc, XBT_NULL,
+            libxl__sprintf(gc, "%s/device/vif/%d/backend-id",
+                dompath, devid));
+    if (backend_domid)
+        nic->backend_domid = atoi(backend_domid);
+
     rc = 0;
 out:
     GC_FREE;
@@ -3019,14 +3041,14 @@ 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;
 
-    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);
     if (dir) {
         libxl_device_nic *tmp;
         tmp = realloc(*nics, sizeof (libxl_device_nic) * (*nnics + n));
@@ -3034,13 +3056,22 @@ static int libxl__append_nic_list_of_type(libxl__gc *gc,
             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;
+            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;
+            libxl__device_nic_from_xs_be(gc, be_path, pnic);
+            be_domid = libxl__xs_read(gc, XBT_NULL,
+                    libxl__sprintf(gc, "%s/%s/backend-id", fe_path, *dir));
+            if (be_domid)
+                pnic->backend_domid = atoi(be_domid);
+            else
+                pnic->backend_domid = LIBXL_TOOLSTACK_DOMID;
+            *nnics += 1;
         }
     }
     return 0;
-- 
1.8.1.4

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

[Ian Campbell]

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@invisiblethingslab.com>
> ---
>  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.

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

[Marek Marczykowski]

[-- Attachment #1.1: Type: text/plain, Size: 3824 bytes --]

On 29.04.2013 10:48, Ian Campbell wrote:
> 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@invisiblethingslab.com>
>> ---
>>  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?

And perhaps with getting backend path from frontend device, which will save
one additional duplicated line of code. More on this below.

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

I'm afraid this is the case. Domains (both backend and frontend) can control
content of device directory, but not existence of directory itself (with
obvious exception of dom0 aka toolstack domain). So if we check if device
paths - both frontend and backend - points to each other, it should be
reasonably safe.

(...)

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

And checking if $be_path/frontend points back to the right device.

This means *_from_xs_be needs original frontend path (or at least frontend
domid), which means it should be really *_from_xs_fe. The general workflow
would be:
1. get backend device path
2. get backend domid
3. check if backend domid matches backend device path (enforcing
/local/domain/%d/backend scheme)
4. check if backend domid owns backend device path (redundant with previous one?)
5. check if backend/frontend entry points back to original frontend
6. proceed to original *_from_xs_be

Items 1-5 can be folded into common helper, called at the beginning of every
*_from_xs_fe function.

What do you think?

-- 
Best Regards,
Marek Marczykowski
Invisible Things Lab


[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 555 bytes --]

[-- Attachment #2: Type: text/plain, Size: 126 bytes --]

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

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

[Ian Campbell]

On Wed, 2013-05-08 at 00:56 +0100, Marek Marczykowski wrote:
> On 29.04.2013 10:48, Ian Campbell wrote:
> > 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@invisiblethingslab.com>
> >> ---
> >>  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?
> 
> And perhaps with getting backend path from frontend device, which will save
> one additional duplicated line of code. More on this below.
> 
> > 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).
> 
> I'm afraid this is the case. Domains (both backend and frontend) can control
> content of device directory, but not existence of directory itself (with
> obvious exception of dom0 aka toolstack domain). So if we check if device
> paths - both frontend and backend - points to each other, it should be
> reasonably safe.
> 
> (...)
> 
> > 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?
> 
> And checking if $be_path/frontend points back to the right device.
> 
> This means *_from_xs_be needs original frontend path (or at least frontend
> domid), which means it should be really *_from_xs_fe. The general workflow
> would be:
> 1. get backend device path
> 2. get backend domid
> 3. check if backend domid matches backend device path (enforcing
> /local/domain/%d/backend scheme)
> 4. check if backend domid owns backend device path (redundant with previous one?)
> 5. check if backend/frontend entry points back to original frontend
> 6. proceed to original *_from_xs_be
> 
> Items 1-5 can be folded into common helper, called at the beginning of every
> *_from_xs_fe function.
> 
> What do you think?

Sounds plausible.

WRT the redundancy of #4 -- I think it can't hurt to double check.

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

[Roger Pau Monné]

On 28/04/13 01:12, 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@invisiblethingslab.com>
> ---
>  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",

You can use GCSPRINTF and libxl__xs_read_checked here, we expect new
code to drop the use of the old helpers and instead use the new ones.

> +                dompath, devid));
> +    if (backend_domid)
> +        disk->backend_domid = atoi(backend_domid);
> +
>  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);
>      if (dir) {

I think this check should be:

if (dir && n)

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

No functional change, but I think is clearer to use:

rc = libxl__device_disk_from_xs_be(gc, be_path, pdisk);
if (rc) goto out;

>                  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;
>              *ndisks += 1;
>          }
>      }
> @@ -2991,7 +3007,7 @@ int libxl_devid_to_device_nic(libxl_ctx *ctx, uint32_t domid,
>                                int devid, libxl_device_nic *nic)
>  {
>      GC_INIT(ctx);
> -    char *dompath, *path;
> +    char *dompath, *path, *backend_domid;
>      int rc = ERROR_FAIL;
>  
>      libxl_device_nic_init(nic);
> @@ -3007,6 +3023,12 @@ int libxl_devid_to_device_nic(libxl_ctx *ctx, uint32_t domid,
>  
>      libxl__device_nic_from_xs_be(gc, path, nic);
>  
> +    backend_domid = libxl__xs_read(gc, XBT_NULL,
> +            libxl__sprintf(gc, "%s/device/vif/%d/backend-id",
> +                dompath, devid));
> +    if (backend_domid)
> +        nic->backend_domid = atoi(backend_domid);
> +
>      rc = 0;
>  out:
>      GC_FREE;
> @@ -3019,14 +3041,14 @@ 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;
>  
> -    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);
>      if (dir) {

if (dir && n)

>          libxl_device_nic *tmp;
>          tmp = realloc(*nics, sizeof (libxl_device_nic) * (*nnics + n));
> @@ -3034,13 +3056,22 @@ static int libxl__append_nic_list_of_type(libxl__gc *gc,
>              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;
> +            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;
> +            libxl__device_nic_from_xs_be(gc, be_path, pnic);
> +            be_domid = libxl__xs_read(gc, XBT_NULL,
> +                    libxl__sprintf(gc, "%s/%s/backend-id", fe_path, *dir));
> +            if (be_domid)
> +                pnic->backend_domid = atoi(be_domid);
> +            else
> +                pnic->backend_domid = LIBXL_TOOLSTACK_DOMID;
> +            *nnics += 1;
>          }
>      }
>      return 0;
> 

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

[Ian Jackson]

Marek Marczykowski writes ("[PATCH 1/2] libxl: do not assume Dom0 backend while listing disks and nics"):
> 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.

After this change, can a guest cause a backend to be leaked when the
domain is destroyed ?  If it deletes the contents of the frontend
directory in xenstore, I think the device will no longer show up in
the lists and so won't be deleted when the guest goes away.

Would iterating over all domains looking for backends for a particular
frontend domain work ?  That would allow a rogue guest to cause
entries to appear in the list of course, by pretending to be a
backend domain...

Ian.

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

[Ian Campbell]

On Wed, 2013-05-01 at 11:29 +0100, Ian Jackson wrote:
> Marek Marczykowski writes ("[PATCH 1/2] libxl: do not assume Dom0 backend while listing disks and nics"):
> > 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.
> 
> After this change, can a guest cause a backend to be leaked when the
> domain is destroyed ?  If it deletes the contents of the frontend
> directory in xenstore, I think the device will no longer show up in
> the lists and so won't be deleted when the guest goes away.

I would have hoped that XS perms on key nodes, like the backend link
would prevent this, but since the actual frontend directory is guest
writeable I rather expect we can't make this so.

> Would iterating over all domains looking for backends for a particular
> frontend domain work ?  That would allow a rogue guest to cause
> entries to appear in the list of course, by pretending to be a
> backend domain...

Or should libxl keep a shadow list of devices for the domain in its
private xs directory?

Ian.

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

[Marek Marczykowski]

[-- Attachment #1.1: Type: text/plain, Size: 2091 bytes --]

On 01.05.2013 13:43, Ian Campbell wrote:
> On Wed, 2013-05-01 at 11:29 +0100, Ian Jackson wrote:
>> Marek Marczykowski writes ("[PATCH 1/2] libxl: do not assume Dom0 backend while listing disks and nics"):
>>> 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.
>>
>> After this change, can a guest cause a backend to be leaked when the
>> domain is destroyed ?  If it deletes the contents of the frontend
>> directory in xenstore, I think the device will no longer show up in
>> the lists and so won't be deleted when the guest goes away.
> 
> I would have hoped that XS perms on key nodes, like the backend link
> would prevent this, but since the actual frontend directory is guest
> writeable I rather expect we can't make this so.
> 
>> Would iterating over all domains looking for backends for a particular
>> frontend domain work ?  That would allow a rogue guest to cause
>> entries to appear in the list of course, by pretending to be a
>> backend domain...
> 
> Or should libxl keep a shadow list of devices for the domain in its
> private xs directory?

IMHO listing frontend "device/$TYPE" entries is sufficient compromise.
Downsides:
1. rogue frontend domain will be able to make leak backend xenstore entries
Upsides:
1. all devices will be listed/cleaned up on destroy, not only those
dom0-backed (assuming no downside "1" occurred)
2. will not introduce additional complexity (either scanning all backends, or
keeping *in sync* additional shadow copy of devices)

The current state (without this patch) will always miss all non-dom0 backed
devices, not only in case of rogue domain. Additionally IMHO possible leak
(downside 1) is bearable b/c backend driver watches frontend "state" entry and
if it disappear - will clean up the device. So the leak is only xenstore
entries, not any real device.

-- 
Best Regards,
Marek Marczykowski
Invisible Things Lab


[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 555 bytes --]

[-- Attachment #2: Type: text/plain, Size: 126 bytes --]

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

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

[Marek Marczykowski]

[-- Attachment #1.1: Type: text/plain, Size: 1464 bytes --]

On 01.05.2013 12:29, Ian Jackson wrote:
> Marek Marczykowski writes ("[PATCH 1/2] libxl: do not assume Dom0 backend while listing disks and nics"):
>> 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.
> 
> After this change, can a guest cause a backend to be leaked when the
> domain is destroyed ?  If it deletes the contents of the frontend
> directory in xenstore, I think the device will no longer show up in
> the lists and so won't be deleted when the guest goes away.

Which is currently the problem for every non-dom0 backend, even without
malicious domain action.
Currently I've some python script which watch xenstore and remove leftover
backends...

> Would iterating over all domains looking for backends for a particular
> frontend domain work ?  That would allow a rogue guest to cause
> entries to appear in the list of course, by pretending to be a
> backend domain...

Perhaps frontend domain shouldn't have permissions to remove device directory,
only modify some of entries, like state, feature-* etc. Does xenstore support
something like:
1. allow creating new entries and modify some existing
2. disallow modify and/or remove some entries, in the same directory
?

-- 
Best Regards / Pozdrawiam,
Marek Marczykowski
Invisible Things Lab


[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 555 bytes --]

[-- Attachment #2: Type: text/plain, Size: 126 bytes --]

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

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

[Ian Campbell]

On Wed, 2013-05-01 at 21:52 +0100, Marek Marczykowski wrote:
> On 01.05.2013 12:29, Ian Jackson wrote:
> > Marek Marczykowski writes ("[PATCH 1/2] libxl: do not assume Dom0 backend while listing disks and nics"):
> >> 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.
> > 
> > After this change, can a guest cause a backend to be leaked when the
> > domain is destroyed ?  If it deletes the contents of the frontend
> > directory in xenstore, I think the device will no longer show up in
> > the lists and so won't be deleted when the guest goes away.
> 
> Which is currently the problem for every non-dom0 backend, even without
> malicious domain action.
> Currently I've some python script which watch xenstore and remove leftover
> backends...
> 
> > Would iterating over all domains looking for backends for a particular
> > frontend domain work ?  That would allow a rogue guest to cause
> > entries to appear in the list of course, by pretending to be a
> > backend domain...
> 
> Perhaps frontend domain shouldn't have permissions to remove device directory,
> only modify some of entries, like state, feature-* etc. Does xenstore support
> something like:
> 1. allow creating new entries and modify some existing
> 2. disallow modify and/or remove some entries, in the same directory

I'm reasonably certain that in order to enable #1 you cannot have #2 (or
vice versa), since create/remove permissions is tied to the perms of the
containing directory. Or at least I think so, but I do know that XS
perms are a bit quirky. You could have a play with xenstore-chmod though
and see what you can see.

http://wiki.xen.org/wiki/XenBus#Permissions seems to be the best (AKA
only!) reference for the Xenbus permissions model I can find.

Ian.

[PATCH 2/2] libxl: fix spelling of "backend-id" for vtpm

[Marek Marczykowski]

Signed-off-by: Marek Marczykowski <marmarek@invisiblethingslab.com>
---
 tools/libxl/libxl.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
index e2c678a..7d0cd10 100644
--- a/tools/libxl/libxl.c
+++ b/tools/libxl/libxl.c
@@ -1855,7 +1855,7 @@ libxl_device_vtpm *libxl_device_vtpm_list(libxl_ctx *ctx, uint32_t domid, int *n
           vtpm->devid = atoi(*dir);
 
           tmp = libxl__xs_read(gc, XBT_NULL,
-                GCSPRINTF("%s/%s/backend_id",
+                GCSPRINTF("%s/%s/backend-id",
                    fe_path, *dir));
           vtpm->backend_domid = atoi(tmp);
 
-- 
1.8.1.4

Re: [PATCH 2/2] libxl: fix spelling of "backend-id" for vtpm

[Ian Campbell]

On Sun, 2013-04-28 at 00:17 +0100, Marek Marczykowski wrote:

Adding Daniel. We need to confirm that this doesn't break compatibility
with existing frontends. If it does then we just have to live with it
IMHO.

> Signed-off-by: Marek Marczykowski <marmarek@invisiblethingslab.com>
> ---
>  tools/libxl/libxl.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
> index e2c678a..7d0cd10 100644
> --- a/tools/libxl/libxl.c
> +++ b/tools/libxl/libxl.c
> @@ -1855,7 +1855,7 @@ libxl_device_vtpm *libxl_device_vtpm_list(libxl_ctx *ctx, uint32_t domid, int *n
>            vtpm->devid = atoi(*dir);
>  
>            tmp = libxl__xs_read(gc, XBT_NULL,
> -                GCSPRINTF("%s/%s/backend_id",
> +                GCSPRINTF("%s/%s/backend-id",
>                     fe_path, *dir));
>            vtpm->backend_domid = atoi(tmp);
>  

Re: [PATCH 2/2] libxl: fix spelling of "backend-id" for vtpm

[Daniel De Graaf]

On 04/29/2013 04:40 AM, Ian Campbell wrote:
> On Sun, 2013-04-28 at 00:17 +0100, Marek Marczykowski wrote:
>
> Adding Daniel. We need to confirm that this doesn't break compatibility
> with existing frontends. If it does then we just have to live with it
> IMHO.

None of the frontends I can find rely on backend_id; in fact, they all rely
on "backend-id" so this is a bug. The code in question is only reading, so
it also couldn't have introduced bad frontend code elsewhere.

>> Signed-off-by: Marek Marczykowski <marmarek@invisiblethingslab.com>
>> ---
>>   tools/libxl/libxl.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
>> index e2c678a..7d0cd10 100644
>> --- a/tools/libxl/libxl.c
>> +++ b/tools/libxl/libxl.c
>> @@ -1855,7 +1855,7 @@ libxl_device_vtpm *libxl_device_vtpm_list(libxl_ctx *ctx, uint32_t domid, int *n
>>             vtpm->devid = atoi(*dir);
>>
>>             tmp = libxl__xs_read(gc, XBT_NULL,
>> -                GCSPRINTF("%s/%s/backend_id",
>> +                GCSPRINTF("%s/%s/backend-id",
>>                      fe_path, *dir));
>>             vtpm->backend_domid = atoi(tmp);
>>

Re: [PATCH 2/2] libxl: fix spelling of "backend-id" for vtpm

[Ian Campbell]

On Mon, 2013-04-29 at 14:26 +0100, Daniel De Graaf wrote:
> On 04/29/2013 04:40 AM, Ian Campbell wrote:
> > On Sun, 2013-04-28 at 00:17 +0100, Marek Marczykowski wrote:
> >
> > Adding Daniel. We need to confirm that this doesn't break compatibility
> > with existing frontends. If it does then we just have to live with it
> > IMHO.
> 
> None of the frontends I can find rely on backend_id; in fact, they all rely
> on "backend-id" so this is a bug.

Thanks for confirming.

>  The code in question is only reading, so
> it also couldn't have introduced bad frontend code elsewhere.

Oh, yeah, I failed to notice that!

Does this change have your Ack/Reviewed-by then?

Ian.

Re: [PATCH 2/2] libxl: fix spelling of "backend-id" for vtpm

[Ian Campbell]

On Mon, 2013-04-29 at 14:51 +0100, Ian Campbell wrote:
> On Mon, 2013-04-29 at 14:26 +0100, Daniel De Graaf wrote:
> > On 04/29/2013 04:40 AM, Ian Campbell wrote:
> > > On Sun, 2013-04-28 at 00:17 +0100, Marek Marczykowski wrote:
> > >
> > > Adding Daniel. We need to confirm that this doesn't break compatibility
> > > with existing frontends. If it does then we just have to live with it
> > > IMHO.
> > 
> > None of the frontends I can find rely on backend_id; in fact, they all rely
> > on "backend-id" so this is a bug.
> 
> Thanks for confirming.
> 
> >  The code in question is only reading, so
> > it also couldn't have introduced bad frontend code elsewhere.
> 
> Oh, yeah, I failed to notice that!
> 
> Does this change have your Ack/Reviewed-by then?

I've acked it myself and applied.

Ian.