[PATCH] device_cgroup: do not use rule acceptance function to validate access

[PATCH] device_cgroup: do not use rule acceptance function to validate access

[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:

	# 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 <aris-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>

diff --git a/security/device_cgroup.c b/security/device_cgroup.c
index 8365909..d390d21 100644
--- a/security/device_cgroup.c
+++ b/security/device_cgroup.c
@@ -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;

Re: [PATCH] device_cgroup: do not use rule acceptance function to validate access

[Tejun Heo]

Hello,

On Mon, Apr 14, 2014 at 10:47:54AM -0400, Aristeu Rozanski wrote:
> 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

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

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

> After review, this should be considered for stable series.
> 
> Signed-off-by: Aristeu Rozanski <aris-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>

Can you please cc stable on the next posting?

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

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.

Thanks.

-- 
tejun

Re: [PATCH] device_cgroup: do not use rule acceptance function to validate access

[Aristeu Rozanski]

Hi Tejun,

On Mon, Apr 14, 2014 at 02:03:23PM -0400, Tejun Heo wrote:
> On Mon, Apr 14, 2014 at 10:47:54AM -0400, Aristeu Rozanski wrote:
> > 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
> 
> It'd be nice to explain how this is broken and why the code paths
> can't be shared.

ok

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

ok

> > After review, this should be considered for stable series.
> > 
> > Signed-off-by: Aristeu Rozanski <aris-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> 
> Can you please cc stable on the next posting?

ok

> > -	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;
> 
> Can't we at least factor out the above part and share it between the
> two functions?

ok

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

-- 
Aristeu

Re: [PATCH] device_cgroup: do not use rule acceptance function to validate access

[Tejun Heo]

Hey,

On Mon, Apr 14, 2014 at 02:16:12PM -0400, Aristeu Rozanski wrote:
> > 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?

In this patch but if you can reduce duplications in reasonable way in
other areas, that'd also be great. :)

Thanks!

-- 
tejun

Re: [PATCH] device_cgroup: do not use rule acceptance function to validate access

[Serge Hallyn]

Quoting Aristeu Rozanski (aris-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org):
> 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 ... :)

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

Definately.

> Signed-off-by: Aristeu Rozanski <aris-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>

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.

> diff --git a/security/device_cgroup.c b/security/device_cgroup.c
> index 8365909..d390d21 100644
> --- a/security/device_cgroup.c
> +++ b/security/device_cgroup.c
> @@ -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;

Re: [PATCH] device_cgroup: do not use rule acceptance function to validate access

[Aristeu Rozanski]

Hi Serge,

On Tue, Apr 15, 2014 at 10:57:52AM -0500, Serge Hallyn wrote:
> Quoting Aristeu Rozanski (aris-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org):
> > 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 ... :)

Yes, trying to get this right this time. The problem itself isn't
simple, comparing ranges when adding new rules on the child and the
possible difference between the meaning of the exception (is it to
disallow access in that range or to allow?) makes things complicated.

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

Yes, don't worry about it, I'm working on a v2 that will makes things
easier to read. If not, I'll keep working on it until it looks simple
enough.

-- 
Aristeu