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:21:36 +0100 Message-ID: <53B17270.4010903@citrix.com> References: <1404135906-6224-1-git-send-email-tamas.k.lengyel@tum.de> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Received: from mail6.bemta4.messagelabs.com ([85.158.143.247]) by lists.xen.org with esmtp (Exim 4.72) (envelope-from ) id 1X1cW6-0002J3-5e for xen-devel@lists.xenproject.org; Mon, 30 Jun 2014 14:25:10 +0000 In-Reply-To: <1404135906-6224-1-git-send-email-tamas.k.lengyel@tum.de> 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 , xen-devel@lists.xenproject.org Cc: Tamas K Lengyel , ian.jackson@eu.citrix.com, ian.campbell@citrix.com, stefano.stabellini@eu.citrix.com List-Id: xen-devel@lists.xenproject.org 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