On Mon, Jun 30, 2014 at 4:21 PM, Andrew Cooper <andrew.cooper3@citrix.com> 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@tum.de>
> ---
>  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.