Re: [Xen-devel] [PATCH] libxc: Pause & unpause the domain in xc_mem_event_enable based on its initial state.

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 30/06/14 15:33, Tamas K Lengyel wrote:

On Mon, Jun 30, 2014 at 4:21 PM, Andrew Cooper <andrew.cooper3@xxxxxxxxxx> wrote:

On 30/06/14 14:45, Tamas K Lengyel wrote:
> In an attempt to mitigate XSA-99, xc_mem_event_enable ensures that the domain is paused
> for the duration of the event ring setup. However, it disregards the initial state of
> the domain, which might already be paused, resulting in 1) an uneccessary hypercall to
> pause it again and 2) unpauses it unconditionally which is an opaque and potentially
> unwanted side-effect. This patch fixes both issues.
>
> Signed-off-by: Tamas K Lengyel <tamas.k.lengyel@xxxxxx>
> ---
> Âtools/libxc/xc_mem_event.c | 30 +++++++++++++++++++++++++-----
> Â1 file changed, 25 insertions(+), 5 deletions(-)
>
> diff --git a/tools/libxc/xc_mem_event.c b/tools/libxc/xc_mem_event.c
> index 0b2eecb..5cf74d0 100644
> --- a/tools/libxc/xc_mem_event.c
> +++ b/tools/libxc/xc_mem_event.c
> @@ -62,6 +62,7 @@ void *xc_mem_event_enable(xc_interface *xch, domid_t domain_id, int param,
> Â Â Âvoid *ring_page = NULL;
> Â Â Âunsigned long pfn;
> Â Â Âxen_pfn_t ring_pfn, mmap_pfn;
> + Â Âxc_domaininfo_t dom_info;
> Â Â Âunsigned int op, mode;
> Â Â Âint rc1, rc2, saved_errno;
>
> @@ -71,14 +72,24 @@ void *xc_mem_event_enable(xc_interface *xch, domid_t domain_id, int param,
> Â Â Â Â Âreturn NULL;
> Â Â Â}
>
> - Â Â/* Pause the domain for ring page setup */
> - Â Ârc1 = xc_domain_pause(xch, domain_id);
> - Â Âif ( rc1 != 0 )
> + Â Ârc1 = xc_domain_getinfolist(xch, domain_id, 1, &dom_info);
> + Â Âif ( rc1 != 1 || dom_info.domain != domain_id )
> Â Â Â{
> - Â Â Â ÂPERROR("Unable to pause domain\n");
> + Â Â Â ÂPERROR("Error getting domain info\n");
> Â Â Â Â Âreturn NULL;
> Â Â Â}
>
> + Â Â/* Pause the domain for ring page setup if it isn't already */
> + Â Âif( !(dom_info.flags & XEN_DOMINF_paused) )
> + Â Â{
> + Â Â Â Ârc1 = xc_domain_pause(xch, domain_id);
> + Â Â Â Âif ( rc1 != 0 )
> + Â Â Â Â{
> + Â Â Â Â Â ÂPERROR("Unable to pause domain\n");
> + Â Â Â Â Â Âreturn NULL;
> + Â Â Â Â}
> + Â Â}

As indicated in the other thread regarding the correctness of properly
refcounting in Xen, this is a TOCTOU race which doesn't fix the problem
in the general case.

It is my opinion that this is impossible to correctly fix in the dom0
userspace.

~Andrew

Indeed, if there are multiple user-space tools trying to control the domain, this patch doesn't ensure safety. That issue is however beyond the scope of what this patch is trying to address.

This patch doesn't even assure safety for two threads in the same process dealing with the same domain.

~Andrew

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