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