Xen devel - Feb 2008 - compat_mmuext_op - continuation code is wrong

289         err = do_mmuext_op(nat_ops, i | preempt_mask, pdone, foreigndom);

If preempt_mask is set, we immediately hit a pre-emption, then we get:

2003     if ( unlikely(count & MMU_UPDATE_PREEMPTED) )
2004     {
2005         count &= ~MMU_UPDATE_PREEMPTED;
2006         if ( unlikely(!guest_handle_is_null(pdone)) )
2007             (void)copy_from_guest(&done, pdone, 1);
2008     }

But we never initialised the guest''s "pdone" at any point. So
we can
copy in an uninit stack value into ''done'', add
''i'' to it, then copy it
back out. On Solaris, we assert that the success count == what we passed
in, so this breaks Solaris 32/64

I''m running on 3.1 with a couple of fixes from unstable to make it work
at all. The below patch works for us but I''m not confident in its
correctness.

regards
john

diff --git a/xen/arch/x86/x86_64/compat/mm.c b/xen/arch/x86/x86_64/compat/mm.c
--- a/xen/arch/x86/x86_64/compat/mm.c
+++ b/xen/arch/x86/x86_64/compat/mm.c
@@ -4,6 +4,7 @@
 #include <xen/multicall.h>
 #include <compat/memory.h>
 #include <compat/xen.h>
+#include <xen/config.h>
 
 int compat_set_gdt(XEN_GUEST_HANDLE(uint) frame_list, unsigned int entries)
 {
@@ -180,6 +181,7 @@ int compat_mmuext_op(XEN_GUEST_HANDLE(mm
                      unsigned int foreigndom)
 {
     unsigned int i, preempt_mask;
+    unsigned int origcount;
     int rc = 0;
     XEN_GUEST_HANDLE(mmuext_op_t) nat_ops;
 
@@ -191,6 +193,8 @@ int compat_mmuext_op(XEN_GUEST_HANDLE(mm
 
     set_xen_guest_handle(nat_ops, (void
*)COMPAT_ARG_XLAT_VIRT_START(current->vcpu_id));
 
+    origcount = count;
+
     for ( ; count; count -= i )
     {
         mmuext_op_t *nat_op = nat_ops.p;
@@ -199,6 +203,17 @@ int compat_mmuext_op(XEN_GUEST_HANDLE(mm
 
         if ( hypercall_preempt_check() )
         {
+            /*
+             * If this is the first time around, and we''ve not
+             * pre-empted before, we must initialise pdone (see start of
+             * do_mmuext_op()).
+             */
+            if (!preempt_mask && origcount == count)
+            {
+                unsigned int zero = 0;
+                (void) copy_to_guest(pdone, &zero, 1);
+            }
+
             rc = hypercall_create_continuation(
                 __HYPERVISOR_mmuext_op, "hihi",
                 cmp_uops, count | MMU_UPDATE_PREEMPTED, pdone, foreigndom);

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

On 20/2/08 17:21, "John Levon" <levon@movementarian.org> wrote:
> But we never initialised the guest''s "pdone" at any
point. So we can
> copy in an uninit stack value into ''done'', add
''i'' to it, then copy it
> back out. On Solaris, we assert that the success count == what we passed
> in, so this breaks Solaris 32/64
> 
> I''m running on 3.1 with a couple of fixes from unstable to make it
work
> at all. The below patch works for us but I''m not confident in its
> correctness.
Better to remove the preempt check altogether, as it''s already done
(correctly) in do_mmuext_op(). That''s what I''ll check in.

 -- Keir



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

On Wed, Feb 20, 2008 at 06:16:58PM +0000, Keir Fraser wrote:
> > But we never initialised the guest''s "pdone" at any
point. So we can
> > copy in an uninit stack value into ''done'', add
''i'' to it, then copy it
> > back out. On Solaris, we assert that the success count == what we
passed
> > in, so this breaks Solaris 32/64
> > 
> > I''m running on 3.1 with a couple of fixes from unstable to
make it work
> > at all. The below patch works for us but I''m not confident in
its
> > correctness.
> 
> Better to remove the preempt check altogether, as it''s already
done
> (correctly) in do_mmuext_op(). That''s what I''ll check in.
That''s what we initially tested, and works for us. Thanks.

john

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