WARNING - OLD ARCHIVES

This is an archived copy of the Xen.org mailing list, which we have preserved to ensure that existing links to archives are not broken. The live archive, which contains the latest emails, can be found at http://lists.xen.org/
   
 
 
Xen 
 
Home Products Support Community News
 
   
 

xen-devel

[Xen-devel] [PATCH] acm_ops.c cleanup

To: Keir Fraser <Keir.Fraser@xxxxxxxxxxxx>, Xen Mailing List <xen-devel@xxxxxxxxxxxxxxxxxxx>
Subject: [Xen-devel] [PATCH] acm_ops.c cleanup
From: Reiner Sailer <sailer@xxxxxxxxxx>
Date: Fri, 25 Nov 2005 00:32:04 -0500
Cc: Rusty Russell <rusty@xxxxxxxxxxxxxxx>, Tony Breeds <tony@xxxxxxxxxxxxxxxxxx>
Delivery-date: Fri, 25 Nov 2005 05:33:16 +0000
Envelope-to: www-data@xxxxxxxxxxxxxxxxxxx
List-help: <mailto:xen-devel-request@lists.xensource.com?subject=help>
List-id: Xen developer discussion <xen-devel.lists.xensource.com>
List-post: <mailto:xen-devel@lists.xensource.com>
List-subscribe: <http://lists.xensource.com/cgi-bin/mailman/listinfo/xen-devel>, <mailto:xen-devel-request@lists.xensource.com?subject=subscribe>
List-unsubscribe: <http://lists.xensource.com/cgi-bin/mailman/listinfo/xen-devel>, <mailto:xen-devel-request@lists.xensource.com?subject=unsubscribe>
Sender: xen-devel-bounces@xxxxxxxxxxxxxxxxxxx
User-agent: Mozilla Thunderbird 1.0.7 (Windows/20050923)
Keir Fraser <Keir.Fraser@xxxxxxxxxxxx> wrote on 11/24/2005 05:18:28 AM:
>
> On 24 Nov 2005, at 04:48, Tony Breeds wrote:
>
> > Hi All,
> >    xen/common/acm_ops.c, check for a NULL pointer and then
> > cheerfully dereferences it.
> >
> > Signed-off-by: Tony Breeds <tony@xxxxxxxxxxxxxxxxxx>
>
> Applied, but nearby code goto's out so I changed the patch to do that.
>
> In fact there seems to be no convention in that function as to whether
> returning immediately or goto'ing out (and doing dome return-code
> cooking) is the right thing to do. Should all the returns be goto
> out's?

I cleaned this file up and elimnated returns inside the switch statement. When we need locks, we can place them now around the switch statement.

I also included the comments from Rusty and now return -EPERM for denied permission errors.

Thanks
Reiner

Signed-off: Reiner Sailer <sailer@xxxxxxxxxx>


 xen/common/acm_ops.c |  179 +++++++++++++++++++++++++--------------------------
 1 files changed, 90 insertions(+), 89 deletions(-)

