XSM instead of IS_PRIV

XSM instead of IS_PRIV

[Shakeel Butt]

Are there any plans to replace IS_PRIV altogether with corresponding
XSM hooks? There was an old discussion (in 2007) on the mailing list
on similar issue and it was proposed that IS_PRIV should be replaced
with XSM hooks. Now still IS_PRIV is being used in latest xen-unstable
branch. So, is there a reason to keep using IS_PRIV?

thanks,
Shakeel

Re: XSM instead of IS_PRIV

[Daniel De Graaf]

On 07/28/2012 10:54 AM, Shakeel Butt wrote:
> Are there any plans to replace IS_PRIV altogether with corresponding
> XSM hooks? There was an old discussion (in 2007) on the mailing list
> on similar issue and it was proposed that IS_PRIV should be replaced
> with XSM hooks. Now still IS_PRIV is being used in latest xen-unstable
> branch. So, is there a reason to keep using IS_PRIV?
> 
> thanks,
> Shakeel
> 

I have a patch starting this change, which I had been planning to finish
and post after 4.3 branches. It makes the majority of IS_PRIV calls depend
on !define(XSM_ENABLE) and changes the dummy XSM module hooks to preserve
the semantics of XSM being disabled at compile time. It doesn't currently
cover every IS_PRIV call (hardware access is mostly not covered) or most
of the IS_PRIV_FOR calls. However, it does allow xl to be used in a domU
to manage other domains (list, pause, destroy, etc) and only xenstore
permission issues need to be resolved in order to do things like add
block/network devices.

Once this is done, it will make sense to change the XSM header file so
that the appropriate xsm_* hooks resolve to IS_PRIV when XSM is not
enabled. This would allow the IS_PRIV hooks covered by XSM to be
completely removed, assuming the Xen developers agree that the XSM hooks
cover all all the important code. Some, such as the create domain hook,
are deep enough in the calling code that callers getting permission denied
might be able to cause issues by e.g. rolling over the new domain ID counter.

-- 
Daniel De Graaf
National Security Agency

[PATCH RFC] xsm: use XSM instead of IS_PRIV where duplicated

[Daniel De Graaf]

When performing dom0 disaggregation, many of the management functions
normally handled by dom0 are instead handled by other domains. Instead
of requiring that all such domains be privileged, the IS_PRIV checks on
operations can be removed leaving the more fine-grained XSM checks to
protect these operations.

This has previously been done for the getdomaininfo domctl which is
required for xenstore.

Signed-off-by: Daniel De Graaf <dgdegra@tycho.nsa.gov>
---
 xen/arch/x86/mm.c                 |   4 ++
 xen/arch/x86/physdev.c            |  18 +++--
 xen/arch/x86/platform_hypercall.c |  10 +++
 xen/common/domctl.c               |  39 ++++++++++-
 xen/common/sysctl.c               |  33 +++++++++-
 xen/xsm/dummy.c                   | 134 ++++++++++++++++++++++++++++++++++++++
 xen/xsm/flask/hooks.c             |   9 +++
 7 files changed, 237 insertions(+), 10 deletions(-)

diff --git a/xen/arch/x86/mm.c b/xen/arch/x86/mm.c
index 9338575..78f47c8 100644
--- a/xen/arch/x86/mm.c
+++ b/xen/arch/x86/mm.c
@@ -4790,8 +4790,10 @@ long arch_memory_op(int op, XEN_GUEST_HANDLE(void) arg)
         XEN_GUEST_HANDLE(e820entry_t) buffer;
         unsigned int i;
 
+#ifndef XSM_ENABLE
         if ( !IS_PRIV(current->domain) )
             return -EINVAL;
+#endif
 
         rc = xsm_machine_memory_map();
         if ( rc )
@@ -4868,9 +4870,11 @@ long arch_memory_op(int op, XEN_GUEST_HANDLE(void) arg)
         struct domain *d;
         struct p2m_domain *p2m;
 
+#ifndef XSM_ENABLE
         /* Support DOMID_SELF? */
         if ( !IS_PRIV(current->domain) )
             return -EPERM;
