cgroup: denying device doesn't work with 'rw' mode string

PATCH In review Amos Kong 8 participants 17 messages Oct 15, 2011 – May 22, 2012 2 messages not in the archive

Subsystems (2)

Security (LSM) cgroup

lore

Share & tools

lore

Amos Kong

author

2011-10-15 00:39 UTC #0 lore raw

# mount -t cgroup -o devices none /cgroup
# mkdir /cgroups/devices
# ls -l /dev/vg/lv
lrwxrwxrwx. 1 root root 7 Oct 14 19:03 /dev/vg/lv -> ../dm-3
# ls -l /dev/dm-3
brw-rw----. 1 root disk 253, 3 Oct 14 19:03 /dev/dm-3

------- test1

# echo a > devices/devices.allow
# echo 'b 253:3 rwm' > devices/devices.deny
                ^^^
# echo $$ > task
# dd if=/dev/zero of=/dev/dm-3 bs=1M count=1
couldn't write to /dev/dm-3 successfully

------- test2
deny read-write permission of dm-3, but it doesn't effect.

# echo a > devices/devices.allow
# echo 'b 253:2 rw' > devices.deny
                ^^
# echo $$ > task
# dd if=/dev/zero of=/dev/dm-3 bs=1M count=1
can write to /dev/dm-3 successfully ???

-----------

related upstream commit:
commit 08ce5f16ee466ffc5bf243800deeecd77d9eaf50

Author: Serge E. Hallyn <[email protected]>
Date:   Tue Apr 29 01:00:10 2008 -0700

cgroups: implement device whitelist

cgroup tracks and enforces open and mknod restrictions on device files, so 'm' are always needed in the mode string? 'rw' is uneffective?

Amos Kong

author

2012-05-18 03:37 UTC #1 lore raw

CC Li Zefan <[email protected]>, Tejun Heo <[email protected]>, [email protected]

Quoted 38 lines · Amos Kong

# mount -t cgroup -o devices none /cgroup
# mkdir /cgroups/devices
# ls -l /dev/vg/lv
lrwxrwxrwx. 1 root root 7 Oct 14 19:03 /dev/vg/lv -> ../dm-3
# ls -l /dev/dm-3
brw-rw----. 1 root disk 253, 3 Oct 14 19:03 /dev/dm-3
------- test1
deny read-write permission of dm-3, but it doesn't effect.
# echo a > devices/devices.allow
# echo 'b 253:2 rw' > devices.deny
               ^^
# echo $$ > task
# dd if=/dev/zero of=/dev/dm-3 bs=1M count=1
can write to /dev/dm-3 successfully  (problem exists)
------- test2
# echo a > devices/devices.allow
# echo 'b 253:3 rwm' > devices/devices.deny
               ^^^
# echo $$ > task
# dd if=/dev/zero of=/dev/dm-3 bs=1M count=1
couldn't write to /dev/dm-3 successfully
-----------
related upstream commit:
commit 08ce5f16ee466ffc5bf243800deeecd77d9eaf50
Author: Serge E. Hallyn <[email protected]>
Date: Tue Apr 29 01:00:10 2008 -0700
cgroups: implement device whitelist
cgroup tracks and enforces open and mknod restrictions on device files, so 'm' are always needed in the mode string? 'rw' is ineffective?

Li Zefan

2012-05-18 03:56 UTC #2 lore raw

Quoted 16 lines · Amos Kong

CC Li Zefan <[email protected]>, Tejun Heo <[email protected]>, [email protected]
Quoted 13 lines · Amos Kong
# mount -t cgroup -o devices none /cgroup
# mkdir /cgroups/devices
# ls -l /dev/vg/lv
lrwxrwxrwx. 1 root root 7 Oct 14 19:03 /dev/vg/lv -> ../dm-3
# ls -l /dev/dm-3
brw-rw----. 1 root disk 253, 3 Oct 14 19:03 /dev/dm-3
------- test1
deny read-write permission of dm-3, but it doesn't effect.
# echo a > devices/devices.allow
# echo 'b 253:2 rw' > devices.deny

253:2 ??

Quoted 8 lines

               ^^
