2012-10-22 13:45:57

by Aristeu Rozanski

Subject: [PATCH 4/4] device_cgroup: add proper checking when changing default behavior

Before changing a group's default behavior to ALLOW, we must check if its
parent's behavior is also ALLOW.

Cc: Tejun Heo <[email protected]>
Cc: Li Zefan <[email protected]>
Cc: James Morris <[email protected]>
Cc: Pavel Emelyanov <[email protected]>
Cc: Serge Hallyn <[email protected]>
Cc: Jiri Slaby <[email protected]>
Signed-off-by: Aristeu Rozanski <[email protected]>

---
security/device_cgroup.c | 19 ++++++++++++++++++-
1 file changed, 18 insertions(+), 1 deletion(-)

--- github.orig/security/device_cgroup.c 2012-10-19 16:36:50.135790476 -0400
+++ github/security/device_cgroup.c 2012-10-19 16:48:50.710978125 -0400
@@ -344,6 +344,17 @@ static int parent_has_perm(struct dev_cg
return may_access(parent, ex);
}

+/**
+ * may_allow_all - checks if it's possible to change the behavior to
+ * allow based on parent's rules.
+ * @parent: device cgroup's parent
+ * returns: != 0 in case it's allowed, 0 otherwise
+ */
+static inline int may_allow_all(struct dev_cgroup *parent)
+{
+ return parent->behavior == DEVCG_DEFAULT_ALLOW;
+}
+
/*
* Modify the exception list using allow/deny rules.
* CAP_SYS_ADMIN is needed for this. It's at least separate from CAP_MKNOD
@@ -364,6 +375,8 @@ static int devcgroup_update_access(struc
char temp[12]; /* 11 + 1 characters needed for a u32 */
int count, rc;
struct dev_exception_item ex;
+ struct cgroup *p = devcgroup->css.cgroup;
+ struct dev_cgroup *parent = cgroup_to_devcgroup(p->parent);

if (!capable(CAP_SYS_ADMIN))
return -EPERM;
@@ -375,9 +388,13 @@ memset(&ex, 0, sizeof(ex));
case 'a':
switch (filetype) {
case DEVCG_ALLOW:
- if (!parent_has_perm(devcgroup, &ex))
+ if (!may_allow_all(parent))
return -EPERM;
dev_exception_clean(devcgroup);
+ rc = dev_exceptions_copy(&devcgroup->exceptions,
+ &parent->exceptions);
+ if (rc)
+ return rc;
devcgroup->behavior = DEVCG_DEFAULT_ALLOW;
break;
case DEVCG_DENY:

2012-10-22 16:16:18

by Serge Hallyn

Subject: Re: [PATCH 4/4] device_cgroup: add proper checking when changing default behavior

Quoting Aristeu Rozanski ([email protected]):
> Before changing a group's default behavior to ALLOW, we must check if its
> parent's behavior is also ALLOW.
>
> Cc: Tejun Heo <[email protected]>
> Cc: Li Zefan <[email protected]>
> Cc: James Morris <[email protected]>
> Cc: Pavel Emelyanov <[email protected]>
> Cc: Serge Hallyn <[email protected]>

Acked-by: Serge E. Hallyn <[email protected]>

Thanks, Aristeu.

> Cc: Jiri Slaby <[email protected]>
> Signed-off-by: Aristeu Rozanski <[email protected]>
>
> ---
> security/device_cgroup.c | 19 ++++++++++++++++++-
> 1 file changed, 18 insertions(+), 1 deletion(-)
>
> --- github.orig/security/device_cgroup.c 2012-10-19 16:36:50.135790476 -0400
> +++ github/security/device_cgroup.c 2012-10-19 16:48:50.710978125 -0400
> @@ -344,6 +344,17 @@ static int parent_has_perm(struct dev_cg
> return may_access(parent, ex);
> }
>
> +/**
> + * may_allow_all - checks if it's possible to change the behavior to
> + * allow based on parent's rules.
> + * @parent: device cgroup's parent
> + * returns: != 0 in case it's allowed, 0 otherwise
> + */
> +static inline int may_allow_all(struct dev_cgroup *parent)
> +{
> + return parent->behavior == DEVCG_DEFAULT_ALLOW;
> +}
> +
> /*
> * Modify the exception list using allow/deny rules.
> * CAP_SYS_ADMIN is needed for this. It's at least separate from CAP_MKNOD
> @@ -364,6 +375,8 @@ static int devcgroup_update_access(struc
> char temp[12]; /* 11 + 1 characters needed for a u32 */
> int count, rc;
> struct dev_exception_item ex;
> + struct cgroup *p = devcgroup->css.cgroup;
> + struct dev_cgroup *parent = cgroup_to_devcgroup(p->parent);
>
> if (!capable(CAP_SYS_ADMIN))
> return -EPERM;
> @@ -375,9 +388,13 @@ memset(&ex, 0, sizeof(ex));
> case 'a':
> switch (filetype) {
> case DEVCG_ALLOW:
> - if (!parent_has_perm(devcgroup, &ex))
> + if (!may_allow_all(parent))
> return -EPERM;
> dev_exception_clean(devcgroup);
> + rc = dev_exceptions_copy(&devcgroup->exceptions,
> + &parent->exceptions);
> + if (rc)
> + return rc;
> devcgroup->behavior = DEVCG_DEFAULT_ALLOW;
> break;
> case DEVCG_DENY:
>