+#endif
 
         if ( copy_from_guest(&target, arg, 1) )
             return -EFAULT;
diff --git a/xen/arch/x86/physdev.c b/xen/arch/x86/physdev.c
index b0458fd..f3caa74 100644
--- a/xen/arch/x86/physdev.c
+++ b/xen/arch/x86/physdev.c
@@ -430,12 +430,15 @@ ret_t do_physdev_op(int cmd, XEN_GUEST_HANDLE(void) arg)
         ret = -EFAULT;
         if ( copy_from_guest(&apic, arg, 1) != 0 )
             break;
-        ret = -EPERM;
-        if ( !IS_PRIV(v->domain) )
-            break;
+#ifdef XSM_ENABLE
         ret = xsm_apic(v->domain, cmd);
         if ( ret )
             break;
+#else
+        ret = -EPERM;
+        if ( !IS_PRIV(v->domain) )
+            break;
+#endif
         ret = ioapic_guest_read(apic.apic_physbase, apic.reg, &apic.value);
         if ( copy_to_guest(arg, &apic, 1) != 0 )
             ret = -EFAULT;
@@ -447,12 +450,15 @@ ret_t do_physdev_op(int cmd, XEN_GUEST_HANDLE(void) arg)
         ret = -EFAULT;
         if ( copy_from_guest(&apic, arg, 1) != 0 )
             break;
-        ret = -EPERM;
-        if ( !IS_PRIV(v->domain) )
-            break;
+#ifdef XSM_ENABLE
         ret = xsm_apic(v->domain, cmd);
         if ( ret )
             break;
+#else
+        ret = -EPERM;
+        if ( !IS_PRIV(v->domain) )
+            break;
+#endif
         ret = ioapic_guest_write(apic.apic_physbase, apic.reg, apic.value);
         break;
     }
diff --git a/xen/arch/x86/platform_hypercall.c b/xen/arch/x86/platform_hypercall.c
index 88880b0..099fe71 100644
--- a/xen/arch/x86/platform_hypercall.c
+++ b/xen/arch/x86/platform_hypercall.c
@@ -65,8 +65,10 @@ ret_t do_platform_op(XEN_GUEST_HANDLE(xen_platform_op_t) u_xenpf_op)
     ret_t ret = 0;
     struct xen_platform_op curop, *op = &curop;
 
+#ifndef XSM_ENABLE
     if ( !IS_PRIV(current->domain) )
         return -EPERM;
+#endif
 
     if ( copy_from_guest(op, u_xenpf_op, 1) )
         return -EFAULT;
