[PATCH] cgroup: fix device deny of DEV_ALL

[PATCH] cgroup: fix device deny of DEV_ALL

[Amos Kong]

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

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

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

[Serge Hallyn]

Quoting Amos Kong (akong-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org):
> @ 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-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> ---
>  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-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA@public.gmane.org
> https://lists.linuxfoundation.org/mailman/listinfo/containers

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

[Li Zefan]

Serge Hallyn wrote:

> Quoting Amos Kong (akong-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org):
>> @ 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.


No, you'll see the entry has been changed to 'a *:* m', so I think we
should at least fix this.

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

[Serge E. Hallyn]

Quoting Li Zefan (lizefan-hv44wF8Li93QT0dZR+AlfA@public.gmane.org):
> Serge Hallyn wrote:
> 
> > Quoting Amos Kong (akong-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org):
> >> @ 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.
> 
> 
> 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.

thanks,
-serge

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

[Serge E. Hallyn]

At line 135, there is

if (walk->type == DEV_ALL)
	goto remove;

I wonder if that was meant to be 'if (wh->type == DEV_ALL)'.  That
seems to fit better with what I would have meant to have happen.
But it's already handled by line 342.  So I think deleting lines
135-136 might be best.  What do you think?

Thanks again, Amos and Li.

-serge

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

[Amos Kong]

On 22/05/12 09:54, Serge E. Hallyn wrote:
> Quoting Li Zefan (lizefan-hv44wF8Li93QT0dZR+AlfA@public.gmane.org):
>> Serge Hallyn wrote:
>>
>>> Quoting Amos Kong (akong-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org):
>>>> @ 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'?


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

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;
                 if (!walk->access) {
                         list_del_rcu(&walk->list);
                         kfree_rcu(walk, rcu);


-- 
			Amos.

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

[Amos Kong]

On 22/05/12 10:08, Serge E. Hallyn wrote:
> At line 135, there is
>
> if (walk->type == DEV_ALL)
> 	goto remove;
>
> I wonder if that was meant to be 'if (wh->type == DEV_ALL)'.  That
> seems to fit better with what I would have meant to have happen.
> But it's already handled by line 342.  So I think deleting lines
> 135-136 might be best.  What do you think?

Hi Serge,

If we expect nothing changed ('a *:* rwm' doesn't change),
delete 135-136 will be ok.

But I have explained my patch in another email, I also think
it's unnecessary to remove 'a *:* rwm' before executing:
  @ echo 'b 253:3 rw'>  devices.deny

-- 
			Amos.

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

[Serge Hallyn]

Quoting Amos Kong (akong-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org):
> On 22/05/12 09:54, Serge E. Hallyn wrote:
> >Quoting Li Zefan (lizefan-hv44wF8Li93QT0dZR+AlfA@public.gmane.org):
> >>Serge Hallyn wrote:
> >>
> >>>Quoting Amos Kong (akong-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org):
> >>>>@ 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