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

[Andrew Cooper <andrew.cooper3@citrix.com>]

[-- Attachment #1.1: Type: text/plain, Size: 3080 bytes --]

On 30/06/14 15:33, Tamas K Lengyel wrote:
> On Mon, Jun 30, 2014 at 4:21 PM, Andrew Cooper
> <andrew.cooper3@citrix.com <mailto: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
>     <mailto: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.

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

~Andrew

[-- Attachment #1.2: Type: text/html, Size: 5752 bytes --]

[-- Attachment #2: Type: text/plain, Size: 126 bytes --]

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