Quoting Amos Kong (akong@xxxxxxxxxx):
> On 22/05/12 09:54, Serge E. Hallyn wrote:
> >Quoting Li Zefan (lizefan@xxxxxxxxxx):
> >>Serge Hallyn wrote:
> >>
> >>>Quoting Amos Kong (akong@xxxxxxxxxx):
> >>>>@ 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
_______________________________________________
Containers mailing list
Containers@xxxxxxxxxxxxxxxxxxxxxxxxxx
https://lists.linuxfoundation.org/mailman/listinfo/containers
