Re: [RFC 1/4] libxl: Learned to send FD through QMP to QEMU

[Anthony PERARD <anthony.perard@citrix.com>]

On Tue, Mar 27, 2018 at 11:29:35AM +0100, Ian Jackson wrote:
> (George, CC'ing you wrt your depriv doc patch - see below.)
> 
> Anthony PERARD writes ("[RFC 1/4] libxl: Learned to send FD through QMP to QEMU"):
> > +    /* File descriptor to send to QEMU on the next command */
> > +    int fd_to_send;
> 
> I did wonder if this was a layering violation, or a poor API in some
> other sense.  AFAICT it isn't, and libxl__qmp_handler is completely
> transparent to everything in libxl_qmp.c.
> 
> I think this whole file would benefit from some doc comments about the
> internal interfaces.  Particularly, something describing the boundary
> between operation-specific code and the generic qmp_send machinery
> would help review of both (i) new operations and (ii) extensions of
> the generic machinery.

I'll try to write some documentation.

> Looking at this and the next patch, I think (almost?) every user of
> this new feature will need to tell qmp_send to call
> qmp_fdset_add_fd_callback.  Is that right ?  Maybe this means we want
> to provide a more cooked version.

Not exactly. We can add a parameter to "add-fd" to use a specific
"fdset-id".

> Anthony PERARD writes ("[RFC 2/4] libxl: Have QEMU save its state to a file descriptor"):
> > In case QEMU have restricted access to the system, open the file for it,
> > and QEMU will save its state to this file descritor.
> 
> This 2nd patch looks reasonable, but it prompted to notice two new
> kinds of hazard introduced by the deprivileging design goal:
> 
> >  int libxl__qmp_save(libxl__gc *gc, int domid, const char *filename, bool live)
> >  {
> ...
> > +    rc = qmp_synchronous_send(qmp, "add-fd", NULL,
> > +                              qmp_fdset_add_fd_callback, &new_fdset,
> > +                              qmp->timeout);
> > +    if (rc)
> > +        goto out;
> 
> By this point, a depriv'd qemu must be assumed to be compromised by
> its guest - ie we must treat it as hostile.
> 
> This is not consistent with use of qmp_synchronous_send, because
> qmp_synchronous_send will block with both the domain and ctx locks
> held.  That is, a malicious qemu can deny service; it even has the
> ability to prevent its serviced domain from being destroyed.
> 
> Secondly, the point about qemu now being malicious means that we need
> to audit the code which handles communications with qemu for safety.
> 
> I think this means that:
> 
>  * George's todo list patch for the depriv doc should mention
>    the need to replace qmp_synchronous_send with qemp_send.
> 
>  * Likewise it should mention the need for this audit.
> 
>  * We should write a comment somewhere (near the top of libxl_qmp.c
>    perhaps) warning developers not to treat qemu as trusted.  That
>    would usefully fit into your own series.
> 
> I volunteer to do the audit.  Some internal commentary about the
> internal interfaces (as I discuss above) would be helpful for that.

I think we would need to rewrite part of libxl_qmp.c to use the
libxl__ev_*, so QEMU would not be able to block libxl.

-- 
Anthony PERARD

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel