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

[Ian Jackson <ian.jackson@xxxxxxxxxx>]

(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"):
> Adding the ability to send a file descriptor from libxl to QEMU via the
> QMP interface. This will be use with the "add-fd" QMP command.

The code looks plausible.

> +    /* 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.

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.

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.

Thanks,
Ian.

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