Re: [Xen-devel] [PATCH v2 3/3] [GSOC14] FIX:- Race condition between initializing shared ring and mempaging.

[Ian Campbell <Ian.Campbell@xxxxxxxxxx>]

On Mon, 2014-06-16 at 23:50 +0530, Dushyant Behl wrote:
> This patch is part of the work done under the gsoc project -
> Lazy Restore Using Memory Paging.
> 
> This patch is meant to fix a known race condition bug in mempaging
> ring setup routines. The race conditon was between initializing

"condition"

> mem paging and initializing shared ring, earlier the code initialized
> mem paging before removing the ring page from guest's physical map
> which could enable the guest to interfere with the ring initialisation.
> Now the code removes the page from the guest's physical map before
> enabling mempaging so that the guest cannot clobber the ring after
> we initialise it.
> 
> Signed-off-by: Dushyant Behl <myselfdushyantbehl@xxxxxxxxx>
> Reviewed-by: Andres Lagar-Cavilla <andres@xxxxxxxxxxxxxxxx>
> Reviewed-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
> ---
>  tools/libxc/xc_mem_paging_setup.c | 24 ++++++++++++++++++++----
>  1 file changed, 20 insertions(+), 4 deletions(-)
> 
> diff --git a/tools/libxc/xc_mem_paging_setup.c 
> b/tools/libxc/xc_mem_paging_setup.c
> index 679c606..a4c4798 100644
> --- a/tools/libxc/xc_mem_paging_setup.c
> +++ b/tools/libxc/xc_mem_paging_setup.c
> @@ -73,6 +73,24 @@ int xc_mem_paging_ring_setup(xc_interface *xch,
>          }
>      }
>  
> +    /*
> +     * Remove the page from guest's physical map so that the guest will not
> +     * be able to break security by pretending to be toolstack for a while.
> +     */
> +    if ( xc_domain_decrease_reservation_exact(xch, domain_id, 1, 0, 
> ring_page) )

ring_page here is a void * mapping of the page, isn't it? Not a
xen_pfn_t[] array of pfns, so surely this isn't right.

> @@ -106,14 +124,12 @@ int xc_mem_paging_ring_setup(xc_interface *xch,
>      }
>      *port = rc;
>  
> -    /* Initialise ring */
> -    SHARED_RING_INIT((mem_event_sring_t *)ring_page);
> -    BACK_RING_INIT(back_ring, (mem_event_sring_t *)ring_page, PAGE_SIZE);
> +    DPRINTF("enabled mempaging");
>  
>      /* Now that the ring is set, remove it from the guest's physmap */
>      if ( xc_domain_decrease_reservation_exact(xch, domain_id, 1, 0, 
> &ring_pfn) )

This looks like the correct invocation to me. If you correct the one
above though isn't this one then redundant though?

>      {
> -        PERROR("Failed to remove ring from guest physmap");
> +        PERROR("Failed to remove ring_pfn from guest physmap");
>          return -1;
>      }
>  



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