From mboxrd@z Thu Jan 1 00:00:00 1970 From: Andrew Cooper Subject: Re: [PATCH] libxc: Pause & unpause the domain in xc_mem_event_enable based on its initial state. Date: Mon, 30 Jun 2014 15:40:21 +0100 Message-ID: <53B176D5.60809@citrix.com> References: <1404135906-6224-1-git-send-email-tamas.k.lengyel@tum.de> <53B17270.4010903@citrix.com> Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="===============4130094203271051369==" Return-path: Received: from mail6.bemta14.messagelabs.com ([193.109.254.103]) by lists.xen.org with esmtp (Exim 4.72) (envelope-from ) id 1X1ckt-0003fq-11 for xen-devel@lists.xenproject.org; Mon, 30 Jun 2014 14:40:27 +0000 In-Reply-To: List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Tamas K Lengyel Cc: xen-devel@lists.xenproject.org, ian.jackson@eu.citrix.com, Ian Campbell , stefano.stabellini@eu.citrix.com List-Id: xen-devel@lists.xenproject.org --===============4130094203271051369== Content-Type: multipart/alternative; boundary="------------050708050803040603000205" --------------050708050803040603000205 Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: 7bit On 30/06/14 15:33, Tamas K Lengyel wrote: > On Mon, Jun 30, 2014 at 4:21 PM, Andrew Cooper > > 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 > > > --- > > 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 --------------050708050803040603000205 Content-Type: text/html; charset="UTF-8" Content-Transfer-Encoding: 8bit
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> 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.

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

~Andrew
--------------050708050803040603000205-- --===============4130094203271051369== Content-Type: text/plain; charset="us-ascii" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit Content-Disposition: inline _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel --===============4130094203271051369==--