@@ -501,6 +503,10 @@ ret_t do_platform_op(XEN_GUEST_HANDLE(xen_platform_op_t) u_xenpf_op)
     {
         struct xenpf_pcpu_version *ver = &op->u.pcpu_version;
 
+        ret = xsm_getcpuinfo();
+        if ( ret )
+            break;
+
         if ( !get_cpu_maps() )
         {
             ret = -EBUSY;
@@ -618,6 +624,10 @@ ret_t do_platform_op(XEN_GUEST_HANDLE(xen_platform_op_t) u_xenpf_op)
     {
         uint32_t idle_nums;
 
+        ret = xsm_pm_op();
+        if ( ret )
+            break;
+
         switch(op->u.core_parking.type)
         {
         case XEN_CORE_PARKING_SET:
diff --git a/xen/common/domctl.c b/xen/common/domctl.c
index a880288..7200a4c 100644
--- a/xen/common/domctl.c
+++ b/xen/common/domctl.c
@@ -267,14 +267,51 @@ long do_domctl(XEN_GUEST_HANDLE(xen_domctl_t) u_domctl)
         break;
     }
 #ifdef XSM_ENABLE
+    /* All of these have XSM checks which replace the IS_PRIV check */
+    case XEN_DOMCTL_setvcpucontext:
+    case XEN_DOMCTL_pausedomain:
+    case XEN_DOMCTL_unpausedomain:
+    case XEN_DOMCTL_resumedomain:
+    case XEN_DOMCTL_max_vcpus:
+    case XEN_DOMCTL_destroydomain:
+    case XEN_DOMCTL_setvcpuaffinity:
+    case XEN_DOMCTL_getvcpuaffinity:
+    case XEN_DOMCTL_scheduler_op:
     case XEN_DOMCTL_getdomaininfo:
+    case XEN_DOMCTL_getvcpucontext:
     case XEN_DOMCTL_getvcpuinfo:
+    case XEN_DOMCTL_max_mem:
+    case XEN_DOMCTL_setdomainhandle:
+    case XEN_DOMCTL_setdebugging:
+    case XEN_DOMCTL_settimeoffset:
+    case XEN_DOMCTL_subscribe:
+    case XEN_DOMCTL_disable_migrate:
+    case XEN_DOMCTL_set_virq_handler:
+
+    case XEN_DOMCTL_getpageframeinfo:
+    case XEN_DOMCTL_getmemlist:
+    case XEN_DOMCTL_hypercall_init:
+    case XEN_DOMCTL_sethvmcontext:
+    case XEN_DOMCTL_gethvmcontext:
+    case XEN_DOMCTL_gethvmcontext_partial:
+    case XEN_DOMCTL_set_address_size:
+    case XEN_DOMCTL_get_address_size:
+    case XEN_DOMCTL_set_machine_address_size:
+    case XEN_DOMCTL_get_machine_address_size:
+    case XEN_DOMCTL_sendtrigger:
+    case XEN_DOMCTL_setvcpuextstate:
+    case XEN_DOMCTL_getvcpuextstate:
+    case XEN_DOMCTL_pin_mem_cacheattr:
+    case XEN_DOMCTL_set_ext_vcpucontext:
+    case XEN_DOMCTL_get_ext_vcpucontext:
+    case XEN_DOMCTL_mem_event_op:
+    case XEN_DOMCTL_mem_sharing_op:
+    case XEN_DOMCTL_set_access_required:
         break;
 #endif
     default:
         if ( !IS_PRIV(current->domain) )
             return -EPERM;
-        break;
     }
 
     if ( !domctl_lock_acquire() )
diff --git a/xen/common/sysctl.c b/xen/common/sysctl.c
index ea68278..ce3e351 100644
--- a/xen/common/sysctl.c
+++ b/xen/common/sysctl.c
@@ -33,15 +33,42 @@ long do_sysctl(XEN_GUEST_HANDLE(xen_sysctl_t) u_sysctl)
     struct xen_sysctl curop, *op = &curop;
     static DEFINE_SPINLOCK(sysctl_lock);
 
-    if ( !IS_PRIV(current->domain) )
-        return -EPERM;
-
     if ( copy_from_guest(op, u_sysctl, 1) )
         return -EFAULT;
 
     if ( op->interface_version != XEN_SYSCTL_INTERFACE_VERSION )
         return -EACCES;
 
+    switch ( op->cmd )
+    {
+#ifdef XSM_ENABLE
+    /* All of these have XSM checks which replace the IS_PRIV check */
+    case XEN_SYSCTL_readconsole:
+    case XEN_SYSCTL_tbuf_op:
+    case XEN_SYSCTL_sched_id:
+    case XEN_SYSCTL_getdomaininfolist:
+    case XEN_SYSCTL_perfc_op:
+    case XEN_SYSCTL_lockprof_op:
+    case XEN_SYSCTL_debug_keys:
+    case XEN_SYSCTL_getcpuinfo:
+    case XEN_SYSCTL_availheap:
+    case XEN_SYSCTL_get_pmstat:
+    case XEN_SYSCTL_pm_op:
+    case XEN_SYSCTL_page_offline_op:
+    case XEN_SYSCTL_cpupool_op:
+    case XEN_SYSCTL_scheduler_op:
+
+    case XEN_SYSCTL_physinfo:
+    case XEN_SYSCTL_topologyinfo:
+    case XEN_SYSCTL_numainfo:
+    case XEN_SYSCTL_cpu_hotplug:
+        break;
+#endif
+    default:
+        if ( !IS_PRIV(current->domain) )
+            return -EPERM;
+    }
+
     /*
      * Trylock here avoids deadlock with an existing sysctl critical section
      * which might (for some current or future reason) want to synchronise
diff --git a/xen/xsm/dummy.c b/xen/xsm/dummy.c
index 0c273a5..1653801 100644
--- a/xen/xsm/dummy.c
+++ b/xen/xsm/dummy.c
@@ -21,21 +21,29 @@ static void dummy_security_domaininfo(struct domain *d,
 
 static int dummy_setvcpucontext(struct domain *d)
 {
+    if ( !IS_PRIV(current->domain) )
+        return -EPERM;
     return 0;
 }
 
 static int dummy_pausedomain (struct domain *d)
 {
+    if ( !IS_PRIV(current->domain) )
+        return -EPERM;
     return 0;
 }
 
 static int dummy_unpausedomain (struct domain *d)
 {
+    if ( !IS_PRIV(current->domain) )
+        return -EPERM;
     return 0;
 }
 
 static int dummy_resumedomain (struct domain *d)
 {
+    if ( !IS_PRIV(current->domain) )
+        return -EPERM;
     return 0;
 }
 
@@ -46,21 +54,29 @@ static int dummy_domain_create(struct domain *d, u32 ssidref)
 
 static int dummy_max_vcpus(struct domain *d)
 {
+    if ( !IS_PRIV(current->domain) )
+        return -EPERM;
     return 0;
 }
 
 static int dummy_destroydomain (struct domain *d)
 {
+    if ( !IS_PRIV(current->domain) )
+        return -EPERM;
     return 0;
 }
 
 static int dummy_vcpuaffinity (int cmd, struct domain *d)
 {
+    if ( !IS_PRIV(current->domain) )
+        return -EPERM;
     return 0;
 }
 
 static int dummy_scheduler (struct domain *d)
 {
+    if ( !IS_PRIV(current->domain) )
+        return -EPERM;
     return 0;
 }
 
@@ -73,6 +89,8 @@ static int dummy_getdomaininfo (struct domain *d)
 
 static int dummy_getvcpucontext (struct domain *d)
 {
+    if ( !IS_PRIV(current->domain) )
+        return -EPERM;
     return 0;
 }
 
@@ -85,81 +103,113 @@ static int dummy_getvcpuinfo (struct domain *d)
 
 static int dummy_domain_settime (struct domain *d)
 {
+    if ( !IS_PRIV(current->domain) )
+        return -EPERM;
     return 0;
 }
 
 static int dummy_set_target (struct domain *d, struct domain *e)
 {
+    if ( !IS_PRIV(current->domain) )
+        return -EPERM;
     return 0;
 }
 
 static int dummy_domctl(struct domain *d, int cmd)
 {
+    if ( !IS_PRIV(current->domain) )
+        return -EPERM;
     return 0;
 }
 
 static int dummy_set_virq_handler(struct domain *d, uint32_t virq)
 {
+    if ( !IS_PRIV(current->domain) )
+        return -EPERM;
     return 0;
 }
 
 static int dummy_tbufcontrol (void)
 {
+    if ( !IS_PRIV(current->domain) )
+        return -EPERM;
     return 0;
 }
 
 static int dummy_readconsole (uint32_t clear)
 {
+    if ( !IS_PRIV(current->domain) )
+        return -EPERM;
     return 0;
 }
 
 static int dummy_sched_id (void)
 {
+    if ( !IS_PRIV(current->domain) )
+        return -EPERM;
     return 0;
 }
 
 static int dummy_setdomainmaxmem (struct domain *d)
 {
+    if ( !IS_PRIV(current->domain) )
+        return -EPERM;
     return 0;
 }
 
 static int dummy_setdomainhandle (struct domain *d)
 {
+    if ( !IS_PRIV(current->domain) )
+        return -EPERM;
     return 0;
 }
 
 static int dummy_setdebugging (struct domain *d)
 {
+    if ( !IS_PRIV(current->domain) )
+        return -EPERM;
     return 0;
 }
 
 static int dummy_perfcontrol (void)
 {
+    if ( !IS_PRIV(current->domain) )
+        return -EPERM;
     return 0;
 }
 
 static int dummy_debug_keys (void)
 {
+    if ( !IS_PRIV(current->domain) )
+        return -EPERM;
     return 0;
 }
 
 static int dummy_getcpuinfo (void)
 {
+    if ( !IS_PRIV(current->domain) )
+        return -EPERM;
     return 0;
 }
 
 static int dummy_get_pmstat (void)
 {
+    if ( !IS_PRIV(current->domain) )
+        return -EPERM;
     return 0;
 }
 
 static int dummy_setpminfo (void)
 {
+    if ( !IS_PRIV(current->domain) )
+        return -EPERM;
     return 0;
 }
 
 static int dummy_pm_op (void)
 {
+    if ( !IS_PRIV(current->domain) )
+        return -EPERM;
     return 0;
 }
 
@@ -170,6 +220,8 @@ static int dummy_do_mca (void)
 
 static int dummy_availheap (void)
 {
+    if ( !IS_PRIV(current->domain) )
+        return -EPERM;
     return 0;
 }
 
@@ -297,6 +349,20 @@ static char *dummy_show_security_evtchn (struct domain *d, const struct evtchn *
     return NULL;
 }
 
+static int dummy_get_pod_target(struct domain *d)
+{
+    if ( !IS_PRIV(current->domain) )
+        return -EPERM;
+    return 0;
+}
+
+static int dummy_set_pod_target(struct domain *d)
+{
+    if ( !IS_PRIV(current->domain) )
+        return -EPERM;
+    return 0;
+}
+
 static int dummy_test_assign_device (uint32_t machine_bdf)
 {
     return 0;
@@ -314,11 +380,15 @@ static int dummy_deassign_device (struct domain *d, uint32_t machine_bdf)
 
 static int dummy_resource_plug_core (void)
 {
+    if ( !IS_PRIV(current->domain) )
+        return -EPERM;
     return 0;
 }
 
 static int dummy_resource_unplug_core (void)
 {
+    if ( !IS_PRIV(current->domain) )
+        return -EPERM;
     return 0;
 }
 
@@ -349,21 +419,29 @@ static int dummy_resource_setup_misc (void)
 
 static int dummy_page_offline (uint32_t cmd)
 {
+    if ( !IS_PRIV(current->domain) )
+        return -EPERM;
     return 0;
 }
 
 static int dummy_lockprof (void)
 {
+    if ( !IS_PRIV(current->domain) )
+        return -EPERM;
     return 0;
 }
 
 static int dummy_cpupool_op (void)
 {
+    if ( !IS_PRIV(current->domain) )
+        return -EPERM;
     return 0;
 }
 
 static int dummy_sched_op (void)
 {
+    if ( !IS_PRIV(current->domain) )
+        return -EPERM;
     return 0;
 }
 
@@ -407,31 +485,43 @@ static int dummy_shadow_control (struct domain *d, uint32_t op)
 
 static int dummy_getpageframeinfo (struct page_info *page)
 {
+    if ( !IS_PRIV(current->domain) )
+        return -EPERM;
     return 0;
 }
 
 static int dummy_getmemlist (struct domain *d)
 {
+    if ( !IS_PRIV(current->domain) )
+        return -EPERM;
     return 0;
 }
 
 static int dummy_hypercall_init (struct domain *d)
 {
+    if ( !IS_PRIV(current->domain) )
+        return -EPERM;
     return 0;
 }
 
 static int dummy_hvmcontext (struct domain *d, uint32_t cmd)
 {
+    if ( !IS_PRIV(current->domain) )
+        return -EPERM;
     return 0;
 }
 
 static int dummy_address_size (struct domain *d, uint32_t cmd)
 {
+    if ( !IS_PRIV(current->domain) )
+        return -EPERM;
     return 0;
 }
 
 static int dummy_machine_address_size (struct domain *d, uint32_t cmd)
 {
+    if ( !IS_PRIV(current->domain) )
+        return -EPERM;
     return 0;
 }
 
@@ -462,66 +552,99 @@ static int dummy_hvm_inject_msi (struct domain *d)
 
 static int dummy_mem_event (struct domain *d)
 {
+    if ( !IS_PRIV(current->domain) )
+        return -EPERM;
     return 0;
 }
 
 static int dummy_mem_sharing (struct domain *d)
 {
+    if ( !IS_PRIV(current->domain) )
+        return -EPERM;
     return 0;
 }
 
 static int dummy_apic (struct domain *d, int cmd)
 {
+    if ( !IS_PRIV(current->domain) )
+        return -EPERM;
     return 0;
 }
 
 static int dummy_xen_settime (void)
 {
+    if ( !IS_PRIV(current->domain) )
+        return -EPERM;
     return 0;
 }
 
 static int dummy_memtype (uint32_t access)
 {
+    if ( !IS_PRIV(current->domain) )
+        return -EPERM;
     return 0;
 }
 
 static int dummy_microcode (void)
 {
+    if ( !IS_PRIV(current->domain) )
+        return -EPERM;
     return 0;
 }
 
 static int dummy_physinfo (void)
 {
+    if ( !IS_PRIV(current->domain) )
+        return -EPERM;
     return 0;
 }
 
 static int dummy_platform_quirk (uint32_t quirk)
 {
+    if ( !IS_PRIV(current->domain) )
+        return -EPERM;
     return 0;
 }
 
 static int dummy_firmware_info (void)
 {
+    if ( !IS_PRIV(current->domain) )
+        return -EPERM;
+    return 0;
+}
+
+static int dummy_efi_call (void)
+{
+    if ( !IS_PRIV(current->domain) )
+        return -EPERM;
     return 0;
 }
 
 static int dummy_acpi_sleep (void)
 {
+    if ( !IS_PRIV(current->domain) )
+        return -EPERM;
     return 0;
 }
 
 static int dummy_change_freq (void)
 {
+    if ( !IS_PRIV(current->domain) )
+        return -EPERM;
     return 0;
 }
 
 static int dummy_getidletime (void)
 {
+    if ( !IS_PRIV(current->domain) )
+        return -EPERM;
     return 0;
 }
 
 static int dummy_machine_memory_map (void)
 {
+    if ( !IS_PRIV(current->domain) )
+        return -EPERM;
     return 0;
 }
 
@@ -559,6 +682,8 @@ static int dummy_remove_from_physmap (struct domain *d1, struct domain *d2)
 
 static int dummy_sendtrigger (struct domain *d)
 {
+    if ( !IS_PRIV(current->domain) )
+        return -EPERM;
     return 0;
 }
 
@@ -569,16 +694,22 @@ static int dummy_bind_pt_irq (struct domain *d, struct xen_domctl_bind_pt_irq *b
 
 static int dummy_pin_mem_cacheattr (struct domain *d)
 {
+    if ( !IS_PRIV(current->domain) )
+        return -EPERM;
     return 0;
 }
 
 static int dummy_ext_vcpucontext (struct domain *d, uint32_t cmd)
 {
+    if ( !IS_PRIV(current->domain) )
+        return -EPERM;
     return 0;
 }
 
 static int dummy_vcpuextstate (struct domain *d, uint32_t cmd)
 {
+    if ( !IS_PRIV(current->domain) )
+        return -EPERM;
     return 0;
 }
 
@@ -654,6 +785,8 @@ void xsm_fixup_ops (struct xsm_operations *ops)
     set_to_dummy_if_null(ops, alloc_security_evtchn);
     set_to_dummy_if_null(ops, free_security_evtchn);
     set_to_dummy_if_null(ops, show_security_evtchn);
+    set_to_dummy_if_null(ops, get_pod_target);
+    set_to_dummy_if_null(ops, set_pod_target);
 
     set_to_dummy_if_null(ops, memory_adjust_reservation);
     set_to_dummy_if_null(ops, memory_stat_reservation);
@@ -713,6 +846,7 @@ void xsm_fixup_ops (struct xsm_operations *ops)
     set_to_dummy_if_null(ops, physinfo);
     set_to_dummy_if_null(ops, platform_quirk);
     set_to_dummy_if_null(ops, firmware_info);
+    set_to_dummy_if_null(ops, efi_call);
     set_to_dummy_if_null(ops, acpi_sleep);
     set_to_dummy_if_null(ops, change_freq);
     set_to_dummy_if_null(ops, getidletime);
diff --git a/xen/xsm/flask/hooks.c b/xen/xsm/flask/hooks.c
index a7e4f2a..dfe44e4 100644
--- a/xen/xsm/flask/hooks.c
+++ b/xen/xsm/flask/hooks.c
@@ -551,6 +551,9 @@ static int flask_domain_settime(struct domain *d)
 
 static int flask_set_target(struct domain *d, struct domain *e)
 {
+    /* Note: replace with check on setting another domain's target */
+    if ( !IS_PRIV(current->domain) )
+        return -EPERM;
     return domain_has_perm(d, e, SECCLASS_DOMAIN, DOMAIN__SET_TARGET);
 }
 
@@ -1158,6 +1161,11 @@ static int flask_firmware_info(void)
     return domain_has_xen(current->domain, XEN__FIRMWARE);
 }
 