# echo $$ > task
# dd if=/dev/zero of=/dev/dm-3 bs=1M count=1
can write to /dev/dm-3 successfully  (problem exists)

------- test2

# echo a > devices/devices.allow
# echo 'b 253:3 rwm' > devices/devices.deny

253:3 !!

Quoted 4 lines

               ^^^
# echo $$ > task
# dd if=/dev/zero of=/dev/dm-3 bs=1M count=1
couldn't write to /dev/dm-3 successfully

Amos Kong

author

2012-05-18 04:31 UTC #3 lore raw

Quoted 13 lines · Li Zefan

Quoted 9 lines · Amos Kong
CC Li Zefan <[email protected]>, Tejun Heo <[email protected]>, [email protected]
Quoted 6 lines · Amos Kong
# mount -t cgroup -o devices none /cgroup
# mkdir /cgroups/devices
# ls -l /dev/vg/lv
lrwxrwxrwx. 1 root root 7 Oct 14 19:03 /dev/vg/lv -> ../dm-3
# ls -l /dev/dm-3
brw-rw----. 1 root disk 253, 3 Oct 14 19:03 /dev/dm-3

Quoted 8 lines

Quoted 5 lines
Quoted 5 lines
------- test1
deny read-write permission of dm-3, but it doesn't effect.
# echo a > devices/devices.allow
# echo 'b 253:2 rw' > devices.deny
253:2 ??

sorry, typo

# echo 'b 253:3 rw' > devices.deny      # But read-write permission is
not denied

Quoted 16 lines

Quoted 8 lines

# echo $$ > task
# dd if=/dev/zero of=/dev/dm-3 bs=1M count=1
can write to /dev/dm-3 successfully  (problem exists)

------- test2

# echo a > devices/devices.allow
# echo 'b 253:3 rwm' > devices/devices.deny

253:3 !!

Quoted 4 lines

^^^

# echo $$ > task
# dd if=/dev/zero of=/dev/dm-3 bs=1M count=1
couldn't write to /dev/dm-3 successfully

In devcgroup_create(), we create a new whitelist, and add first entry which type is 'DEV_ALL'.
Execute "# echo 'b 253:3 rw' > devices/devices.deny", dev_whitelist_rm() will update access
of first entry to 3, but type of first entry is also 'DEV_ALL'

