[PATCH 0/2] libxl: fixes for driver domains

[PATCH 0/2] libxl: fixes for driver domains

[Roger Pau Monne]

This small series contains fixes for block-list and block-detach when 
the backend of the disk is running on a driver domain.

[PATCH 1/2] libxl: correctly list disks served by driver domains in block-list

[Roger Pau Monne]

The block-list command was not able to lists disks with backends
running on domains different than Dom0, because it was only looking on
the backend xenstore path of Dom0. Fix this by instead fetching the
disks from the DomU xenstore entries.

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
Reported-by: G.R. <firemeteor@users.sourceforge.net>
Cc: Ian Jackson <Ian.Jackson@eu.citrix.com>
Cc: Ian Campbell <ian.campbell@citrix.com>
---
 tools/libxl/libxl.c |   72 +++++++++++++++++---------------------------------
 1 files changed, 25 insertions(+), 47 deletions(-)

diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
index 81785df..4aa2fe3 100644
--- a/tools/libxl/libxl.c
+++ b/tools/libxl/libxl.c
@@ -2324,61 +2324,39 @@ out:
     return rc;
 }
 
-
-static int libxl__append_disk_list_of_type(libxl__gc *gc,
-                                           uint32_t domid,
-                                           const char *type,
-                                           libxl_device_disk **disks,
-                                           int *ndisks)
-{
-    char *be_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) {
-        libxl_device_disk *tmp;
-        tmp = realloc(*disks, sizeof (libxl_device_disk) * (*ndisks + n));
-        if (tmp == NULL)
-            return ERROR_NOMEM;
-        *disks = tmp;
-        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)))
-                goto out;
-            pdisk->backend_domid = 0;
-            *ndisks += 1;
-        }
-    }
-out:
-    return rc;
-}
-
 libxl_device_disk *libxl_device_disk_list(libxl_ctx *ctx, uint32_t domid, int *num)
 {
     GC_INIT(ctx);
     libxl_device_disk *disks = NULL;
-    int rc;
+    int rc, i;
+    unsigned int xs_num;
+    char *fe_path, **devs;
+    const char *be_path;
 
     *num = 0;
 
-    rc = libxl__append_disk_list_of_type(gc, domid, "vbd", &disks, num);
-    if (rc) goto out_err;
-
-    rc = libxl__append_disk_list_of_type(gc, domid, "tap", &disks, num);
-    if (rc) goto out_err;
-
-    rc = libxl__append_disk_list_of_type(gc, domid, "qdisk", &disks, num);
-    if (rc) goto out_err;
+    fe_path = libxl__sprintf(gc, "/local/domain/%d/device/vbd", domid);
+    devs = libxl__xs_directory(gc, XBT_NULL, fe_path, &xs_num);
+    if (!devs)
+        /* Domain has no disks */
+        goto out;
+    disks = libxl__calloc(NOGC, xs_num, sizeof(*disks));
+    if (!disks)
+        goto out_err;
+    for (i = 0; i < xs_num; i++) {
+        fe_path = GCSPRINTF("/local/domain/%d/device/vbd/%s/backend",
+                            domid, devs[i]);
+        rc = libxl__xs_read_checked(gc, XBT_NULL, fe_path, &be_path);
+        if (rc)
+            goto out_err;
+        rc = libxl__device_disk_from_xs_be(gc, be_path, &disks[*num]);
+        if (rc)
+            goto out_err;
+        (*num)++;
+        assert(*num <= xs_num);
+    }
 
+out:
     GC_FREE;
     return disks;
 
-- 
1.7.7.5 (Apple Git-26)


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

[PATCH 2/2] libxl: fix libxl__device_disk_from_xs_be to parse backend domid

[Roger Pau Monne]

libxl__device_disk_from_xs_be was ignoring the backend domid, setting
it to 0 by default. Fix this by parsing the backend disk path in order
to fetch the backend domid.

This fixes the issue reported when trying to block-detach disks that
have it's backend on a driver domain.

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
Reported-by: G.R. <firemeteor@users.sourceforge.net>
Cc: Ian Jackson <Ian.Jackson@eu.citrix.com>
Cc: Ian Campbell <ian.campbell@citrix.com>
---
 tools/libxl/libxl.c |    7 +++++++
 1 files changed, 7 insertions(+), 0 deletions(-)

diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
index 4aa2fe3..ada9d38 100644
--- a/tools/libxl/libxl.c
+++ b/tools/libxl/libxl.c
@@ -2232,9 +2232,16 @@ static int libxl__device_disk_from_xs_be(libxl__gc *gc,
     libxl_ctx *ctx = libxl__gc_owner(gc);
     unsigned int len;
     char *tmp;
+    int rc;
 
     libxl_device_disk_init(disk);
 
+    rc = sscanf(be_path, "/local/domain/%d/", &disk->backend_domid);
+    if (rc != 1) {
+        LOG(ERROR, "Unable to fetch device backend domid from %s", be_path);
+        goto cleanup;
+    }
+
     /* "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);
-- 
1.7.7.5 (Apple Git-26)


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

Re: [PATCH 1/2] libxl: correctly list disks served by driver domains in block-list

[Ian Campbell]

On Fri, 2013-09-06 at 12:36 +0200, Roger Pau Monne wrote:
> The block-list command was not able to lists disks with backends
> running on domains different than Dom0, because it was only looking on
> the backend xenstore path of Dom0. Fix this by instead fetching the
> disks from the DomU xenstore entries.

Need to be a bit careful here about reading from potentially guest
controllable keys. This should mostly be a question of permissions.

> +    fe_path = libxl__sprintf(gc, "/local/domain/%d/device/vbd", domid);

Are guests able to create new subdirectories under here?

> +    devs = libxl__xs_directory(gc, XBT_NULL, fe_path, &xs_num);
> +    if (!devs)
> +        /* Domain has no disks */
> +        goto out;
> +    disks = libxl__calloc(NOGC, xs_num, sizeof(*disks));
> +    if (!disks)
> +        goto out_err;
> +    for (i = 0; i < xs_num; i++) {
> +        fe_path = GCSPRINTF("/local/domain/%d/device/vbd/%s/backend",
> +                            domid, devs[i]);

Is this path writeable by the guest? The containing directory is I
think, so this needs to include delete and recreate type situations
(although ISTR xenstore not having the posix like semantics here).

If the guest can remove and recreate then we should check the current
owner of the key is e.g. the toolstack domain or whoever should be
trusted to won the key, within the same transaction as the read itself.


> +        rc = libxl__xs_read_checked(gc, XBT_NULL, fe_path, &be_path);
> +        if (rc)
> +            goto out_err;
> +        rc = libxl__device_disk_from_xs_be(gc, be_path, &disks[*num]);
> +        if (rc)
> +            goto out_err;
> +        (*num)++;
> +        assert(*num <= xs_num);
> +    }
>  
> +out:
>      GC_FREE;
>      return disks;
>  

Re: [PATCH 2/2] libxl: fix libxl__device_disk_from_xs_be to parse backend domid

[Ian Campbell]

On Fri, 2013-09-06 at 12:36 +0200, Roger Pau Monne wrote:
> libxl__device_disk_from_xs_be was ignoring the backend domid, setting
> it to 0 by default. Fix this by parsing the backend disk path in order
> to fetch the backend domid.
> 
> This fixes the issue reported when trying to block-detach disks that
> have it's backend on a driver domain.
> 
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> Reported-by: G.R. <firemeteor@users.sourceforge.net>
> Cc: Ian Jackson <Ian.Jackson@eu.citrix.com>
> Cc: Ian Campbell <ian.campbell@citrix.com>

It's a bit lame that there's no better way but this is indeed what
people (including frontends) do, so:
Acked-by: Ian Campbell <ian.campbell@citrix.com>

I'm pretty sure this can go in without patch 1/2 being in yet, but
please confirm.

> ---
>  tools/libxl/libxl.c |    7 +++++++
>  1 files changed, 7 insertions(+), 0 deletions(-)
> 
> diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
> index 4aa2fe3..ada9d38 100644
> --- a/tools/libxl/libxl.c
> +++ b/tools/libxl/libxl.c
> @@ -2232,9 +2232,16 @@ static int libxl__device_disk_from_xs_be(libxl__gc *gc,
>      libxl_ctx *ctx = libxl__gc_owner(gc);
>      unsigned int len;
>      char *tmp;
> +    int rc;
>  
>      libxl_device_disk_init(disk);
>  
> +    rc = sscanf(be_path, "/local/domain/%d/", &disk->backend_domid);
> +    if (rc != 1) {
> +        LOG(ERROR, "Unable to fetch device backend domid from %s", be_path);
> +        goto cleanup;
> +    }
> +
>      /* "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);



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

Re: [PATCH 1/2] libxl: correctly list disks served by driver domains in block-list

[Roger Pau Monné]

On 10/09/13 12:11, Ian Campbell wrote:
> On Fri, 2013-09-06 at 12:36 +0200, Roger Pau Monne wrote:
>> The block-list command was not able to lists disks with backends
>> running on domains different than Dom0, because it was only looking on
>> the backend xenstore path of Dom0. Fix this by instead fetching the
>> disks from the DomU xenstore entries.
> 
> Need to be a bit careful here about reading from potentially guest
> controllable keys. This should mostly be a question of permissions.
> 
>> +    fe_path = libxl__sprintf(gc, "/local/domain/%d/device/vbd", domid);
> 
> Are guests able to create new subdirectories under here?

Yes

> 
>> +    devs = libxl__xs_directory(gc, XBT_NULL, fe_path, &xs_num);
>> +    if (!devs)
>> +        /* Domain has no disks */
>> +        goto out;
>> +    disks = libxl__calloc(NOGC, xs_num, sizeof(*disks));
>> +    if (!disks)
>> +        goto out_err;
>> +    for (i = 0; i < xs_num; i++) {
>> +        fe_path = GCSPRINTF("/local/domain/%d/device/vbd/%s/backend",
>> +                            domid, devs[i]);
> 
> Is this path writeable by the guest? The containing directory is I
> think, so this needs to include delete and recreate type situations
> (although ISTR xenstore not having the posix like semantics here).

Yes, the whole directory including the backend entry is writeable by the
guest.

> 
> If the guest can remove and recreate then we should check the current
> owner of the key is e.g. the toolstack domain or whoever should be
> trusted to won the key, within the same transaction as the read itself.

I'm sorry but I'm not following you here, I assume you are speaking
about the frontend node that points to the backend ie:

/local/domain/<domid>/device/vbd/<devid>/backend

Permissions on this node are:

domid: <domid> perms: 0
domid: 0 perms: 1

If the guest changes this node, or recreates it permissions will still
be the same.

Re: [PATCH 2/2] libxl: fix libxl__device_disk_from_xs_be to parse backend domid

[Roger Pau Monné]

On 10/09/13 12:14, Ian Campbell wrote:
> On Fri, 2013-09-06 at 12:36 +0200, Roger Pau Monne wrote:
>> libxl__device_disk_from_xs_be was ignoring the backend domid, setting
>> it to 0 by default. Fix this by parsing the backend disk path in order
>> to fetch the backend domid.
>>
>> This fixes the issue reported when trying to block-detach disks that
>> have it's backend on a driver domain.
>>
>> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
>> Reported-by: G.R. <firemeteor@users.sourceforge.net>
>> Cc: Ian Jackson <Ian.Jackson@eu.citrix.com>
>> Cc: Ian Campbell <ian.campbell@citrix.com>
> 
> It's a bit lame that there's no better way but this is indeed what
> people (including frontends) do, so:
> Acked-by: Ian Campbell <ian.campbell@citrix.com>
> 
> I'm pretty sure this can go in without patch 1/2 being in yet, but
> please confirm.

Indeed, it can go in without 1 being committed.


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

Re: [PATCH 1/2] libxl: correctly list disks served by driver domains in block-list

[Ian Campbell]

On Tue, 2013-09-10 at 14:06 +0200, Roger Pau Monné wrote:
> On 10/09/13 12:11, Ian Campbell wrote:
> > On Fri, 2013-09-06 at 12:36 +0200, Roger Pau Monne wrote:
> >> The block-list command was not able to lists disks with backends
> >> running on domains different than Dom0, because it was only looking on
> >> the backend xenstore path of Dom0. Fix this by instead fetching the
> >> disks from the DomU xenstore entries.
> > 
> > Need to be a bit careful here about reading from potentially guest
> > controllable keys. This should mostly be a question of permissions.
> > 
> >> +    fe_path = libxl__sprintf(gc, "/local/domain/%d/device/vbd", domid);
> > 
> > Are guests able to create new subdirectories under here?
> 
> Yes
> 
> > 
> >> +    devs = libxl__xs_directory(gc, XBT_NULL, fe_path, &xs_num);
> >> +    if (!devs)
> >> +        /* Domain has no disks */
> >> +        goto out;
> >> +    disks = libxl__calloc(NOGC, xs_num, sizeof(*disks));
> >> +    if (!disks)
> >> +        goto out_err;
> >> +    for (i = 0; i < xs_num; i++) {
> >> +        fe_path = GCSPRINTF("/local/domain/%d/device/vbd/%s/backend",
> >> +                            domid, devs[i]);
> > 
> > Is this path writeable by the guest? The containing directory is I
> > think, so this needs to include delete and recreate type situations
> > (although ISTR xenstore not having the posix like semantics here).
> 
> Yes, the whole directory including the backend entry is writeable by the
> guest.
> 
> > 
> > If the guest can remove and recreate then we should check the current
> > owner of the key is e.g. the toolstack domain or whoever should be
> > trusted to won the key, within the same transaction as the read itself.
> 
> I'm sorry but I'm not following you here, I assume you are speaking
> about the frontend node that points to the backend ie:
> 
> /local/domain/<domid>/device/vbd/<devid>/backend
> 
> Permissions on this node are:
> 
> domid: <domid> perms: 0
> domid: 0 perms: 1

What are perms 0 and 1? We normally use r/w/b in xenstore speak.

> If the guest changes this node, or recreates it permissions will still
> be the same.

I was hoping we might be able to spot this case because the permissions
would have changed.

So the backend path in the frontend dir might point somewhere which is
maliciously controlled by the guest, or by another guest, something
which I don't think libxl_foo_from_xs_be is prepared to deal with. We
need to mitigate this somehow. Can we tighten the permissions
on /local/domain/<domid>/device/vbd/<devid>/backend such that only the
toolstack domain can fiddle with it?

Ian



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

Re: [PATCH 1/2] libxl: correctly list disks served by driver domains in block-list

[Roger Pau Monné]

On 10/09/13 14:54, Ian Campbell wrote:
> On Tue, 2013-09-10 at 14:06 +0200, Roger Pau Monné wrote:
>> On 10/09/13 12:11, Ian Campbell wrote:
>>> On Fri, 2013-09-06 at 12:36 +0200, Roger Pau Monne wrote:
>>>> The block-list command was not able to lists disks with backends
>>>> running on domains different than Dom0, because it was only looking on
>>>> the backend xenstore path of Dom0. Fix this by instead fetching the
>>>> disks from the DomU xenstore entries.
>>>
>>> Need to be a bit careful here about reading from potentially guest
>>> controllable keys. This should mostly be a question of permissions.
>>>
>>>> +    fe_path = libxl__sprintf(gc, "/local/domain/%d/device/vbd", domid);
>>>
>>> Are guests able to create new subdirectories under here?
>>
>> Yes
>>
>>>
>>>> +    devs = libxl__xs_directory(gc, XBT_NULL, fe_path, &xs_num);
>>>> +    if (!devs)
>>>> +        /* Domain has no disks */
>>>> +        goto out;
>>>> +    disks = libxl__calloc(NOGC, xs_num, sizeof(*disks));
>>>> +    if (!disks)
>>>> +        goto out_err;
>>>> +    for (i = 0; i < xs_num; i++) {
>>>> +        fe_path = GCSPRINTF("/local/domain/%d/device/vbd/%s/backend",
>>>> +                            domid, devs[i]);
>>>
>>> Is this path writeable by the guest? The containing directory is I
>>> think, so this needs to include delete and recreate type situations
>>> (although ISTR xenstore not having the posix like semantics here).
>>
>> Yes, the whole directory including the backend entry is writeable by the
>> guest.
>>
>>>
>>> If the guest can remove and recreate then we should check the current
>>> owner of the key is e.g. the toolstack domain or whoever should be
>>> trusted to won the key, within the same transaction as the read itself.
>>
>> I'm sorry but I'm not following you here, I assume you are speaking
>> about the frontend node that points to the backend ie:
>>
>> /local/domain/<domid>/device/vbd/<devid>/backend
>>
>> Permissions on this node are:
>>
>> domid: <domid> perms: 0
>> domid: 0 perms: 1
> 
> What are perms 0 and 1? We normally use r/w/b in xenstore speak.

This is what xs_get_permissions reports, according to xenstore_lib.h:

0 = XS_PERM_NONE
1 = XS_PERM_READ

Which doesn't make sense IMHO, because the guest has XS_PERM_NONE and is
able to read/write on that path (I'm probably missing something here).

> 
>> If the guest changes this node, or recreates it permissions will still
>> be the same.
> 
> I was hoping we might be able to spot this case because the permissions
> would have changed.
> 
> So the backend path in the frontend dir might point somewhere which is
> maliciously controlled by the guest, or by another guest, something
> which I don't think libxl_foo_from_xs_be is prepared to deal with. We
> need to mitigate this somehow. Can we tighten the permissions
> on /local/domain/<domid>/device/vbd/<devid>/backend such that only the
> toolstack domain can fiddle with it?

That was my first thought, I think we can safely make this
(/local/domain/<domid>/device/vbd/<devid>/backend) read-only for the
guest, no well-behaved frontend should ever try to change that.

libxl__devices_destroy logic already relies on
/local/domain/<domid>/device/vbd/<devid>/backend pointing to the backend
xenstore path.


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

Re: [PATCH 1/2] libxl: correctly list disks served by driver domains in block-list

[Ian Campbell]

On Tue, 2013-09-10 at 15:55 +0200, Roger Pau Monné wrote:
> On 10/09/13 14:54, Ian Campbell wrote:
> > On Tue, 2013-09-10 at 14:06 +0200, Roger Pau Monné wrote:
> >> On 10/09/13 12:11, Ian Campbell wrote:
> >>> On Fri, 2013-09-06 at 12:36 +0200, Roger Pau Monne wrote:
> >>>> The block-list command was not able to lists disks with backends
> >>>> running on domains different than Dom0, because it was only looking on
> >>>> the backend xenstore path of Dom0. Fix this by instead fetching the
> >>>> disks from the DomU xenstore entries.
> >>>
> >>> Need to be a bit careful here about reading from potentially guest
> >>> controllable keys. This should mostly be a question of permissions.
> >>>
> >>>> +    fe_path = libxl__sprintf(gc, "/local/domain/%d/device/vbd", domid);
> >>>
> >>> Are guests able to create new subdirectories under here?
> >>
> >> Yes
> >>
> >>>
> >>>> +    devs = libxl__xs_directory(gc, XBT_NULL, fe_path, &xs_num);
> >>>> +    if (!devs)
> >>>> +        /* Domain has no disks */
> >>>> +        goto out;
> >>>> +    disks = libxl__calloc(NOGC, xs_num, sizeof(*disks));
> >>>> +    if (!disks)
> >>>> +        goto out_err;
> >>>> +    for (i = 0; i < xs_num; i++) {
> >>>> +        fe_path = GCSPRINTF("/local/domain/%d/device/vbd/%s/backend",
> >>>> +                            domid, devs[i]);
> >>>
> >>> Is this path writeable by the guest? The containing directory is I
> >>> think, so this needs to include delete and recreate type situations
> >>> (although ISTR xenstore not having the posix like semantics here).
> >>
> >> Yes, the whole directory including the backend entry is writeable by the
> >> guest.
> >>
> >>>
> >>> If the guest can remove and recreate then we should check the current
> >>> owner of the key is e.g. the toolstack domain or whoever should be
> >>> trusted to won the key, within the same transaction as the read itself.
> >>
> >> I'm sorry but I'm not following you here, I assume you are speaking
> >> about the frontend node that points to the backend ie:
> >>
> >> /local/domain/<domid>/device/vbd/<devid>/backend
> >>
> >> Permissions on this node are:
> >>
> >> domid: <domid> perms: 0
> >> domid: 0 perms: 1
> > 
> > What are perms 0 and 1? We normally use r/w/b in xenstore speak.
> 
> This is what xs_get_permissions reports, according to xenstore_lib.h:
> 
> 0 = XS_PERM_NONE
> 1 = XS_PERM_READ
> 
> Which doesn't make sense IMHO, because the guest has XS_PERM_NONE and is
> able to read/write on that path (I'm probably missing something here).

The fact that XS permissions are barking mad ;-)

Assuming 
     domid: <domid> perms: 0
     domid: 0 perms: 1

is the ordered list returned by xs_get_permissions then...

The first entry is the owner, how inherently has full read/write
permissions. The perms in this entry are the defaults for anyone not
listed in a subsequent entry. The subsequent entries just list which
domain has which permission.

So the above says that <domid> owns the key and can do what it likes,
dom0 has read only permission, and everyone else has no permissions.


http://wiki.xen.org/wiki/XenBus#Permissions has some details.

> 
> > 
> >> If the guest changes this node, or recreates it permissions will still
> >> be the same.
> > 
> > I was hoping we might be able to spot this case because the permissions
> > would have changed.
> > 
> > So the backend path in the frontend dir might point somewhere which is
> > maliciously controlled by the guest, or by another guest, something
> > which I don't think libxl_foo_from_xs_be is prepared to deal with. We
> > need to mitigate this somehow. Can we tighten the permissions
> > on /local/domain/<domid>/device/vbd/<devid>/backend such that only the
> > toolstack domain can fiddle with it?
> 
> That was my first thought, I think we can safely make this
> (/local/domain/<domid>/device/vbd/<devid>/backend) read-only for the
> guest, no well-behaved frontend should ever try to change that.

Correct. The node should probably be owned by the toolstack domain and
the guest should jsut be given write permissions, so:
     domid: <toolstack> perms: 0
     domid: <domid> perms: 1

Then you just need to be sure that the guest cannot do xenstore-rm +
xenstore-write to get around that. I think that isn't allowed in the
xenstore permissions model.

> libxl__devices_destroy logic already relies on
> /local/domain/<domid>/device/vbd/<devid>/backend pointing to the backend
> xenstore path.

Oops!

Ian.


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

[PATCH] libxl: set permissions for xs frontend entry pointing to xs backend

[Roger Pau Monne]

libxl doesn't currently set the permissions of entries like:

/local/domain/<domid>/device/<dev_type>/<devid>/backend

This allows the guest to change this xenstore entries to point to a
different backend path, or to malicious xenstore path forged by the
guest itself. libxl currently relies on this path being valid in order
to perform the unplug of devices in libxl__devices_destroy, so we
should prevent the guest from modifying this xenstore entry.

This patch sets the permisions of said path to be the same as a
backend xenstore entry (owned by the toolstack domain, readable by the
guest).

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
Cc: Ian Jackson <Ian.Jackson@eu.citrix.com>
Cc: Ian Campbell <ian.campbell@citrix.com>
---
 tools/libxl/libxl_device.c |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

diff --git a/tools/libxl/libxl_device.c b/tools/libxl/libxl_device.c
index ea845b7..d7e7161 100644
--- a/tools/libxl/libxl_device.c
+++ b/tools/libxl/libxl_device.c
@@ -126,6 +126,8 @@ retry_transaction:
             xs_set_permissions(ctx->xsh, t, frontend_path,
                                frontend_perms, ARRAY_SIZE(frontend_perms));
         xs_write(ctx->xsh, t, libxl__sprintf(gc, "%s/backend", frontend_path), backend_path, strlen(backend_path));
+        xs_set_permissions(ctx->xsh, t, GCSPRINTF("%s/backend", frontend_path),
+                           backend_perms, ARRAY_SIZE(backend_perms));
         if (fents)
             libxl__xs_writev_perms(gc, t, frontend_path, fents,
                                    frontend_perms, ARRAY_SIZE(frontend_perms));
-- 
1.7.7.5 (Apple Git-26)


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

Re: [PATCH] libxl: set permissions for xs frontend entry pointing to xs backend

[Ian Campbell]

On Tue, 2013-09-10 at 16:54 +0200, Roger Pau Monne wrote:
> libxl doesn't currently set the permissions of entries like:
> 
> /local/domain/<domid>/device/<dev_type>/<devid>/backend
> 
> This allows the guest to change this xenstore entries to point to a
> different backend path, or to malicious xenstore path forged by the
> guest itself. libxl currently relies on this path being valid in order
> to perform the unplug of devices in libxl__devices_destroy, so we
> should prevent the guest from modifying this xenstore entry.
> 
> This patch sets the permisions of said path to be the same as a
> backend xenstore entry (owned by the toolstack domain, readable by the
> guest).

and just to confirm: despite having r/w access to the containing
directory, the guest cannot remove this node and recreate it?

> 
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> Cc: Ian Jackson <Ian.Jackson@eu.citrix.com>
> Cc: Ian Campbell <ian.campbell@citrix.com>
> ---
>  tools/libxl/libxl_device.c |    2 ++
>  1 files changed, 2 insertions(+), 0 deletions(-)
> 
> diff --git a/tools/libxl/libxl_device.c b/tools/libxl/libxl_device.c
> index ea845b7..d7e7161 100644
> --- a/tools/libxl/libxl_device.c
> +++ b/tools/libxl/libxl_device.c
> @@ -126,6 +126,8 @@ retry_transaction:
>              xs_set_permissions(ctx->xsh, t, frontend_path,
>                                 frontend_perms, ARRAY_SIZE(frontend_perms));
>          xs_write(ctx->xsh, t, libxl__sprintf(gc, "%s/backend", frontend_path), backend_path, strlen(backend_path));
> +        xs_set_permissions(ctx->xsh, t, GCSPRINTF("%s/backend", frontend_path),
> +                           backend_perms, ARRAY_SIZE(backend_perms));
>          if (fents)
>              libxl__xs_writev_perms(gc, t, frontend_path, fents,
>                                     frontend_perms, ARRAY_SIZE(frontend_perms));



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

Re: [PATCH] libxl: set permissions for xs frontend entry pointing to xs backend

[Roger Pau Monné]

On 10/09/13 17:02, Ian Campbell wrote:
> On Tue, 2013-09-10 at 16:54 +0200, Roger Pau Monne wrote:
>> libxl doesn't currently set the permissions of entries like:
>>
>> /local/domain/<domid>/device/<dev_type>/<devid>/backend
>>
>> This allows the guest to change this xenstore entries to point to a
>> different backend path, or to malicious xenstore path forged by the
>> guest itself. libxl currently relies on this path being valid in order
>> to perform the unplug of devices in libxl__devices_destroy, so we
>> should prevent the guest from modifying this xenstore entry.
>>
>> This patch sets the permisions of said path to be the same as a
>> backend xenstore entry (owned by the toolstack domain, readable by the
>> guest).
> 
> and just to confirm: despite having r/w access to the containing
> directory, the guest cannot remove this node and recreate it?

No, it can't (I've tried it):

root@debian:~# xenstore-rm /local/domain/54/device/vbd/51712/backend
xenstore-rm: could not remove path /local/domain/54/device/vbd/51712/backend

Re: [PATCH] libxl: set permissions for xs frontend entry pointing to xs backend

[Ian Campbell]

On Tue, 2013-09-10 at 17:03 +0200, Roger Pau Monné wrote:
> On 10/09/13 17:02, Ian Campbell wrote:
> > On Tue, 2013-09-10 at 16:54 +0200, Roger Pau Monne wrote:
> >> libxl doesn't currently set the permissions of entries like:
> >>
> >> /local/domain/<domid>/device/<dev_type>/<devid>/backend
> >>
> >> This allows the guest to change this xenstore entries to point to a
> >> different backend path, or to malicious xenstore path forged by the
> >> guest itself. libxl currently relies on this path being valid in order
> >> to perform the unplug of devices in libxl__devices_destroy, so we
> >> should prevent the guest from modifying this xenstore entry.
> >>
> >> This patch sets the permisions of said path to be the same as a
> >> backend xenstore entry (owned by the toolstack domain, readable by the
> >> guest).
> > 
> > and just to confirm: despite having r/w access to the containing
> > directory, the guest cannot remove this node and recreate it?
> 
> No, it can't (I've tried it):
> 
> root@debian:~# xenstore-rm /local/domain/54/device/vbd/51712/backend
> xenstore-rm: could not remove path /local/domain/54/device/vbd/51712/backend

Perfect. Thanks!

I think when I come to commit I will append to the changelog:

        The xenstore permissions model does not allow domains to remove
        directories which they do not own, despite having read/write
        access to the containing directory.
        
Ian


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

Re: [PATCH] libxl: set permissions for xs frontend entry pointing to xs backend

[Ian Jackson]

Roger Pau Monne writes ("[PATCH] libxl: set permissions for xs frontend entry pointing to xs backend"):
> libxl doesn't currently set the permissions of entries like:
> 
> /local/domain/<domid>/device/<dev_type>/<devid>/backend
> 
> This allows the guest to change this xenstore entries to point to a
> different backend path, or to malicious xenstore path forged by the
> guest itself. libxl currently relies on this path being valid in order
> to perform the unplug of devices in libxl__devices_destroy, so we
> should prevent the guest from modifying this xenstore entry.

Is it sufficient to set the permissions on "backend" - does that
prevent the guest deleting the whole subtree ?

Really it would be better to make the unplug not depend on this path.

This is a security issue, so CCing security@.  It appears to have
been discovered in public on xen-devel, so shouldn't be embargoed.

Ian.

Re: [PATCH] libxl: set permissions for xs frontend entry pointing to xs backend

[Ian Campbell]

On Tue, 2013-09-10 at 16:12 +0100, Ian Jackson wrote:
> Roger Pau Monne writes ("[PATCH] libxl: set permissions for xs frontend entry pointing to xs backend"):
> > libxl doesn't currently set the permissions of entries like:
> > 
> > /local/domain/<domid>/device/<dev_type>/<devid>/backend
> > 
> > This allows the guest to change this xenstore entries to point to a
> > different backend path, or to malicious xenstore path forged by the
> > guest itself. libxl currently relies on this path being valid in order
> > to perform the unplug of devices in libxl__devices_destroy, so we
> > should prevent the guest from modifying this xenstore entry.
> 
> Is it sufficient to set the permissions on "backend" - does that
> prevent the guest deleting the whole subtree ?

Yes. See the rest of this thread.

> Really it would be better to make the unplug not depend on this path.

Given the above, why?

> This is a security issue, so CCing security@.  It appears to have
> been discovered in public on xen-devel, so shouldn't be embargoed.

Ack.
Ian.

Re: [PATCH] libxl: set permissions for xs frontend entry pointing to xs backend

[Ian Jackson]

Ian Campbell writes ("Re: [PATCH] libxl: set permissions for xs frontend entry pointing to xs backend"):
> On Tue, 2013-09-10 at 16:12 +0100, Ian Jackson wrote:
> > Is it sufficient to set the permissions on "backend" - does that
> > prevent the guest deleting the whole subtree ?
> 
> Yes. See the rest of this thread.
> 
> > Really it would be better to make the unplug not depend on this path.
> 
> Given the above, why?

I think it's conceptually wrong.  We should be iterating over devices
using information in our own part of the xenstore tree.

Ian.

Re: [PATCH] libxl: set permissions for xs frontend entry pointing to xs backend

[Roger Pau Monné]

On 10/09/13 17:12, Ian Jackson wrote:
> Roger Pau Monne writes ("[PATCH] libxl: set permissions for xs frontend entry pointing to xs backend"):
>> libxl doesn't currently set the permissions of entries like:
>>
>> /local/domain/<domid>/device/<dev_type>/<devid>/backend
>>
>> This allows the guest to change this xenstore entries to point to a
>> different backend path, or to malicious xenstore path forged by the
>> guest itself. libxl currently relies on this path being valid in order
>> to perform the unplug of devices in libxl__devices_destroy, so we
>> should prevent the guest from modifying this xenstore entry.
> 
> Is it sufficient to set the permissions on "backend" - does that
> prevent the guest deleting the whole subtree ?

No, the guest can still delete the whole subtree, but it can not
recreate it (because the parent directory
/local/domain/<domid>/device/<dev_type>/ is not writeable by the guest).

> Really it would be better to make the unplug not depend on this path.
> 
> This is a security issue, so CCing security@.  It appears to have
> been discovered in public on xen-devel, so shouldn't be embargoed.
> 
> Ian.
> 

Re: [PATCH] libxl: set permissions for xs frontend entry pointing to xs backend

[Ian Campbell]

On Tue, 2013-09-10 at 16:19 +0100, Ian Jackson wrote:
> Ian Campbell writes ("Re: [PATCH] libxl: set permissions for xs frontend entry pointing to xs backend"):
> > On Tue, 2013-09-10 at 16:12 +0100, Ian Jackson wrote:
> > > Is it sufficient to set the permissions on "backend" - does that
> > > prevent the guest deleting the whole subtree ?
> > 
> > Yes. See the rest of this thread.
> > 
> > > Really it would be better to make the unplug not depend on this path.
> > 
> > Given the above, why?
> 
> I think it's conceptually wrong.  We should be iterating over devices
> using information in our own part of the xenstore tree.

So we need /libxl/domain/<domid>/devices/vdb/<devid> then?

Ian.

Re: [PATCH] libxl: set permissions for xs frontend entry pointing to xs backend

[Ian Campbell]

On Tue, 2013-09-10 at 17:19 +0200, Roger Pau Monné wrote:
> On 10/09/13 17:12, Ian Jackson wrote:
> > Roger Pau Monne writes ("[PATCH] libxl: set permissions for xs frontend entry pointing to xs backend"):
> >> libxl doesn't currently set the permissions of entries like:
> >>
> >> /local/domain/<domid>/device/<dev_type>/<devid>/backend
> >>
> >> This allows the guest to change this xenstore entries to point to a
> >> different backend path, or to malicious xenstore path forged by the
> >> guest itself. libxl currently relies on this path being valid in order
> >> to perform the unplug of devices in libxl__devices_destroy, so we
> >> should prevent the guest from modifying this xenstore entry.
> > 
> > Is it sufficient to set the permissions on "backend" - does that
> > prevent the guest deleting the whole subtree ?
> 
> No, the guest can still delete the whole subtree,

That's surprising given that it can't delete the backend node itself. It
relies on their not being two consecutive guest owned nodes in a path,
otherwise it can delete the subtree of the second and recreate using the
permissions on the first. Bit of a bear trap that one!




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

Re: [PATCH] libxl: set permissions for xs frontend entry pointing to xs backend

[Ian Jackson]

Ian Campbell writes ("Re: [PATCH] libxl: set permissions for xs frontend entry pointing to xs backend"):
> On Tue, 2013-09-10 at 16:19 +0100, Ian Jackson wrote:
> > I think it's conceptually wrong.  We should be iterating over devices
> > using information in our own part of the xenstore tree.
> 
> So we need /libxl/domain/<domid>/devices/vdb/<devid> then?

If we don't want to scour all the driver domains for backends, yes.

I guess we should have the quick permissions fix first.

Ian.

Re: [PATCH 2/2] libxl: fix libxl__device_disk_from_xs_be to parse backend domid

[Ian Campbell]

On Tue, 2013-09-10 at 14:08 +0200, Roger Pau Monné wrote:
> On 10/09/13 12:14, Ian Campbell wrote:
> > On Fri, 2013-09-06 at 12:36 +0200, Roger Pau Monne wrote:
> >> libxl__device_disk_from_xs_be was ignoring the backend domid, setting
> >> it to 0 by default. Fix this by parsing the backend disk path in order
> >> to fetch the backend domid.
> >>
> >> This fixes the issue reported when trying to block-detach disks that
> >> have it's backend on a driver domain.
> >>
> >> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> >> Reported-by: G.R. <firemeteor@users.sourceforge.net>
> >> Cc: Ian Jackson <Ian.Jackson@eu.citrix.com>
> >> Cc: Ian Campbell <ian.campbell@citrix.com>
> > 
> > It's a bit lame that there's no better way but this is indeed what
> > people (including frontends) do, so:
> > Acked-by: Ian Campbell <ian.campbell@citrix.com>
> > 
> > I'm pretty sure this can go in without patch 1/2 being in yet, but
> > please confirm.
> 
> Indeed, it can go in without 1 being committed.

Done.



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