All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH 0/2] Post-XSA-127 cleanup
@ 2015-04-01 15:31 Andrew Cooper
  2015-04-01 15:31 ` [PATCH 1/2] x86/domctl: cleanup Andrew Cooper
  2015-04-01 15:31 ` [PATCH 2/2] x86/domctl: Don't allow a toolstack domain to pause itself Andrew Cooper
  0 siblings, 2 replies; 8+ messages in thread
From: Andrew Cooper @ 2015-04-01 15:31 UTC (permalink / raw)
  To: Xen-devel; +Cc: Andrew Cooper

XSA-127 was actually discovered while doing the cleanup in patch 1

Patch 2 contains futher domain/vcpu pause related issues, but all in subops
still excluded by the XSA-77 list, which do not qualify as a security issue.

Andrew Cooper (2):
  x86/domctl: cleanup
  x86/domctl: Don't allow a toolstack domain to pause itself

 xen/arch/x86/domctl.c |  280 +++++++++++++++++++++----------------------------
 1 file changed, 117 insertions(+), 163 deletions(-)

-- 
1.7.10.4

^ permalink raw reply	[flat|nested] 8+ messages in thread

* [PATCH 1/2] x86/domctl: cleanup
  2015-04-01 15:31 [PATCH 0/2] Post-XSA-127 cleanup Andrew Cooper
@ 2015-04-01 15:31 ` Andrew Cooper
  2015-04-01 20:13   ` Konrad Rzeszutek Wilk
  2015-04-13 14:27   ` Jan Beulich
  2015-04-01 15:31 ` [PATCH 2/2] x86/domctl: Don't allow a toolstack domain to pause itself Andrew Cooper
  1 sibling, 2 replies; 8+ messages in thread
From: Andrew Cooper @ 2015-04-01 15:31 UTC (permalink / raw)
  To: Xen-devel; +Cc: Andrew Cooper, Keir Fraser, Jan Beulich

 * latch curr/currd once at start
 * drop redundant "ret = 0" and braces
 * use "copyback = 1" when appropriate
 * move break statements inside case-specific braced scopes
 * don't bother check for NULL before calling xfree()
 * eliminate trailing whitespace
 * Xen style corrections

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
CC: Keir Fraser <keir@xen.org>
CC: Jan Beulich <JBeulich@suse.com>
---
 xen/arch/x86/domctl.c |  276 ++++++++++++++++++++-----------------------------
 1 file changed, 113 insertions(+), 163 deletions(-)

diff --git a/xen/arch/x86/domctl.c b/xen/arch/x86/domctl.c
index 32d3fcd..bcbdf95 100644
--- a/xen/arch/x86/domctl.c
+++ b/xen/arch/x86/domctl.c
@@ -1,6 +1,6 @@
 /******************************************************************************
  * Arch-specific domctl.c
- * 
+ *
  * Copyright (c) 2002-2006, K A Fraser
  */
 
@@ -39,7 +39,7 @@
 
 static int gdbsx_guest_mem_io(
     domid_t domid, struct xen_domctl_gdbsx_memio *iop)
