device_cgroup: do not use rule acceptance function to validate access

may_access() is currently used to validate both new exceptions from children groups and to check if an access is allowed. This not only makes it hard to understand and maintain, but it's also incorrect.

It currently allows one to:

	# mkdir new_group
	# cd new_group
	# echo $$ >tasks
	# echo "c 1:3 w" >devices.deny
	# echo >/dev/null
	# echo $?
	0

This patch implements the device file access check separately and fixes the issue.

This is broken since c39a2a3018f8065cb5ea38b0314c1bbedb2cfa0d

After review, this should be considered for stable series.

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

security/device_cgroup.c +4 -1

-	rc = may_access(dev_cgroup, &ex, dev_cgroup->behavior);
+	behavior = dev_cgroup->behavior;
+	list_for_each_entry_rcu(ex, &dev_cgroup->exceptions, list) {
+		if (type == DEV_BLOCK && !(ex->type & DEV_BLOCK))
+			continue;

Quoted 6 lines · Aristeu Rozanski

may_access() is currently used to validate both new exceptions from children groups and to check if an access is allowed. This not only makes it hard to understand and maintain, but it's also incorrect.

It currently allows one to:

Quoted 7 lines

	# mkdir new_group
	# cd new_group
	# echo $$ >tasks
	# echo "c 1:3 w" >devices.deny
	# echo >/dev/null
	# echo $?
	0

It'd be nice to explain how this is broken and why the code paths can't be shared.

Quoted 4 lines

This patch implements the device file access check separately and fixes the issue.

This is broken since c39a2a3018f8065cb5ea38b0314c1bbedb2cfa0d

Please use 12-digits-of-SHA1 ("Subject of the patch") format.

Quoted 3 lines

After review, this should be considered for stable series.

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

Can you please cc stable on the next posting?

Quoted 10 lines

security/device_cgroup.c +9 -1

-	rc = may_access(dev_cgroup, &ex, dev_cgroup->behavior);
+	behavior = dev_cgroup->behavior;
+	list_for_each_entry_rcu(ex, &dev_cgroup->exceptions, list) {
+		if (type == DEV_BLOCK && !(ex->type & DEV_BLOCK))
+			continue;
+		if (type == DEV_CHAR && !(ex->type & DEV_CHAR))
+			continue;
+		if (ex->major != ~0 && major != ex->major)
+			continue;
+		if (ex->minor != ~0 && minor != ex->minor)

security/device_cgroup.c static int __devcgroup_check_permission(short type, u32 major, u32 minor, · line 726

Quoted 5 lines

security/device_cgroup.c +5 -0

+			continue;
+		if (access & (~ex->access))
+			continue;
+		match = true;
+		break;

Can't we at least factor out the above part and share it between the two functions?

Currently, there's a lot of duplication in rather delicate code and it's not clear where they differ and why.

Quoted 2 lines

Currently, there's a lot of duplication in rather delicate code and it's not clear where they differ and why.

Are you referring to the duplication in this patch or other areas too?

may_access() is currently used to validate both new exceptions from children groups and to check if an access is allowed. This not only makes it hard to understand and maintain, but it's also incorrect.

It currently allows one to:

	# mkdir new_group
	# cd new_group
	# echo $$ >tasks
	# echo "c 1:3 w" >devices.deny
	# echo >/dev/null
	# echo $?
	0

This patch implements the device file access check separately and fixes the issue.

This is broken since c39a2a3018f8065cb5ea38b0314c1bbedb2cfa0d

After review, this should be considered for stable series.

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

security/device_cgroup.c +24 -10

index 8365909..d390d21 100644
@@ -704,21 +704,35 @@ static int __devcgroup_check_permission(short type, u32 major, u32 minor,
 				        short access)
 {
 	struct dev_cgroup *dev_cgroup;
-	struct dev_exception_item ex;
-	int rc;
-
-	memset(&ex, 0, sizeof(ex));
-	ex.type = type;
-	ex.major = major;
-	ex.minor = minor;
-	ex.access = access;
+	struct dev_exception_item *ex;
+	enum devcg_behavior behavior;
+	bool match = false;
 
 	rcu_read_lock();
 	dev_cgroup = task_devcgroup(current);
-	rc = may_access(dev_cgroup, &ex, dev_cgroup->behavior);
+	behavior = dev_cgroup->behavior;
+	list_for_each_entry_rcu(ex, &dev_cgroup->exceptions, list) {
+		if (type == DEV_BLOCK && !(ex->type & DEV_BLOCK))
+			continue;
+		if (type == DEV_CHAR && !(ex->type & DEV_CHAR))
+			continue;
+		if (ex->major != ~0 && major != ex->major)
+			continue;
+		if (ex->minor != ~0 && minor != ex->minor)
+			continue;
+		if (access & (~ex->access))
+			continue;
+		match = true;
+		break;
+	}
 	rcu_read_unlock();
 
-	if (!rc)
+	if (behavior == DEVCG_DEFAULT_ALLOW && match)
+		/* access matches a rule to disallow access */
+		return -EPERM;
+
+	if (behavior == DEVCG_DEFAULT_DENY && !match)
+		/* no exceptions found, default action is to deny access */
 		return -EPERM;
 
 	return 0;

Quoted 3 lines · Aristeu Rozanski

may_access() is currently used to validate both new exceptions from children groups and to check if an access is allowed. This not only makes it hard to understand and maintain, but it's also incorrect.

Heh, well one might argue that may_access() was more approriate at __devcgroup_check_permission(), should remain simpler, and parent_has_perm() should be the wrapper doing the extra bits. That would be easier to read imo.

In what you have here, you end up duplicating the list_for_each_entry_rcu. I think you should at least have a common fn for that.

I don't want to sound pedantic, but since the point of this patch is that simplicity/ease-of-reading would have prevented the bug ... :)

I *think* it's ok, and I appreciate you finding and fixing the bug, but I don't think the result is as easy to read as it could be, so if you don't mind I'd like to wait for a new version to ack. If you disagree with me, let me know and I'll re-read and (presumably) ack.