[PATCH] cgroup: fix device deny of DEV_ALL
Serge Hallyn
serge.hallyn at canonical.com
Mon May 21 14:03:24 UTC 2012
Quoting Amos Kong (akong at redhat.com):
> @ mount -t cgroup -o devices none /cgroup
> @ mkdir /cgroups/devices
> @ ls -l /dev/dm-3
> brw-rw----. 1 root disk 253, 3 Oct 14 19:03 /dev/dm-3
> @ echo 'b 253:3 rw' > devices.deny
> but I can still write it by 'dd if=/dev/zero of=/dev/dm-3'
>
> In devcgroup_create(), we create a new whitelist, and add first
> entry which type is 'DEV_ALL'. Execute "# echo 'b 253:3 rw' >
> devices.deny", dev_whitelist_rm() will update access of first
> entry to 1(m), but type of first entry is still 'DEV_ALL'.
Hi,
thanks. You raise a good point, but I think it needs some discussion.
What happens right now is that if you have the 'a *:* rwm' entry and do
echo 'b 253:3 rw' > devices.deny, then when you next cat devices.list you
will still see the 'a *:* rwm' entry. So there should be no confusion
over why the dd succeeds. You didn't remove the entry, because there
was no match echoed into devices.deny.
You have to remove the existing whitelist entries, then add the entries
you want. In particular, catting into devices.deny will not try to be
smart by slicing an existing whitelist entry into (matching, nonmatching)
parts so as to remove the matching and keep nonmatching.
If you'd like to submit a patch to change that, I'm quite sure I would
ack it. The problem is that your patch doesn't do that (unless I'm grossly
misunderstanding). Rather, it will remove both (matching, nonmatching).
Of course, in your example above, (nonmatching) would amount to a huge
set of rules, so in the end I'm not sure it is worth it.
Note that the devices cgroup was meant to be a simple, useful stop-gap
until the user and devices namespaces are ready. The user namespace is
getting close, but devices ns still needs to be designed (hopefully at
plumber's). So I don't mind improving on the devices cgroup. It's
turned out to be quite useful. But I don't want to replace one (simple,
easy to verify, but) incomplete user interface with a different one.
There are sure to be existing users who would be broken. In fact, it's
possbile that "fixing" the incomplete behavior would bother some users,
though I suspect the improvement would be worth it to them.
So for this particular patch, NACK. But thanks for bringing it up.
thanks,
-serge
> Execute dd cmd to write device, __devcgroup_inode_permission()
> will be called, permission checking will pass if entry type
> is 'DEV_ALL'. So write operation of 'dd' is not denied.
>
> Currently 'access' is updated by not be used, this patch updated
> the type,major,minor of first entry, then permission checking
> would work.
>
> Signed-off-by: Amos Kong <akong at redhat.com>
> ---
> security/device_cgroup.c | 5 +++++
> 1 files changed, 5 insertions(+), 0 deletions(-)
>
> diff --git a/security/device_cgroup.c b/security/device_cgroup.c
> index c43a332..d16b4bc 100644
> --- a/security/device_cgroup.c
> +++ b/security/device_cgroup.c
> @@ -146,6 +146,11 @@ static void dev_whitelist_rm(struct dev_cgroup *dev_cgroup,
>
> remove:
> walk->access &= ~wh->access;
> + if (walk->type == DEV_ALL) {
> + walk->type = wh->type;
> + walk->major = wh->major;
> + walk->minor = wh->minor;
> + }
> if (!walk->access) {
> list_del_rcu(&walk->list);
> kfree_rcu(walk, rcu);
>
> _______________________________________________
> Containers mailing list
> Containers at lists.linux-foundation.org
> https://lists.linuxfoundation.org/mailman/listinfo/containers
More information about the Containers
mailing list