device_cgroup: fix unchecked cgroup parent usage

PATCH In review Aristeu Rozanski 4 participants 5 messages Oct 31, 2012 – Nov 1, 2012

Subsystems (2)

Security (LSM) cgroup

lore

Share & tools

lore

Aristeu Rozanski

author

2012-10-31 16:04 UTC #0 lore raw

PATCH 1/1 device_cgroup: fix unchecked cgroup parent usage

In 4cef7299b4786879a3e113e84084a72b24590c5b the cgroup parent usage is unchecked. root will not have a parent and trying to use device.{allow,deny} will cause problems. For some reason my stressing scripts didn't test the root directory so I didn't catch it on my regular tests.

Andrew, Tejun, this patch needs to make Linus tree ASAP or a revert for 4cef7299b4786879a3e113e84084a72b24590c5b.

Cc Andrew Morton <[email protected]>

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 +10 -2

@@ -352,6 +352,8 @@
  */
 static inline int may_allow_all(struct dev_cgroup *parent)
 {
+	if (!parent)
+		return 1;
 	return parent->behavior == DEVCG_DEFAULT_ALLOW;
 }

@@ -376,11 +378,14 @@
 	int count, rc;
 	struct dev_exception_item ex;
 	struct cgroup *p = devcgroup->css.cgroup;
-	struct dev_cgroup *parent = cgroup_to_devcgroup(p->parent);
+	struct dev_cgroup *parent = NULL;
 
 	if (!capable(CAP_SYS_ADMIN))
 		return -EPERM;
 
+	if (p->parent)
+		parent = cgroup_to_devcgroup(p->parent);
+
 	memset(&ex, 0, sizeof(ex));
 	b = buffer;

@@ -391,11 +396,14 @@
 			if (!may_allow_all(parent))
 				return -EPERM;
 			dev_exception_clean(devcgroup);
+			devcgroup->behavior = DEVCG_DEFAULT_ALLOW;
+			if (!parent)
+				break;
+
 			rc = dev_exceptions_copy(&devcgroup->exceptions,
 						 &parent->exceptions);
 			if (rc)
 				return rc;
-			devcgroup->behavior = DEVCG_DEFAULT_ALLOW;
 			break;
 		case DEVCG_DENY:
 			dev_exception_clean(devcgroup);

Tejun Heo

maintainer Acked-by on v1 1/1

2012-10-31 16:23 UTC #1 lore raw

Quoted 8 lines · Aristeu Rozanski

In 4cef7299b4786879a3e113e84084a72b24590c5b the cgroup parent usage is unchecked. root will not have a parent and trying to use device.{allow,deny} will cause problems. For some reason my stressing scripts didn't test the root directory so I didn't catch it on my regular tests.
Andrew, Tejun, this patch needs to make Linus tree ASAP or a revert for 4cef7299b4786879a3e113e84084a72b24590c5b.

Acked-by Tejun Heo <[email protected]>

Andrew, can you please pick this one up?

Thanks.

Signature · 1 line

tejun

Serge Hallyn

Acked-by Serge E. Hallyn on v1 1/1

2012-10-31 17:21 UTC #2 lore raw

In reply to Aristeu Rozanski

Quoted 9 lines · Aristeu Rozanski

In 4cef7299b4786879a3e113e84084a72b24590c5b the cgroup parent usage is unchecked. root will not have a parent and trying to use device.{allow,deny} will cause problems. For some reason my stressing scripts didn't test the root directory so I didn't catch it on my regular tests.
Andrew, Tejun, this patch needs to make Linus tree ASAP or a revert for 4cef7299b4786879a3e113e84084a72b24590c5b.

Quoted 6 lines

Cc Andrew Morton <akpm-de/[email protected]>
Cc Tejun Heo <[email protected]>
Cc Li Zefan <[email protected]>
Cc James Morris <jmorris-gx6/[email protected]>
Cc Pavel Emelyanov <[email protected]>
Cc Serge Hallyn <[email protected]>