Index: xen-unstable.hg-shype/xen/common/acm_ops.c
===================================================================
--- xen-unstable.hg-shype.orig/xen/common/acm_ops.c
+++ xen-unstable.hg-shype/xen/common/acm_ops.c
@@ -49,15 +49,11 @@ enum acm_operation {
 
 int acm_authorize_acm_ops(struct domain *d, enum acm_operation pops)
 {
-    /* all policy management functions are restricted to privileged domains,
-     * soon we will introduce finer-grained privileges for policy operations
-     */
+    /* currently, policy management functions are restricted to privileged 
domains */
     if (!IS_PRIV(d))
-    {
-        printk("%s: ACM management authorization denied ERROR!\n", __func__);
-        return ACM_ACCESS_DENIED;
-    }
-    return ACM_ACCESS_PERMITTED;
+        return -EPERM;
+
+    return 0;
 }
 
 long do_acm_op(struct acm_op * u_acm_op)
@@ -65,10 +61,8 @@ long do_acm_op(struct acm_op * u_acm_op)
     long ret = 0;
     struct acm_op curop, *op = &curop;
 
-    /* check here policy decision for policy commands */
-    /* for now allow DOM0 only, later indepedently    */
     if (acm_authorize_acm_ops(current->domain, POLICY))
-        return -EACCES;
+        return -EPERM;
 
     if (copy_from_user(op, u_acm_op, sizeof(*op)))
         return -EFAULT;
@@ -80,43 +74,32 @@ long do_acm_op(struct acm_op * u_acm_op)
     {
     case ACM_SETPOLICY:
     {
-        if (acm_authorize_acm_ops(current->domain, SETPOLICY))
-            return -EACCES;
-        printkd("%s: setting policy.\n", __func__);
-        ret = acm_set_policy(op->u.setpolicy.pushcache,
-                             op->u.setpolicy.pushcache_size, 1);
-        if (ret == ACM_OK)
-            ret = 0;
-        else
-            ret = -ESRCH;
+        ret = acm_authorize_acm_ops(current->domain, SETPOLICY);
+        if (!ret)
+            ret = acm_set_policy(op->u.setpolicy.pushcache,
+                                 op->u.setpolicy.pushcache_size, 1);
     }
     break;
 
     case ACM_GETPOLICY:
     {
-        if (acm_authorize_acm_ops(current->domain, GETPOLICY))
-            return -EACCES;
-        printkd("%s: getting policy.\n", __func__);
-        ret = acm_get_policy(op->u.getpolicy.pullcache,
-                             op->u.getpolicy.pullcache_size);
-        if (ret == ACM_OK)
-            ret = 0;
-        else
-            ret = -ESRCH;
+        ret = acm_authorize_acm_ops(current->domain, GETPOLICY);
+        if (!ret)
+            ret = acm_get_policy(op->u.getpolicy.pullcache,
+                                 op->u.getpolicy.pullcache_size);
+        if (!ret)
+            copy_to_user(u_acm_op, op, sizeof(*op));
     }
     break;
 
     case ACM_DUMPSTATS:
     {
-        if (acm_authorize_acm_ops(current->domain, DUMPSTATS))
-            return -EACCES;
-        printkd("%s: dumping statistics.\n", __func__);
-        ret = acm_dump_statistics(op->u.dumpstats.pullcache,
-                                  op->u.dumpstats.pullcache_size);
-        if (ret == ACM_OK)
-            ret = 0;
-        else
-            ret = -ESRCH;
+        ret = acm_authorize_acm_ops(current->domain, DUMPSTATS);
+        if (!ret)
+            ret = acm_dump_statistics(op->u.dumpstats.pullcache,
+                                      op->u.dumpstats.pullcache_size);
+        if (!ret)
+            copy_to_user(u_acm_op, op, sizeof(*op));
     }
     break;
 
@@ -124,31 +107,39 @@ long do_acm_op(struct acm_op * u_acm_op)
     {
         ssidref_t ssidref;
 
-        if (acm_authorize_acm_ops(current->domain, GETSSID))
-            return -EACCES;
-        printkd("%s: getting SSID.\n", __func__);
+        ret = acm_authorize_acm_ops(current->domain, GETSSID);
+        if (ret)
+            break;
+
         if (op->u.getssid.get_ssid_by == SSIDREF)
             ssidref = op->u.getssid.id.ssidref;
-        else if (op->u.getssid.get_ssid_by == DOMAINID) {
+        else if (op->u.getssid.get_ssid_by == DOMAINID)
+        {
             struct domain *subj = find_domain_by_id(op->u.getssid.id.domainid);
             if (!subj)
-                return -ESRCH; /* domain not found */
-            if (subj->ssid == NULL) {
+            {
+                ret = -ESRCH; /* domain not found */
+                break;
+            }
+            if (subj->ssid == NULL)
+            {
                 put_domain(subj);
-                return -ESRCH;
+                ret = -ESRCH;
+                break;
             }
             ssidref = ((struct acm_ssid_domain *)(subj->ssid))->ssidref;
             put_domain(subj);
-        } else
-            return -ESRCH;
-
+        }
+        else
+        {
+            ret = -ESRCH;
+            break;
+        }
         ret = acm_get_ssid(ssidref,
                            op->u.getssid.ssidbuf,
                            op->u.getssid.ssidbuf_size);
-        if (ret == ACM_OK)
-            ret = 0;
-        else
-            ret = -ESRCH;
+        if (!ret)
+            copy_to_user(u_acm_op, op, sizeof(*op));
     }
     break;
 
@@ -156,51 +147,75 @@ long do_acm_op(struct acm_op * u_acm_op)
     {
         ssidref_t ssidref1, ssidref2;
 
-        if (acm_authorize_acm_ops(current->domain, GETDECISION)) {
-            ret = -EACCES;
-            goto out;
-        }
-        printkd("%s: getting access control decision.\n", __func__);
-        if (op->u.getdecision.get_decision_by1 == SSIDREF) {
+        ret = acm_authorize_acm_ops(current->domain, GETDECISION);
+        if (ret)
+            break;
+
+        if (op->u.getdecision.get_decision_by1 == SSIDREF)
             ssidref1 = op->u.getdecision.id1.ssidref;
-        }
-        else if (op->u.getdecision.get_decision_by1 == DOMAINID) {
+        else if (op->u.getdecision.get_decision_by1 == DOMAINID)
+        {
             struct domain *subj = 
find_domain_by_id(op->u.getdecision.id1.domainid);
-            if (!subj) {
+            if (!subj)
+            {
                 ret = -ESRCH; /* domain not found */
-                goto out;
+                break;
             }
-            if (subj->ssid == NULL) {
+            if (subj->ssid == NULL)
+            {
                 put_domain(subj);
                 ret = -ESRCH;
-                goto out;
+                break;
             }
             ssidref1 = ((struct acm_ssid_domain *)(subj->ssid))->ssidref;
             put_domain(subj);
-        } else {
+        }
+        else
+        {
             ret = -ESRCH;
-            goto out;
+            break;
         }
-        if (op->u.getdecision.get_decision_by2 == SSIDREF) {
+        if (op->u.getdecision.get_decision_by2 == SSIDREF)
             ssidref2 = op->u.getdecision.id2.ssidref;
-        }
-        else if (op->u.getdecision.get_decision_by2 == DOMAINID) {
+        else if (op->u.getdecision.get_decision_by2 == DOMAINID)
+        {
             struct domain *subj = 
find_domain_by_id(op->u.getdecision.id2.domainid);
-            if (!subj) {
+            if (!subj)
+            {
                 ret = -ESRCH; /* domain not found */
-                goto out;
+                break;;
             }
-            if (subj->ssid == NULL) {
+            if (subj->ssid == NULL)
+            {
                 put_domain(subj);
-                return -ESRCH;
+                ret = -ESRCH;
+                break;
             }
             ssidref2 = ((struct acm_ssid_domain *)(subj->ssid))->ssidref;
             put_domain(subj);
-        } else {
+        }
+        else
+        {
             ret = -ESRCH;
-            goto out;
+            break;
         }
         ret = acm_get_decision(ssidref1, ssidref2, op->u.getdecision.hook);
+
+        if (ret == ACM_ACCESS_PERMITTED)
+        {
+            op->u.getdecision.acm_decision = ACM_ACCESS_PERMITTED;
+            ret = 0;
+        }
+        else if  (ret == ACM_ACCESS_DENIED)
+        {
+            op->u.getdecision.acm_decision = ACM_ACCESS_DENIED;
+            ret = 0;
+        }
+        else
+            ret = -ESRCH;
+
+        if (!ret)
+            copy_to_user(u_acm_op, op, sizeof(*op));
     }
     break;
 
@@ -208,20 +223,6 @@ long do_acm_op(struct acm_op * u_acm_op)
         ret = -ESRCH;
     }
 
- out:
-    if (ret == ACM_ACCESS_PERMITTED) {
-        op->u.getdecision.acm_decision = ACM_ACCESS_PERMITTED;
-        ret = 0;
-    } else if  (ret == ACM_ACCESS_DENIED) {
-        op->u.getdecision.acm_decision = ACM_ACCESS_DENIED;
-        ret = 0;
-    } else {
-        op->u.getdecision.acm_decision = ACM_ACCESS_DENIED;
-        if (ret > 0)
-            ret = -ret;
-    }
-    /* copy decision back to user space */
-    copy_to_user(u_acm_op, op, sizeof(*op));
     return ret;
 }
 
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel
<Prev in Thread] Current Thread [Next in Thread>
  • [Xen-devel] [PATCH] acm_ops.c cleanup, Reiner Sailer <=