[PATCH] lock down hypercall continuation encoding masks

[PATCH] lock down hypercall continuation encoding masks

[Jan Beulich]

[-- Attachment #1: Type: text/plain, Size: 10991 bytes --]

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>

--- 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_GUE
 
     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);
     }
--- 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, X
         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, X
         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, X
         if ( rc == -ERESTART )
         {
             rc = hypercall_create_continuation(
-                __HYPERVISOR_memory_op, "lh", op, arg);
+                __HYPERVISOR_memory_op, "lh", cmd, arg);
         }
         else if ( rc >= 0 )
         {
--- 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 
     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 
         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 
             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;
--- 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_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_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
             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;
--- 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 c
 
     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) \
--- a/xen/common/grant_table.c
+++ b/xen/common/grant_table.c
@@ -62,6 +62,12 @@ integer_param("gnttab_max_frames", max_g
 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:
     {
--- 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;
--- a/xen/common/memory.c
+++ b/xen/common/memory.c
@@ -779,16 +779,25 @@ long do_memory_op(unsigned long cmd, XEN
         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
         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
         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
         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.
--- 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)



[-- Attachment #2: continuation-masks.patch --]
[-- Type: text/plain, Size: 11038 bytes --]

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>

--- 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_GUE
 
     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);
     }
--- 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, X
         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, X
         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, X
         if ( rc == -ERESTART )
         {
             rc = hypercall_create_continuation(
-                __HYPERVISOR_memory_op, "lh", op, arg);
+                __HYPERVISOR_memory_op, "lh", cmd, arg);
         }
         else if ( rc >= 0 )
         {
--- 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 
     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 
         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 
             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;
--- 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_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_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
             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;
--- 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 c
 
     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) \
