Marek Marczykowski
2013-Apr-27 23:12 UTC
[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.
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
Marek Marczykowski
2013-Apr-27 23:17 UTC
[PATCH 2/2] libxl: fix spelling of "backend-id" for vtpm
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
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
Ian Campbell
2013-Apr-29 08:40 UTC
Re: [PATCH 2/2] libxl: fix spelling of "backend-id" for vtpm
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); >
Ian Campbell
2013-Apr-29 08:48 UTC
Re: [PATCH 1/2] libxl: do not assume Dom0 backend while listing disks and nics
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.
Roger Pau Monné
2013-Apr-29 08:56 UTC
Re: [PATCH 1/2] libxl: do not assume Dom0 backend while listing disks and nics
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; >
Daniel De Graaf
2013-Apr-29 13:26 UTC
Re: [PATCH 2/2] libxl: fix spelling of "backend-id" for vtpm
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); >>
Ian Campbell
2013-Apr-29 13:51 UTC
Re: [PATCH 2/2] libxl: fix spelling of "backend-id" for vtpm
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.
Ian Campbell
2013-Apr-30 10:58 UTC
Re: [PATCH 2/2] libxl: fix spelling of "backend-id" for vtpm
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.
Ian Jackson
2013-May-01 10:29 UTC
Re: [PATCH 1/2] libxl: do not assume Dom0 backend while listing disks and nics
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.
Ian Campbell
2013-May-01 11:43 UTC
Re: [PATCH 1/2] libxl: do not assume Dom0 backend while listing disks and nics
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.
Marek Marczykowski
2013-May-01 20:52 UTC
Re: [PATCH 1/2] libxl: do not assume Dom0 backend while listing disks and nics
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 _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Ian Campbell
2013-May-02 08:30 UTC
Re: [PATCH 1/2] libxl: do not assume Dom0 backend while listing disks and nics
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 directoryI''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.
Marek Marczykowski
2013-May-07 23:13 UTC
Re: [PATCH 1/2] libxl: do not assume Dom0 backend while listing disks and nics
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 _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Marek Marczykowski
2013-May-07 23:56 UTC
Re: [PATCH 1/2] libxl: do not assume Dom0 backend while listing disks and nics
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 _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Ian Campbell
2013-May-08 09:27 UTC
Re: [PATCH 1/2] libxl: do not assume Dom0 backend while listing disks and nics
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.