+static int flask_efi_call(void)
+{
+    return domain_has_xen(current->domain, XEN__FIRMWARE);
+}
+
 static int flask_acpi_sleep(void)
 {
     return domain_has_xen(current->domain, XEN__SLEEP);
@@ -1521,6 +1529,7 @@ static struct xsm_operations flask_ops = {
     .physinfo = flask_physinfo,
     .platform_quirk = flask_platform_quirk,
     .firmware_info = flask_firmware_info,
+    .efi_call = flask_efi_call,
     .acpi_sleep = flask_acpi_sleep,
     .change_freq = flask_change_freq,
     .getidletime = flask_getidletime,
-- 
1.7.11.2

Re: [PATCH RFC] xsm: use XSM instead of IS_PRIV where duplicated

[Jan Beulich]

>>> On 30.07.12 at 21:49, Daniel De Graaf <dgdegra@tycho.nsa.gov> wrote:
> --- a/xen/arch/x86/mm.c
> +++ b/xen/arch/x86/mm.c
> @@ -4790,8 +4790,10 @@ long arch_memory_op(int op, XEN_GUEST_HANDLE(void) arg)
>          XEN_GUEST_HANDLE(e820entry_t) buffer;
>          unsigned int i;
>  
> +#ifndef XSM_ENABLE
>          if ( !IS_PRIV(current->domain) )
>              return -EINVAL;
> +#endif

This recurring a number of times probably warrants some
abstraction, to avoid the #ifdef-ery?

> --- a/xen/arch/x86/physdev.c
> +++ b/xen/arch/x86/physdev.c
> @@ -430,12 +430,15 @@ ret_t do_physdev_op(int cmd, XEN_GUEST_HANDLE(void) 
> arg)
>          ret = -EFAULT;
>          if ( copy_from_guest(&apic, arg, 1) != 0 )
>              break;
> -        ret = -EPERM;
> -        if ( !IS_PRIV(v->domain) )
> -            break;
> +#ifdef XSM_ENABLE
>          ret = xsm_apic(v->domain, cmd);
>          if ( ret )
>              break;
> +#else
> +        ret = -EPERM;
> +        if ( !IS_PRIV(v->domain) )
> +            break;
> +#endif

Can't this be moved into the dummy stub, just like for other
cases?

Jan

Re: [PATCH RFC] xsm: use XSM instead of IS_PRIV where duplicated

[Daniel De Graaf]

On 07/31/2012 03:18 AM, Jan Beulich wrote:
>>>> On 30.07.12 at 21:49, Daniel De Graaf <dgdegra@tycho.nsa.gov> wrote:
>> --- a/xen/arch/x86/mm.c
>> +++ b/xen/arch/x86/mm.c
>> @@ -4790,8 +4790,10 @@ long arch_memory_op(int op, XEN_GUEST_HANDLE(void) arg)
>>          XEN_GUEST_HANDLE(e820entry_t) buffer;
>>          unsigned int i;
>>  
>> +#ifndef XSM_ENABLE
>>          if ( !IS_PRIV(current->domain) )
>>              return -EINVAL;
>> +#endif
> 
> This recurring a number of times probably warrants some
> abstraction, to avoid the #ifdef-ery?
> 
>> --- a/xen/arch/x86/physdev.c
>> +++ b/xen/arch/x86/physdev.c
>> @@ -430,12 +430,15 @@ ret_t do_physdev_op(int cmd, XEN_GUEST_HANDLE(void) 
>> arg)
>>          ret = -EFAULT;
>>          if ( copy_from_guest(&apic, arg, 1) != 0 )
>>              break;
>> -        ret = -EPERM;
>> -        if ( !IS_PRIV(v->domain) )
>> -            break;
>> +#ifdef XSM_ENABLE
>>          ret = xsm_apic(v->domain, cmd);
>>          if ( ret )
>>              break;
>> +#else
>> +        ret = -EPERM;
>> +        if ( !IS_PRIV(v->domain) )
>> +            break;
>> +#endif
> 
> Can't this be moved into the dummy stub, just like for other
> cases?
> 
> Jan
> 

I think the best solution here is to eliminate the explicit IS_PRIV checks
and have the XSM header switch between IS_PRIV and the XSM hooks depending
on XSM_ENABLE - all the calling code will look like the #ifdef XSM_ENABLE
version.

This ends up duplicating a lot of code between xsm/dummy.c and xsm/xsm.h,
so I'm considering doing something like this:

/* dummy.h: */

static int XSM_DEFAULT(apic)(struct domain *d, int cmd)
{
    if ( !IS_PRIV(current->domain) )
        return -EPERM;
    return 0;
}

/* dummy.c: */

#define XSM_DEFAULT(x) dummy_ ## x
#include "xsm/dummy.h"

struct xsm_operations dummy_xsm_ops;
/* set_to_dummy_if_null calls to populate */

/* xsm.h: */

#ifdef XSM_ENABLE
/* existing xsm_* inline wrappers around xsm_call */
#else
#define XSM_DEFAULT(x) inline xsm_ ## x
#include "xsm/dummy.h"
#endif

This would also allow the compiler to catch XSM function omissions in
dummy.c, which is currently not done - leaving them to be caught via
null function pointer dereferences at runtime when XSM is compiled but
no policy is loaded.

-- 
Daniel De Graaf
National Security Agency

Re: XSM instead of IS_PRIV

[Shakeel Butt]

> Once this is done, it will make sense to change the XSM header file so
> that the appropriate xsm_* hooks resolve to IS_PRIV when XSM is not
> enabled. This would allow the IS_PRIV hooks covered by XSM to be
> completely removed, assuming the Xen developers agree that the XSM hooks
> cover all all the important code.

Thanks for replying. So, is there consensus among the Xen community
to adopt this patch (xsm hooks replacing IS_PRIV) on upstream Xen?

thanks,
Shakeel

Re: XSM instead of IS_PRIV

[Keir Fraser]

On 01/08/2012 03:51, "Shakeel Butt" <shakeel.butt@gmail.com> wrote:

>> Once this is done, it will make sense to change the XSM header file so
>> that the appropriate xsm_* hooks resolve to IS_PRIV when XSM is not
>> enabled. This would allow the IS_PRIV hooks covered by XSM to be
>> completely removed, assuming the Xen developers agree that the XSM hooks
>> cover all all the important code.
> 
> Thanks for replying. So, is there consensus among the Xen community
> to adopt this patch (xsm hooks replacing IS_PRIV) on upstream Xen?

Assuming it can be done cleanly and comprehensively, with same behaviour as
IS_PRIV for an out-of-the-box setup, yes. What's not to like about that? :)

 -- Keir

> thanks,
> Shakeel
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel