[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH 1/2] libxl: correctly list disks served by driver domains in block-list
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
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |