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

[Marek Marczykowski <marmarek@xxxxxxxxxxxxxxxxxxxxxx>]

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?

-- 
Best Regards,
Marek Marczykowski
Invisible Things Lab

Attachment: signature.asc
Description: OpenPGP digital signature

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