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

[Tamas K Lengyel <tamas.k.lengyel@xxxxxx>]

On Mon, Jun 30, 2014 at 4:21 PM, Andrew Cooper <andrew.cooper3@xxxxxxxxxx> 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@xxxxxx>
> ---
> Â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.

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