Quoting Aristeu Rozanski (aris@xxxxxxxxxx):
> 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@xxxxxxxxxx>
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;
--
To unsubscribe from this list: send the line "unsubscribe cgroups" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
