[Xen-devel] [Patch][ACM] fix remaining xen handle conversion and fix domain destroy bug

[Reiner Sailer <sailer@xxxxxxxxxx>]

This patch completes the conversion of acm_ops using Xen handles instead 
of void pointers. It also fixes a recently slipped in acm bug that hangs 
Xen when a domain is destroyed.
Regards
Reiner

---
 tools/security/secpol_tool.c |    7 ++++---
 xen/acm/acm_core.c           |    5 ++---
 xen/acm/acm_policy.c         |   41 +++++++++++++++++++++++------------------
 xen/common/acm_ops.c         |    2 +-
 xen/common/dom0_ops.c        |    4 ++--
 xen/include/acm/acm_core.h   |    9 +++++----
 xen/include/acm/acm_hooks.h  |   18 ++++++++++++++++--
 7 files changed, 53 insertions(+), 33 deletions(-)

Index: xen-unstable-test/tools/security/secpol_tool.c
===================================================================
--- xen-unstable-test.orig/tools/security/secpol_tool.c
+++ xen-unstable-test/tools/security/secpol_tool.c
@@ -229,6 +229,7 @@ void acm_dump_policy_buffer(void *buf, i
 
 #define PULL_CACHE_SIZE                8192
 uint8_t pull_buffer[PULL_CACHE_SIZE];
+
 int acm_domain_getpolicy(int xc_handle)
 {
     struct acm_getpolicy getpolicy;
@@ -236,7 +237,7 @@ int acm_domain_getpolicy(int xc_handle)
 
     memset(pull_buffer, 0x00, sizeof(pull_buffer));
     getpolicy.interface_version = ACM_INTERFACE_VERSION;
-    getpolicy.pullcache = (void *) pull_buffer;
+    set_xen_guest_handle(getpolicy.pullcache, pull_buffer);
     getpolicy.pullcache_size = sizeof(pull_buffer);
     ret = xc_acm_op(xc_handle, ACMOP_getpolicy, &getpolicy, sizeof(getpolicy));
 
@@ -281,7 +282,7 @@ int acm_domain_loadpolicy(int xc_handle,
         /* dump it and then push it down into xen/acm */
         acm_dump_policy_buffer(buffer, len);
         setpolicy.interface_version = ACM_INTERFACE_VERSION;
-        setpolicy.pushcache = (void *) buffer;
+        set_xen_guest_handle(setpolicy.pushcache, buffer);
         setpolicy.pushcache_size = len;
         ret = xc_acm_op(xc_handle, ACMOP_setpolicy, &setpolicy, 
sizeof(setpolicy));
 
@@ -330,7 +331,7 @@ int acm_domain_dumpstats(int xc_handle)
 
     memset(stats_buffer, 0x00, sizeof(stats_buffer));
     dumpstats.interface_version = ACM_INTERFACE_VERSION;
-    dumpstats.pullcache = (void *) stats_buffer;
+    set_xen_guest_handle(dumpstats.pullcache, stats_buffer);
     dumpstats.pullcache_size = sizeof(stats_buffer);
     ret = xc_acm_op(xc_handle, ACMOP_dumpstats, &dumpstats, sizeof(dumpstats));
 
Index: xen-unstable-test/xen/acm/acm_core.c
===================================================================
--- xen-unstable-test.orig/xen/acm/acm_core.c
+++ xen-unstable-test/xen/acm/acm_core.c
@@ -222,9 +222,8 @@ acm_setup(unsigned int *initrdidx,
         pol = (struct acm_policy_buffer *)_policy_start;
         if (ntohl(pol->magic) == ACM_MAGIC)
         {
-            rc = acm_set_policy((void *)_policy_start,
-                                (u32)_policy_len,
-                                0);
+            rc = do_acm_set_policy((void *)_policy_start,
+                                   (u32)_policy_len);
             if (rc == ACM_OK)
             {
                 printkd("Policy len  0x%lx, start at 
%p.\n",_policy_len,_policy_start);
Index: xen-unstable-test/xen/acm/acm_policy.c
===================================================================
--- xen-unstable-test.orig/xen/acm/acm_policy.c
+++ xen-unstable-test/xen/acm/acm_policy.c
@@ -26,36 +26,43 @@
 #include <xen/lib.h>
 #include <xen/delay.h>
 #include <xen/sched.h>
+#include <xen/guest_access.h>
 #include <acm/acm_core.h>
 #include <public/acm_ops.h>
 #include <acm/acm_hooks.h>
 #include <acm/acm_endian.h>
 
 int
-acm_set_policy(XEN_GUEST_HANDLE(void) buf, u32 buf_size, int isuserbuffer)
+acm_set_policy(XEN_GUEST_HANDLE(void) buf, u32 buf_size)
 {
     u8 *policy_buffer = NULL;
-    struct acm_policy_buffer *pol;
+    int ret = -EFAULT;
  
     if (buf_size < sizeof(struct acm_policy_buffer))
         return -EFAULT;
 
-    /* 1. copy buffer from domain */
+    /* copy buffer from guest domain */
     if ((policy_buffer = xmalloc_array(u8, buf_size)) == NULL)
         return -ENOMEM;
 
-    if (isuserbuffer) {
-        if (copy_from_guest(policy_buffer, buf, buf_size))
-        {
-            printk("%s: Error copying!\n",__func__);
-            goto error_free;
-        }
-    } else
-        memcpy(policy_buffer, buf, buf_size);
+    if (copy_from_guest(policy_buffer, buf, buf_size))
+    {
+        printk("%s: Error copying!\n",__func__);
+        goto error_free;
+    }
+    ret = do_acm_set_policy(policy_buffer, buf_size);
+
+ error_free:
+    xfree(policy_buffer);
+    return ret;
+}
 
-    /* 2. some sanity checking */
-    pol = (struct acm_policy_buffer *)policy_buffer;
 
+int
+do_acm_set_policy(void *buf, u32 buf_size)
+{
+    struct acm_policy_buffer *pol = (struct acm_policy_buffer *)buf;
+    /* some sanity checking */
     if ((ntohl(pol->magic) != ACM_MAGIC) ||
         (buf_size != ntohl(pol->len)) ||
         (ntohl(pol->policy_version) != ACM_POLICY_VERSION))
@@ -85,33 +92,31 @@ acm_set_policy(XEN_GUEST_HANDLE(void) bu
     /* get bin_policy lock and rewrite policy (release old one) */
     write_lock(&acm_bin_pol_rwlock);
 
-    /* 3. set label reference name */
+    /* set label reference name */
     if (acm_set_policy_reference(buf + ntohl(pol->policy_reference_offset),
                                  ntohl(pol->primary_buffer_offset) -
                                  ntohl(pol->policy_reference_offset)))
         goto error_lock_free;
 
-    /* 4. set primary policy data */
+    /* set primary policy data */
     if (acm_primary_ops->set_binary_policy(buf + 
ntohl(pol->primary_buffer_offset),
                                            ntohl(pol->secondary_buffer_offset) 
-
                                            ntohl(pol->primary_buffer_offset)))
         goto error_lock_free;
 
-    /* 5. set secondary policy data */
+    /* set secondary policy data */
     if (acm_secondary_ops->set_binary_policy(buf + 
ntohl(pol->secondary_buffer_offset),
                                              ntohl(pol->len) - 
                                              
ntohl(pol->secondary_buffer_offset)))
         goto error_lock_free;
 
     write_unlock(&acm_bin_pol_rwlock);
-    xfree(policy_buffer);
     return ACM_OK;
 
  error_lock_free:
     write_unlock(&acm_bin_pol_rwlock);
  error_free:
     printk("%s: Error setting policy.\n", __func__);
-    xfree(policy_buffer);
     return -EFAULT;
 }
 
Index: xen-unstable-test/xen/common/acm_ops.c
===================================================================
--- xen-unstable-test.orig/xen/common/acm_ops.c
+++ xen-unstable-test/xen/common/acm_ops.c
@@ -69,7 +69,7 @@ long do_acm_op(int cmd, XEN_GUEST_HANDLE
             return -EACCES;
 
         rc = acm_set_policy(setpolicy.pushcache,
-                            setpolicy.pushcache_size, 1);
+                            setpolicy.pushcache_size);
         break;
     }
 
Index: xen-unstable-test/xen/common/dom0_ops.c
===================================================================
--- xen-unstable-test.orig/xen/common/dom0_ops.c
+++ xen-unstable-test/xen/common/dom0_ops.c
@@ -701,9 +701,9 @@ long do_dom0_op(XEN_GUEST_HANDLE(dom0_op
     spin_unlock(&dom0_lock);
 
     if (!ret)
-        acm_post_dom0_op(op, ssid);
+        acm_post_dom0_op(op, &ssid);
     else
-        acm_fail_dom0_op(op, ssid);
+        acm_fail_dom0_op(op, &ssid);
 
     return ret;
 }
Index: xen-unstable-test/xen/include/acm/acm_core.h
===================================================================
--- xen-unstable-test.orig/xen/include/acm/acm_core.h
+++ xen-unstable-test/xen/include/acm/acm_core.h
@@ -121,10 +121,11 @@ struct ste_ssid {
 int acm_init_domain_ssid(domid_t id, ssidref_t ssidref);
 void acm_free_domain_ssid(struct acm_ssid_domain *ssid);
 int acm_init_binary_policy(u32 policy_code);
-int acm_set_policy(void *buf, u32 buf_size, int isuserbuffer);
-int acm_get_policy(void *buf, u32 buf_size);
-int acm_dump_statistics(void *buf, u16 buf_size);
-int acm_get_ssid(ssidref_t ssidref, u8 *buf, u16 buf_size);
+int acm_set_policy(XEN_GUEST_HANDLE(void) buf, u32 buf_size);
+int do_acm_set_policy(void *buf, u32 buf_size);
+int acm_get_policy(XEN_GUEST_HANDLE(void) buf, u32 buf_size);
+int acm_dump_statistics(XEN_GUEST_HANDLE(void) buf, u16 buf_size);
+int acm_get_ssid(ssidref_t ssidref, XEN_GUEST_HANDLE(void) buf, u16 buf_size);
 int acm_get_decision(ssidref_t ssidref1, ssidref_t ssidref2, u32 hook);
 int acm_set_policy_reference(u8 * buf, u32 buf_size);
 int acm_dump_policy_reference(u8 *buf, u32 buf_size);
Index: xen-unstable-test/xen/include/acm/acm_hooks.h
===================================================================
--- xen-unstable-test.orig/xen/include/acm/acm_hooks.h
+++ xen-unstable-test/xen/include/acm/acm_hooks.h
@@ -273,7 +273,12 @@ static inline void acm_post_dom0_op(stru
             op->u.createdomain.domain, op->u.createdomain.ssidref);
         break;
     case DOM0_DESTROYDOMAIN:
-        acm_post_domain_destroy(ssid, op->u.destroydomain.domain);
+        if (*ssid == NULL) {
+            printkd("%s: ERROR. SSID unset.\n",
+                    __func__);
+            break;
+        }
+        acm_post_domain_destroy(*ssid, op->u.destroydomain.domain);
         /* free security ssid for the destroyed domain (also if null policy */
         acm_free_domain_ssid((struct acm_ssid_domain *)(*ssid));
         *ssid = NULL;
@@ -281,13 +286,22 @@ static inline void acm_post_dom0_op(stru
     }
 }
 
-static inline void acm_fail_dom0_op(struct dom0_op *op, void *ssid) 
+static inline void acm_fail_dom0_op(struct dom0_op *op, void **ssid)
 {
     switch(op->cmd) {
     case DOM0_CREATEDOMAIN:
         acm_fail_domain_create(
             current->domain->ssid, op->u.createdomain.ssidref);
         break;
+    case DOM0_DESTROYDOMAIN:
+        /*  we don't handle domain destroy failure but at least free the ssid 
*/
+        if (*ssid == NULL) {
+            printkd("%s: ERROR. SSID unset.\n",
+                    __func__);
+            break;
+        }
+        acm_free_domain_ssid((struct acm_ssid_domain *)(*ssid));
+        *ssid = NULL;
     }
 }
 


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