.. static void dev_whitelist_rm(struct dev_cgroup *dev_cgroup, ...) {
.. list_for_each_entry_safe(walk, tmp, &dev_cgroup->whitelist, list) {
.. if (walk->type == DEV_ALL)
.. goto remove;

If the type is 'DEV_ALL', will try to remove it without checking major/minor/..

.. remove:
.. walk->access &= ~wh->access;

access of first entry will be updated to 7(mrw) & ~4(w) = 3

.. if (!walk->access) {

                first entry will not be deleted, because walk->access is not 0

.. list_del_rcu(&walk->list);
.. kfree_rcu(walk, rcu);

Execute dd cmd to write device, __devcgroup_inode_permission() will be called. The type of first list entry is 'DEV_ALL', just pass this permission checking. (write operation will not be denied)

.. int __devcgroup_inode_permission(struct inode *inode, int mask) {
.. ....
.. dev_cgroup = task_devcgroup(current);
.. list_for_each_entry_rcu(wh, &dev_cgroup->whitelist, list) {
.. if (wh->type & DEV_ALL)
.. goto found;

// If type is 'DEV_ALL', pass permission check

.. ....
.. if ((mask & MAY_WRITE) && !(wh->access & ACC_WRITE))
.. continue;
.. found:
.. rcu_read_unlock();
.. return 0;
..

Amos Kong

2012-05-18 08:19 UTC #5 lore raw

PATCH 1/1 cgroup: fix device deny of DEV_ALL

@ 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 <[email protected]>

---
 security/device_cgroup.c |    5 +++++
 1 files changed, 5 insertions(+), 0 deletions(-)

security/device_cgroup.c +5 -0

index c43a332..d16b4bc 100644
@@ -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);

Serge Hallyn

2012-05-21 14:03 UTC #6 lore raw

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

Quoted 11 lines · 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'.

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

Quoted 34 lines

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 <[email protected]>
---
 security/device_cgroup.c |    5 +++++
 1 files changed, 5 insertions(+), 0 deletions(-)
security/device_cgroup.c +5 -0
index c43a332..d16b4bc 100644
@@ -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

Li Zefan

2012-05-22 00:34 UTC #7 lore raw

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

Quoted 15 lines · Serge Hallyn

Quoted 11 lines · 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'.
Hi,

Quoted 7 lines

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.

Serge E. Hallyn

maintainer

2012-05-22 01:52 UTC #8 lore raw

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

Quoted 26 lines · Li Zefan

Quoted 15 lines · Serge Hallyn
Quoted 11 lines · 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'.
Hi,
Quoted 7 lines
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.

Quoted 2 lines

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

Serge E. Hallyn

maintainer

2012-05-22 02:06 UTC #9 lore raw

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

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

Amos Kong

2012-05-22 02:23 UTC #10 lore raw

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

Quoted 9 lines · 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?

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

Signature · 1 line

			Amos.

A message that is not in the archive (sent off-list or not yet received): [email protected]

Amos Kong

2012-05-22 02:23 UTC #12 lore raw

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

Quoted 9 lines · 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?

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

Signature · 1 line

			Amos.

A message that is not in the archive (sent off-list or not yet received): [email protected]

Serge E. Hallyn

2012-05-22 02:08 UTC #14 lore raw

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

At line 135, there is

if (walk->type == DEV_ALL)

	goto remove;

Thanks again, Amos and Li.

-serge

Amos Kong

2012-05-22 02:14 UTC #15 lore raw

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

In reply to a message not in the archive

Quoted 20 lines · Serge E. Hallyn

Quoted 19 lines · Li Zefan
Quoted 17 lines · Serge Hallyn
Quoted 11 lines · 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'.
Hi,
thanks. You raise a good point, but I think it needs some discussion.

Quoted 7 lines

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

Quoted 4 lines

Quoted 2 lines
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'?

security/device_cgroup.c +2 -1

index c43a332..e619a34 100644
@@ -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);

Signature · 1 line

			Amos.

Amos Kong

2012-05-22 02:14 UTC #16 lore raw

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

In reply to Serge E. Hallyn

Quoted 20 lines · Serge E. Hallyn

Quoted 19 lines · Li Zefan
Quoted 17 lines · Serge Hallyn
Quoted 11 lines · 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'.
Hi,
thanks. You raise a good point, but I think it needs some discussion.

Quoted 7 lines

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

Quoted 4 lines

Quoted 2 lines
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'?

security/device_cgroup.c +2 -1

index c43a332..e619a34 100644
@@ -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);

Signature · 1 line

			Amos.

Serge Hallyn

2012-05-22 12:48 UTC #17 lore raw

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

Quoted 26 lines · Amos Kong

Quoted 24 lines · Serge E. Hallyn
Quoted 23 lines · Li Zefan
Quoted 21 lines · Serge Hallyn
Quoted 11 lines · 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'.
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.

Quoted 8 lines

Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
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)

Quoted 6 lines

Quoted 4 lines
Quoted 2 lines
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.

security/device_cgroup.c static void dev_whitelist_rm(struct dev_cgroup · line 150

Quoted 12 lines

security/device_cgroup.c +2 -1

index c43a332..e619a34 100644
@@ -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().

Quoted 7 lines

security/device_cgroup.c +0 -0

                if (!walk->access) {
                        list_del_rcu(&walk->list);

                        kfree_rcu(walk, rcu);

Signature · 1 line

			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

Serge Hallyn

2012-05-21 14:03 UTC #18 lore raw

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

In reply to Amos Kong

Quoted 11 lines · 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'.

Hi,

thanks. You raise a good point, but I think it needs some discussion.

So for this particular patch, NACK. But thanks for bringing it up.

thanks,
-serge

Quoted 34 lines

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 <[email protected]>
---
 security/device_cgroup.c |    5 +++++
 1 files changed, 5 insertions(+), 0 deletions(-)
security/device_cgroup.c +5 -0
index c43a332..d16b4bc 100644
@@ -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