Re: [PATCH v3 1/6] libxl: allow /local/domain/$LIBXL_TOOLSTACK_DOMID/device-model/$DOMID to be written by $DOMID

[Ian Campbell <ian.campbell@citrix.com>]

On Wed, 2015-06-10 at 11:09 +0100, Stefano Stabellini wrote:
> The device model is going to restrict its xenstore connection to $DOMID
> level.

Am I correct in concluding that only oxenstored supports XS_RESTRICT? I
don't see it in C xenstored at all.

>  Let qemu-xen access
> /local/domain/$LIBXL_TOOLSTACK_DOMID/device-model/$DOMID, as it is
> required by QEMU to read/write the physmap. It doesn't contain any
> information the guest is not already fully aware of.

docs/misc/xenstore-paths.markdown should also be updated to reflect the
changes here (and document any keys which are missing, since you seem to
list more here than the doc contains).

> Add a maximum limit of physmap entries to save, so that the guest cannot
> DOS the toolstack.

How will we cope when the limit needs to be increased in the future for
some reason?

There is an interesting general issue here which is that we have
XS_RESTRICT which changes a connection to be treated as having the
permissions of a given target domain instead of the originating
privileged domain.

This means that in order to reduce the privileges of one thing we have
to increase the privilege of the guest itself (by granting access to
those paths), which seems rather counter-intuitive.

What we really want is a new privilege type which is "read/write to
connections which are _privileged_ over $domid, but not $domid itself"
and for XS_RESTRICT to imply that.

Retrofitting something like that to xenstored would be tricky I suspect.

When the physmap stuff was added doing it via xenstore was convenient
because we weren't concerning ourselves with this deprivileging. How
that we are though perhaps we should think about whether this is still
appropriate and consider using a QMP command to request the list instead
for example.

> @@ -1698,6 +1700,9 @@ int libxl__toolstack_save(uint32_t domid, uint8_t **buf,
>                  &num);
>      count = num;
>  
> +    if (count > MAX_PHYSMAP_ENTRIES)
> +        return -1;

Probably worth logging some sort of clue here.

Ian.