Re: [Xen-devel] [PATCH v3 3/3] tools/libxc/xc_mem_paging_setup.c: FIX- Race condition between initializing shared ring and mempaging.

[Andres Lagar Cavilla <andres@xxxxxxxxxxxxxxxx>]

On Thu, Aug 7, 2014 at 4:40 PM, Dushyant Behl <myselfdushyantbehl@xxxxxxxxx> wrote:

This patch is meant to fix a known race condition bug in mempaging
ring setup routines. The race condition was between initializing
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>

(+ IanC)

Â

This patch has a few problems, but raises a couple interesting points about xc_mem_event_enable. See below:

---
Âtools/libxc/xc_mem_paging_setup.c | 29 +++++++++++++++++++++++++----
Â1 file changed, 25 insertions(+), 4 deletions(-)

diff --git a/tools/libxc/xc_mem_paging_setup.c b/tools/libxc/xc_mem_paging_setup.c
index 9741dc9..12469b4 100644
--- a/tools/libxc/xc_mem_paging_setup.c
+++ b/tools/libxc/xc_mem_paging_setup.c
@@ -76,6 +76,22 @@ int xc_mem_paging_ring_setup(xc_interface *xch,
    Â}
  Â}

+ Â Â/* Clear the ring_pfn */
+ Â Âmemset(ring_page, 0, PAGE_SIZE);

Once xc_mem_event_enable maps the ring page, surely there is no downside to clearing it out? i.e. moving this memset there should help.Â

+
+ Â Ârc = xc_domain_pause(xch, domain_id);
+ Â Âif ( rc != 0 )
+ Â Â{
+ Â Â Â ÂPERROR("Unable to pause domain");
+ Â Â Â Âreturn -1;
+ Â Â}
+ Â ÂDPRINTF("Domain pause successful");
+
+ Â Â/* Initialise ring */
+ Â ÂSHARED_RING_INIT((mem_event_sring_t *)ring_page);

Likewise. Once xc_mem_event_enable has mapped (and cleared) the ring page, and since it's holding the domain paused, there is no downside in applying SHARED_RING_INIT, methinks. Every consumer of xc_mem_event_enable is bound to do that. And doing it prior to enabling mem event and unpausing the domain ensures Xen will have a proper ring to place events at.

If those two make sense, then we can cut a patch against xc_mem_event_enable.

Thanks

AndresÂ

+ Â ÂBACK_RING_INIT(back_ring, (mem_event_sring_t *)ring_page, PAGE_SIZE);
+ Â ÂDPRINTF("ininialized shared ring");
+
  Â/* Initialise Xen */
  Ârc = xc_mem_paging_enable(xch, domain_id, evtchn_port);
  Âif ( rc != 0 )
@@ -99,6 +115,7 @@ int xc_mem_paging_ring_setup(xc_interface *xch,
    Â}
    Âreturn -1;
  Â}
+ Â ÂDPRINTF("enabled mempaging");

  Â/* Bind event notification */
  Ârc = xc_evtchn_bind_interdomain(xce_handle, domain_id, *evtchn_port);
@@ -109,10 +126,6 @@ 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);
-
  Â/* 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) )
  Â{
@@ -120,6 +133,14 @@ int xc_mem_paging_ring_setup(xc_interface *xch,
    Âreturn -1;
  Â}

+ Â Ârc = xc_domain_unpause(xch, domain_id);
+ Â Âif ( rc != 0 )
+ Â Â{
+ Â Â Â ÂPERROR("Unable to unpause domain");
+ Â Â Â Âreturn -1;
+ Â Â}
+ Â ÂDPRINTF("Domain unpause successful");
+
  Âreturn 0;
Â}

--
1.9.1

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