Acked-by Serge E. Hallyn <serge.hallyn-GeWIH/[email protected]>

Quoted 46 lines

Cc Jiri Slaby <[email protected]>

Signed-off-by Aristeu Rozanski <[email protected]>

security/device_cgroup.c +10 -2

@@ -352,6 +352,8 @@
  */
 static inline int may_allow_all(struct dev_cgroup *parent)
 {
+	if (!parent)
+		return 1;
 	return parent->behavior == DEVCG_DEFAULT_ALLOW;
 }

@@ -376,11 +378,14 @@
 	int count, rc;
 	struct dev_exception_item ex;
 	struct cgroup *p = devcgroup->css.cgroup;
-	struct dev_cgroup *parent = cgroup_to_devcgroup(p->parent);
+	struct dev_cgroup *parent = NULL;
 
 	if (!capable(CAP_SYS_ADMIN))
 		return -EPERM;
 
+	if (p->parent)
+		parent = cgroup_to_devcgroup(p->parent);
+
 	memset(&ex, 0, sizeof(ex));
 	b = buffer;

@@ -391,11 +396,14 @@
 			if (!may_allow_all(parent))
 				return -EPERM;
 			dev_exception_clean(devcgroup);
+			devcgroup->behavior = DEVCG_DEFAULT_ALLOW;
+			if (!parent)
+				break;
+
 			rc = dev_exceptions_copy(&devcgroup->exceptions,
 						 &parent->exceptions);
 			if (rc)
 				return rc;
-			devcgroup->behavior = DEVCG_DEFAULT_ALLOW;
 			break;
 		case DEVCG_DENY:
 			dev_exception_clean(devcgroup);

Andrew Morton

2012-10-31 23:35 UTC #3 lore raw

In reply to Aristeu Rozanski

Quoted 3 lines · On Wed, 31 Oct 2012 12:04:30 -0400 Aristeu Rozanski…

In 4cef7299b4786879a3e113e84084a72b24590c5b the cgroup parent usage is unchecked. root will not have a parent and trying to use device.{allow,deny} will cause problems.

From my reading of the code "problems" means "kernel null pointer dereference".

security/device_cgroup.c line 353

Quoted 9 lines

For some reason my stressing
scripts didn't test the root directory so I didn't catch it on my regular tests.
security/device_cgroup.c +0 -0
@@ -352,6 +352,8 @@
  */
 static inline int may_allow_all(struct dev_cgroup *parent)

offtopic: this function could quite neatly have a bool return type.

security/device_cgroup.c line 356

Quoted 3 lines

security/device_cgroup.c +2 -0
 {
+	if (!parent)
+		return 1;

hm. Does it need a comment explaining what and why? I guess not... just.

Quoted 2 lines

security/device_cgroup.c +0 -0

 	return parent->behavior == DEVCG_DEFAULT_ALLOW;
 }

Aristeu Rozanski

author

2012-11-01 13:28 UTC #4 lore raw

Quoted 9 lines · Andrew Morton

Quoted 3 lines · On Wed, 31 Oct 2012 12:04:30 -0400 Aristeu Rozanski…
In 4cef7299b4786879a3e113e84084a72b24590c5b the cgroup parent usage is unchecked. root will not have a parent and trying to use device.{allow,deny} will cause problems.
From my reading of the code "problems" means "kernel null pointer dereference".

yes. you want me to resubmit it fixing it or you want to update it?

Quoted 3 lines

Quoted 1 line
static inline int may_allow_all(struct dev_cgroup *parent)
offtopic: this function could quite neatly have a bool return type.

ok, will do.

Quoted 5 lines

security/device_cgroup.c line 356
Quoted 3 lines
{
security/device_cgroup.c +2 -0
+	if (!parent)
+		return 1;
hm. Does it need a comment explaining what and why? I guess not... just.

it's because the root's parent has to always allows full access to all devices. I can add a comment when I turn into a bool if you want.

Signature · 1 line

Aristeu