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

[Dushyant Behl <myselfdushyantbehl@xxxxxxxxx>]

On Mon, Jul 21, 2014 at 9:29 PM, Andres Lagar Cavilla
<andres@xxxxxxxxxxxxxxxx> wrote:
>
> On Jul 21, 2014 7:40 AM, "Andrew Cooper" <andrew.cooper3@xxxxxxxxxx> wrote:
>>
>> On 21/07/14 15:11, Andres Lagar Cavilla wrote:
>>>
>>> On Mon, Jul 21, 2014 at 5:01 AM, Andrew Cooper
>>> <andrew.cooper3@xxxxxxxxxx> wrote:
>>>>
>>>> On 21/07/14 08:31, Andres Lagar Cavilla wrote:
>>>>>
>>>>> On Sun, Jul 20, 2014 at 5:49 PM, Dushyant Behl
>>>>> <myselfdushyantbehl@xxxxxxxxx> wrote:
>>>>>>
>>>>>> Ian,
>>>>>>
>>>>>> Sorry for this extremely late response.
>>>>>>
>>>>>> On Fri, Jun 27, 2014 at 4:03 PM, Ian Campbell
>>>>>> <Ian.Campbell@xxxxxxxxxx> 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@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.
>>>>>>
>>>>>> 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.
>>>
>>> It's deja vu all over again!
>>>
>>> Note that the XSA commmit, 6ae2df93c27, does exactly that. Enable
>>> paging/access/sharing, and only after that decrease reservation (and after
>>> that unpause). So the window is open ... methinks?
>>
>>
>> No - it does a domain pause around this set of critical operations, so the
>> guest is guaranteed not to be running, and therefore cannot interfere.
>
> Right. Just as the patches in this thread do, since introduced. So no issue
> per se.

Sorry but actually we don't have xc_domain_pause and unpause calls
around ring setup in xenpaging code.


>
> I think
>
> Andres

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