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

[Roger Pau Monnà <roger.pau@xxxxxxxxxx>]

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