Re: [PATCH] cgroup: fix device deny of DEV_ALL

[Serge Hallyn <serge.hallyn@canonical.com>]

Quoting Amos Kong (akong@redhat.com):
> On 22/05/12 09:54, Serge E. Hallyn wrote:
> >Quoting Li Zefan (lizefan@huawei.com):
> >>Serge Hallyn wrote:
> >>
> >>>Quoting Amos Kong (akong@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.
> 
> Hi serge,
> 
> My patch updated type,major,minor, it _equals to_ remove 'a *:* rwm'
> and add 'b *:* m'
> It's a clear logic, why need to manually remove 'a *:* rwm'?

Because until now, (apart from the special case 'a',) the devices.deny
rules have been very simple - you have to match an exact existing entry
as seen in devices.list.

I guess that was never explicitly written anywhere.  So the only reason
not to change it (apart from simplicity) is that, if I happen to have

	a *:* rwm

and accidentally give myself

	for seq in `1 254`; do
		echo "b *:$i rwm" > devices.allow
	done

and want to undo it, now i can't remove those without also removing
a *:* rwm.  (which I might not be able to get back)

> >>No, you'll see the entry has been changed to 'a *:* m', so I think we
> >>should at least fix this.
> >
> >Yikes.  Agreed.  That's a bug.
> 
> which bug? should not update walk->access if wh->access is not 'rwm'?

Well, in light of morning, I'm not so sure this is bad.  It doesn't fit
with what I am saying above that I wanted :)  But if I had 'a *:* rwm'
and I say I don't want to be able to rw to b 254:3, then leaving me
with only 'a *:* m' does achieve that.

Still I would prefer to have to match the entries in devices.list.

> diff --git a/security/device_cgroup.c b/security/device_cgroup.c
> index c43a332..e619a34 100644
> --- a/security/device_cgroup.c
> +++ b/security/device_cgroup.c
> @@ -145,7 +145,8 @@ static void dev_whitelist_rm(struct dev_cgroup
> *dev_cgroup,
>                         continue;
> 
>  remove:
> -               walk->access &= ~wh->access;
> +               if (walk->type != DEV_ALL || wh->access == ACC_MASK)
> +                       walk->access &= ~wh->access;

I'm not following what this is actually meant to do.  It'll do the
same thing as the original code, unless walk->type == DEV_ALL and
wh->access != ACC_MASK, but that is never the case per
devcgroup_update_access().

>                 if (!walk->access) {
>                         list_del_rcu(&walk->list);
>                         kfree_rcu(walk, rcu);
> 
> 
> -- 
> 			Amos.

Lastly, perhaps what we actually want to do is implement blacklists
instead of a pure whitelist?  So we could then really say "allow
everything except b 254:3 rw" with two entries.

But, while it may be nice to talk about that, I have not seen any
cases where someone actually wanted that.  For containers, at least,
a pure whitelist has always been right.

thanks,
-serge