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

[Ian Campbell <Ian.Campbell@xxxxxxxxxx>]

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@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel