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

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

[Tamas K Lengyel]

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;
+        }
+    }
+
     /* Get the pfn of the ring page */
     rc1 = xc_get_hvm_param(xch, domain_id, param, &pfn);
     if ( rc1 != 0 )
@@ -154,7 +165,16 @@ void *xc_mem_event_enable(xc_interface *xch, domid_t domain_id, int param,
  out:
     saved_errno = errno;
 
-    rc2 = xc_domain_unpause(xch, domain_id);
+    /* Only unpause the domain if it was running originally */
+    if( !(dom_info.flags & XEN_DOMINF_paused) )
+    {
+        rc2 = xc_domain_unpause(xch, domain_id);
+    }
+    else
+    {
+        rc2 = 0;
+    }
+
     if ( rc1 != 0 || rc2 != 0 )
     {
         if ( rc2 != 0 )
-- 
2.0.0

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

[Andrew Cooper]

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

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

[Tamas K Lengyel]

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

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.

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

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

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

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

[Andrew Cooper]

[-- 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

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

[Tamas K Lengyel]

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

On Mon, Jun 30, 2014 at 4:40 PM, Andrew Cooper <andrew.cooper3@citrix.com>
wrote:

>  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
>

Again, I agree, but that is not the problem this patch is trying to
address. The problem you are talking about is present xc_mem_event_enable
as it is right now as well. For example, nothing prevents another
thread/process from unpausing the domain while the event ring setup is in
process. This patch operates with the same assumption the current code
makes: only one user-space tool/thread is controlling the domain at a time.
This patch just ensures the domain is left in the same state it was before
the function was called.

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

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

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

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

[Aravindh Puthiyaparambil (aravindp)]

>>> 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
>
>Again, I agree, but that is not the problem this patch is trying to address. The
>problem you are talking about is present xc_mem_event_enable as it is right
>now as well. For example, nothing prevents another thread/process from
>unpausing the domain while the event ring setup is in process. This patch
>operates with the same assumption the current code makes: only one user-
>space tool/thread is controlling the domain at a time. This patch just ensures
>the domain is left in the same state it was before the function was called.

Tamas, 

Thanks for finding this issue. I agree with Andrew that the correct fix for this can only be done in the hypervisor. But I see there is some resistance to that. If that is the case, I would hope this patch is accepted. You could also add a comment about this situation in xenctrl.h.

Thanks,
Aravindh

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

[Jan Beulich]

>>> On 01.07.14 at 00:54, <aravindp@cisco.com> wrote:
> Thanks for finding this issue. I agree with Andrew that the correct fix for 
> this can only be done in the hypervisor. But I see there is some resistance 
> to that. If that is the case, I would hope this patch is accepted. You could 
> also add a comment about this situation in xenctrl.h.

If my "works as intended" regarding the current non-refcounting behavior
was (mis)interpreted as resistance, then the latest with Andrew's most
reply this should be clear to no longer be a concern. Quite obviously the
current way of how this works was done with the assumption that all
tool stack activity goes through a single arbitrating instance (xend),
which clearly became a wrong assumption quite a while back.

The only thing missing is the patch that I think Andrew said yesterday
would be on its way.

Jan

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

[Aravindh Puthiyaparambil (aravindp)]

>> Thanks for finding this issue. I agree with Andrew that the correct
>> fix for this can only be done in the hypervisor. But I see there is
>> some resistance to that. If that is the case, I would hope this patch
>> is accepted. You could also add a comment about this situation in xenctrl.h.
>
>If my "works as intended" regarding the current non-refcounting behavior
>was (mis)interpreted as resistance, then the latest with Andrew's most reply
>this should be clear to no longer be a concern. Quite obviously the current
>way of how this works was done with the assumption that all tool stack
>activity goes through a single arbitrating instance (xend), which clearly became
>a wrong assumption quite a while back.
>
>The only thing missing is the patch that I think Andrew said yesterday would
>be on its way.

Glad to hear that. Sorry about misinterpreting your response.

Cheers,
Aravindh