lock down hypercall continuation encoding masks

Andrew validly points out that even if these masks aren't a formal part
of the hypercall interface, we aren't free to change them: A guest
suspended for migration in the middle of a continuation would fail to
work if resumed on a hypervisor using a different value. Hence add
respective comments to their definitions.

Additionally, to help future extensibility as well as in the spirit of
reducing undefined behavior as much as possible, refuse hypercalls made
with the respective bits non-zero when the respective sub-ops don't
make use of those bits.

Reported-by: Andrew Cooper <andrew.cooper3@citrix.com>
Signed-off-by: Jan Beulich <jbeulich@suse.com>
Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>
Acked-by: Ian Campbell <ian.campbell@citrix.com>
Acked-by: Tim Deegan <tim@xen.org>
Release-Acked-by: Konrad Rzeszutek Wilk <Konrad.wilk@oracle.com>

diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
index 51ffc90a57bcd297ffc774d35628fedb576e29e4..bc414ff2288bda0a1c6cffeae9d7f0db921801b9 100644 (file)
--- a/xen/arch/x86/hvm/hvm.c
+++ b/xen/arch/x86/hvm/hvm.c
@@ -5458,16 +5458,34 @@ static int hvmop_destroy_ioreq_server(
     return rc;
 }
 
+/*
+ * Note that this value is effectively part of the ABI, even if we don't need
+ * to make it a formal part of it: A guest suspended for migration in the
+ * middle of a continuation would fail to work if resumed on a hypervisor
+ * using a different value.
+ */
 #define HVMOP_op_mask 0xff
 
 long do_hvm_op(unsigned long op, XEN_GUEST_HANDLE_PARAM(void) arg)
 
 {
     struct domain *curr_d = current->domain;
-    unsigned long start_iter = op & ~HVMOP_op_mask;
+    unsigned long start_iter, mask;
     long rc = 0;
 
-    switch ( op &= HVMOP_op_mask )
+    switch ( op & HVMOP_op_mask )
+    {
+    default:
+        mask = ~0UL;
+        break;
+    case HVMOP_modified_memory:
+    case HVMOP_set_mem_type:
+        mask = HVMOP_op_mask;
+        break;
+    }
+
+    start_iter = op & ~mask;
+    switch ( op &= mask )
     {
     case HVMOP_create_ioreq_server:
         rc = hvmop_create_ioreq_server(
@@ -6120,7 +6138,7 @@ long do_hvm_op(unsigned long op, XEN_GUEST_HANDLE_PARAM(void) arg)
 
     if ( rc == -ERESTART )
     {
-        ASSERT(!(start_iter & HVMOP_op_mask));
+        ASSERT(!(start_iter & mask));
         rc = hypercall_create_continuation(__HYPERVISOR_hvm_op, "lh",
                                            op | start_iter, arg);
     }

diff --git a/xen/arch/x86/mm.c b/xen/arch/x86/mm.c
index 522c43d7267f8d76b98b39ee61dfefce52ebf99d..6e9c2c0a3518d2e06f05b995ca8b1ac3cdf2e4a3 100644 (file)
--- a/xen/arch/x86/mm.c
+++ b/xen/arch/x86/mm.c
@@ -4661,9 +4661,8 @@ int xenmem_add_to_physmap_one(
 long arch_memory_op(unsigned long cmd, XEN_GUEST_HANDLE_PARAM(void) arg)
 {
     int rc;
-    int op = cmd & MEMOP_CMD_MASK;
 
-    switch ( op )
+    switch ( cmd )
     {
     case XENMEM_set_memory_map:
     {
@@ -4837,7 +4836,7 @@ long arch_memory_op(unsigned long cmd, XEN_GUEST_HANDLE_PARAM(void) arg)
         if ( d == NULL )
             return -ESRCH;
 
-        if ( op == XENMEM_set_pod_target )
+        if ( cmd == XENMEM_set_pod_target )
             rc = xsm_set_pod_target(XSM_PRIV, d);
         else
             rc = xsm_get_pod_target(XSM_PRIV, d);
@@ -4845,7 +4844,7 @@ long arch_memory_op(unsigned long cmd, XEN_GUEST_HANDLE_PARAM(void) arg)
         if ( rc != 0 )
             goto pod_target_out_unlock;
 
-        if ( op == XENMEM_set_pod_target )
+        if ( cmd == XENMEM_set_pod_target )
         {
             if ( target.target_pages > d->max_pages )
             {
@@ -4859,7 +4858,7 @@ long arch_memory_op(unsigned long cmd, XEN_GUEST_HANDLE_PARAM(void) arg)
         if ( rc == -ERESTART )
         {
             rc = hypercall_create_continuation(
-                __HYPERVISOR_memory_op, "lh", op, arg);
+                __HYPERVISOR_memory_op, "lh", cmd, arg);
         }
         else if ( rc >= 0 )
         {

diff --git a/xen/arch/x86/x86_64/compat/mm.c b/xen/arch/x86/x86_64/compat/mm.c
index dce3f1fd02f0e9ef642fdf7748ae43d0278702b1..f90f611f52ff4fff806c3ac03b0b2b370b919a8b 100644 (file)
--- a/xen/arch/x86/x86_64/compat/mm.c
+++ b/xen/arch/x86/x86_64/compat/mm.c
@@ -53,9 +53,8 @@ int compat_arch_memory_op(unsigned long cmd, XEN_GUEST_HANDLE_PARAM(void) arg)
     compat_pfn_t mfn;
     unsigned int i;
     int rc = 0;
-    int op = cmd & MEMOP_CMD_MASK;
 
-    switch ( op )
+    switch ( cmd )
     {
     case XENMEM_set_memory_map:
     {
@@ -192,7 +191,7 @@ int compat_arch_memory_op(unsigned long cmd, XEN_GUEST_HANDLE_PARAM(void) arg)
         xen_mem_event_op_t meo;
         if ( copy_from_guest(&meo, arg, 1) )
             return -EFAULT;
-        rc = do_mem_event_op(op, meo.domain, (void *) &meo);
+        rc = do_mem_event_op(cmd, meo.domain, &meo);
         if ( !rc && __copy_to_guest(arg, &meo, 1) )
             return -EFAULT;
         break;
@@ -205,7 +204,7 @@ int compat_arch_memory_op(unsigned long cmd, XEN_GUEST_HANDLE_PARAM(void) arg)
             return -EFAULT;
         if ( mso.op == XENMEM_sharing_op_audit )
             return mem_sharing_audit(); 
-        rc = do_mem_event_op(op, mso.domain, (void *) &mso);
+        rc = do_mem_event_op(cmd, mso.domain, &mso);
         if ( !rc && __copy_to_guest(arg, &mso, 1) )
             return -EFAULT;
         break;

diff --git a/xen/arch/x86/x86_64/mm.c b/xen/arch/x86/x86_64/mm.c
index 8e5a1a18290bed258bdc8c68b4e5d5c4773bae30..d631aee614e5a005777fbe928c75a2749e99a529 100644 (file)
--- a/xen/arch/x86/x86_64/mm.c
+++ b/xen/arch/x86/x86_64/mm.c
@@ -906,9 +906,8 @@ long subarch_memory_op(unsigned long cmd, XEN_GUEST_HANDLE_PARAM(void) arg)
     xen_pfn_t mfn, last_mfn;
     unsigned int i;
     long rc = 0;
-    int op = cmd & MEMOP_CMD_MASK;
 
-    switch ( op )
+    switch ( cmd )
     {
     case XENMEM_machphys_mfn_list:
         if ( copy_from_guest(&xmml, arg, 1) )
@@ -989,7 +988,7 @@ long subarch_memory_op(unsigned long cmd, XEN_GUEST_HANDLE_PARAM(void) arg)
         xen_mem_event_op_t meo;
         if ( copy_from_guest(&meo, arg, 1) )
             return -EFAULT;
-        rc = do_mem_event_op(op, meo.domain, (void *) &meo);
+        rc = do_mem_event_op(cmd, meo.domain, &meo);
         if ( !rc && __copy_to_guest(arg, &meo, 1) )
             return -EFAULT;
         break;
@@ -1002,7 +1001,7 @@ long subarch_memory_op(unsigned long cmd, XEN_GUEST_HANDLE_PARAM(void) arg)
             return -EFAULT;
         if ( mso.op == XENMEM_sharing_op_audit )
             return mem_sharing_audit(); 
-        rc = do_mem_event_op(op, mso.domain, (void *) &mso);
+        rc = do_mem_event_op(cmd, mso.domain, &mso);
         if ( !rc && __copy_to_guest(arg, &mso, 1) )
             return -EFAULT;
         break;

diff --git a/xen/common/compat/grant_table.c b/xen/common/compat/grant_table.c
index 03682894f158079cca8f02b584466690c7ab0a51..f8c60a1bdf2d1bcb117189549b1ae2c75517347d 100644 (file)
--- a/xen/common/compat/grant_table.c
+++ b/xen/common/compat/grant_table.c
@@ -65,6 +65,8 @@ int compat_grant_table_op(unsigned int cmd,
 
     set_xen_guest_handle(cnt_uop, NULL);
     cmd_op = cmd & GNTTABOP_CMD_MASK;
+    if ( cmd_op != GNTTABOP_cache_flush )
+        cmd_op = cmd;
     switch ( cmd_op )
     {
 #define CASE(name) \

diff --git a/xen/common/grant_table.c b/xen/common/grant_table.c
index 8fba92343de46d3cf8daf1194d857a6e31ab5cdd..fe52b63e6e8ccaf926935bb60fe9a3f23137b7eb 100644 (file)
--- a/xen/common/grant_table.c
+++ b/xen/common/grant_table.c
@@ -62,6 +62,12 @@ integer_param("gnttab_max_frames", max_grant_frames);
 static unsigned int __read_mostly max_maptrack_frames;
 integer_param("gnttab_max_maptrack_frames", max_maptrack_frames);
 
+/*
+ * Note that the three values below are effectively part of the ABI, even if
+ * we don't need to make them a formal part of it: A guest suspended for
+ * migration in the middle of a continuation would fail to work if resumed on
+ * a hypervisor using different values.
+ */
 #define GNTTABOP_CONTINUATION_ARG_SHIFT 12
 #define GNTTABOP_CMD_MASK               ((1<<GNTTABOP_CONTINUATION_ARG_SHIFT)-1)
 #define GNTTABOP_ARG_MASK               (~GNTTABOP_CMD_MASK)
@@ -2624,9 +2630,12 @@ do_grant_table_op(
     
     if ( (int)count < 0 )
         return -EINVAL;
+
+    if ( (cmd &= GNTTABOP_CMD_MASK) != GNTTABOP_cache_flush && opaque_in )
+        return -ENOSYS;
     
     rc = -EFAULT;
-    switch ( cmd &= GNTTABOP_CMD_MASK )
+    switch ( cmd )
     {
     case GNTTABOP_map_grant_ref:
     {

diff --git a/xen/common/mem_access.c b/xen/common/mem_access.c
index 6c2724bef5bf0d15347b18b8d61e334746ccf6d3..d8aac5fb61f0efd31bc1827b704b41b183c8d44c 100644 (file)
--- a/xen/common/mem_access.c
+++ b/xen/common/mem_access.c
@@ -58,6 +58,7 @@ void mem_access_resume(struct domain *d)
 int mem_access_memop(unsigned long cmd,
                      XEN_GUEST_HANDLE_PARAM(xen_mem_access_op_t) arg)
 {
+    unsigned long start_iter = cmd & ~MEMOP_CMD_MASK;
     long rc;
     xen_mem_access_op_t mao;
     struct domain *d;
@@ -84,14 +85,16 @@ int mem_access_memop(unsigned long cmd,
     switch ( mao.op )
     {
     case XENMEM_access_op_resume:
-        mem_access_resume(d);
-        rc = 0;
+        if ( unlikely(start_iter) )
+            rc = -ENOSYS;
+        else
+        {
+            mem_access_resume(d);
+            rc = 0;
+        }
         break;
 
     case XENMEM_access_op_set_access:
-    {
-        unsigned long start_iter = cmd & ~MEMOP_CMD_MASK;
-
         rc = -EINVAL;
         if ( (mao.pfn != ~0ull) &&
              (mao.nr < start_iter ||
@@ -108,12 +111,15 @@ int mem_access_memop(unsigned long cmd,
                                                XENMEM_access_op | rc, arg);
         }
         break;
-    }
 
     case XENMEM_access_op_get_access:
     {
         xenmem_access_t access;
 
+        rc = -ENOSYS;
+        if ( unlikely(start_iter) )
+            break;
+
         rc = -EINVAL;
         if ( (mao.pfn > domain_get_maximum_gpfn(d)) && mao.pfn != ~0ull )
             break;

diff --git a/xen/common/memory.c b/xen/common/memory.c
index 9f21bd3b81d65aae9355afc70e8f21685322f140..234dae62fec10e753a45a5bcebf398dd19ff3277 100644 (file)
--- a/xen/common/memory.c
+++ b/xen/common/memory.c
@@ -779,16 +779,25 @@ long do_memory_op(unsigned long cmd, XEN_GUEST_HANDLE_PARAM(void) arg)
         break;
 
     case XENMEM_exchange:
+        if ( unlikely(start_extent) )
+            return -ENOSYS;
+
         rc = memory_exchange(guest_handle_cast(arg, xen_memory_exchange_t));
         break;
 
     case XENMEM_maximum_ram_page:
+        if ( unlikely(start_extent) )
+            return -ENOSYS;
+
         rc = max_page;
         break;
 
     case XENMEM_current_reservation:
     case XENMEM_maximum_reservation:
     case XENMEM_maximum_gpfn:
+        if ( unlikely(start_extent) )
+            return -ENOSYS;
+
         if ( copy_from_guest(&domid, arg, 1) )
             return -EFAULT;
 
@@ -912,6 +921,9 @@ long do_memory_op(unsigned long cmd, XEN_GUEST_HANDLE_PARAM(void) arg)
         struct page_info *page;
         struct domain *d;
 
+        if ( unlikely(start_extent) )
+            return -ENOSYS;
+
         if ( copy_from_guest(&xrfp, arg, 1) )
             return -EFAULT;
 
@@ -945,6 +957,9 @@ long do_memory_op(unsigned long cmd, XEN_GUEST_HANDLE_PARAM(void) arg)
         break;
 
     case XENMEM_claim_pages:
+        if ( unlikely(start_extent) )
+            return -ENOSYS;
+
         if ( copy_from_guest(&reservation, arg, 1) )
             return -EFAULT;
 
@@ -977,6 +992,9 @@ long do_memory_op(unsigned long cmd, XEN_GUEST_HANDLE_PARAM(void) arg)
         unsigned int dom_vnodes, dom_vranges, dom_vcpus;
         struct vnuma_info tmp;
 
+        if ( unlikely(start_extent) )
+            return -ENOSYS;
+
         /*
          * Guest passes nr_vnodes, number of regions and nr_vcpus thus
          * we know how much memory guest has allocated.

diff --git a/xen/include/xen/hypercall.h b/xen/include/xen/hypercall.h
index a9e522937dc7480994b26c6b4d88a0a728c37cd2..8c55779ff801d3d97cfca0150c438211d861cafe 100644 (file)
--- a/xen/include/xen/hypercall.h
+++ b/xen/include/xen/hypercall.h
@@ -57,6 +57,11 @@ do_platform_op(
  * To allow safe resume of do_memory_op() after preemption, we need to know
  * at what point in the page list to resume. For this purpose I steal the
  * high-order bits of the @cmd parameter, which are otherwise unused and zero.
+ *
+ * Note that both of these values are effectively part of the ABI, even if
+ * we don't need to make them a formal part of it: A guest suspended for
+ * migration in the middle of a continuation would fail to work if resumed on
+ * a hypervisor using different values.
  */
 #define MEMOP_EXTENT_SHIFT 6 /* cmd[:6] == start_extent */
 #define MEMOP_CMD_MASK     ((1 << MEMOP_EXTENT_SHIFT) - 1)