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

[Ian Campbell <Ian.Campbell@xxxxxxxxxx>]

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@xxxxxxxxxxxxxxxxxxxxxx>
> >> ---
> >>  tools/libxl/libxl.c | 71 
> >> ++++++++++++++++++++++++++++++++++++++---------------
> >>  1 file changed, 51 insertions(+), 20 deletions(-)
> >>
> >> diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
> >> index c386004..e2c678a 100644
> >> --- a/tools/libxl/libxl.c
> >> +++ b/tools/libxl/libxl.c
> >> @@ -2308,7 +2308,7 @@ int libxl_vdev_to_device_disk(libxl_ctx *ctx, 
> >> uint32_t domid,
> >>                                const char *vdev, libxl_device_disk *disk)
> >>  {
> >>      GC_INIT(ctx);
> >> -    char *dompath, *path;
> >> +    char *dompath, *path, *backend_domid;
> >>      int devid = libxl__device_disk_dev_number(vdev, NULL, NULL);
> >>      int rc = ERROR_FAIL;
> >>  
> >> @@ -2328,6 +2328,13 @@ int libxl_vdev_to_device_disk(libxl_ctx *ctx, 
> >> uint32_t domid,
> >>          goto out;
> >>  
> >>      rc = libxl__device_disk_from_xs_be(gc, path, disk);
> >> +
> >> +    backend_domid = libxl__xs_read(gc, XBT_NULL,
> >> +            libxl__sprintf(gc, "%s/device/vbd/%d/backend-id",
> >> +                dompath, devid));
> >> +    if (backend_domid)
> >> +        disk->backend_domid = atoi(backend_domid);
> > 
> > I think this should be folded into ..._from_xs_be, either by parsing the
> > path argument or by doing the appropriate lookup via the frontend-path
> > node inside the function. Since I guess this will be common to both NIC
> > and disk perhaps a helper for from_xs_be to call would be appropriate?
> 
> 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.




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