[PATCH 0/4] Rebase device_cgroup v2 patchset

[PATCH 0/4] Rebase device_cgroup v2 patchset

[Aristeu Rozanski]

This patchset rebases the v2 of the patchset since the v1 was pushed into -rc1
instead. The last patch, not present on previous patchset, fixes the
permission check when allowing everything in a cgroup.

 device_cgroup.c |   87 +++++++++++++++++++++++++++++++++++++++-----------------
 1 file changed, 61 insertions(+), 26 deletions(-)

Cc: Dave Jones <davej@redhat.com>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Tejun Heo <tj@kernel.org>
Cc: Li Zefan <lizefan@huawei.com>
Cc: James Morris <jmorris@namei.org>
Cc: Pavel Emelyanov <xemul@openvz.org>
Cc: Serge Hallyn <serge.hallyn@canonical.com>
Cc: Jiri Slaby <jslaby@suse.cz>
Signed-off-by: Aristeu Rozanski <aris@redhat.com>

-- 
Aristeu

[PATCH 1/4] cgroup: fix invalid rcu dereference

[Aristeu Rozanski]

[-- Attachment #1: rcu.patch --]
[-- Type: text/plain, Size: 3762 bytes --]

From: Jiri Slaby <jslaby@suse.cz>

Commit "device_cgroup: convert device_cgroup internally to policy +
exceptions" removed rcu locks which are needed in task_devcgroup
called in this chain: devcgroup_inode_mknod OR
__devcgroup_inode_permission -> __devcgroup_inode_permission ->
task_devcgroup -> task_subsys_state -> task_subsys_state_check.

Change the code so that task_devcgroup is safely called with rcu read
lock held.

===============================
[ INFO: suspicious RCU usage. ]
3.6.0-rc5-next-20120913+ #42 Not tainted
-------------------------------
include/linux/cgroup.h:553 suspicious rcu_dereference_check() usage!

other info that might help us debug this:

rcu_scheduler_active = 1, debug_locks = 0
2 locks held by kdevtmpfs/23:
 #0:  (sb_writers){.+.+.+}, at: [<ffffffff8116873f>]
mnt_want_write+0x1f/0x50
 #1:  (&sb->s_type->i_mutex_key#3/1){+.+.+.}, at: [<ffffffff811558af>]
kern_path_create+0x7f/0x170

stack backtrace:
Pid: 23, comm: kdevtmpfs Not tainted 3.6.0-rc5-next-20120913+ #42
Call Trace:
 [<ffffffff810c638d>] lockdep_rcu_suspicious+0xfd/0x130
 [<ffffffff8121541d>] devcgroup_inode_mknod+0x19d/0x240
 [<ffffffff8107bf54>] ? ns_capable+0x44/0x80
 [<ffffffff81156b21>] vfs_mknod+0x71/0xf0
 [<ffffffff813a8332>] handle_create.isra.2+0x72/0x200
 [<ffffffff813a85d4>] devtmpfsd+0x114/0x140
 [<ffffffff813a84c0>] ? handle_create.isra.2+0x200/0x200
 [<ffffffff81093ad6>] kthread+0xd6/0xe0
 [<ffffffff81654f24>] kernel_thread_helper+0x4/0x10
 [<ffffffff8165369d>] ? retint_restore_args+0xe/0xe
 [<ffffffff81093a00>] ? kthread_create_on_node+0x140/0x140
 [<ffffffff81654f20>] ? gs_change+0xb/0xb

Cc: Dave Jones <davej@redhat.com>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Tejun Heo <tj@kernel.org>
Cc: Li Zefan <lizefan@huawei.com>
Cc: James Morris <jmorris@namei.org>
Cc: Pavel Emelyanov <xemul@openvz.org>
Cc: Serge Hallyn <serge.hallyn@canonical.com>
Signed-off-by: Jiri Slaby <jslaby@suse.cz>

---

And this should fix it.

 security/device_cgroup.c |   15 +++++++--------
 1 file changed, 7 insertions(+), 8 deletions(-)

--- github.orig/security/device_cgroup.c	2012-10-17 11:11:08.514793906 -0400
+++ github/security/device_cgroup.c	2012-10-19 16:35:37.936804289 -0400
@@ -533,10 +533,10 @@ struct cgroup_subsys devices_subsys = {
  *
  * returns 0 on success, -EPERM case the operation is not permitted
  */
-static int __devcgroup_check_permission(struct dev_cgroup *dev_cgroup,
-					short type, u32 major, u32 minor,
+static int __devcgroup_check_permission(short type, u32 major, u32 minor,
 				        short access)
 {
+	struct dev_cgroup *dev_cgroup;
 	struct dev_exception_item ex;
 	int rc;
 
@@ -547,6 +547,7 @@ 	memset(&ex, 0, sizeof(ex));
 	ex.access = access;
 
 	rcu_read_lock();
+	dev_cgroup = task_devcgroup(current);
 	rc = may_access(dev_cgroup, &ex);
 	rcu_read_unlock();
 
@@ -558,7 +559,6 @@ 	return 0;
 
 int __devcgroup_inode_permission(struct inode *inode, int mask)
 {
-	struct dev_cgroup *dev_cgroup = task_devcgroup(current);
 	short type, access = 0;
 
 	if (S_ISBLK(inode->i_mode))
@@ -570,13 +570,12 @@ 	short type, access = 0;
 	if (mask & MAY_READ)
 		access |= ACC_READ;
 
-	return __devcgroup_check_permission(dev_cgroup, type, imajor(inode),
-					    iminor(inode), access);
+	return __devcgroup_check_permission(type, imajor(inode), iminor(inode),
+			access);
 }
 
 int devcgroup_inode_mknod(int mode, dev_t dev)
 {
-	struct dev_cgroup *dev_cgroup = task_devcgroup(current);
 	short type;
 
 	if (!S_ISBLK(mode) && !S_ISCHR(mode))
@@ -587,7 +586,7 @@ 		return 0;
 	else
 		type = DEV_CHAR;
 
-	return __devcgroup_check_permission(dev_cgroup, type, MAJOR(dev),
-					    MINOR(dev), ACC_MKNOD);
+	return __devcgroup_check_permission(type, MAJOR(dev), MINOR(dev),
+			ACC_MKNOD);
 
 }

Re: [PATCH 1/4] cgroup: fix invalid rcu dereference

[Serge Hallyn]

Quoting Aristeu Rozanski (aris-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org):
> From: Jiri Slaby <jslaby-AlSwsSmVLrQ@public.gmane.org>
> 
> Commit "device_cgroup: convert device_cgroup internally to policy +
> exceptions" removed rcu locks which are needed in task_devcgroup
> called in this chain: devcgroup_inode_mknod OR
> __devcgroup_inode_permission -> __devcgroup_inode_permission ->
> task_devcgroup -> task_subsys_state -> task_subsys_state_check.
> 
> Change the code so that task_devcgroup is safely called with rcu read
> lock held.
> 
> ===============================
> [ INFO: suspicious RCU usage. ]
> 3.6.0-rc5-next-20120913+ #42 Not tainted
> -------------------------------
> include/linux/cgroup.h:553 suspicious rcu_dereference_check() usage!
> 
> other info that might help us debug this:
> 
> rcu_scheduler_active = 1, debug_locks = 0
> 2 locks held by kdevtmpfs/23:
>  #0:  (sb_writers){.+.+.+}, at: [<ffffffff8116873f>]
> mnt_want_write+0x1f/0x50
>  #1:  (&sb->s_type->i_mutex_key#3/1){+.+.+.}, at: [<ffffffff811558af>]
> kern_path_create+0x7f/0x170
> 
> stack backtrace:
> Pid: 23, comm: kdevtmpfs Not tainted 3.6.0-rc5-next-20120913+ #42
> Call Trace:
>  [<ffffffff810c638d>] lockdep_rcu_suspicious+0xfd/0x130
>  [<ffffffff8121541d>] devcgroup_inode_mknod+0x19d/0x240
>  [<ffffffff8107bf54>] ? ns_capable+0x44/0x80
>  [<ffffffff81156b21>] vfs_mknod+0x71/0xf0
>  [<ffffffff813a8332>] handle_create.isra.2+0x72/0x200
>  [<ffffffff813a85d4>] devtmpfsd+0x114/0x140
>  [<ffffffff813a84c0>] ? handle_create.isra.2+0x200/0x200
>  [<ffffffff81093ad6>] kthread+0xd6/0xe0
>  [<ffffffff81654f24>] kernel_thread_helper+0x4/0x10
>  [<ffffffff8165369d>] ? retint_restore_args+0xe/0xe
>  [<ffffffff81093a00>] ? kthread_create_on_node+0x140/0x140
>  [<ffffffff81654f20>] ? gs_change+0xb/0xb
> 
> Cc: Dave Jones <davej-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> Cc: Andrew Morton <akpm-de/tnXTf+JLsfHDXvbKv3WD2FQJk+8+b@public.gmane.org>
> Cc: Tejun Heo <tj-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
> Cc: Li Zefan <lizefan-hv44wF8Li93QT0dZR+AlfA@public.gmane.org>
> Cc: James Morris <jmorris-gx6/JNMH7DfYtjvyW6yDsg@public.gmane.org>
> Cc: Pavel Emelyanov <xemul-GEFAQzZX7r8dnm+yROfE0A@public.gmane.org>
> Cc: Serge Hallyn <serge.hallyn-Z7WLFzj8eWMS+FvcfC7Uqw@public.gmane.org>

Acked-by: Serge E. Hallyn <serge.hallyn-GeWIH/nMZzLQT0dZR+AlfA@public.gmane.org>

> Signed-off-by: Jiri Slaby <jslaby-AlSwsSmVLrQ@public.gmane.org>
> 
> ---
> 
> And this should fix it.
> 
>  security/device_cgroup.c |   15 +++++++--------
>  1 file changed, 7 insertions(+), 8 deletions(-)
> 
> --- github.orig/security/device_cgroup.c	2012-10-17 11:11:08.514793906 -0400
> +++ github/security/device_cgroup.c	2012-10-19 16:35:37.936804289 -0400
> @@ -533,10 +533,10 @@ struct cgroup_subsys devices_subsys = {
>   *
>   * returns 0 on success, -EPERM case the operation is not permitted
>   */
> -static int __devcgroup_check_permission(struct dev_cgroup *dev_cgroup,
> -					short type, u32 major, u32 minor,
> +static int __devcgroup_check_permission(short type, u32 major, u32 minor,
>  				        short access)
>  {
> +	struct dev_cgroup *dev_cgroup;
>  	struct dev_exception_item ex;
>  	int rc;
>  
> @@ -547,6 +547,7 @@ 	memset(&ex, 0, sizeof(ex));
>  	ex.access = access;
>  
>  	rcu_read_lock();
> +	dev_cgroup = task_devcgroup(current);
>  	rc = may_access(dev_cgroup, &ex);
>  	rcu_read_unlock();
>  
> @@ -558,7 +559,6 @@ 	return 0;
>  
>  int __devcgroup_inode_permission(struct inode *inode, int mask)
>  {
> -	struct dev_cgroup *dev_cgroup = task_devcgroup(current);
>  	short type, access = 0;
>  
>  	if (S_ISBLK(inode->i_mode))
> @@ -570,13 +570,12 @@ 	short type, access = 0;
>  	if (mask & MAY_READ)
>  		access |= ACC_READ;
>  
> -	return __devcgroup_check_permission(dev_cgroup, type, imajor(inode),
> -					    iminor(inode), access);
> +	return __devcgroup_check_permission(type, imajor(inode), iminor(inode),
> +			access);
>  }
>  
>  int devcgroup_inode_mknod(int mode, dev_t dev)
>  {
> -	struct dev_cgroup *dev_cgroup = task_devcgroup(current);
>  	short type;
>  
>  	if (!S_ISBLK(mode) && !S_ISCHR(mode))
> @@ -587,7 +586,7 @@ 		return 0;
>  	else
>  		type = DEV_CHAR;
>  
> -	return __devcgroup_check_permission(dev_cgroup, type, MAJOR(dev),
> -					    MINOR(dev), ACC_MKNOD);
> +	return __devcgroup_check_permission(type, MAJOR(dev), MINOR(dev),
> +			ACC_MKNOD);
>  
>  }
> 

Re: [PATCH 1/4] cgroup: fix invalid rcu dereference

[Jiri Slaby]

On 10/22/2012 03:45 PM, Aristeu Rozanski wrote:
> From: Jiri Slaby <jslaby-AlSwsSmVLrQ@public.gmane.org>

No, I'm *not* the author of the patch... Or at least I don't remember
:P. Where did you take it from?

> Commit "device_cgroup: convert device_cgroup internally to policy +
> exceptions" removed rcu locks which are needed in task_devcgroup
> called in this chain: devcgroup_inode_mknod OR
> __devcgroup_inode_permission -> __devcgroup_inode_permission ->
> task_devcgroup -> task_subsys_state -> task_subsys_state_check.
> 
> Change the code so that task_devcgroup is safely called with rcu read
> lock held.
> 
> ===============================
> [ INFO: suspicious RCU usage. ]
> 3.6.0-rc5-next-20120913+ #42 Not tainted
> -------------------------------
> include/linux/cgroup.h:553 suspicious rcu_dereference_check() usage!
> 
> other info that might help us debug this:
> 
> rcu_scheduler_active = 1, debug_locks = 0
> 2 locks held by kdevtmpfs/23:
>  #0:  (sb_writers){.+.+.+}, at: [<ffffffff8116873f>]
> mnt_want_write+0x1f/0x50
>  #1:  (&sb->s_type->i_mutex_key#3/1){+.+.+.}, at: [<ffffffff811558af>]
> kern_path_create+0x7f/0x170
> 
> stack backtrace:
> Pid: 23, comm: kdevtmpfs Not tainted 3.6.0-rc5-next-20120913+ #42
> Call Trace:
>  [<ffffffff810c638d>] lockdep_rcu_suspicious+0xfd/0x130
>  [<ffffffff8121541d>] devcgroup_inode_mknod+0x19d/0x240
>  [<ffffffff8107bf54>] ? ns_capable+0x44/0x80
>  [<ffffffff81156b21>] vfs_mknod+0x71/0xf0
>  [<ffffffff813a8332>] handle_create.isra.2+0x72/0x200
>  [<ffffffff813a85d4>] devtmpfsd+0x114/0x140
>  [<ffffffff813a84c0>] ? handle_create.isra.2+0x200/0x200
>  [<ffffffff81093ad6>] kthread+0xd6/0xe0
>  [<ffffffff81654f24>] kernel_thread_helper+0x4/0x10
>  [<ffffffff8165369d>] ? retint_restore_args+0xe/0xe
>  [<ffffffff81093a00>] ? kthread_create_on_node+0x140/0x140
>  [<ffffffff81654f20>] ? gs_change+0xb/0xb
> 
> Cc: Dave Jones <davej-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> Cc: Andrew Morton <akpm-de/tnXTf+JLsfHDXvbKv3WD2FQJk+8+b@public.gmane.org>
> Cc: Tejun Heo <tj-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
> Cc: Li Zefan <lizefan-hv44wF8Li93QT0dZR+AlfA@public.gmane.org>
> Cc: James Morris <jmorris-gx6/JNMH7DfYtjvyW6yDsg@public.gmane.org>
> Cc: Pavel Emelyanov <xemul-GEFAQzZX7r8dnm+yROfE0A@public.gmane.org>
> Cc: Serge Hallyn <serge.hallyn-Z7WLFzj8eWMS+FvcfC7Uqw@public.gmane.org>
> Signed-off-by: Jiri Slaby <jslaby-AlSwsSmVLrQ@public.gmane.org>
> 
> ---
> 
> And this should fix it.
> 
>  security/device_cgroup.c |   15 +++++++--------
>  1 file changed, 7 insertions(+), 8 deletions(-)
> 
> --- github.orig/security/device_cgroup.c	2012-10-17 11:11:08.514793906 -0400
> +++ github/security/device_cgroup.c	2012-10-19 16:35:37.936804289 -0400
> @@ -533,10 +533,10 @@ struct cgroup_subsys devices_subsys = {
>   *
>   * returns 0 on success, -EPERM case the operation is not permitted
>   */
> -static int __devcgroup_check_permission(struct dev_cgroup *dev_cgroup,
> -					short type, u32 major, u32 minor,
> +static int __devcgroup_check_permission(short type, u32 major, u32 minor,
>  				        short access)
>  {
> +	struct dev_cgroup *dev_cgroup;
>  	struct dev_exception_item ex;
>  	int rc;
>  
> @@ -547,6 +547,7 @@ 	memset(&ex, 0, sizeof(ex));
>  	ex.access = access;
>  
>  	rcu_read_lock();
> +	dev_cgroup = task_devcgroup(current);
>  	rc = may_access(dev_cgroup, &ex);
>  	rcu_read_unlock();
>  
> @@ -558,7 +559,6 @@ 	return 0;
>  
>  int __devcgroup_inode_permission(struct inode *inode, int mask)
>  {
> -	struct dev_cgroup *dev_cgroup = task_devcgroup(current);
>  	short type, access = 0;
>  
>  	if (S_ISBLK(inode->i_mode))
> @@ -570,13 +570,12 @@ 	short type, access = 0;
>  	if (mask & MAY_READ)
>  		access |= ACC_READ;
>  
> -	return __devcgroup_check_permission(dev_cgroup, type, imajor(inode),
> -					    iminor(inode), access);
> +	return __devcgroup_check_permission(type, imajor(inode), iminor(inode),
> +			access);
>  }
>  
>  int devcgroup_inode_mknod(int mode, dev_t dev)
>  {
> -	struct dev_cgroup *dev_cgroup = task_devcgroup(current);
>  	short type;
>  
>  	if (!S_ISBLK(mode) && !S_ISCHR(mode))
> @@ -587,7 +586,7 @@ 		return 0;
>  	else
>  		type = DEV_CHAR;
>  
> -	return __devcgroup_check_permission(dev_cgroup, type, MAJOR(dev),
> -					    MINOR(dev), ACC_MKNOD);
> +	return __devcgroup_check_permission(type, MAJOR(dev), MINOR(dev),
> +			ACC_MKNOD);
>  
>  }
> 


-- 
js
suse labs

Re: [PATCH 1/4] cgroup: fix invalid rcu dereference

[Aristeu Rozanski]

On Tue, Oct 23, 2012 at 02:50:48PM +0200, Jiri Slaby wrote:
> No, I'm *not* the author of the patch... Or at least I don't remember
> :P. Where did you take it from?

<1347615612-11450-1-git-send-email-jslaby-AlSwsSmVLrQ@public.gmane.org> 
Date: Fri, 14 Sep 2012 11:40:12 +0200

you didn't include a "From:" on your email, so I assumed it was from
you. it was sent to akpm with others and linux-kernel in cc

-- 
Aristeu

Re: [PATCH 1/4] cgroup: fix invalid rcu dereference

[Jiri Slaby]

On 10/23/2012 03:17 PM, Aristeu Rozanski wrote:
> On Tue, Oct 23, 2012 at 02:50:48PM +0200, Jiri Slaby wrote:
>> No, I'm *not* the author of the patch... Or at least I don't remember
>> :P. Where did you take it from?
> 
> <1347615612-11450-1-git-send-email-jslaby-AlSwsSmVLrQ@public.gmane.org> 
> Date: Fri, 14 Sep 2012 11:40:12 +0200
> 
> you didn't include a "From:" on your email, so I assumed it was from
> you. it was sent to akpm with others and linux-kernel in cc

Ah, ok, you convinced me that I'm the author of the patch :).


-- 
js
suse labs

[PATCH 2/4] device_cgroup: rename deny_all to behavior

[Aristeu Rozanski]

[-- Attachment #1: deny_all.patch --]
[-- Type: text/plain, Size: 3700 bytes --]

This was done in a v2 patch but v1 ended up being committed. The variable name
is less confusing and stores the default behavior when no matching exception
exists.


Cc: Dave Jones <davej@redhat.com>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Tejun Heo <tj@kernel.org>
Cc: Li Zefan <lizefan@huawei.com>
Cc: James Morris <jmorris@namei.org>
Cc: Pavel Emelyanov <xemul@openvz.org>
Cc: Serge Hallyn <serge.hallyn@canonical.com>
Cc: Jiri Slaby <jslaby@suse.cz>
Signed-off-by: Aristeu Rozanski <aris@redhat.com>

---
 security/device_cgroup.c |   25 ++++++++++++++-----------
 1 file changed, 14 insertions(+), 11 deletions(-)

--- github.orig/security/device_cgroup.c	2012-10-19 16:35:37.936804289 -0400
+++ github/security/device_cgroup.c	2012-10-19 16:35:46.366102913 -0400
@@ -42,7 +42,10 @@ struct dev_exception_item {
 struct dev_cgroup {
 	struct cgroup_subsys_state css;
 	struct list_head exceptions;
-	bool deny_all;
+	enum {
+		DEVCG_DEFAULT_ALLOW,
+		DEVCG_DEFAULT_DENY,
+	} behavior;
 };
 
 static inline struct dev_cgroup *css_to_devcgroup(struct cgroup_subsys_state *s)
@@ -182,13 +185,13 @@ static struct cgroup_subsys_state *devcg
 	parent_cgroup = cgroup->parent;
 
 	if (parent_cgroup == NULL)
-		dev_cgroup->deny_all = false;
+		dev_cgroup->behavior = DEVCG_DEFAULT_ALLOW;
 	else {
 		parent_dev_cgroup = cgroup_to_devcgroup(parent_cgroup);
 		mutex_lock(&devcgroup_mutex);
 		ret = dev_exceptions_copy(&dev_cgroup->exceptions,
 					  &parent_dev_cgroup->exceptions);
-		dev_cgroup->deny_all = parent_dev_cgroup->deny_all;
+		dev_cgroup->behavior = parent_dev_cgroup->behavior;
 		mutex_unlock(&devcgroup_mutex);
 		if (ret) {
 			kfree(dev_cgroup);
@@ -260,7 +263,7 @@ static int devcgroup_seq_read(struct cgr
 	 * - List the exceptions in case the default policy is to deny
 	 * This way, the file remains as a "whitelist of devices"
 	 */
-	if (devcgroup->deny_all == false) {
+	if (devcgroup->behavior == DEVCG_DEFAULT_ALLOW) {
 		set_access(acc, ACC_MASK);
 		set_majmin(maj, ~0);
 		set_majmin(min, ~0);
@@ -314,12 +317,12 @@ 		if (ex->minor != ~0 && ex->minor != re
 	 * In two cases we'll consider this new exception valid:
 	 * - the dev cgroup has its default policy to allow + exception list:
 	 *   the new exception should *not* match any of the exceptions
-	 *   (!deny_all, !match)
+	 *   (behavior == DEVCG_DEFAULT_ALLOW, !match)
 	 * - the dev cgroup has its default policy to deny + exception list:
 	 *   the new exception *should* match the exceptions
-	 *   (deny_all, match)
+	 *   (behavior == DEVCG_DEFAULT_DENY, match)
 	 */
-	if (dev_cgroup->deny_all == match)
+	if ((dev_cgroup->behavior == DEVCG_DEFAULT_DENY) == match)
 		return 1;
 	return 0;
 }
@@ -375,11 +378,11 @@ 	memset(&ex, 0, sizeof(ex));
 			if (!parent_has_perm(devcgroup, &ex))
 				return -EPERM;
 			dev_exception_clean(devcgroup);
-			devcgroup->deny_all = false;
+			devcgroup->behavior = DEVCG_DEFAULT_ALLOW;
 			break;
 		case DEVCG_DENY:
 			dev_exception_clean(devcgroup);
-			devcgroup->deny_all = true;
+			devcgroup->behavior = DEVCG_DEFAULT_DENY;
 			break;
 		default:
 			return -EINVAL;
@@ -452,7 +455,7 @@ 		case '\0':
 		 * an matching exception instead. And be silent about it: we
 		 * don't want to break compatibility
 		 */
-		if (devcgroup->deny_all == false) {
+		if (devcgroup->behavior == DEVCG_DEFAULT_ALLOW) {
 			dev_exception_rm(devcgroup, &ex);
 			return 0;
 		}
@@ -463,7 +466,7 @@ 			return 0;
 		 * an matching exception instead. And be silent about it: we
 		 * don't want to break compatibility
 		 */
-		if (devcgroup->deny_all == true) {
+		if (devcgroup->behavior == DEVCG_DEFAULT_DENY) {
 			dev_exception_rm(devcgroup, &ex);
 			return 0;
 		}

Re: [PATCH 2/4] device_cgroup: rename deny_all to behavior

[Serge Hallyn]

Quoting Aristeu Rozanski (aris@redhat.com):
> This was done in a v2 patch but v1 ended up being committed. The variable name
> is less confusing and stores the default behavior when no matching exception
> exists.
> 
> 
> Cc: Dave Jones <davej@redhat.com>
> Cc: Andrew Morton <akpm@linux-foundation.org>
> Cc: Tejun Heo <tj@kernel.org>
> Cc: Li Zefan <lizefan@huawei.com>
> Cc: James Morris <jmorris@namei.org>
> Cc: Pavel Emelyanov <xemul@openvz.org>
> Cc: Serge Hallyn <serge.hallyn@canonical.com>

Acked-by: Serge E. Hallyn <serge.hallyn@ubuntu.com>

> Cc: Jiri Slaby <jslaby@suse.cz>
> Signed-off-by: Aristeu Rozanski <aris@redhat.com>
> 
> ---
>  security/device_cgroup.c |   25 ++++++++++++++-----------
>  1 file changed, 14 insertions(+), 11 deletions(-)
> 
> --- github.orig/security/device_cgroup.c	2012-10-19 16:35:37.936804289 -0400
> +++ github/security/device_cgroup.c	2012-10-19 16:35:46.366102913 -0400
> @@ -42,7 +42,10 @@ struct dev_exception_item {
>  struct dev_cgroup {
>  	struct cgroup_subsys_state css;
>  	struct list_head exceptions;
> -	bool deny_all;
> +	enum {
> +		DEVCG_DEFAULT_ALLOW,
> +		DEVCG_DEFAULT_DENY,
> +	} behavior;
>  };
>  
>  static inline struct dev_cgroup *css_to_devcgroup(struct cgroup_subsys_state *s)
> @@ -182,13 +185,13 @@ static struct cgroup_subsys_state *devcg
>  	parent_cgroup = cgroup->parent;
>  
>  	if (parent_cgroup == NULL)
> -		dev_cgroup->deny_all = false;
> +		dev_cgroup->behavior = DEVCG_DEFAULT_ALLOW;
>  	else {
>  		parent_dev_cgroup = cgroup_to_devcgroup(parent_cgroup);
>  		mutex_lock(&devcgroup_mutex);
>  		ret = dev_exceptions_copy(&dev_cgroup->exceptions,
>  					  &parent_dev_cgroup->exceptions);
> -		dev_cgroup->deny_all = parent_dev_cgroup->deny_all;
> +		dev_cgroup->behavior = parent_dev_cgroup->behavior;
>  		mutex_unlock(&devcgroup_mutex);
>  		if (ret) {
>  			kfree(dev_cgroup);
> @@ -260,7 +263,7 @@ static int devcgroup_seq_read(struct cgr
>  	 * - List the exceptions in case the default policy is to deny
>  	 * This way, the file remains as a "whitelist of devices"
>  	 */
> -	if (devcgroup->deny_all == false) {
> +	if (devcgroup->behavior == DEVCG_DEFAULT_ALLOW) {
>  		set_access(acc, ACC_MASK);
>  		set_majmin(maj, ~0);
>  		set_majmin(min, ~0);
> @@ -314,12 +317,12 @@ 		if (ex->minor != ~0 && ex->minor != re
>  	 * In two cases we'll consider this new exception valid:
>  	 * - the dev cgroup has its default policy to allow + exception list:
>  	 *   the new exception should *not* match any of the exceptions
> -	 *   (!deny_all, !match)
> +	 *   (behavior == DEVCG_DEFAULT_ALLOW, !match)
>  	 * - the dev cgroup has its default policy to deny + exception list:
>  	 *   the new exception *should* match the exceptions
> -	 *   (deny_all, match)
> +	 *   (behavior == DEVCG_DEFAULT_DENY, match)
>  	 */
> -	if (dev_cgroup->deny_all == match)
> +	if ((dev_cgroup->behavior == DEVCG_DEFAULT_DENY) == match)
>  		return 1;
>  	return 0;
>  }
> @@ -375,11 +378,11 @@ 	memset(&ex, 0, sizeof(ex));
>  			if (!parent_has_perm(devcgroup, &ex))
>  				return -EPERM;
>  			dev_exception_clean(devcgroup);
> -			devcgroup->deny_all = false;
> +			devcgroup->behavior = DEVCG_DEFAULT_ALLOW;
>  			break;
>  		case DEVCG_DENY:
>  			dev_exception_clean(devcgroup);
> -			devcgroup->deny_all = true;
> +			devcgroup->behavior = DEVCG_DEFAULT_DENY;
>  			break;
>  		default:
>  			return -EINVAL;
> @@ -452,7 +455,7 @@ 		case '\0':
>  		 * an matching exception instead. And be silent about it: we
>  		 * don't want to break compatibility
>  		 */
> -		if (devcgroup->deny_all == false) {
> +		if (devcgroup->behavior == DEVCG_DEFAULT_ALLOW) {
>  			dev_exception_rm(devcgroup, &ex);
>  			return 0;
>  		}
> @@ -463,7 +466,7 @@ 			return 0;
>  		 * an matching exception instead. And be silent about it: we
>  		 * don't want to break compatibility
>  		 */
> -		if (devcgroup->deny_all == true) {
> +		if (devcgroup->behavior == DEVCG_DEFAULT_DENY) {
>  			dev_exception_rm(devcgroup, &ex);
>  			return 0;
>  		}
> 

[PATCH 3/4] device_cgroup: stop using simple_strtoul()

[Aristeu Rozanski]

[-- Attachment #1: kstrtou32.patch --]
[-- Type: text/plain, Size: 1929 bytes --]

This patch converts the code to use kstrtou32() instead of simple_strtoul()
which is deprecated. The real size of the variables are u32, so use kstrtou32
instead of kstrtoul


Cc: Dave Jones <davej@redhat.com>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Tejun Heo <tj@kernel.org>
Cc: Li Zefan <lizefan@huawei.com>
Cc: James Morris <jmorris@namei.org>
Cc: Pavel Emelyanov <xemul@openvz.org>
Cc: Serge Hallyn <serge.hallyn@canonical.com>
Cc: Jiri Slaby <jslaby@suse.cz>
Signed-off-by: Aristeu Rozanski <aris@redhat.com>

---
 security/device_cgroup.c |   28 ++++++++++++++++++++++------
 1 file changed, 22 insertions(+), 6 deletions(-)

--- github.orig/security/device_cgroup.c	2012-10-19 16:35:46.366102913 -0400
+++ github/security/device_cgroup.c	2012-10-19 16:35:50.801229331 -0400
@@ -361,8 +361,8 @@ static int devcgroup_update_access(struc
 				   int filetype, const char *buffer)
 {
 	const char *b;
-	char *endp;
-	int count;
+	char temp[12];		/* 11 + 1 characters needed for a u32 */
+	int count, rc;
 	struct dev_exception_item ex;
 
 	if (!capable(CAP_SYS_ADMIN))
@@ -405,8 +405,16 @@ 		return 0;
 		ex.major = ~0;
 		b++;
 	} else if (isdigit(*b)) {
-		ex.major = simple_strtoul(b, &endp, 10);
-		b = endp;
+		memset(temp, 0, sizeof(temp));
+		for (count = 0; count < sizeof(temp) - 1; count++) {
+			temp[count] = *b;
+			b++;
+			if (!isdigit(*b))
+				break;
+		}
+		rc = kstrtou32(temp, 10, &ex.major);
+		if (rc)
+			return -EINVAL;
 	} else {
 		return -EINVAL;
 	}
@@ -419,8 +427,16 @@ 		ex.major = simple_strtoul(b, &endp, 10
 		ex.minor = ~0;
 		b++;
 	} else if (isdigit(*b)) {
-		ex.minor = simple_strtoul(b, &endp, 10);
-		b = endp;
+		memset(temp, 0, sizeof(temp));
+		for (count = 0; count < sizeof(temp) - 1; count++) {
+			temp[count] = *b;
+			b++;
+			if (!isdigit(*b))
+				break;
+		}
+		rc = kstrtou32(temp, 10, &ex.minor);
+		if (rc)
+			return -EINVAL;
 	} else {
 		return -EINVAL;
 	}

Re: [PATCH 3/4] device_cgroup: stop using simple_strtoul()

[Serge Hallyn]

Quoting Aristeu Rozanski (aris-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org):
> This patch converts the code to use kstrtou32() instead of simple_strtoul()
> which is deprecated. The real size of the variables are u32, so use kstrtou32
> instead of kstrtoul
> 
> 
> Cc: Dave Jones <davej-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> Cc: Andrew Morton <akpm-de/tnXTf+JLsfHDXvbKv3WD2FQJk+8+b@public.gmane.org>
> Cc: Tejun Heo <tj-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
> Cc: Li Zefan <lizefan-hv44wF8Li93QT0dZR+AlfA@public.gmane.org>
> Cc: James Morris <jmorris-gx6/JNMH7DfYtjvyW6yDsg@public.gmane.org>
> Cc: Pavel Emelyanov <xemul-GEFAQzZX7r8dnm+yROfE0A@public.gmane.org>
> Cc: Serge Hallyn <serge.hallyn-Z7WLFzj8eWMS+FvcfC7Uqw@public.gmane.org>

Acked-by: Serge E. Hallyn <serge.hallyn-GeWIH/nMZzLQT0dZR+AlfA@public.gmane.org>

> Cc: Jiri Slaby <jslaby-AlSwsSmVLrQ@public.gmane.org>
> Signed-off-by: Aristeu Rozanski <aris-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> 
> ---
>  security/device_cgroup.c |   28 ++++++++++++++++++++++------
>  1 file changed, 22 insertions(+), 6 deletions(-)
> 
> --- github.orig/security/device_cgroup.c	2012-10-19 16:35:46.366102913 -0400
> +++ github/security/device_cgroup.c	2012-10-19 16:35:50.801229331 -0400
> @@ -361,8 +361,8 @@ static int devcgroup_update_access(struc
>  				   int filetype, const char *buffer)
>  {
>  	const char *b;
> -	char *endp;
> -	int count;
> +	char temp[12];		/* 11 + 1 characters needed for a u32 */
> +	int count, rc;
>  	struct dev_exception_item ex;
>  
>  	if (!capable(CAP_SYS_ADMIN))
> @@ -405,8 +405,16 @@ 		return 0;
>  		ex.major = ~0;
>  		b++;
>  	} else if (isdigit(*b)) {
> -		ex.major = simple_strtoul(b, &endp, 10);
> -		b = endp;
> +		memset(temp, 0, sizeof(temp));
> +		for (count = 0; count < sizeof(temp) - 1; count++) {
> +			temp[count] = *b;
> +			b++;
> +			if (!isdigit(*b))
> +				break;
> +		}
> +		rc = kstrtou32(temp, 10, &ex.major);
> +		if (rc)
> +			return -EINVAL;
>  	} else {
>  		return -EINVAL;
>  	}
> @@ -419,8 +427,16 @@ 		ex.major = simple_strtoul(b, &endp, 10
>  		ex.minor = ~0;
>  		b++;
>  	} else if (isdigit(*b)) {
> -		ex.minor = simple_strtoul(b, &endp, 10);
> -		b = endp;
> +		memset(temp, 0, sizeof(temp));
> +		for (count = 0; count < sizeof(temp) - 1; count++) {
> +			temp[count] = *b;
> +			b++;
> +			if (!isdigit(*b))
> +				break;
> +		}
> +		rc = kstrtou32(temp, 10, &ex.minor);
> +		if (rc)
> +			return -EINVAL;
>  	} else {
>  		return -EINVAL;
>  	}
> 

[PATCH 4/4] device_cgroup: add proper checking when changing default behavior

[Aristeu Rozanski]

[-- Attachment #1: fix-perm.patch --]
[-- Type: text/plain, Size: 1944 bytes --]

Before changing a group's default behavior to ALLOW, we must check if its
parent's behavior is also ALLOW.

Cc: Tejun Heo <tj@kernel.org>
Cc: Li Zefan <lizefan@huawei.com>
Cc: James Morris <jmorris@namei.org>
Cc: Pavel Emelyanov <xemul@openvz.org>
Cc: Serge Hallyn <serge.hallyn@canonical.com>
Cc: Jiri Slaby <jslaby@suse.cz>
Signed-off-by: Aristeu Rozanski <aris@redhat.com>

---
 security/device_cgroup.c |   19 ++++++++++++++++++-
 1 file changed, 18 insertions(+), 1 deletion(-)

--- github.orig/security/device_cgroup.c	2012-10-19 16:36:50.135790476 -0400
+++ github/security/device_cgroup.c	2012-10-19 16:48:50.710978125 -0400
@@ -344,6 +344,17 @@ static int parent_has_perm(struct dev_cg
 	return may_access(parent, ex);
 }
 
+/**
+ * may_allow_all - checks if it's possible to change the behavior to
+ *		   allow based on parent's rules.
+ * @parent: device cgroup's parent
+ * returns: != 0 in case it's allowed, 0 otherwise
+ */
+static inline int may_allow_all(struct dev_cgroup *parent)
+{
+	return parent->behavior == DEVCG_DEFAULT_ALLOW;
+}
+
 /*
  * Modify the exception list using allow/deny rules.
  * CAP_SYS_ADMIN is needed for this.  It's at least separate from CAP_MKNOD
@@ -364,6 +375,8 @@ static int devcgroup_update_access(struc
 	char temp[12];		/* 11 + 1 characters needed for a u32 */
 	int count, rc;
 	struct dev_exception_item ex;
+	struct cgroup *p = devcgroup->css.cgroup;
+	struct dev_cgroup *parent = cgroup_to_devcgroup(p->parent);
 
 	if (!capable(CAP_SYS_ADMIN))
 		return -EPERM;
@@ -375,9 +388,13 @@ 	memset(&ex, 0, sizeof(ex));
 	case 'a':
 		switch (filetype) {
 		case DEVCG_ALLOW:
-			if (!parent_has_perm(devcgroup, &ex))
+			if (!may_allow_all(parent))
 				return -EPERM;
 			dev_exception_clean(devcgroup);
+			rc = dev_exceptions_copy(&devcgroup->exceptions,
+						 &parent->exceptions);
+			if (rc)
+				return rc;
 			devcgroup->behavior = DEVCG_DEFAULT_ALLOW;
 			break;
 		case DEVCG_DENY:

Re: [PATCH 4/4] device_cgroup: add proper checking when changing default behavior

[Serge Hallyn]

Quoting Aristeu Rozanski (aris-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org):
> Before changing a group's default behavior to ALLOW, we must check if its
> parent's behavior is also ALLOW.
> 
> Cc: Tejun Heo <tj-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
> Cc: Li Zefan <lizefan-hv44wF8Li93QT0dZR+AlfA@public.gmane.org>
> Cc: James Morris <jmorris-gx6/JNMH7DfYtjvyW6yDsg@public.gmane.org>
> Cc: Pavel Emelyanov <xemul-GEFAQzZX7r8dnm+yROfE0A@public.gmane.org>
> Cc: Serge Hallyn <serge.hallyn-Z7WLFzj8eWMS+FvcfC7Uqw@public.gmane.org>

Acked-by: Serge E. Hallyn <serge.hallyn-GeWIH/nMZzLQT0dZR+AlfA@public.gmane.org>

Thanks, Aristeu.

> Cc: Jiri Slaby <jslaby-AlSwsSmVLrQ@public.gmane.org>
> Signed-off-by: Aristeu Rozanski <aris-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> 
> ---
>  security/device_cgroup.c |   19 ++++++++++++++++++-
>  1 file changed, 18 insertions(+), 1 deletion(-)
> 
> --- github.orig/security/device_cgroup.c	2012-10-19 16:36:50.135790476 -0400
> +++ github/security/device_cgroup.c	2012-10-19 16:48:50.710978125 -0400
> @@ -344,6 +344,17 @@ static int parent_has_perm(struct dev_cg
>  	return may_access(parent, ex);
>  }
>  
> +/**
> + * may_allow_all - checks if it's possible to change the behavior to
> + *		   allow based on parent's rules.
> + * @parent: device cgroup's parent
> + * returns: != 0 in case it's allowed, 0 otherwise
> + */
> +static inline int may_allow_all(struct dev_cgroup *parent)
> +{
> +	return parent->behavior == DEVCG_DEFAULT_ALLOW;
> +}
> +
>  /*
>   * Modify the exception list using allow/deny rules.
>   * CAP_SYS_ADMIN is needed for this.  It's at least separate from CAP_MKNOD
> @@ -364,6 +375,8 @@ static int devcgroup_update_access(struc
>  	char temp[12];		/* 11 + 1 characters needed for a u32 */
>  	int count, rc;
>  	struct dev_exception_item ex;
> +	struct cgroup *p = devcgroup->css.cgroup;
> +	struct dev_cgroup *parent = cgroup_to_devcgroup(p->parent);
>  
>  	if (!capable(CAP_SYS_ADMIN))
>  		return -EPERM;
> @@ -375,9 +388,13 @@ 	memset(&ex, 0, sizeof(ex));
>  	case 'a':
>  		switch (filetype) {
>  		case DEVCG_ALLOW:
> -			if (!parent_has_perm(devcgroup, &ex))
> +			if (!may_allow_all(parent))
>  				return -EPERM;
>  			dev_exception_clean(devcgroup);
> +			rc = dev_exceptions_copy(&devcgroup->exceptions,
> +						 &parent->exceptions);
> +			if (rc)
> +				return rc;
>  			devcgroup->behavior = DEVCG_DEFAULT_ALLOW;
>  			break;
>  		case DEVCG_DENY:
> 

Re: [PATCH 0/4] Rebase device_cgroup v2 patchset

[Andrew Morton]

On Mon, 22 Oct 2012 09:45:36 -0400
Aristeu Rozanski <aris-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote:

> This patchset rebases the v2 of the patchset since the v1 was pushed into -rc1
> instead. The last patch, not present on previous patchset, fixes the
> permission check when allowing everything in a cgroup.
> 

I grabbed all four, thanks.  Shall send them on to Linus in a week or
so, after a bit more review and some linux-next testing.

I've been trying to work out why I merged the v1 patchset rather than
v2 and cannot find that v2 patchset anywhere.  When was it sent out?

Re: [PATCH 0/4] Rebase device_cgroup v2 patchset

[Aristeu Rozanski]

Hi Andrew,
On Mon, Oct 22, 2012 at 12:58:38PM -0700, Andrew Morton wrote:
> On Mon, 22 Oct 2012 09:45:36 -0400
> Aristeu Rozanski <aris-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote:
> 
> > This patchset rebases the v2 of the patchset since the v1 was pushed into -rc1
> > instead. The last patch, not present on previous patchset, fixes the
> > permission check when allowing everything in a cgroup.
> > 
> 
> I grabbed all four, thanks.  Shall send them on to Linus in a week or
> so, after a bit more review and some linux-next testing.

thanks, much appreciated

> I've been trying to work out why I merged the v1 patchset rather than
> v2 and cannot find that v2 patchset anywhere.  When was it sent out?

It was sent to linux-kernel and cgroups@vger on Sep 04th, here's the
msgid of patch 0 for reference:
<20120904143419.892872876-cd6kKtb6gxi3M6m420IelR/sF2h8X+2i0E9HWUfgJXw@public.gmane.org> 

-- 
Aristeu

Re: [PATCH 0/4] Rebase device_cgroup v2 patchset

[Serge Hallyn]

Hi,

so now that the device cgroup properly respects hierarchy, not allowing
a cgroup to be given greater permission than its parent, should we consider
relaxing the capability checks?

There are two capable(CAP_SYS_ADMIN) checks in deice_cgroup.c: one in
devcgroup_can_attach() to protect changing another task's cgroup, and
one in devcgroup_update_access() to protect writes to the devices.allow
and devices.deny files.

I think the first should be changed to a check for ns_capable() to
the victim's user_ns.  Something like 

--- a/security/device_cgroup.c
+++ b/security/device_cgroup.c
@@ -70,10 +70,16 @@ static int devcgroup_can_attach(struct cgroup *new_cgrp,
                                struct cgroup_taskset *set)
 {
        struct task_struct *task = cgroup_taskset_first(set);
+       struct user_namespace *ns;
+       int ret = -EPERM;

-       if (current != task && !capable(CAP_SYS_ADMIN))
-               return -EPERM;
-       return 0;
+       if (current == task)
+               return 0;
+
+       ns = userns_get(task);;
+       ret = ns_capable(ns, CAP_SYS_ADMIN) ? 0 : -EPERM;
+       put_user_ns(ns);
+       return ret;
 }

 /*

For the second, the hierarchy support should let us ignore concerns
about unprivileged users escalating privilege, but I'm trying to decide
whether we  need to worry about the sendmail capability class of bugs.
My sense is actually the answer is no, and we can drop the capable()
check altogether.  The reason is that while userspace frequently doesn't
properly handle a failing system call due to unexpected lack of partial
privilege, I wouldn't expect any setuid root program to ignore failure
to open or mknod a device file (and proceed into a bad failure mode).
Does this sound rasonable, or a recipe for disaster?

-serge

Re: [PATCH 0/4] Rebase device_cgroup v2 patchset

[Aristeu Rozanski]

On Tue, May 14, 2013 at 10:05:39AM -0500, Serge Hallyn wrote:
> so now that the device cgroup properly respects hierarchy, not allowing
> a cgroup to be given greater permission than its parent, should we consider
> relaxing the capability checks?
> 
> There are two capable(CAP_SYS_ADMIN) checks in deice_cgroup.c: one in
> devcgroup_can_attach() to protect changing another task's cgroup, and
> one in devcgroup_update_access() to protect writes to the devices.allow
> and devices.deny files.
> 
> I think the first should be changed to a check for ns_capable() to
> the victim's user_ns.  Something like 
> 
> --- a/security/device_cgroup.c
> +++ b/security/device_cgroup.c
> @@ -70,10 +70,16 @@ static int devcgroup_can_attach(struct cgroup *new_cgrp,
>                                 struct cgroup_taskset *set)
>  {
>         struct task_struct *task = cgroup_taskset_first(set);
> +       struct user_namespace *ns;
> +       int ret = -EPERM;
> 
> -       if (current != task && !capable(CAP_SYS_ADMIN))
> -               return -EPERM;
> -       return 0;
> +       if (current == task)
> +               return 0;
> +
> +       ns = userns_get(task);;
> +       ret = ns_capable(ns, CAP_SYS_ADMIN) ? 0 : -EPERM;
> +       put_user_ns(ns);
> +       return ret;
>  }

wouldn't this allow a userns root to move a task in the same userns into
a parent cgroup? I believe than anything but moving down the hierarchy
would be very complicated to verify (how far up can you go).

> For the second, the hierarchy support should let us ignore concerns
> about unprivileged users escalating privilege, but I'm trying to decide
> whether we  need to worry about the sendmail capability class of bugs.

You have a pointer for more information on those?

> My sense is actually the answer is no, and we can drop the capable()
> check altogether.  The reason is that while userspace frequently doesn't
> properly handle a failing system call due to unexpected lack of partial
> privilege, I wouldn't expect any setuid root program to ignore failure
> to open or mknod a device file (and proceed into a bad failure mode).
> Does this sound rasonable, or a recipe for disaster?

The second case sounds ok to me

-- 
Aristeu

Re: [PATCH 0/4] Rebase device_cgroup v2 patchset

[Serge Hallyn]

Quoting Aristeu Rozanski (aris-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org):
> On Tue, May 14, 2013 at 10:05:39AM -0500, Serge Hallyn wrote:
> > so now that the device cgroup properly respects hierarchy, not allowing
> > a cgroup to be given greater permission than its parent, should we consider
> > relaxing the capability checks?
> > 
> > There are two capable(CAP_SYS_ADMIN) checks in deice_cgroup.c: one in
> > devcgroup_can_attach() to protect changing another task's cgroup, and
> > one in devcgroup_update_access() to protect writes to the devices.allow
> > and devices.deny files.
> > 
> > I think the first should be changed to a check for ns_capable() to
> > the victim's user_ns.  Something like 
> > 
> > --- a/security/device_cgroup.c
> > +++ b/security/device_cgroup.c
> > @@ -70,10 +70,16 @@ static int devcgroup_can_attach(struct cgroup *new_cgrp,
> >                                 struct cgroup_taskset *set)
> >  {
> >         struct task_struct *task = cgroup_taskset_first(set);
> > +       struct user_namespace *ns;
> > +       int ret = -EPERM;
> > 
> > -       if (current != task && !capable(CAP_SYS_ADMIN))
> > -               return -EPERM;
> > -       return 0;
> > +       if (current == task)
> > +               return 0;
> > +
> > +       ns = userns_get(task);;
> > +       ret = ns_capable(ns, CAP_SYS_ADMIN) ? 0 : -EPERM;
> > +       put_user_ns(ns);
> > +       return ret;
> >  }
> 
> wouldn't this allow a userns root to move a task in the same userns into
> a parent cgroup? I believe than anything but moving down the hierarchy
> would be very complicated to verify (how far up can you go).

But only if they are able to open the tasks file for writing, which
they shouldn't be able to do, right?

> > For the second, the hierarchy support should let us ignore concerns
> > about unprivileged users escalating privilege, but I'm trying to decide
> > whether we  need to worry about the sendmail capability class of bugs.
> 
> You have a pointer for more information on those?

Darn - unfortunately the best description of it, which was at
http://userweb.kernel.org/~morgan/sendmail-capabilities-war-story.html
is no longer there since userweb was taken down, and it was never
captured by archive.org.  There's a brief description in
http://lwn.net/Articles/280279/ at the paragraph starting with "The
memory of the sendmail-capabilities bug from 2000..."

> > My sense is actually the answer is no, and we can drop the capable()
> > check altogether.  The reason is that while userspace frequently doesn't
> > properly handle a failing system call due to unexpected lack of partial
> > privilege, I wouldn't expect any setuid root program to ignore failure
> > to open or mknod a device file (and proceed into a bad failure mode).
> > Does this sound rasonable, or a recipe for disaster?
> 
> The second case sounds ok to me

-serge

Re: [PATCH 0/4] Rebase device_cgroup v2 patchset

[Eric W. Biederman]

Serge Hallyn <serge.hallyn-GeWIH/nMZzLQT0dZR+AlfA@public.gmane.org> writes:

> Quoting Aristeu Rozanski (aris-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org):
>> On Tue, May 14, 2013 at 10:05:39AM -0500, Serge Hallyn wrote:
>> > so now that the device cgroup properly respects hierarchy, not allowing
>> > a cgroup to be given greater permission than its parent, should we consider
>> > relaxing the capability checks?
>> > 
>> > There are two capable(CAP_SYS_ADMIN) checks in deice_cgroup.c: one in
>> > devcgroup_can_attach() to protect changing another task's cgroup, and
>> > one in devcgroup_update_access() to protect writes to the devices.allow
>> > and devices.deny files.
>> > 
>> > I think the first should be changed to a check for ns_capable() to
>> > the victim's user_ns.  Something like 
>> > 
>> > --- a/security/device_cgroup.c
>> > +++ b/security/device_cgroup.c
>> > @@ -70,10 +70,16 @@ static int devcgroup_can_attach(struct cgroup *new_cgrp,
>> >                                 struct cgroup_taskset *set)
>> >  {
>> >         struct task_struct *task = cgroup_taskset_first(set);
>> > +       struct user_namespace *ns;
>> > +       int ret = -EPERM;
>> > 
>> > -       if (current != task && !capable(CAP_SYS_ADMIN))
>> > -               return -EPERM;
>> > -       return 0;
>> > +       if (current == task)
>> > +               return 0;
>> > +
>> > +       ns = userns_get(task);;
>> > +       ret = ns_capable(ns, CAP_SYS_ADMIN) ? 0 : -EPERM;
>> > +       put_user_ns(ns);
>> > +       return ret;
>> >  }
>> 
>> wouldn't this allow a userns root to move a task in the same userns into
>> a parent cgroup? I believe than anything but moving down the hierarchy
>> would be very complicated to verify (how far up can you go).
>
> But only if they are able to open the tasks file for writing, which
> they shouldn't be able to do, right?

That should be looked at very closely.  There are some funny exploits of
setuid root applications writing to files that have required some
additional permission checks on /proc/<pid>/uid_map.  I think the
cgroups files may be vulnerable to some of the same kind of exploits.

Certainly we should be verifying that the opener of the file had the
capabilities we are trying to use to avoid being open to those kinds of
problems.

I am trying to see the utilitity of the proposed patch.  It doesn't
allow mknod.  So what is the benefit of having the user namespace bits?

Is the point to allow the userns root to remove access to selected
devices from it's children even if the DAC permissions would allow the
access?

>> > For the second, the hierarchy support should let us ignore concerns
>> > about unprivileged users escalating privilege, but I'm trying to decide
>> > whether we  need to worry about the sendmail capability class of bugs.
>> 
>> You have a pointer for more information on those?
>
> Darn - unfortunately the best description of it, which was at
> http://userweb.kernel.org/~morgan/sendmail-capabilities-war-story.html
> is no longer there since userweb was taken down, and it was never
> captured by archive.org.  There's a brief description in
> http://lwn.net/Articles/280279/ at the paragraph starting with "The
> memory of the sendmail-capabilities bug from 2000..."

I think the sendmail website has something as well.  The core problem
was programs not dealing safely with having some of their privileges
removed.  As I recall an unprivileged attacker at one point could drop
the CAP_SYS_SETUID on sendmail and get it to deliver mail as root.

I took a good hard look at this issue before implementing limits on
setuid in the user namespace and it appears that the authors of setuid
application learned from this as well and I could not find an single
program that would call setuid without out checking it's return code.

That said I haven't looked at open or mknod, and usually we are talking
about calls that aren't made by suid apps so I think there is a fair
chance that dropping some of those permissions could cause issues.

The first danger that crosses my mind is what happens if you remove
access to /dev/tty from a normal application that would trying and log
strange goings on to a user if they could.

Shrug mostly I don't see the advantage of this change.

Eric


>> > My sense is actually the answer is no, and we can drop the capable()
>> > check altogether.  The reason is that while userspace frequently doesn't
>> > properly handle a failing system call due to unexpected lack of partial
>> > privilege, I wouldn't expect any setuid root program to ignore failure
>> > to open or mknod a device file (and proceed into a bad failure mode).
>> > Does this sound rasonable, or a recipe for disaster?
>> 
>> The second case sounds ok to me
>
> -serge

Re: [PATCH 0/4] Rebase device_cgroup v2 patchset

[Serge E. Hallyn]

Quoting Eric W. Biederman (ebiederm-aS9lmoZGLiVWk0Htik3J/w@public.gmane.org):
> Serge Hallyn <serge.hallyn-GeWIH/nMZzLQT0dZR+AlfA@public.gmane.org> writes:
> 
> > Quoting Aristeu Rozanski (aris-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org):
> >> On Tue, May 14, 2013 at 10:05:39AM -0500, Serge Hallyn wrote:
> >> > so now that the device cgroup properly respects hierarchy, not allowing
> >> > a cgroup to be given greater permission than its parent, should we consider
> >> > relaxing the capability checks?
> >> > 
> >> > There are two capable(CAP_SYS_ADMIN) checks in deice_cgroup.c: one in
> >> > devcgroup_can_attach() to protect changing another task's cgroup, and
> >> > one in devcgroup_update_access() to protect writes to the devices.allow
> >> > and devices.deny files.
> >> > 
> >> > I think the first should be changed to a check for ns_capable() to
> >> > the victim's user_ns.  Something like 
> >> > 
> >> > --- a/security/device_cgroup.c
> >> > +++ b/security/device_cgroup.c
> >> > @@ -70,10 +70,16 @@ static int devcgroup_can_attach(struct cgroup *new_cgrp,
> >> >                                 struct cgroup_taskset *set)
> >> >  {
> >> >         struct task_struct *task = cgroup_taskset_first(set);
> >> > +       struct user_namespace *ns;
> >> > +       int ret = -EPERM;
> >> > 
> >> > -       if (current != task && !capable(CAP_SYS_ADMIN))
> >> > -               return -EPERM;
> >> > -       return 0;
> >> > +       if (current == task)
> >> > +               return 0;
> >> > +
> >> > +       ns = userns_get(task);;
> >> > +       ret = ns_capable(ns, CAP_SYS_ADMIN) ? 0 : -EPERM;
> >> > +       put_user_ns(ns);
> >> > +       return ret;
> >> >  }
> >> 
> >> wouldn't this allow a userns root to move a task in the same userns into
> >> a parent cgroup? I believe than anything but moving down the hierarchy
> >> would be very complicated to verify (how far up can you go).
> >
> > But only if they are able to open the tasks file for writing, which
> > they shouldn't be able to do, right?
> 
> That should be looked at very closely.  There are some funny exploits of
> setuid root applications writing to files that have required some
> additional permission checks on /proc/<pid>/uid_map.  I think the
> cgroups files may be vulnerable to some of the same kind of exploits.
> 
> Certainly we should be verifying that the opener of the file had the
> capabilities we are trying to use to avoid being open to those kinds of
> problems.
> 
> I am trying to see the utilitity of the proposed patch.  It doesn't
> allow mknod.  So what is the benefit of having the user namespace bits?

I'm still thinking through it, which is why I haven't sent a real
patch.  What I'm working on is the unprivileged startup of a container.
Right now most things are not allowed in a private user ns, so device
cgroup is not as useful.  But it should be possible eventually to use
block devices, which the original unprivileged user owned, by chowning
the blockdev to a user mapped into the target userns.

The unprivileged user may want to use devices cgroup so he can chown
the loop file into the container, but only allow read-only mounts, for
instance.

> Is the point to allow the userns root to remove access to selected
> devices from it's children even if the DAC permissions would allow the
> access?

Yes I think that's it - except userns root before forking the container
init (and venturing into the really untrusted category).

...

> That said I haven't looked at open or mknod, and usually we are talking
> about calls that aren't made by suid apps so I think there is a fair
> chance that dropping some of those permissions could cause issues.
> The first danger that crosses my mind is what happens if you remove
> access to /dev/tty from a normal application that would trying and log
> strange goings on to a user if they could.

If they were going to do that over tty, that would be to the malicious
user anyway, so that should just either be ignored, or result in the
program exiting early.

> Shrug mostly I don't see the advantage of this change.

It's also possible that this will end up being worked around by the new
(not-yet-designed) interface/library which Tejun wants people to use,
sitting above the cgroupfs.  At least at a first layer.

Anyway this isn't urgent, as it's not in the way for general unprivileged
container creation.  But in general if we don't need the check to be
capable(), it would be better to introduce the right check.

-serge

Re: [PATCH 0/4] Rebase device_cgroup v2 patchset

[Serge E. Hallyn]

Quoting Serge E. Hallyn (serge-A9i7LUbDfNHQT0dZR+AlfA@public.gmane.org):
> Quoting Eric W. Biederman (ebiederm-aS9lmoZGLiVWk0Htik3J/w@public.gmane.org):
> > Serge Hallyn <serge.hallyn-GeWIH/nMZzLQT0dZR+AlfA@public.gmane.org> writes:
> > 
> > > Quoting Aristeu Rozanski (aris-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org):
> > >> On Tue, May 14, 2013 at 10:05:39AM -0500, Serge Hallyn wrote:
> > >> > so now that the device cgroup properly respects hierarchy, not allowing
> > >> > a cgroup to be given greater permission than its parent, should we consider
> > >> > relaxing the capability checks?
> > >> > 
> > >> > There are two capable(CAP_SYS_ADMIN) checks in deice_cgroup.c: one in
> > >> > devcgroup_can_attach() to protect changing another task's cgroup, and
> > >> > one in devcgroup_update_access() to protect writes to the devices.allow
> > >> > and devices.deny files.
> > >> > 
> > >> > I think the first should be changed to a check for ns_capable() to
> > >> > the victim's user_ns.  Something like 
> > >> > 
> > >> > --- a/security/device_cgroup.c
> > >> > +++ b/security/device_cgroup.c
> > >> > @@ -70,10 +70,16 @@ static int devcgroup_can_attach(struct cgroup *new_cgrp,
> > >> >                                 struct cgroup_taskset *set)
> > >> >  {
> > >> >         struct task_struct *task = cgroup_taskset_first(set);
> > >> > +       struct user_namespace *ns;
> > >> > +       int ret = -EPERM;
> > >> > 
> > >> > -       if (current != task && !capable(CAP_SYS_ADMIN))
> > >> > -               return -EPERM;
> > >> > -       return 0;
> > >> > +       if (current == task)
> > >> > +               return 0;
> > >> > +
> > >> > +       ns = userns_get(task);;
> > >> > +       ret = ns_capable(ns, CAP_SYS_ADMIN) ? 0 : -EPERM;
> > >> > +       put_user_ns(ns);
> > >> > +       return ret;
> > >> >  }
> > >> 
> > >> wouldn't this allow a userns root to move a task in the same userns into
> > >> a parent cgroup? I believe than anything but moving down the hierarchy
> > >> would be very complicated to verify (how far up can you go).
> > >
> > > But only if they are able to open the tasks file for writing, which
> > > they shouldn't be able to do, right?
> > 
> > That should be looked at very closely.  There are some funny exploits of
> > setuid root applications writing to files that have required some
> > additional permission checks on /proc/<pid>/uid_map.  I think the
> > cgroups files may be vulnerable to some of the same kind of exploits.
> > 
> > Certainly we should be verifying that the opener of the file had the
> > capabilities we are trying to use to avoid being open to those kinds of
> > problems.
> > 
> > I am trying to see the utilitity of the proposed patch.  It doesn't
> > allow mknod.  So what is the benefit of having the user namespace bits?
> 
> I'm still thinking through it, which is why I haven't sent a real
> patch.  What I'm working on is the unprivileged startup of a container.
> Right now most things are not allowed in a private user ns, so device
> cgroup is not as useful.  But it should be possible eventually to use
> block devices, which the original unprivileged user owned, by chowning
> the blockdev to a user mapped into the target userns.
> 
> The unprivileged user may want to use devices cgroup so he can chown
> the loop file into the container, but only allow read-only mounts, for
> instance.
> 
> > Is the point to allow the userns root to remove access to selected
> > devices from it's children even if the DAC permissions would allow the
> > access?
> 
> Yes I think that's it - except userns root before forking the container
> init (and venturing into the really untrusted category).
> 
> ...
> 
> > That said I haven't looked at open or mknod, and usually we are talking
> > about calls that aren't made by suid apps so I think there is a fair
> > chance that dropping some of those permissions could cause issues.
> > The first danger that crosses my mind is what happens if you remove
> > access to /dev/tty from a normal application that would trying and log
> > strange goings on to a user if they could.
> 
> If they were going to do that over tty, that would be to the malicious
> user anyway, so that should just either be ignored, or result in the
> program exiting early.
> 
> > Shrug mostly I don't see the advantage of this change.
> 
> It's also possible that this will end up being worked around by the new
> (not-yet-designed) interface/library which Tejun wants people to use,
> sitting above the cgroupfs.  At least at a first layer.
> 
> Anyway this isn't urgent, as it's not in the way for general unprivileged
> container creation.  But in general if we don't need the check to be
> capable(), it would be better to introduce the right check.
> 
> -serge

I'm terribly sorry, Andrew, I have no idea how that address for you got
into my address book.  (Corrected)  fwiw the thread can be followed at
https://lkml.org/lkml/2013/5/14/363 .

-serge