Re: [Xen-devel] [RFC 1/4] libxl: Learned to send FD through QMP to QEMU
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, _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
Re: [Xen-devel] [RFC 1/4] libxl: Learned to send FD through QMP to QEMU
On Tue, Mar 27, 2018 at 11:58:45AM +0100, George Dunlap wrote:
> On 03/27/2018 11:29 AM, Ian Jackson wrote:
> > 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, _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.
>
> Will qmp_synchronous_send() wait forever, or is there a timeout?
There is some kind of timeout, but I'm not sure it is true at all time.
This is a few functions that does handle connection/send/receive:
- qmp_open()
this one as a 5s timeout on connecting to the socket.
- qmp_send()
This use write/sendmsg with no timeout, but the fd is set to
O_NONBLOCK.
- qmp_next()
This function use select with a 5s timeout, so read should not block.
But I think the timout is reset every time something have been read
from the socket.
So I guess a malicious qemu could have the qmp_next() function wait
forever.
Also I think every time a "response" or an "event" is processed,
qmp_next() will return, and qmp_synchronous_send() will call qmp_next
again until it got the response it is waiting for.
So a few opportunity to wait forever.
--
Anthony PERARD
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [RFC 1/4] libxl: Learned to send FD through QMP to QEMU
On 03/27/2018 11:29 AM, 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"):
>> 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, _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.
Will qmp_synchronous_send() wait forever, or is there a timeout?
In any case, we certainly do need to remember to treat QEMU as hostile
and audit the interactions with it. This will help the stubdom case as
well.
I'll add it to the list.
-George
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [RFC 1/4] libxl: Learned to send FD through QMP to QEMU
(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, _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@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