-{   
+{
     ulong l_uva = (ulong)iop->uva;
     iop->remain = dbg_rw_mem(
         (dbgva_t)iop->gva, (dbgbyte_t *)l_uva, iop->len, domid,
@@ -53,6 +53,8 @@ long arch_do_domctl(
     struct xen_domctl *domctl, struct domain *d,
     XEN_GUEST_HANDLE_PARAM(xen_domctl_t) u_domctl)
 {
+    struct vcpu *curr = current;
+    struct domain *currd = curr->domain;
     long ret = 0;
     bool_t copyback = 0;
     unsigned long i;
@@ -61,15 +63,13 @@ long arch_do_domctl(
     {
 
     case XEN_DOMCTL_shadow_op:
-    {
         ret = paging_domctl(d, &domctl->u.shadow_op,
                             guest_handle_cast(u_domctl, void), 0);
         if ( ret == -ERESTART )
             return hypercall_create_continuation(__HYPERVISOR_arch_1,
                                                  "h", u_domctl);
         copyback = 1;
-    }
-    break;
+        break;
 
     case XEN_DOMCTL_ioport_permission:
     {
@@ -79,8 +79,7 @@ long arch_do_domctl(
 
         if ( (fp + np) <= fp || (fp + np) > MAX_IOPORTS )
             ret = -EINVAL;
-        else if ( !ioports_access_permitted(current->domain,
-                                            fp, fp + np - 1) ||
+        else if ( !ioports_access_permitted(currd, fp, fp + np - 1) ||
                   xsm_ioport_permission(XSM_HOOK, d, fp, fp + np - 1, allow) )
             ret = -EPERM;
         else if ( allow )
@@ -89,8 +88,8 @@ long arch_do_domctl(
             ret = ioports_deny_access(d, fp, fp + np - 1);
         if ( !ret )
             memory_type_changed(d);
+        break;
     }
-    break;
 
     case XEN_DOMCTL_getpageframeinfo:
     {
@@ -127,16 +126,16 @@ long arch_do_domctl(
                     break;
                 }
             }
-            
+
             put_page(page);
         }
 
         copyback = 1;
+        break;
     }
-    break;
 
     case XEN_DOMCTL_getpageframeinfo3:
-        if (!has_32bit_shinfo(current->domain))
+        if ( !has_32bit_shinfo(currd) )
         {
             unsigned int n, j;
             unsigned int num = domctl->u.getpageframeinfo3.num;
@@ -150,7 +149,7 @@ long arch_do_domctl(
                 break;
             }
 
-            page = alloc_domheap_page(current->domain, MEMF_no_owner);
+            page = alloc_domheap_page(currd, MEMF_no_owner);
             if ( !page )
             {
                 ret = -ENOMEM;
@@ -251,7 +250,7 @@ long arch_do_domctl(
             ret = -ENOMEM;
             break;
         }
- 
+
         ret = 0;
         for ( n = 0; n < num; )
         {
@@ -266,9 +265,9 @@ long arch_do_domctl(
                 ret = -EFAULT;
                 break;
             }
-     
+
             for ( j = 0; j < k; j++ )
-            {      
+            {
                 struct page_info *page;
                 unsigned long gfn = arr32[j];
 
@@ -320,8 +319,8 @@ long arch_do_domctl(
         }
 
         free_xenheap_page(arr32);
+        break;
     }
-    break;
 
     case XEN_DOMCTL_getmemlist:
     {
@@ -329,7 +328,8 @@ long arch_do_domctl(
         uint64_t mfn;
         struct page_info *page;
 
-        if ( unlikely(d->is_dying) ) {
+        if ( unlikely(d->is_dying) )
+        {
             ret = -EINVAL;
             break;
         }
@@ -346,7 +346,7 @@ long arch_do_domctl(
          * rather than trying to fix it we restrict it for the time being.
          */
         if ( /* No nested locks inside copy_to_guest_offset(). */
-             paging_mode_external(current->domain) ||
+             paging_mode_external(currd) ||
              /* Arbitrary limit capping processing time. */
              max_pfns > GB(4) / PAGE_SIZE )
         {
@@ -375,8 +375,8 @@ long arch_do_domctl(
 
         domctl->u.getmemlist.num_pfns = i;
         copyback = 1;
+        break;
     }
-    break;
 
     case XEN_DOMCTL_hypercall_init:
     {
@@ -403,15 +403,15 @@ long arch_do_domctl(
         unmap_domain_page(hypercall_page);
 
         put_page_and_type(page);
+        break;
     }
-    break;
 
     case XEN_DOMCTL_sethvmcontext:
-    { 
+    {
         struct hvm_domain_context c = { .size = domctl->u.hvmcontext.size };
 
         ret = -EINVAL;
-        if ( !is_hvm_domain(d) ) 
+        if ( !is_hvm_domain(d) )
             goto sethvmcontext_out;
 
         ret = -ENOMEM;
@@ -419,7 +419,7 @@ long arch_do_domctl(
             goto sethvmcontext_out;
 
         ret = -EFAULT;
-        if ( copy_from_guest(c.data, domctl->u.hvmcontext.buffer, c.size) != 0)
+        if ( copy_from_guest(c.data, domctl->u.hvmcontext.buffer, c.size) != 0 )
             goto sethvmcontext_out;
 
         domain_pause(d);
@@ -427,17 +427,16 @@ long arch_do_domctl(
         domain_unpause(d);
 
     sethvmcontext_out:
-        if ( c.data != NULL )
-            xfree(c.data);
+        xfree(c.data);
+        break;
     }
-    break;
 
     case XEN_DOMCTL_gethvmcontext:
-    { 
+    {
         struct hvm_domain_context c = { 0 };
 
         ret = -EINVAL;
-        if ( !is_hvm_domain(d) ) 
+        if ( !is_hvm_domain(d) )
             goto gethvmcontext_out;
 
         c.size = hvm_save_size(d);
@@ -447,12 +446,12 @@ long arch_do_domctl(
             /* Client is querying for the correct buffer size */
             domctl->u.hvmcontext.size = c.size;
             ret = 0;
-            goto gethvmcontext_out;            
+            goto gethvmcontext_out;
         }
 
         /* Check that the client has a big enough buffer */
         ret = -ENOSPC;
-        if ( domctl->u.hvmcontext.size < c.size ) 
+        if ( domctl->u.hvmcontext.size < c.size )
             goto gethvmcontext_out;
 
         /* Allocate our own marshalling buffer */
@@ -470,16 +469,13 @@ long arch_do_domctl(
 
     gethvmcontext_out:
         copyback = 1;
-
-        if ( c.data != NULL )
-            xfree(c.data);
+        xfree(c.data);
+        break;
     }
-    break;
 
     case XEN_DOMCTL_gethvmcontext_partial:
-    { 
         ret = -EINVAL;
-        if ( !is_hvm_domain(d) ) 
+        if ( !is_hvm_domain(d) )
             break;
 
         domain_pause(d);
@@ -487,12 +483,9 @@ long arch_do_domctl(
                            domctl->u.hvmcontext_partial.instance,
                            domctl->u.hvmcontext_partial.buffer);
         domain_unpause(d);
-    }
-    break;
-
+        break;
 
     case XEN_DOMCTL_set_address_size:
-    {
         switch ( domctl->u.address_size.size )
         {
         case 32:
@@ -505,39 +498,25 @@ long arch_do_domctl(
             ret = (domctl->u.address_size.size == BITS_PER_LONG) ? 0 : -EINVAL;
             break;
         }
-    }
-    break;
+        break;
 
     case XEN_DOMCTL_get_address_size:
-    {
         domctl->u.address_size.size =
             is_pv_32on64_domain(d) ? 32 : BITS_PER_LONG;
-
-        ret = 0;
         copyback = 1;
-    }
-    break;
+        break;
 
     case XEN_DOMCTL_set_machine_address_size:
-    {
-        ret = -EBUSY;
         if ( d->tot_pages > 0 )
-            break;
-
-        d->arch.physaddr_bitsize = domctl->u.address_size.size;
-
-        ret = 0;
-    }
-    break;
+            ret = -EBUSY;
+        else
+            d->arch.physaddr_bitsize = domctl->u.address_size.size;
+        break;
 
     case XEN_DOMCTL_get_machine_address_size:
-    {
         domctl->u.address_size.size = d->arch.physaddr_bitsize;
-
-        ret = 0;
         copyback = 1;
-    }
-    break;
+        break;
 
     case XEN_DOMCTL_sendtrigger:
     {
@@ -551,40 +530,34 @@ long arch_do_domctl(
         switch ( domctl->u.sendtrigger.trigger )
         {
         case XEN_DOMCTL_SENDTRIGGER_NMI:
-        {
             ret = 0;
             if ( !test_and_set_bool(v->nmi_pending) )
                 vcpu_kick(v);
-        }
-        break;
+            break;
 
         case XEN_DOMCTL_SENDTRIGGER_POWER:
-        {
             ret = -EINVAL;
-            if ( is_hvm_domain(d) ) 
+            if ( is_hvm_domain(d) )
             {
                 ret = 0;
                 hvm_acpi_power_button(d);
             }
-        }
-        break;
+            break;
 
         case XEN_DOMCTL_SENDTRIGGER_SLEEP:
-        {
             ret = -EINVAL;
-            if ( is_hvm_domain(d) ) 
+            if ( is_hvm_domain(d) )
             {
                 ret = 0;
                 hvm_acpi_sleep_button(d);
             }
-        }
-        break;
+            break;
 
         default:
             ret = -ENOSYS;
         }
+        break;
     }
-    break;
 
     case XEN_DOMCTL_bind_pt_irq:
     {
@@ -601,7 +574,7 @@ long arch_do_domctl(
 
         irq = domain_pirq_to_irq(d, bind->machine_irq);
         ret = -EPERM;
-        if ( irq <= 0 || !irq_access_permitted(current->domain, irq) )
+        if ( irq <= 0 || !irq_access_permitted(currd, irq) )
             break;
 
         ret = -ESRCH;
@@ -614,8 +587,8 @@ long arch_do_domctl(
         if ( ret < 0 )
             printk(XENLOG_G_ERR "pt_irq_create_bind failed (%ld) for dom%d\n",
                    ret, d->domain_id);
+        break;
     }
-    break;    
 
     case XEN_DOMCTL_unbind_pt_irq:
     {
@@ -623,7 +596,7 @@ long arch_do_domctl(
         int irq = domain_pirq_to_irq(d, bind->machine_irq);
 
         ret = -EPERM;
-        if ( irq <= 0 || !irq_access_permitted(current->domain, irq) )
+        if ( irq <= 0 || !irq_access_permitted(currd, irq) )
             break;
 
         ret = xsm_unbind_pt_irq(XSM_HOOK, d, bind);
@@ -639,8 +612,8 @@ long arch_do_domctl(
         if ( ret < 0 )
             printk(XENLOG_G_ERR "pt_irq_destroy_bind failed (%ld) for dom%d\n",
                    ret, d->domain_id);
+        break;
     }
-    break;
 
     case XEN_DOMCTL_ioport_mapping:
     {
@@ -663,7 +636,7 @@ long arch_do_domctl(
         }
 
         ret = -EPERM;
-        if ( !ioports_access_permitted(current->domain, fmp, fmp + np - 1) )
+        if ( !ioports_access_permitted(currd, fmp, fmp + np - 1) )
             break;
 
         ret = xsm_ioport_mapping(XSM_HOOK, d, fmp, fmp + np - 1, add);
@@ -719,33 +692,29 @@ long arch_do_domctl(
                     break;
                 }
             ret = ioports_deny_access(d, fmp, fmp + np - 1);
-            if ( ret && is_hardware_domain(current->domain) )
+            if ( ret && is_hardware_domain(currd) )
                 printk(XENLOG_ERR
                        "ioport_map: error %ld denying dom%d access to [%x,%x]\n",
                        ret, d->domain_id, fmp, fmp + np - 1);
         }
         if ( !ret )
             memory_type_changed(d);
+        break;
     }
-    break;
 
     case XEN_DOMCTL_pin_mem_cacheattr:
-    {
         ret = hvm_set_mem_pinned_cacheattr(
             d, domctl->u.pin_mem_cacheattr.start,
             domctl->u.pin_mem_cacheattr.end,
             domctl->u.pin_mem_cacheattr.type);
-    }
-    break;
+        break;
 
     case XEN_DOMCTL_set_ext_vcpucontext:
     case XEN_DOMCTL_get_ext_vcpucontext:
     {
-        struct xen_domctl_ext_vcpucontext *evc;
+        struct xen_domctl_ext_vcpucontext *evc = &domctl->u.ext_vcpucontext;
         struct vcpu *v;
 
-        evc = &domctl->u.ext_vcpucontext;
-
         ret = -ESRCH;
         if ( (evc->vcpu >= d->max_vcpus) ||
              ((v = d->vcpu[evc->vcpu]) == NULL) )
@@ -753,7 +722,7 @@ long arch_do_domctl(
 
         if ( domctl->cmd == XEN_DOMCTL_get_ext_vcpucontext )
         {
-            if ( v == current ) /* no vcpu_pause() */
+            if ( v == curr ) /* no vcpu_pause() */
                 break;
 
             evc->size = sizeof(*evc);
@@ -794,7 +763,7 @@ long arch_do_domctl(
         }
         else
         {
-            if ( d == current->domain ) /* no domain_pause() */
+            if ( d == currd ) /* no domain_pause() */
                 break;
             ret = -EINVAL;
             if ( evc->size < offsetof(typeof(*evc), vmce) )
@@ -848,8 +817,8 @@ long arch_do_domctl(
 
             domain_unpause(d);
         }
+        break;
     }
-    break;
 
     case XEN_DOMCTL_set_cpuid:
     {
@@ -872,60 +841,50 @@ long arch_do_domctl(
                   (cpuid->input[1] == ctl->input[1])) )
                 break;
         }
-        
+
         if ( i < MAX_CPUID_INPUT )
             *cpuid = *ctl;
         else if ( unused )
             *unused = *ctl;
         else
             ret = -ENOENT;
+        break;
     }
-    break;
 
     case XEN_DOMCTL_gettscinfo:
-    {
-        xen_guest_tsc_info_t info;
-
-        ret = -EINVAL;
-        if ( d == current->domain ) /* no domain_pause() */
-            break;
-
-        domain_pause(d);
-        tsc_get_info(d, &info.tsc_mode,
-                        &info.elapsed_nsec,
-                        &info.gtsc_khz,
-                        &info.incarnation);
-        if ( copy_to_guest(domctl->u.tsc_info.out_info, &info, 1) )
-            ret = -EFAULT;
+        if ( d == currd ) /* no domain_pause() */
+            ret = -EINVAL;
         else
-            ret = 0;
-        domain_unpause(d);
-    }
-    break;
+        {
+            xen_guest_tsc_info_t info;
+
+            domain_pause(d);
+            tsc_get_info(d, &info.tsc_mode,
+                         &info.elapsed_nsec,
+                         &info.gtsc_khz,
+                         &info.incarnation);
+            domain_unpause(d);
+            copyback = 1;
+        }
+        break;
 
     case XEN_DOMCTL_settscinfo:
-    {
-        ret = -EINVAL;
-        if ( d == current->domain ) /* no domain_pause() */
-            break;
-
-        domain_pause(d);
-        tsc_set_info(d, domctl->u.tsc_info.info.tsc_mode,
-                     domctl->u.tsc_info.info.elapsed_nsec,
-                     domctl->u.tsc_info.info.gtsc_khz,
-                     domctl->u.tsc_info.info.incarnation);
-        domain_unpause(d);
-
-        ret = 0;
-    }
-    break;
+        if ( d == currd ) /* no domain_pause() */
+            ret = -EINVAL;
+        else
+        {
+            domain_pause(d);
+            tsc_set_info(d, domctl->u.tsc_info.info.tsc_mode,
+                         domctl->u.tsc_info.info.elapsed_nsec,
+                         domctl->u.tsc_info.info.gtsc_khz,
+                         domctl->u.tsc_info.info.incarnation);
+            domain_unpause(d);
+        }
+        break;
 
     case XEN_DOMCTL_suppress_spurious_page_faults:
-    {
         d->arch.suppress_spurious_page_faults = 1;
-        ret = 0;
-    }
-    break;
+        break;
 
     case XEN_DOMCTL_debug_op:
     {
@@ -941,19 +900,15 @@ long arch_do_domctl(
             break;
 
         ret = hvm_debug_op(v, domctl->u.debug_op.op);
+        break;
     }
-    break;
 
     case XEN_DOMCTL_gdbsx_guestmemio:
-    {
-        domctl->u.gdbsx_guest_memio.remain =
-            domctl->u.gdbsx_guest_memio.len;
-
+        domctl->u.gdbsx_guest_memio.remain = domctl->u.gdbsx_guest_memio.len;
         ret = gdbsx_guest_mem_io(domctl->domain, &domctl->u.gdbsx_guest_memio);
         if ( !ret )
            copyback = 1;
-    }
-    break;
+        break;
 
     case XEN_DOMCTL_gdbsx_pausevcpu:
     {
@@ -967,8 +922,8 @@ long arch_do_domctl(
              (v = d->vcpu[domctl->u.gdbsx_pauseunp_vcpu.vcpu]) == NULL )
             break;
         ret = vcpu_pause_by_systemcontroller(v);
+        break;
     }
-    break;
 
     case XEN_DOMCTL_gdbsx_unpausevcpu:
     {
@@ -985,9 +940,9 @@ long arch_do_domctl(
         if ( ret == -EINVAL )
             printk(XENLOG_G_WARNING
                    "WARN: d%d attempting to unpause %pv which is not paused\n",
-                   current->domain->domain_id, v);
+                   currd->domain_id, v);
+        break;
     }
-    break;
 
     case XEN_DOMCTL_gdbsx_domstatus:
     {
@@ -1009,29 +964,26 @@ long arch_do_domctl(
                 }
             }
         }
-        ret = 0;
         copyback = 1;
+        break;
     }
-    break;
 
     case XEN_DOMCTL_setvcpuextstate:
     case XEN_DOMCTL_getvcpuextstate:
     {
-        struct xen_domctl_vcpuextstate *evc;
+        struct xen_domctl_vcpuextstate *evc = &domctl->u.vcpuextstate;
         struct vcpu *v;
         uint32_t offset = 0;
 
 #define PV_XSAVE_SIZE(xcr0) (2 * sizeof(uint64_t) + xstate_ctxt_size(xcr0))
 
-        evc = &domctl->u.vcpuextstate;
-
         ret = -ESRCH;
         if ( (evc->vcpu >= d->max_vcpus) ||
              ((v = d->vcpu[evc->vcpu]) == NULL) )
             goto vcpuextstate_out;
 
         ret = -EINVAL;
-        if ( v == current ) /* no vcpu_pause() */
+        if ( v == curr ) /* no vcpu_pause() */
             goto vcpuextstate_out;
 
         if ( domctl->cmd == XEN_DOMCTL_getvcpuextstate )
@@ -1137,31 +1089,26 @@ long arch_do_domctl(
     vcpuextstate_out:
         if ( domctl->cmd == XEN_DOMCTL_getvcpuextstate )
             copyback = 1;
+        break;
     }
-    break;
 
     case XEN_DOMCTL_mem_sharing_op:
-    {
         ret = mem_sharing_domctl(d, &domctl->u.mem_sharing_op);
-    }
-    break;
+        break;
 
 #if P2M_AUDIT
     case XEN_DOMCTL_audit_p2m:
-    {
-        if ( d == current->domain )
-        {
+        if ( d == currd )
             ret = -EPERM;
-            break;
+        else
+        {
+            audit_p2m(d,
+                      &domctl->u.audit_p2m.orphans,
+                      &domctl->u.audit_p2m.m2p_bad,
+                      &domctl->u.audit_p2m.p2m_bad);
+            copyback = 1;
         }
-
-        audit_p2m(d,
-                  &domctl->u.audit_p2m.orphans,
-                  &domctl->u.audit_p2m.m2p_bad,
-                  &domctl->u.audit_p2m.p2m_bad);
-        copyback = 1;
-    }
-    break;
+        break;
 #endif /* P2M_AUDIT */
 
     case XEN_DOMCTL_set_broken_page_p2m:
@@ -1176,8 +1123,8 @@ long arch_do_domctl(
             ret = p2m_change_type_one(d, pfn, pt, p2m_ram_broken);
 
         put_gfn(d, pfn);
+        break;
     }
-    break;
 
     case XEN_DOMCTL_get_vcpu_msrs:
     case XEN_DOMCTL_set_vcpu_msrs:
@@ -1193,7 +1140,7 @@ long arch_do_domctl(
             break;
 
         ret = -EINVAL;
-        if ( (v == current) || /* no vcpu_pause() */
+        if ( (v == curr) || /* no vcpu_pause() */
              !is_pv_domain(d) )
             break;
 
@@ -1303,8 +1250,8 @@ long arch_do_domctl(
                 copyback = 1;
             }
         }
+        break;
     }
-    break;
 
     case XEN_DOMCTL_psr_cmt_op:
         if ( !psr_cmt_enabled() )
@@ -1318,16 +1265,19 @@ long arch_do_domctl(
         case XEN_DOMCTL_PSR_CMT_OP_ATTACH:
             ret = psr_alloc_rmid(d);
             break;
+
         case XEN_DOMCTL_PSR_CMT_OP_DETACH:
             if ( d->arch.psr_rmid > 0 )
                 psr_free_rmid(d);
             else
                 ret = -ENOENT;
             break;
+
         case XEN_DOMCTL_PSR_CMT_OP_QUERY_RMID:
             domctl->u.psr_cmt_op.data = d->arch.psr_rmid;
             copyback = 1;
             break;
+
         default:
             ret = -ENOSYS;
             break;
-- 
1.7.10.4

^ permalink raw reply related	[flat|nested] 8+ messages in thread

* [PATCH 2/2] x86/domctl: Don't allow a toolstack domain to pause itself
  2015-04-01 15:31 [PATCH 0/2] Post-XSA-127 cleanup Andrew Cooper
  2015-04-01 15:31 ` [PATCH 1/2] x86/domctl: cleanup Andrew Cooper
@ 2015-04-01 15:31 ` Andrew Cooper
  2015-04-01 20:14   ` Konrad Rzeszutek Wilk
  1 sibling, 1 reply; 8+ messages in thread
From: Andrew Cooper @ 2015-04-01 15:31 UTC (permalink / raw)
  To: Xen-devel; +Cc: Andrew Cooper, Keir Fraser, Jan Beulich

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
CC: Keir Fraser <keir@xen.org>
CC: Jan Beulich <JBeulich@suse.com>
---
 xen/arch/x86/domctl.c |   12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)

diff --git a/xen/arch/x86/domctl.c b/xen/arch/x86/domctl.c
index bcbdf95..ff3b423 100644
--- a/xen/arch/x86/domctl.c
+++ b/xen/arch/x86/domctl.c
@@ -411,7 +411,8 @@ long arch_do_domctl(
         struct hvm_domain_context c = { .size = domctl->u.hvmcontext.size };
 
         ret = -EINVAL;
-        if ( !is_hvm_domain(d) )
+        if ( (d == currd) || /* no domain_pause() */
+             !is_hvm_domain(d) )
             goto sethvmcontext_out;
 
         ret = -ENOMEM;
@@ -436,7 +437,8 @@ long arch_do_domctl(
         struct hvm_domain_context c = { 0 };
 
         ret = -EINVAL;
-        if ( !is_hvm_domain(d) )
+        if ( (d == currd) || /* no domain_pause() */
+             !is_hvm_domain(d) )
             goto gethvmcontext_out;
 
         c.size = hvm_save_size(d);
@@ -475,7 +477,8 @@ long arch_do_domctl(
 
     case XEN_DOMCTL_gethvmcontext_partial:
         ret = -EINVAL;
-        if ( !is_hvm_domain(d) )
+        if ( (d == currd) || /* no domain_pause() */
+             !is_hvm_domain(d) )
             break;
 
         domain_pause(d);
@@ -896,7 +899,8 @@ long arch_do_domctl(
             break;
 
         ret = -EINVAL;
-        if ( !is_hvm_domain(d))
+        if ( (v == curr) || /* no vcpu_pause() */
+             !is_hvm_domain(d) )
             break;
 
         ret = hvm_debug_op(v, domctl->u.debug_op.op);
-- 
1.7.10.4

^ permalink raw reply related	[flat|nested] 8+ messages in thread

* Re: [PATCH 1/2] x86/domctl: cleanup
  2015-04-01 15:31 ` [PATCH 1/2] x86/domctl: cleanup Andrew Cooper
@ 2015-04-01 20:13   ` Konrad Rzeszutek Wilk
  2015-04-13 14:27   ` Jan Beulich
  1 sibling, 0 replies; 8+ messages in thread
From: Konrad Rzeszutek Wilk @ 2015-04-01 20:13 UTC (permalink / raw)
  To: Andrew Cooper; +Cc: Keir Fraser, Jan Beulich, Xen-devel

On Wed, Apr 01, 2015 at 04:31:02PM +0100, Andrew Cooper wrote:
>  * latch curr/currd once at start
>  * drop redundant "ret = 0" and braces
>  * use "copyback = 1" when appropriate
>  * move break statements inside case-specific braced scopes
>  * don't bother check for NULL before calling xfree()
>  * eliminate trailing whitespace
>  * Xen style corrections
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> CC: Keir Fraser <keir@xen.org>
> CC: Jan Beulich <JBeulich@suse.com>

Reviewed-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> ---
>  xen/arch/x86/domctl.c |  276 ++++++++++++++++++++-----------------------------
>  1 file changed, 113 insertions(+), 163 deletions(-)
> 
> diff --git a/xen/arch/x86/domctl.c b/xen/arch/x86/domctl.c
> index 32d3fcd..bcbdf95 100644
> --- a/xen/arch/x86/domctl.c
> +++ b/xen/arch/x86/domctl.c
> @@ -1,6 +1,6 @@
>  /******************************************************************************
>   * Arch-specific domctl.c
> - * 
> + *
>   * Copyright (c) 2002-2006, K A Fraser
>   */
>  
> @@ -39,7 +39,7 @@
>  
>  static int gdbsx_guest_mem_io(
>      domid_t domid, struct xen_domctl_gdbsx_memio *iop)
> -{   
> +{
>      ulong l_uva = (ulong)iop->uva;
>      iop->remain = dbg_rw_mem(
>          (dbgva_t)iop->gva, (dbgbyte_t *)l_uva, iop->len, domid,
> @@ -53,6 +53,8 @@ long arch_do_domctl(
>      struct xen_domctl *domctl, struct domain *d,
>      XEN_GUEST_HANDLE_PARAM(xen_domctl_t) u_domctl)
>  {
> +    struct vcpu *curr = current;
> +    struct domain *currd = curr->domain;
>      long ret = 0;
>      bool_t copyback = 0;
>      unsigned long i;
> @@ -61,15 +63,13 @@ long arch_do_domctl(
>      {
>  
>      case XEN_DOMCTL_shadow_op:
> -    {
>          ret = paging_domctl(d, &domctl->u.shadow_op,
>                              guest_handle_cast(u_domctl, void), 0);
>          if ( ret == -ERESTART )
>              return hypercall_create_continuation(__HYPERVISOR_arch_1,
>                                                   "h", u_domctl);
>          copyback = 1;
> -    }
> -    break;
> +        break;
>  
>      case XEN_DOMCTL_ioport_permission:
>      {
> @@ -79,8 +79,7 @@ long arch_do_domctl(
>  
>          if ( (fp + np) <= fp || (fp + np) > MAX_IOPORTS )
>              ret = -EINVAL;
> -        else if ( !ioports_access_permitted(current->domain,
> -                                            fp, fp + np - 1) ||
> +        else if ( !ioports_access_permitted(currd, fp, fp + np - 1) ||
>                    xsm_ioport_permission(XSM_HOOK, d, fp, fp + np - 1, allow) )
>              ret = -EPERM;
>          else if ( allow )
> @@ -89,8 +88,8 @@ long arch_do_domctl(
>              ret = ioports_deny_access(d, fp, fp + np - 1);
>          if ( !ret )
>              memory_type_changed(d);
> +        break;
>      }
> -    break;
>  
>      case XEN_DOMCTL_getpageframeinfo:
>      {
> @@ -127,16 +126,16 @@ long arch_do_domctl(
>                      break;
>                  }
>              }
> -            
> +
>              put_page(page);
>          }
>  
>          copyback = 1;
> +        break;
>      }
> -    break;
>  
>      case XEN_DOMCTL_getpageframeinfo3:
> -        if (!has_32bit_shinfo(current->domain))
> +        if ( !has_32bit_shinfo(currd) )
>          {
>              unsigned int n, j;
>              unsigned int num = domctl->u.getpageframeinfo3.num;
> @@ -150,7 +149,7 @@ long arch_do_domctl(
>                  break;
>              }
>  
> -            page = alloc_domheap_page(current->domain, MEMF_no_owner);
> +            page = alloc_domheap_page(currd, MEMF_no_owner);
>              if ( !page )
>              {
>                  ret = -ENOMEM;
> @@ -251,7 +250,7 @@ long arch_do_domctl(
>              ret = -ENOMEM;
>              break;
>          }
> - 
> +
>          ret = 0;
>          for ( n = 0; n < num; )
>          {
> @@ -266,9 +265,9 @@ long arch_do_domctl(
>                  ret = -EFAULT;
>                  break;
>              }
> -     
> +
>              for ( j = 0; j < k; j++ )
> -            {      
> +            {
>                  struct page_info *page;
>                  unsigned long gfn = arr32[j];
>  
> @@ -320,8 +319,8 @@ long arch_do_domctl(
>          }
>  
>          free_xenheap_page(arr32);
> +        break;
>      }
> -    break;
>  
>      case XEN_DOMCTL_getmemlist:
>      {
> @@ -329,7 +328,8 @@ long arch_do_domctl(
>          uint64_t mfn;
>          struct page_info *page;
>  
> -        if ( unlikely(d->is_dying) ) {
> +        if ( unlikely(d->is_dying) )
> +        {
>              ret = -EINVAL;
>              break;
>          }
> @@ -346,7 +346,7 @@ long arch_do_domctl(
>           * rather than trying to fix it we restrict it for the time being.
>           */
>          if ( /* No nested locks inside copy_to_guest_offset(). */
> -             paging_mode_external(current->domain) ||
> +             paging_mode_external(currd) ||
>               /* Arbitrary limit capping processing time. */
>               max_pfns > GB(4) / PAGE_SIZE )
>          {
> @@ -375,8 +375,8 @@ long arch_do_domctl(
>  
>          domctl->u.getmemlist.num_pfns = i;
>          copyback = 1;
> +        break;
>      }
> -    break;
>  
>      case XEN_DOMCTL_hypercall_init:
>      {
> @@ -403,15 +403,15 @@ long arch_do_domctl(
>          unmap_domain_page(hypercall_page);
>  
>          put_page_and_type(page);
> +        break;
>      }
> -    break;
>  
>      case XEN_DOMCTL_sethvmcontext:
> -    { 
> +    {
>          struct hvm_domain_context c = { .size = domctl->u.hvmcontext.size };
>  
>          ret = -EINVAL;
> -        if ( !is_hvm_domain(d) ) 
> +        if ( !is_hvm_domain(d) )
>              goto sethvmcontext_out;
>  
>          ret = -ENOMEM;
> @@ -419,7 +419,7 @@ long arch_do_domctl(
>              goto sethvmcontext_out;
>  
>          ret = -EFAULT;
> -        if ( copy_from_guest(c.data, domctl->u.hvmcontext.buffer, c.size) != 0)
> +        if ( copy_from_guest(c.data, domctl->u.hvmcontext.buffer, c.size) != 0 )
>              goto sethvmcontext_out;
>  
>          domain_pause(d);
> @@ -427,17 +427,16 @@ long arch_do_domctl(
>          domain_unpause(d);
>  
>      sethvmcontext_out:
> -        if ( c.data != NULL )
> -            xfree(c.data);
> +        xfree(c.data);
> +        break;
>      }
> -    break;
>  
>      case XEN_DOMCTL_gethvmcontext:
> -    { 
> +    {
>          struct hvm_domain_context c = { 0 };
>  
>          ret = -EINVAL;
> -        if ( !is_hvm_domain(d) ) 
> +        if ( !is_hvm_domain(d) )
>              goto gethvmcontext_out;
>  
>          c.size = hvm_save_size(d);
> @@ -447,12 +446,12 @@ long arch_do_domctl(
>              /* Client is querying for the correct buffer size */
>              domctl->u.hvmcontext.size = c.size;
>              ret = 0;
> -            goto gethvmcontext_out;            
> +            goto gethvmcontext_out;
>          }
>  
>          /* Check that the client has a big enough buffer */
>          ret = -ENOSPC;
> -        if ( domctl->u.hvmcontext.size < c.size ) 
> +        if ( domctl->u.hvmcontext.size < c.size )
>              goto gethvmcontext_out;
>  
>          /* Allocate our own marshalling buffer */
> @@ -470,16 +469,13 @@ long arch_do_domctl(
>  
>      gethvmcontext_out:
>          copyback = 1;
> -
> -        if ( c.data != NULL )
> -            xfree(c.data);
> +        xfree(c.data);
> +        break;
>      }
> -    break;
>  
>      case XEN_DOMCTL_gethvmcontext_partial:
> -    { 
>          ret = -EINVAL;
> -        if ( !is_hvm_domain(d) ) 
> +        if ( !is_hvm_domain(d) )
>              break;
>  
>          domain_pause(d);
> @@ -487,12 +483,9 @@ long arch_do_domctl(
>                             domctl->u.hvmcontext_partial.instance,
>                             domctl->u.hvmcontext_partial.buffer);
>          domain_unpause(d);
> -    }
> -    break;
> -
> +        break;
>  
>      case XEN_DOMCTL_set_address_size:
> -    {
>          switch ( domctl->u.address_size.size )
>          {
>          case 32:
> @@ -505,39 +498,25 @@ long arch_do_domctl(
>              ret = (domctl->u.address_size.size == BITS_PER_LONG) ? 0 : -EINVAL;
>              break;
>          }
> -    }
> -    break;
> +        break;
>  
>      case XEN_DOMCTL_get_address_size:
> -    {
>          domctl->u.address_size.size =
>              is_pv_32on64_domain(d) ? 32 : BITS_PER_LONG;
> -
> -        ret = 0;
>          copyback = 1;
> -    }
> -    break;
> +        break;
>  
>      case XEN_DOMCTL_set_machine_address_size:
> -    {
> -        ret = -EBUSY;
>          if ( d->tot_pages > 0 )
> -            break;
> -
> -        d->arch.physaddr_bitsize = domctl->u.address_size.size;
> -
> -        ret = 0;
> -    }
> -    break;
> +            ret = -EBUSY;
> +        else
> +            d->arch.physaddr_bitsize = domctl->u.address_size.size;
> +        break;
>  
>      case XEN_DOMCTL_get_machine_address_size:
> -    {
>          domctl->u.address_size.size = d->arch.physaddr_bitsize;
> -
> -        ret = 0;
>          copyback = 1;
> -    }
> -    break;
> +        break;
>  
>      case XEN_DOMCTL_sendtrigger:
>      {
> @@ -551,40 +530,34 @@ long arch_do_domctl(
>          switch ( domctl->u.sendtrigger.trigger )
>          {
>          case XEN_DOMCTL_SENDTRIGGER_NMI:
> -        {
>              ret = 0;
>              if ( !test_and_set_bool(v->nmi_pending) )
>                  vcpu_kick(v);
> -        }
> -        break;
> +            break;
>  
>          case XEN_DOMCTL_SENDTRIGGER_POWER:
> -        {
>              ret = -EINVAL;
> -            if ( is_hvm_domain(d) ) 
> +            if ( is_hvm_domain(d) )
>              {
>                  ret = 0;
>                  hvm_acpi_power_button(d);
>              }
> -        }
> -        break;
> +            break;
>  
>          case XEN_DOMCTL_SENDTRIGGER_SLEEP:
> -        {
>              ret = -EINVAL;
> -            if ( is_hvm_domain(d) ) 
> +            if ( is_hvm_domain(d) )
>              {
>                  ret = 0;
>                  hvm_acpi_sleep_button(d);
>              }
> -        }
> -        break;
> +            break;
>  
>          default:
>              ret = -ENOSYS;
>          }
> +        break;
>      }
> -    break;
>  
>      case XEN_DOMCTL_bind_pt_irq:
>      {
> @@ -601,7 +574,7 @@ long arch_do_domctl(
>  
>          irq = domain_pirq_to_irq(d, bind->machine_irq);
>          ret = -EPERM;
> -        if ( irq <= 0 || !irq_access_permitted(current->domain, irq) )
> +        if ( irq <= 0 || !irq_access_permitted(currd, irq) )
>              break;
>  
>          ret = -ESRCH;
> @@ -614,8 +587,8 @@ long arch_do_domctl(
>          if ( ret < 0 )
>              printk(XENLOG_G_ERR "pt_irq_create_bind failed (%ld) for dom%d\n",
>                     ret, d->domain_id);
> +        break;
>      }
> -    break;    
>  
>      case XEN_DOMCTL_unbind_pt_irq:
>      {
> @@ -623,7 +596,7 @@ long arch_do_domctl(
>          int irq = domain_pirq_to_irq(d, bind->machine_irq);
>  
>          ret = -EPERM;
> -        if ( irq <= 0 || !irq_access_permitted(current->domain, irq) )
> +        if ( irq <= 0 || !irq_access_permitted(currd, irq) )
>              break;
>  
>          ret = xsm_unbind_pt_irq(XSM_HOOK, d, bind);
> @@ -639,8 +612,8 @@ long arch_do_domctl(
>          if ( ret < 0 )
>              printk(XENLOG_G_ERR "pt_irq_destroy_bind failed (%ld) for dom%d\n",
>                     ret, d->domain_id);
> +        break;
>      }
> -    break;
>  
>      case XEN_DOMCTL_ioport_mapping:
>      {
> @@ -663,7 +636,7 @@ long arch_do_domctl(
>          }
>  
>          ret = -EPERM;
> -        if ( !ioports_access_permitted(current->domain, fmp, fmp + np - 1) )
> +        if ( !ioports_access_permitted(currd, fmp, fmp + np - 1) )
>              break;
>  
>          ret = xsm_ioport_mapping(XSM_HOOK, d, fmp, fmp + np - 1, add);
> @@ -719,33 +692,29 @@ long arch_do_domctl(
>                      break;
>                  }
>              ret = ioports_deny_access(d, fmp, fmp + np - 1);
> -            if ( ret && is_hardware_domain(current->domain) )
> +            if ( ret && is_hardware_domain(currd) )
>                  printk(XENLOG_ERR
>                         "ioport_map: error %ld denying dom%d access to [%x,%x]\n",
>                         ret, d->domain_id, fmp, fmp + np - 1);
>          }
>          if ( !ret )
>              memory_type_changed(d);
> +        break;
>      }
> -    break;
>  
>      case XEN_DOMCTL_pin_mem_cacheattr:
> -    {
>          ret = hvm_set_mem_pinned_cacheattr(
>              d, domctl->u.pin_mem_cacheattr.start,
>              domctl->u.pin_mem_cacheattr.end,
>              domctl->u.pin_mem_cacheattr.type);
> -    }
> -    break;
> +        break;
>  
>      case XEN_DOMCTL_set_ext_vcpucontext:
>      case XEN_DOMCTL_get_ext_vcpucontext:
>      {
> -        struct xen_domctl_ext_vcpucontext *evc;
> +        struct xen_domctl_ext_vcpucontext *evc = &domctl->u.ext_vcpucontext;
>          struct vcpu *v;
>  
> -        evc = &domctl->u.ext_vcpucontext;
> -
>          ret = -ESRCH;
>          if ( (evc->vcpu >= d->max_vcpus) ||
>               ((v = d->vcpu[evc->vcpu]) == NULL) )
> @@ -753,7 +722,7 @@ long arch_do_domctl(
>  
>          if ( domctl->cmd == XEN_DOMCTL_get_ext_vcpucontext )
>          {
> -            if ( v == current ) /* no vcpu_pause() */
> +            if ( v == curr ) /* no vcpu_pause() */
>                  break;
>  
>              evc->size = sizeof(*evc);
> @@ -794,7 +763,7 @@ long arch_do_domctl(
>          }
>          else
>          {
> -            if ( d == current->domain ) /* no domain_pause() */
> +            if ( d == currd ) /* no domain_pause() */
>                  break;
>              ret = -EINVAL;
>              if ( evc->size < offsetof(typeof(*evc), vmce) )
> @@ -848,8 +817,8 @@ long arch_do_domctl(
>  
>              domain_unpause(d);
>          }
> +        break;
>      }
> -    break;
>  
>      case XEN_DOMCTL_set_cpuid:
>      {
> @@ -872,60 +841,50 @@ long arch_do_domctl(
>                    (cpuid->input[1] == ctl->input[1])) )
>                  break;
>          }
> -        
> +
>          if ( i < MAX_CPUID_INPUT )
>              *cpuid = *ctl;
>          else if ( unused )
>              *unused = *ctl;
>          else
>              ret = -ENOENT;
> +        break;
>      }
> -    break;
>  
>      case XEN_DOMCTL_gettscinfo:
> -    {
> -        xen_guest_tsc_info_t info;
> -
> -        ret = -EINVAL;
> -        if ( d == current->domain ) /* no domain_pause() */
> -            break;
> -
> -        domain_pause(d);
> -        tsc_get_info(d, &info.tsc_mode,
> -                        &info.elapsed_nsec,
> -                        &info.gtsc_khz,
> -                        &info.incarnation);
> -        if ( copy_to_guest(domctl->u.tsc_info.out_info, &info, 1) )
> -            ret = -EFAULT;
> +        if ( d == currd ) /* no domain_pause() */
> +            ret = -EINVAL;
>          else
> -            ret = 0;
> -        domain_unpause(d);
> -    }
> -    break;
> +        {
> +            xen_guest_tsc_info_t info;
> +
> +            domain_pause(d);
> +            tsc_get_info(d, &info.tsc_mode,
> +                         &info.elapsed_nsec,
> +                         &info.gtsc_khz,
> +                         &info.incarnation);
> +            domain_unpause(d);
> +            copyback = 1;
> +        }
> +        break;
>  
>      case XEN_DOMCTL_settscinfo:
> -    {
> -        ret = -EINVAL;
> -        if ( d == current->domain ) /* no domain_pause() */
> -            break;
> -
> -        domain_pause(d);
> -        tsc_set_info(d, domctl->u.tsc_info.info.tsc_mode,
> -                     domctl->u.tsc_info.info.elapsed_nsec,
> -                     domctl->u.tsc_info.info.gtsc_khz,
> -                     domctl->u.tsc_info.info.incarnation);
> -        domain_unpause(d);
> -
> -        ret = 0;
> -    }
> -    break;
> +        if ( d == currd ) /* no domain_pause() */
> +            ret = -EINVAL;
> +        else
> +        {
> +            domain_pause(d);
> +            tsc_set_info(d, domctl->u.tsc_info.info.tsc_mode,
> +                         domctl->u.tsc_info.info.elapsed_nsec,
> +                         domctl->u.tsc_info.info.gtsc_khz,
> +                         domctl->u.tsc_info.info.incarnation);
> +            domain_unpause(d);
> +        }
> +        break;
>  
>      case XEN_DOMCTL_suppress_spurious_page_faults:
> -    {
>          d->arch.suppress_spurious_page_faults = 1;
> -        ret = 0;
> -    }
> -    break;
> +        break;
>  
>      case XEN_DOMCTL_debug_op:
>      {
> @@ -941,19 +900,15 @@ long arch_do_domctl(
>              break;
>  
>          ret = hvm_debug_op(v, domctl->u.debug_op.op);
> +        break;
>      }
> -    break;
>  
>      case XEN_DOMCTL_gdbsx_guestmemio:
> -    {
> -        domctl->u.gdbsx_guest_memio.remain =
> -            domctl->u.gdbsx_guest_memio.len;
> -
> +        domctl->u.gdbsx_guest_memio.remain = domctl->u.gdbsx_guest_memio.len;
>          ret = gdbsx_guest_mem_io(domctl->domain, &domctl->u.gdbsx_guest_memio);
>          if ( !ret )
>             copyback = 1;
> -    }
> -    break;
> +        break;
>  
>      case XEN_DOMCTL_gdbsx_pausevcpu:
>      {
> @@ -967,8 +922,8 @@ long arch_do_domctl(
>               (v = d->vcpu[domctl->u.gdbsx_pauseunp_vcpu.vcpu]) == NULL )
>              break;
>          ret = vcpu_pause_by_systemcontroller(v);
> +        break;
>      }
> -    break;
>  
>      case XEN_DOMCTL_gdbsx_unpausevcpu:
>      {
> @@ -985,9 +940,9 @@ long arch_do_domctl(
>          if ( ret == -EINVAL )
>              printk(XENLOG_G_WARNING
>                     "WARN: d%d attempting to unpause %pv which is not paused\n",
> -                   current->domain->domain_id, v);
> +                   currd->domain_id, v);
> +        break;
>      }
> -    break;
>  
>      case XEN_DOMCTL_gdbsx_domstatus:
>      {
> @@ -1009,29 +964,26 @@ long arch_do_domctl(
>                  }
>              }
>          }
> -        ret = 0;
>          copyback = 1;
> +        break;
>      }
> -    break;
>  
>      case XEN_DOMCTL_setvcpuextstate:
>      case XEN_DOMCTL_getvcpuextstate:
>      {
> -        struct xen_domctl_vcpuextstate *evc;
> +        struct xen_domctl_vcpuextstate *evc = &domctl->u.vcpuextstate;
>          struct vcpu *v;
>          uint32_t offset = 0;
>  
>  #define PV_XSAVE_SIZE(xcr0) (2 * sizeof(uint64_t) + xstate_ctxt_size(xcr0))
>  
> -        evc = &domctl->u.vcpuextstate;
> -
>          ret = -ESRCH;
>          if ( (evc->vcpu >= d->max_vcpus) ||
>               ((v = d->vcpu[evc->vcpu]) == NULL) )
>              goto vcpuextstate_out;
>  
>          ret = -EINVAL;
> -        if ( v == current ) /* no vcpu_pause() */
> +        if ( v == curr ) /* no vcpu_pause() */
>              goto vcpuextstate_out;
>  
>          if ( domctl->cmd == XEN_DOMCTL_getvcpuextstate )
> @@ -1137,31 +1089,26 @@ long arch_do_domctl(
>      vcpuextstate_out:
>          if ( domctl->cmd == XEN_DOMCTL_getvcpuextstate )
>              copyback = 1;
> +        break;
>      }
> -    break;
>  
>      case XEN_DOMCTL_mem_sharing_op:
> -    {
>          ret = mem_sharing_domctl(d, &domctl->u.mem_sharing_op);
> -    }
> -    break;
> +        break;
>  
>  #if P2M_AUDIT
>      case XEN_DOMCTL_audit_p2m:
> -    {
> -        if ( d == current->domain )
> -        {
> +        if ( d == currd )
>              ret = -EPERM;
> -            break;
> +        else
> +        {
> +            audit_p2m(d,
> +                      &domctl->u.audit_p2m.orphans,
> +                      &domctl->u.audit_p2m.m2p_bad,
> +                      &domctl->u.audit_p2m.p2m_bad);
> +            copyback = 1;
>          }
> -
> -        audit_p2m(d,
> -                  &domctl->u.audit_p2m.orphans,
> -                  &domctl->u.audit_p2m.m2p_bad,
> -                  &domctl->u.audit_p2m.p2m_bad);
> -        copyback = 1;
> -    }
> -    break;
> +        break;
>  #endif /* P2M_AUDIT */
>  
>      case XEN_DOMCTL_set_broken_page_p2m:
> @@ -1176,8 +1123,8 @@ long arch_do_domctl(
>              ret = p2m_change_type_one(d, pfn, pt, p2m_ram_broken);
>  
>          put_gfn(d, pfn);
> +        break;
>      }
> -    break;
>  
>      case XEN_DOMCTL_get_vcpu_msrs:
>      case XEN_DOMCTL_set_vcpu_msrs:
> @@ -1193,7 +1140,7 @@ long arch_do_domctl(
>              break;
>  
>          ret = -EINVAL;
> -        if ( (v == current) || /* no vcpu_pause() */
> +        if ( (v == curr) || /* no vcpu_pause() */
>               !is_pv_domain(d) )
>              break;
>  
> @@ -1303,8 +1250,8 @@ long arch_do_domctl(
>                  copyback = 1;
>              }
>          }
> +        break;
>      }
> -    break;
>  
>      case XEN_DOMCTL_psr_cmt_op:
>          if ( !psr_cmt_enabled() )
> @@ -1318,16 +1265,19 @@ long arch_do_domctl(
>          case XEN_DOMCTL_PSR_CMT_OP_ATTACH:
>              ret = psr_alloc_rmid(d);
>              break;
> +
>          case XEN_DOMCTL_PSR_CMT_OP_DETACH:
>              if ( d->arch.psr_rmid > 0 )
>                  psr_free_rmid(d);
>              else
>                  ret = -ENOENT;
>              break;
> +
>          case XEN_DOMCTL_PSR_CMT_OP_QUERY_RMID:
>              domctl->u.psr_cmt_op.data = d->arch.psr_rmid;
>              copyback = 1;
>              break;
> +
>          default:
>              ret = -ENOSYS;
>              break;
> -- 
> 1.7.10.4
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH 2/2] x86/domctl: Don't allow a toolstack domain to pause itself
  2015-04-01 15:31 ` [PATCH 2/2] x86/domctl: Don't allow a toolstack domain to pause itself Andrew Cooper
@ 2015-04-01 20:14   ` Konrad Rzeszutek Wilk
  0 siblings, 0 replies; 8+ messages in thread
From: Konrad Rzeszutek Wilk @ 2015-04-01 20:14 UTC (permalink / raw)
  To: Andrew Cooper; +Cc: Keir Fraser, Jan Beulich, Xen-devel

On Wed, Apr 01, 2015 at 04:31:03PM +0100, Andrew Cooper wrote:
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> CC: Keir Fraser <keir@xen.org>
> CC: Jan Beulich <JBeulich@suse.com>

Reviewed-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> ---
>  xen/arch/x86/domctl.c |   12 ++++++++----
>  1 file changed, 8 insertions(+), 4 deletions(-)
> 
> diff --git a/xen/arch/x86/domctl.c b/xen/arch/x86/domctl.c
> index bcbdf95..ff3b423 100644
> --- a/xen/arch/x86/domctl.c
> +++ b/xen/arch/x86/domctl.c
> @@ -411,7 +411,8 @@ long arch_do_domctl(
>          struct hvm_domain_context c = { .size = domctl->u.hvmcontext.size };
>  
>          ret = -EINVAL;
> -        if ( !is_hvm_domain(d) )
> +        if ( (d == currd) || /* no domain_pause() */
> +             !is_hvm_domain(d) )
>              goto sethvmcontext_out;
>  
>          ret = -ENOMEM;
> @@ -436,7 +437,8 @@ long arch_do_domctl(
>          struct hvm_domain_context c = { 0 };
>  
>          ret = -EINVAL;
> -        if ( !is_hvm_domain(d) )
> +        if ( (d == currd) || /* no domain_pause() */
> +             !is_hvm_domain(d) )
>              goto gethvmcontext_out;
>  
>          c.size = hvm_save_size(d);
> @@ -475,7 +477,8 @@ long arch_do_domctl(
>  
>      case XEN_DOMCTL_gethvmcontext_partial:
>          ret = -EINVAL;
> -        if ( !is_hvm_domain(d) )
> +        if ( (d == currd) || /* no domain_pause() */
> +             !is_hvm_domain(d) )
>              break;
>  
>          domain_pause(d);
> @@ -896,7 +899,8 @@ long arch_do_domctl(
>              break;
>  
>          ret = -EINVAL;
> -        if ( !is_hvm_domain(d))
> +        if ( (v == curr) || /* no vcpu_pause() */
> +             !is_hvm_domain(d) )
>              break;
>  
>          ret = hvm_debug_op(v, domctl->u.debug_op.op);
> -- 
> 1.7.10.4
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH 1/2] x86/domctl: cleanup
  2015-04-01 15:31 ` [PATCH 1/2] x86/domctl: cleanup Andrew Cooper
  2015-04-01 20:13   ` Konrad Rzeszutek Wilk
@ 2015-04-13 14:27   ` Jan Beulich
  2015-04-13 15:22     ` Andrew Cooper
  1 sibling, 1 reply; 8+ messages in thread
From: Jan Beulich @ 2015-04-13 14:27 UTC (permalink / raw)
  To: Andrew Cooper; +Cc: Keir Fraser, Xen-devel

 >>> On 01.04.15 at 17:31, <andrew.cooper3@citrix.com> wrote:
>      case XEN_DOMCTL_gettscinfo:
> -    {
> -        xen_guest_tsc_info_t info;
> -
> -        ret = -EINVAL;
> -        if ( d == current->domain ) /* no domain_pause() */
> -            break;
> -
> -        domain_pause(d);
> -        tsc_get_info(d, &info.tsc_mode,
> -                        &info.elapsed_nsec,
> -                        &info.gtsc_khz,
> -                        &info.incarnation);
> -        if ( copy_to_guest(domctl->u.tsc_info.out_info, &info, 1) )
> -            ret = -EFAULT;
> +        if ( d == currd ) /* no domain_pause() */
> +            ret = -EINVAL;
>          else
> -            ret = 0;
> -        domain_unpause(d);
> -    }
> -    break;
> +        {
> +            xen_guest_tsc_info_t info;
> +
> +            domain_pause(d);
> +            tsc_get_info(d, &info.tsc_mode,
> +                         &info.elapsed_nsec,
> +                         &info.gtsc_khz,
> +                         &info.incarnation);
> +            domain_unpause(d);
> +            copyback = 1;

If you want to use "copyback" here, you need to pass pointers into
domctl->u.tsc_info.out_info to tsc_get_info().

Jan

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH 1/2] x86/domctl: cleanup
  2015-04-13 14:27   ` Jan Beulich
@ 2015-04-13 15:22     ` Andrew Cooper
  2015-04-13 15:49       ` [PATCH v2 " Andrew Cooper
  0 siblings, 1 reply; 8+ messages in thread
From: Andrew Cooper @ 2015-04-13 15:22 UTC (permalink / raw)
  To: Jan Beulich; +Cc: Keir Fraser, Xen-devel

On 13/04/15 15:27, Jan Beulich wrote:
>  >>> On 01.04.15 at 17:31, <andrew.cooper3@citrix.com> wrote:
>>      case XEN_DOMCTL_gettscinfo:
>> -    {
>> -        xen_guest_tsc_info_t info;
>> -
>> -        ret = -EINVAL;
>> -        if ( d == current->domain ) /* no domain_pause() */
>> -            break;
>> -
>> -        domain_pause(d);
>> -        tsc_get_info(d, &info.tsc_mode,
>> -                        &info.elapsed_nsec,
>> -                        &info.gtsc_khz,
>> -                        &info.incarnation);
>> -        if ( copy_to_guest(domctl->u.tsc_info.out_info, &info, 1) )
>> -            ret = -EFAULT;
>> +        if ( d == currd ) /* no domain_pause() */
>> +            ret = -EINVAL;
>>          else
>> -            ret = 0;
>> -        domain_unpause(d);
>> -    }
>> -    break;
>> +        {
>> +            xen_guest_tsc_info_t info;
>> +
>> +            domain_pause(d);
>> +            tsc_get_info(d, &info.tsc_mode,
>> +                         &info.elapsed_nsec,
>> +                         &info.gtsc_khz,
>> +                         &info.incarnation);
>> +            domain_unpause(d);
>> +            copyback = 1;
> If you want to use "copyback" here, you need to pass pointers into
> domctl->u.tsc_info.out_info to tsc_get_info().

Oops - completely correct.

I shall spin a v2.

~Andrew

^ permalink raw reply	[flat|nested] 8+ messages in thread

* [PATCH v2 1/2] x86/domctl: cleanup
  2015-04-13 15:22     ` Andrew Cooper
@ 2015-04-13 15:49       ` Andrew Cooper
  0 siblings, 0 replies; 8+ messages in thread
From: Andrew Cooper @ 2015-04-13 15:49 UTC (permalink / raw)
  To: Xen-devel; +Cc: Andrew Cooper, Keir Fraser, Jan Beulich

 * latch curr/currd once at start
 * drop redundant "ret = 0" and braces
 * use "copyback = 1" when appropriate
 * move break statements inside case-specific braced scopes
 * don't bother check for NULL before calling xfree()
 * eliminate trailing whitespace
 * Xen style corrections

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Reviewed-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
CC: Keir Fraser <keir@xen.org>
CC: Jan Beulich <JBeulich@suse.com>
---
v2: Fix use of copyback in XEN_DOMCTL_gettscinfo
---
 xen/arch/x86/domctl.c |  274 ++++++++++++++++++++-----------------------------
 1 file changed, 111 insertions(+), 163 deletions(-)

diff --git a/xen/arch/x86/domctl.c b/xen/arch/x86/domctl.c
index 9450795..b6df23a 100644
--- a/xen/arch/x86/domctl.c
+++ b/xen/arch/x86/domctl.c
@@ -1,6 +1,6 @@
 /******************************************************************************
  * Arch-specific domctl.c
- * 
+ *
  * Copyright (c) 2002-2006, K A Fraser
  */
 
@@ -39,7 +39,7 @@
 
 static int gdbsx_guest_mem_io(
     domid_t domid, struct xen_domctl_gdbsx_memio *iop)
-{   
+{
     ulong l_uva = (ulong)iop->uva;
     iop->remain = dbg_rw_mem(
         (dbgva_t)iop->gva, (dbgbyte_t *)l_uva, iop->len, domid,
@@ -53,6 +53,8 @@ long arch_do_domctl(
     struct xen_domctl *domctl, struct domain *d,
     XEN_GUEST_HANDLE_PARAM(xen_domctl_t) u_domctl)
 {
+    struct vcpu *curr = current;
+    struct domain *currd = curr->domain;
     long ret = 0;
     bool_t copyback = 0;
     unsigned long i;
@@ -61,15 +63,13 @@ long arch_do_domctl(
     {
 
     case XEN_DOMCTL_shadow_op:
-    {
         ret = paging_domctl(d, &domctl->u.shadow_op,
                             guest_handle_cast(u_domctl, void), 0);
         if ( ret == -ERESTART )
             return hypercall_create_continuation(__HYPERVISOR_arch_1,
                                                  "h", u_domctl);
         copyback = 1;
-    }
-    break;
+        break;
 
     case XEN_DOMCTL_ioport_permission:
     {
@@ -79,8 +79,7 @@ long arch_do_domctl(
 
         if ( (fp + np) <= fp || (fp + np) > MAX_IOPORTS )
             ret = -EINVAL;
-        else if ( !ioports_access_permitted(current->domain,
-                                            fp, fp + np - 1) ||
+        else if ( !ioports_access_permitted(currd, fp, fp + np - 1) ||
                   xsm_ioport_permission(XSM_HOOK, d, fp, fp + np - 1, allow) )
             ret = -EPERM;
         else if ( allow )
@@ -89,8 +88,8 @@ long arch_do_domctl(
             ret = ioports_deny_access(d, fp, fp + np - 1);
         if ( !ret )
             memory_type_changed(d);
+        break;
     }
-    break;
 
     case XEN_DOMCTL_getpageframeinfo:
     {
@@ -127,16 +126,16 @@ long arch_do_domctl(
                     break;
                 }
             }
-            
+
             put_page(page);
         }
 
         copyback = 1;
+        break;
     }
-    break;
 
     case XEN_DOMCTL_getpageframeinfo3:
-        if (!has_32bit_shinfo(current->domain))
+        if ( !has_32bit_shinfo(currd) )
         {
             unsigned int n, j;
             unsigned int num = domctl->u.getpageframeinfo3.num;
@@ -150,7 +149,7 @@ long arch_do_domctl(
                 break;
             }
 
-            page = alloc_domheap_page(current->domain, MEMF_no_owner);
+            page = alloc_domheap_page(currd, MEMF_no_owner);
             if ( !page )
             {
                 ret = -ENOMEM;
@@ -251,7 +250,7 @@ long arch_do_domctl(
             ret = -ENOMEM;
             break;
         }
- 
+
         ret = 0;
         for ( n = 0; n < num; )
         {
@@ -266,9 +265,9 @@ long arch_do_domctl(
                 ret = -EFAULT;
                 break;
             }
-     
+
             for ( j = 0; j < k; j++ )
-            {      
+            {
                 struct page_info *page;
                 unsigned long gfn = arr32[j];
 
@@ -320,8 +319,8 @@ long arch_do_domctl(
         }
 
         free_xenheap_page(arr32);
+        break;
     }
-    break;
 
     case XEN_DOMCTL_getmemlist:
     {
@@ -329,7 +328,8 @@ long arch_do_domctl(
         uint64_t mfn;
         struct page_info *page;
 
-        if ( unlikely(d->is_dying) ) {
+        if ( unlikely(d->is_dying) )
+        {
             ret = -EINVAL;
             break;
         }
@@ -346,7 +346,7 @@ long arch_do_domctl(
          * rather than trying to fix it we restrict it for the time being.
          */
         if ( /* No nested locks inside copy_to_guest_offset(). */
-             paging_mode_external(current->domain) ||
+             paging_mode_external(currd) ||
              /* Arbitrary limit capping processing time. */
              max_pfns > GB(4) / PAGE_SIZE )
         {
@@ -375,8 +375,8 @@ long arch_do_domctl(
 
         domctl->u.getmemlist.num_pfns = i;
         copyback = 1;
+        break;
     }
-    break;
 
     case XEN_DOMCTL_hypercall_init:
     {
@@ -403,15 +403,15 @@ long arch_do_domctl(
         unmap_domain_page(hypercall_page);
 
         put_page_and_type(page);
+        break;
     }
-    break;
 
     case XEN_DOMCTL_sethvmcontext:
-    { 
+    {
         struct hvm_domain_context c = { .size = domctl->u.hvmcontext.size };
 
         ret = -EINVAL;
-        if ( !is_hvm_domain(d) ) 
+        if ( !is_hvm_domain(d) )
             goto sethvmcontext_out;
 
         ret = -ENOMEM;
@@ -419,7 +419,7 @@ long arch_do_domctl(
             goto sethvmcontext_out;
 
         ret = -EFAULT;
-        if ( copy_from_guest(c.data, domctl->u.hvmcontext.buffer, c.size) != 0)
+        if ( copy_from_guest(c.data, domctl->u.hvmcontext.buffer, c.size) != 0 )
             goto sethvmcontext_out;
 
         domain_pause(d);
@@ -427,17 +427,16 @@ long arch_do_domctl(
         domain_unpause(d);
 
     sethvmcontext_out:
-        if ( c.data != NULL )
-            xfree(c.data);
+        xfree(c.data);
+        break;
     }
-    break;
 
     case XEN_DOMCTL_gethvmcontext:
-    { 
+    {
         struct hvm_domain_context c = { 0 };
 
         ret = -EINVAL;
-        if ( !is_hvm_domain(d) ) 
+        if ( !is_hvm_domain(d) )
             goto gethvmcontext_out;
 
         c.size = hvm_save_size(d);
@@ -447,12 +446,12 @@ long arch_do_domctl(
             /* Client is querying for the correct buffer size */
             domctl->u.hvmcontext.size = c.size;
             ret = 0;
-            goto gethvmcontext_out;            
+            goto gethvmcontext_out;
         }
 
         /* Check that the client has a big enough buffer */
         ret = -ENOSPC;
-        if ( domctl->u.hvmcontext.size < c.size ) 
+        if ( domctl->u.hvmcontext.size < c.size )
             goto gethvmcontext_out;
 
         /* Allocate our own marshalling buffer */
@@ -470,16 +469,13 @@ long arch_do_domctl(
 
     gethvmcontext_out:
         copyback = 1;
-
-        if ( c.data != NULL )
-            xfree(c.data);
+        xfree(c.data);
+        break;
     }
-    break;
 
     case XEN_DOMCTL_gethvmcontext_partial:
-    { 
         ret = -EINVAL;
-        if ( !is_hvm_domain(d) ) 
+        if ( !is_hvm_domain(d) )
             break;
 
         domain_pause(d);
@@ -487,12 +483,9 @@ long arch_do_domctl(
                            domctl->u.hvmcontext_partial.instance,
                            domctl->u.hvmcontext_partial.buffer);
         domain_unpause(d);
-    }
-    break;
-
+        break;
 
     case XEN_DOMCTL_set_address_size:
-    {
         switch ( domctl->u.address_size.size )
         {
         case 32:
@@ -505,39 +498,25 @@ long arch_do_domctl(
             ret = (domctl->u.address_size.size == BITS_PER_LONG) ? 0 : -EINVAL;
             break;
         }
-    }
-    break;
+        break;
 
     case XEN_DOMCTL_get_address_size:
-    {
         domctl->u.address_size.size =
             is_pv_32on64_domain(d) ? 32 : BITS_PER_LONG;
-
-        ret = 0;
         copyback = 1;
-    }
-    break;
+        break;
 
     case XEN_DOMCTL_set_machine_address_size:
-    {
-        ret = -EBUSY;
         if ( d->tot_pages > 0 )
-            break;
-
-        d->arch.physaddr_bitsize = domctl->u.address_size.size;
-
-        ret = 0;
-    }
-    break;
+            ret = -EBUSY;
+        else
+            d->arch.physaddr_bitsize = domctl->u.address_size.size;
+        break;
 
     case XEN_DOMCTL_get_machine_address_size:
-    {
         domctl->u.address_size.size = d->arch.physaddr_bitsize;
-
-        ret = 0;
         copyback = 1;
-    }
-    break;
+        break;
 
     case XEN_DOMCTL_sendtrigger:
     {
@@ -551,40 +530,34 @@ long arch_do_domctl(
         switch ( domctl->u.sendtrigger.trigger )
         {
         case XEN_DOMCTL_SENDTRIGGER_NMI:
-        {
             ret = 0;
             if ( !test_and_set_bool(v->nmi_pending) )
                 vcpu_kick(v);
-        }
-        break;
+            break;
 
         case XEN_DOMCTL_SENDTRIGGER_POWER:
-        {
             ret = -EINVAL;
-            if ( is_hvm_domain(d) ) 
+            if ( is_hvm_domain(d) )
             {
                 ret = 0;
                 hvm_acpi_power_button(d);
             }
-        }
-        break;
+            break;
 
         case XEN_DOMCTL_SENDTRIGGER_SLEEP:
-        {
             ret = -EINVAL;
-            if ( is_hvm_domain(d) ) 
+            if ( is_hvm_domain(d) )
             {
                 ret = 0;
                 hvm_acpi_sleep_button(d);
             }
-        }
-        break;
+            break;
 
         default:
             ret = -ENOSYS;
         }
+        break;
     }
-    break;
 
     case XEN_DOMCTL_bind_pt_irq:
     {
@@ -601,7 +574,7 @@ long arch_do_domctl(
 
         irq = domain_pirq_to_irq(d, bind->machine_irq);
         ret = -EPERM;
-        if ( irq <= 0 || !irq_access_permitted(current->domain, irq) )
+        if ( irq <= 0 || !irq_access_permitted(currd, irq) )
             break;
 
         ret = -ESRCH;
@@ -614,8 +587,8 @@ long arch_do_domctl(
         if ( ret < 0 )
             printk(XENLOG_G_ERR "pt_irq_create_bind failed (%ld) for dom%d\n",
                    ret, d->domain_id);
+        break;
     }
-    break;    
 
     case XEN_DOMCTL_unbind_pt_irq:
     {
@@ -623,7 +596,7 @@ long arch_do_domctl(
         int irq = domain_pirq_to_irq(d, bind->machine_irq);
 
         ret = -EPERM;
-        if ( irq <= 0 || !irq_access_permitted(current->domain, irq) )
+        if ( irq <= 0 || !irq_access_permitted(currd, irq) )
             break;
 
         ret = xsm_unbind_pt_irq(XSM_HOOK, d, bind);
@@ -639,8 +612,8 @@ long arch_do_domctl(
         if ( ret < 0 )
             printk(XENLOG_G_ERR "pt_irq_destroy_bind failed (%ld) for dom%d\n",
                    ret, d->domain_id);
+        break;
     }
-    break;
 
     case XEN_DOMCTL_ioport_mapping:
     {
@@ -663,7 +636,7 @@ long arch_do_domctl(
         }
 
         ret = -EPERM;
-        if ( !ioports_access_permitted(current->domain, fmp, fmp + np - 1) )
+        if ( !ioports_access_permitted(currd, fmp, fmp + np - 1) )
             break;
 
         ret = xsm_ioport_mapping(XSM_HOOK, d, fmp, fmp + np - 1, add);
@@ -719,33 +692,29 @@ long arch_do_domctl(
                     break;
                 }
             ret = ioports_deny_access(d, fmp, fmp + np - 1);
-            if ( ret && is_hardware_domain(current->domain) )
+            if ( ret && is_hardware_domain(currd) )
                 printk(XENLOG_ERR
                        "ioport_map: error %ld denying dom%d access to [%x,%x]\n",
                        ret, d->domain_id, fmp, fmp + np - 1);
         }
         if ( !ret )
             memory_type_changed(d);
+        break;
     }
-    break;
 
     case XEN_DOMCTL_pin_mem_cacheattr:
-    {
         ret = hvm_set_mem_pinned_cacheattr(
             d, domctl->u.pin_mem_cacheattr.start,
             domctl->u.pin_mem_cacheattr.end,
             domctl->u.pin_mem_cacheattr.type);
-    }
-    break;
+        break;
 
     case XEN_DOMCTL_set_ext_vcpucontext:
     case XEN_DOMCTL_get_ext_vcpucontext:
     {
-        struct xen_domctl_ext_vcpucontext *evc;
+        struct xen_domctl_ext_vcpucontext *evc = &domctl->u.ext_vcpucontext;
         struct vcpu *v;
 
-        evc = &domctl->u.ext_vcpucontext;
-
         ret = -ESRCH;
         if ( (evc->vcpu >= d->max_vcpus) ||
              ((v = d->vcpu[evc->vcpu]) == NULL) )
@@ -753,7 +722,7 @@ long arch_do_domctl(
 
         if ( domctl->cmd == XEN_DOMCTL_get_ext_vcpucontext )
         {
-            if ( v == current ) /* no vcpu_pause() */
+            if ( v == curr ) /* no vcpu_pause() */
                 break;
 
             evc->size = sizeof(*evc);
@@ -794,7 +763,7 @@ long arch_do_domctl(
         }
         else
         {
-            if ( d == current->domain ) /* no domain_pause() */
+            if ( d == currd ) /* no domain_pause() */
                 break;
             ret = -EINVAL;
             if ( evc->size < offsetof(typeof(*evc), vmce) )
@@ -848,8 +817,8 @@ long arch_do_domctl(
 
             domain_unpause(d);
         }
+        break;
     }
-    break;
 
     case XEN_DOMCTL_set_cpuid:
     {
@@ -872,60 +841,48 @@ long arch_do_domctl(
                   (cpuid->input[1] == ctl->input[1])) )
                 break;
         }
-        
+
         if ( i < MAX_CPUID_INPUT )
             *cpuid = *ctl;
         else if ( unused )
             *unused = *ctl;
         else
             ret = -ENOENT;
+        break;
     }
-    break;
 
     case XEN_DOMCTL_gettscinfo:
-    {
-        xen_guest_tsc_info_t info;
-
-        ret = -EINVAL;
-        if ( d == current->domain ) /* no domain_pause() */
-            break;
-
-        domain_pause(d);
-        tsc_get_info(d, &info.tsc_mode,
-                        &info.elapsed_nsec,
-                        &info.gtsc_khz,
-                        &info.incarnation);
-        if ( copy_to_guest(domctl->u.tsc_info.out_info, &info, 1) )
-            ret = -EFAULT;
+        if ( d == currd ) /* no domain_pause() */
+            ret = -EINVAL;
         else
-            ret = 0;
-        domain_unpause(d);
-    }
-    break;
+        {
+            domain_pause(d);
+            tsc_get_info(d, &domctl->u.tsc_info.info.tsc_mode,
+                         &domctl->u.tsc_info.info.elapsed_nsec,
+                         &domctl->u.tsc_info.info.gtsc_khz,
+                         &domctl->u.tsc_info.info.incarnation);
+            domain_unpause(d);
+            copyback = 1;
+        }
+        break;
 
     case XEN_DOMCTL_settscinfo:
-    {
-        ret = -EINVAL;
-        if ( d == current->domain ) /* no domain_pause() */
-            break;
-
-        domain_pause(d);
-        tsc_set_info(d, domctl->u.tsc_info.info.tsc_mode,
-                     domctl->u.tsc_info.info.elapsed_nsec,
-                     domctl->u.tsc_info.info.gtsc_khz,
-                     domctl->u.tsc_info.info.incarnation);
-        domain_unpause(d);
-
-        ret = 0;
-    }
-    break;
+        if ( d == currd ) /* no domain_pause() */
+            ret = -EINVAL;
+        else
+        {
+            domain_pause(d);
+            tsc_set_info(d, domctl->u.tsc_info.info.tsc_mode,
+                         domctl->u.tsc_info.info.elapsed_nsec,
+                         domctl->u.tsc_info.info.gtsc_khz,
+                         domctl->u.tsc_info.info.incarnation);
+            domain_unpause(d);
+        }
+        break;
 
     case XEN_DOMCTL_suppress_spurious_page_faults:
-    {
         d->arch.suppress_spurious_page_faults = 1;
-        ret = 0;
-    }
-    break;
+        break;
 
     case XEN_DOMCTL_debug_op:
     {
@@ -941,19 +898,15 @@ long arch_do_domctl(
             break;
 
         ret = hvm_debug_op(v, domctl->u.debug_op.op);
+        break;
     }
-    break;
 
     case XEN_DOMCTL_gdbsx_guestmemio:
-    {
-        domctl->u.gdbsx_guest_memio.remain =
-            domctl->u.gdbsx_guest_memio.len;
-
+        domctl->u.gdbsx_guest_memio.remain = domctl->u.gdbsx_guest_memio.len;
         ret = gdbsx_guest_mem_io(domctl->domain, &domctl->u.gdbsx_guest_memio);
         if ( !ret )
            copyback = 1;
-    }
-    break;
+        break;
 
     case XEN_DOMCTL_gdbsx_pausevcpu:
     {
@@ -967,8 +920,8 @@ long arch_do_domctl(
              (v = d->vcpu[domctl->u.gdbsx_pauseunp_vcpu.vcpu]) == NULL )
             break;
         ret = vcpu_pause_by_systemcontroller(v);
+        break;
     }
-    break;
 
     case XEN_DOMCTL_gdbsx_unpausevcpu:
     {
@@ -985,9 +938,9 @@ long arch_do_domctl(
         if ( ret == -EINVAL )
             printk(XENLOG_G_WARNING
                    "WARN: d%d attempting to unpause %pv which is not paused\n",
-                   current->domain->domain_id, v);
+                   currd->domain_id, v);
+        break;
     }
-    break;
 
     case XEN_DOMCTL_gdbsx_domstatus:
     {
@@ -1009,29 +962,26 @@ long arch_do_domctl(
                 }
             }
         }
-        ret = 0;
         copyback = 1;
+        break;
     }
-    break;
 
     case XEN_DOMCTL_setvcpuextstate:
     case XEN_DOMCTL_getvcpuextstate:
     {
-        struct xen_domctl_vcpuextstate *evc;
+        struct xen_domctl_vcpuextstate *evc = &domctl->u.vcpuextstate;
         struct vcpu *v;
         uint32_t offset = 0;
 
 #define PV_XSAVE_SIZE(xcr0) (2 * sizeof(uint64_t) + xstate_ctxt_size(xcr0))
 
-        evc = &domctl->u.vcpuextstate;
-
         ret = -ESRCH;
         if ( (evc->vcpu >= d->max_vcpus) ||
              ((v = d->vcpu[evc->vcpu]) == NULL) )
             goto vcpuextstate_out;
 
         ret = -EINVAL;
-        if ( v == current ) /* no vcpu_pause() */
+        if ( v == curr ) /* no vcpu_pause() */
             goto vcpuextstate_out;
 
         if ( domctl->cmd == XEN_DOMCTL_getvcpuextstate )
@@ -1137,31 +1087,26 @@ long arch_do_domctl(
     vcpuextstate_out:
         if ( domctl->cmd == XEN_DOMCTL_getvcpuextstate )
             copyback = 1;
+        break;
     }
-    break;
 
     case XEN_DOMCTL_mem_sharing_op:
-    {
         ret = mem_sharing_domctl(d, &domctl->u.mem_sharing_op);
-    }
-    break;
+        break;
 
 #if P2M_AUDIT
     case XEN_DOMCTL_audit_p2m:
-    {
-        if ( d == current->domain )
-        {
+        if ( d == currd )
             ret = -EPERM;
-            break;
+        else
+        {
+            audit_p2m(d,
+                      &domctl->u.audit_p2m.orphans,
+                      &domctl->u.audit_p2m.m2p_bad,
+                      &domctl->u.audit_p2m.p2m_bad);
+            copyback = 1;
         }
-
-        audit_p2m(d,
-                  &domctl->u.audit_p2m.orphans,
-                  &domctl->u.audit_p2m.m2p_bad,
-                  &domctl->u.audit_p2m.p2m_bad);
-        copyback = 1;
-    }
-    break;
+        break;
 #endif /* P2M_AUDIT */
 
     case XEN_DOMCTL_set_broken_page_p2m:
@@ -1176,8 +1121,8 @@ long arch_do_domctl(
             ret = p2m_change_type_one(d, pfn, pt, p2m_ram_broken);
 
         put_gfn(d, pfn);
+        break;
     }
-    break;
 
     case XEN_DOMCTL_get_vcpu_msrs:
     case XEN_DOMCTL_set_vcpu_msrs:
@@ -1193,7 +1138,7 @@ long arch_do_domctl(
             break;
 
         ret = -EINVAL;
-        if ( (v == current) || /* no vcpu_pause() */
+        if ( (v == curr) || /* no vcpu_pause() */
              !is_pv_domain(d) )
             break;
 
@@ -1303,8 +1248,8 @@ long arch_do_domctl(
                 copyback = 1;
             }
         }
+        break;
     }
-    break;
 
     case XEN_DOMCTL_psr_cmt_op:
         if ( !psr_cmt_enabled() )
@@ -1318,16 +1263,19 @@ long arch_do_domctl(
         case XEN_DOMCTL_PSR_CMT_OP_ATTACH:
             ret = psr_alloc_rmid(d);
             break;
+
         case XEN_DOMCTL_PSR_CMT_OP_DETACH:
             if ( d->arch.psr_rmid > 0 )
                 psr_free_rmid(d);
             else
                 ret = -ENOENT;
             break;
+
         case XEN_DOMCTL_PSR_CMT_OP_QUERY_RMID:
             domctl->u.psr_cmt_op.data = d->arch.psr_rmid;
             copyback = 1;
             break;
+
         default:
             ret = -ENOSYS;
             break;
-- 
1.7.10.4

^ permalink raw reply related	[flat|nested] 8+ messages in thread

end of thread, other threads:[~2015-04-13 15:49 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-04-01 15:31 [PATCH 0/2] Post-XSA-127 cleanup Andrew Cooper
2015-04-01 15:31 ` [PATCH 1/2] x86/domctl: cleanup Andrew Cooper
2015-04-01 20:13   ` Konrad Rzeszutek Wilk
2015-04-13 14:27   ` Jan Beulich
2015-04-13 15:22     ` Andrew Cooper
2015-04-13 15:49       ` [PATCH v2 " Andrew Cooper
2015-04-01 15:31 ` [PATCH 2/2] x86/domctl: Don't allow a toolstack domain to pause itself Andrew Cooper
2015-04-01 20:14   ` Konrad Rzeszutek Wilk

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.