Xen project Mailing List

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

On 30/06/14 15:33, Tamas K Lengyel wrote:

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.

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.

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