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

[Andrew Cooper <andrew.cooper3@citrix.com>]

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

On 21/07/14 08:31, Andres Lagar Cavilla wrote:
> On Sun, Jul 20, 2014 at 5:49 PM, Dushyant Behl
> <myselfdushyantbehl@gmail.com <mailto:myselfdushyantbehl@gmail.com>>
> wrote:
>
>     Ian,
>
>     Sorry for this extremely late response.
>
>     On Fri, Jun 27, 2014 at 4:03 PM, Ian Campbell
>     <Ian.Campbell@citrix.com <mailto:Ian.Campbell@citrix.com>> wrote:
>     > 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@gmail.com
>     <mailto:myselfdushyantbehl@gmail.com>>
>     >> Reviewed-by: Andres Lagar-Cavilla <andres@lagarcavilla.org
>     <mailto:andres@lagarcavilla.org>>
>     >> Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com
>     <mailto:andrew.cooper3@citrix.com>>
>     >> ---
>     >>  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.
>
>     Yes, you are correct and the invocation should be with ring_pfn. But
>     if I change this with
>     the invocation done below as -
>     if ( xc_domain_decrease_reservation_exact(xch, domain_id, 1, 0,
>     &ring_pfn) ) 
>
>
>     then the code fails to enable mem paging.
>     Can anyone help as to why this would happen?
>
> The call that enables paging will use the hvm param in the hypervisor
> to look up the page by its pfn within the guest physical address
> space. If you call decrease reservation, then the page is removed from
> the guest phys address space, and the hvm param will point to a hole.
> And the call will fail of course. So you need to call decrease
> reservation strictly after enable paging.
>

You cannot have a period of time where the paging/access/events are
enabled and the guest can interfere with the ring page, as the Xen side
of ring pages are not robust to untrusted input.

How does this series interact with 6ae2df93 ?

~Andrew

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

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

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