--- a/xen/common/grant_table.c
+++ b/xen/common/grant_table.c
@@ -62,6 +62,12 @@ integer_param("gnttab_max_frames", max_g
 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:
     {
--- 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;
--- a/xen/common/memory.c
+++ b/xen/common/memory.c
@@ -779,16 +779,25 @@ long do_memory_op(unsigned long cmd, XEN
         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
         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
         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
         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.
--- 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)

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

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

Re: [PATCH] lock down hypercall continuation encoding masks

[Andrew Cooper]

On 05/12/14 11:31, Jan Beulich wrote:
> 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>

General principle looks good.  A couple of issues.

> --- 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;

This needs a blanket start_iter check, as do_memory_op() has not done so.

The ARM code also needs one, as the caller has applied partial checks.

>  
> -    switch ( op )
> +    switch ( cmd )
>      {
>      case XENMEM_set_memory_map:
>      {
> --- 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_pfn_t mfn, last_mfn;
>      unsigned int i;
>      long rc = 0;
> -    int op = cmd & MEMOP_CMD_MASK;

It is probably best to have a blanket check here even if
arch_memory_op() has a check.  It will reduce the chance of the check
being missed if/when arch_memory_op() gains a presentable subop.

>  
> -    switch ( op )
> +    switch ( cmd )
>      {
>      case XENMEM_machphys_mfn_list:
>          if ( copy_from_guest(&xmml, arg, 1) )
> --- a/xen/common/memory.c
> +++ b/xen/common/memory.c
> @@ -977,6 +992,9 @@ long do_memory_op(unsigned long cmd, XEN
>          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.

XENMEM_get_vnumainfo needs a guard.

~Andrew

Re: [PATCH] lock down hypercall continuation encoding masks

[Jan Beulich]

>>> On 05.12.14 at 15:36, <andrew.cooper3@citrix.com> wrote:
> On 05/12/14 11:31, Jan Beulich wrote:
>> 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>
> 
> General principle looks good.  A couple of issues.
> 
>> --- 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;
> 
> This needs a blanket start_iter check, as do_memory_op() has not done so.

Not sure what you're asking for - why is removing the masking not
sufficient?

> The ARM code also needs one, as the caller has applied partial checks.

The ARM code never applied a mask.

>> --- a/xen/common/memory.c
>> +++ b/xen/common/memory.c
>> @@ -977,6 +992,9 @@ long do_memory_op(unsigned long cmd, XEN
>>          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.
> 
> XENMEM_get_vnumainfo needs a guard.

Again - I don't understand what you're asking for: The hunk above
is modifying the XENMEM_get_vnumainfo case.

Jan

Re: [PATCH] lock down hypercall continuation encoding masks

[Andrew Cooper]

On 05/12/14 14:47, Jan Beulich wrote:
>>>> On 05.12.14 at 15:36, <andrew.cooper3@citrix.com> wrote:
>> On 05/12/14 11:31, Jan Beulich wrote:
>>> 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>
>> General principle looks good.  A couple of issues.
>>
>>> --- 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;
>> This needs a blanket start_iter check, as do_memory_op() has not done so.
> Not sure what you're asking for - why is removing the masking not
> sufficient?

There is no check to ensure that a non-preemptible arch_memoy_op is not
called with a non-zero start_iter.

This location needs something like

if ( cmd & ~MEMOP_CMD_MASK )
    return -ENOSYS;

>
>> The ARM code also needs one, as the caller has applied partial checks.
> The ARM code never applied a mask.

But the common code does, so the ARM code must follow suit for consistency.

Otherwise, we end up with ARM non-preemptible memory subops not failing
with -ENOSYS where primary memory ops would.

>
>>> --- a/xen/common/memory.c
>>> +++ b/xen/common/memory.c
>>> @@ -977,6 +992,9 @@ long do_memory_op(unsigned long cmd, XEN
>>>          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.
>> XENMEM_get_vnumainfo needs a guard.
> Again - I don't understand what you're asking for: The hunk above
> is modifying the XENMEM_get_vnumainfo case.

My apologies - I can't see now why I identified get_vnumainfo as missing
a check.

~Andrew

Re: [PATCH] lock down hypercall continuation encoding masks

[Jan Beulich]

>>> On 05.12.14 at 16:01, <andrew.cooper3@citrix.com> wrote:
> On 05/12/14 14:47, Jan Beulich wrote:
>>>>> On 05.12.14 at 15:36, <andrew.cooper3@citrix.com> wrote:
>>> On 05/12/14 11:31, Jan Beulich wrote:
>>>> 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>
>>> General principle looks good.  A couple of issues.
>>>
>>>> --- 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;
>>> This needs a blanket start_iter check, as do_memory_op() has not done so.
>> Not sure what you're asking for - why is removing the masking not
>> sufficient?
> 
> There is no check to ensure that a non-preemptible arch_memoy_op is not
> called with a non-zero start_iter.
> 
> This location needs something like
> 
> if ( cmd & ~MEMOP_CMD_MASK )
>     return -ENOSYS;

I'm sorry - the default case of sub_arch_memory_op() will ensure
this.

>>> The ARM code also needs one, as the caller has applied partial checks.
>> The ARM code never applied a mask.
> 
> But the common code does, so the ARM code must follow suit for consistency.
> 
> Otherwise, we end up with ARM non-preemptible memory subops not failing
> with -ENOSYS where primary memory ops would.

Again, the default case results in -ENOSYS for any with the high
bits set.

Jan

Re: [PATCH] lock down hypercall continuation encoding masks

[Andrew Cooper]

On 05/12/14 15:18, Jan Beulich wrote:
>>>> On 05.12.14 at 16:01, <andrew.cooper3@citrix.com> wrote:
>> On 05/12/14 14:47, Jan Beulich wrote:
>>>>>> On 05.12.14 at 15:36, <andrew.cooper3@citrix.com> wrote:
>>>> On 05/12/14 11:31, Jan Beulich wrote:
>>>>> 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>
>>>> General principle looks good.  A couple of issues.
>>>>
>>>>> --- 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;
>>>> This needs a blanket start_iter check, as do_memory_op() has not done so.
>>> Not sure what you're asking for - why is removing the masking not
>>> sufficient?
>> There is no check to ensure that a non-preemptible arch_memoy_op is not
>> called with a non-zero start_iter.
>>
>> This location needs something like
>>
>> if ( cmd & ~MEMOP_CMD_MASK )
>>     return -ENOSYS;
> I'm sorry - the default case of sub_arch_memory_op() will ensure
> this.

Ah - I see now.  That is subtle.  Better remember to double check the
first patch which needs to add a preemptible subop.

Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>

Re: [PATCH] lock down hypercall continuation encoding masks

[Ian Campbell]

On Fri, 2014-12-05 at 11:31 +0000, Jan Beulich wrote:
> 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>

Acked-by: Ian Campbell <ian.campbell@citrix.com>

Re: [PATCH] lock down hypercall continuation encoding masks

[Tim Deegan]

At 11:31 +0000 on 05 Dec (1417775465), Jan Beulich wrote:
> 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>

Acked-by: Tim Deegan <tim@xen.org>