Skip to contentSkip to conversation

FAILED: patch "[PATCH] cgroup-v1: Correct privileges check in release_agent writes" failed to apply to 5.10-stable tree

PATCH In review [email protected] 2 participants 8 messages Feb 23, 2022 – Mar 24, 2022

Subsystems (2)
lore
Share & tools
lore

[email protected]

author maintainer
#0 lore raw

The patch below does not apply to the 5.10-stable tree.
If someone wants it applied there, or to any other stable or longterm tree, then please email the backport, including the original git commit id to <[email protected]>.

thanks,

greg k-h

------------------ original commit in Linus's tree ------------------

From 467a726b754f474936980da793b4ff2ec3e382a7 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Michal=20Koutn=C3=BD?= <[email protected]>
Date: Thu, 17 Feb 2022 17:11:28 +0100
Subject: [PATCH] cgroup-v1: Correct privileges check in release_agent writes
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

The idea is to check: a) the owning user_ns of cgroup_ns, b) capabilities in init_user_ns.

The commit 24f600856418 ("cgroup-v1: Require capabilities to set release_agent") got this wrong in the write handler of release_agent since it checked user_ns of the opener (may be different from the owning user_ns of cgroup_ns).
Secondly, to avoid possibly confused deputy, the capability of the opener must be checked.

Fixes 24f600856418 ("cgroup-v1: Require capabilities to set release_agent")
kernel/cgroup/cgroup-v1.c +4 -2
index 0e877dbcfeea..afc6c0e9c966 100644
@@ -546,6 +546,7 @@ static ssize_t cgroup_release_agent_write(struct kernfs_open_file *of,
 					  char *buf, size_t nbytes, loff_t off)
 {
 	struct cgroup *cgrp;
+	struct cgroup_file_ctx *ctx;
 
 	BUILD_BUG_ON(sizeof(cgrp->root->release_agent_path) < PATH_MAX);
 
@@ -553,8 +554,9 @@ static ssize_t cgroup_release_agent_write(struct kernfs_open_file *of,
 	 * Release agent gets called with all capabilities,
 	 * require capabilities to set release agent.
 	 */
-	if ((of->file->f_cred->user_ns != &init_user_ns) ||
-	    !capable(CAP_SYS_ADMIN))
+	ctx = of->priv;
+	if ((ctx->ns->user_ns != &init_user_ns) ||
+	    !file_ns_capable(of->file, &init_user_ns, CAP_SYS_ADMIN))
 		return -EPERM;
 
 	cgrp = cgroup_kn_lock_live(of->kn, false);
maintainer
#1 lore raw

Hello.

Quoted 1 line · On Wed, Feb 23, 2022 at 07:07:12PM +0100, [email protected] wrote:

The patch below does not apply to the 5.10-stable tree.

What do you mean does not apply?

Quoted 6 lines

HEAD is now at 9940314ebfc6 Linux 5.10.108

$ git format-patch -o /tmp/ 467a726b754f474936980da793b4ff2ec3e382a7 -1
/tmp/0001-cgroup-v1-Correct-privileges-check-in-release_agent-.patch
$ git am /tmp/0001-cgroup-v1-Correct-privileges-check-in-release_agent-.patch
Applying: cgroup-v1: Correct privileges check in release_agent writes
$

Thanks,
Michal

author maintainer
#2 lore raw
Quoted 13 lines · Michal Koutný

Hello.

Quoted 1 line · On Wed, Feb 23, 2022 at 07:07:12PM +0100, [email protected] wrote:

The patch below does not apply to the 5.10-stable tree.

What do you mean does not apply?

Quoted 6 lines

HEAD is now at 9940314ebfc6 Linux 5.10.108

$ git format-patch -o /tmp/ 467a726b754f474936980da793b4ff2ec3e382a7 -1
/tmp/0001-cgroup-v1-Correct-privileges-check-in-release_agent-.patch
$ git am /tmp/0001-cgroup-v1-Correct-privileges-check-in-release_agent-.patch
Applying: cgroup-v1: Correct privileges check in release_agent writes
$

Sorry, yes, it applies, but it breaks the build. Try typing 'make' now :)

maintainer
#3 lore raw

PATCH 1/3 cgroup: Allocate cgroup_file_ctx for kernfs_open_file->priv

From: Tejun Heo <[email protected]>

commit 0d2b5955b36250a9428c832664f2079cbf723bec upstream.

of->priv is currently used by each interface file implementation to store private information. This patch collects the current two private data usages into struct cgroup_file_ctx which is allocated and freed by the common path. This allows generic private data which applies to multiple files, which will be used to in the following patch.

Note that cgroup_procs iterator is now embedded as procs.iter in the new cgroup_file_ctx so that it doesn't need to be allocated and freed separately.

v2: union dropped from cgroup_file_ctx and the procs iterator is embedded in
cgroup_file_ctx as suggested by Linus.

v3: Michal pointed out that cgroup1's procs pidlist uses of->priv too.
Converted. Didn't change to embedded allocation as cgroup1 pidlists get stored for caching.

Cc Linus Torvalds <[email protected]>
[mkoutny: v5.10: modify cgroup.pressure handlers, adjust context]
---
 kernel/cgroup/cgroup-internal.h | 17 +++++++++++
 kernel/cgroup/cgroup-v1.c       | 26 ++++++++--------
 kernel/cgroup/cgroup.c          | 54 +++++++++++++++++++++------------
 3 files changed, 65 insertions(+), 32 deletions(-)
kernel/cgroup/cgroup-internal.h +17 -0
index bfbeabc17a9d..cf637bc4ab45 100644
@@ -65,6 +65,23 @@ static inline struct cgroup_fs_context *cgroup_fc2context(struct fs_context *fc)
 	return container_of(kfc, struct cgroup_fs_context, kfc);
 }
 
+struct cgroup_pidlist;
+
+struct cgroup_file_ctx {
+	struct {
+		void			*trigger;
+	} psi;
+
+	struct {
+		bool			started;
+		struct css_task_iter	iter;
+	} procs;
+
+	struct {
+		struct cgroup_pidlist	*pidlist;
+	} procs1;
+};
+
 /*
  * A cgroup can be associated with multiple css_sets as different tasks may
  * belong to different cgroups on different hierarchies.  In the other
kernel/cgroup/cgroup-v1.c +14 -12
index 69fba563c810..40b8d4e4a810 100644
@@ -393,6 +393,7 @@ static void *cgroup_pidlist_start(struct seq_file *s, loff_t *pos)
 	 * next pid to display, if any
 	 */
 	struct kernfs_open_file *of = s->private;
+	struct cgroup_file_ctx *ctx = of->priv;
 	struct cgroup *cgrp = seq_css(s)->cgroup;
 	struct cgroup_pidlist *l;
 	enum cgroup_filetype type = seq_cft(s)->private;
@@ -402,25 +403,24 @@ static void *cgroup_pidlist_start(struct seq_file *s, loff_t *pos)
 	mutex_lock(&cgrp->pidlist_mutex);
 
 	/*
-	 * !NULL @of->priv indicates that this isn't the first start()
-	 * after open.  If the matching pidlist is around, we can use that.
-	 * Look for it.  Note that @of->priv can't be used directly.  It
-	 * could already have been destroyed.
+	 * !NULL @ctx->procs1.pidlist indicates that this isn't the first
+	 * start() after open. If the matching pidlist is around, we can use
+	 * that. Look for it. Note that @ctx->procs1.pidlist can't be used
+	 * directly. It could already have been destroyed.
 	 */
-	if (of->priv)
-		of->priv = cgroup_pidlist_find(cgrp, type);
+	if (ctx->procs1.pidlist)
+		ctx->procs1.pidlist = cgroup_pidlist_find(cgrp, type);
 
 	/*
 	 * Either this is the first start() after open or the matching
 	 * pidlist has been destroyed inbetween.  Create a new one.
 	 */
-	if (!of->priv) {
-		ret = pidlist_array_load(cgrp, type,
-					 (struct cgroup_pidlist **)&of->priv);
+	if (!ctx->procs1.pidlist) {
+		ret = pidlist_array_load(cgrp, type, &ctx->procs1.pidlist);
 		if (ret)
 			return ERR_PTR(ret);
 	}
-	l = of->priv;
+	l = ctx->procs1.pidlist;
 
 	if (pid) {
 		int end = l->length;
@@ -448,7 +448,8 @@ static void *cgroup_pidlist_start(struct seq_file *s, loff_t *pos)
 static void cgroup_pidlist_stop(struct seq_file *s, void *v)
 {
 	struct kernfs_open_file *of = s->private;
-	struct cgroup_pidlist *l = of->priv;
+	struct cgroup_file_ctx *ctx = of->priv;
+	struct cgroup_pidlist *l = ctx->procs1.pidlist;
 
 	if (l)
 		mod_delayed_work(cgroup_pidlist_destroy_wq, &l->destroy_dwork,
@@ -459,7 +460,8 @@ static void cgroup_pidlist_stop(struct seq_file *s, void *v)
 static void *cgroup_pidlist_next(struct seq_file *s, void *v, loff_t *pos)
 {
 	struct kernfs_open_file *of = s->private;
-	struct cgroup_pidlist *l = of->priv;
+	struct cgroup_file_ctx *ctx = of->priv;
+	struct cgroup_pidlist *l = ctx->procs1.pidlist;
 	pid_t *p = v;
 	pid_t *end = l->list + l->length;
 	/*
kernel/cgroup/cgroup.c +34 -20

126 lines not on this page · raw message

Signature · 1 line
2.35.1
maintainer
#4 lore raw

PATCH 2/3 cgroup: Use open-time cgroup namespace for process migration perm checks

From: Tejun Heo <[email protected]>

commit e57457641613fef0d147ede8bd6a3047df588b95 upstream.

cgroup process migration permission checks are performed at write time as whether a given operation is allowed or not is dependent on the content of the write - the PID. This currently uses current's cgroup namespace which is a potential security weakness as it may allow scenarios where a less privileged process tricks a more privileged one into writing into a fd that it created.

This patch makes cgroup remember the cgroup namespace at the time of open and uses it for migration permission checks instad of current's. Note that this only applies to cgroup2 as cgroup1 doesn't have namespace support.

This also fixes a use-after-free bug on cgroupns reported in

https://lore.kernel.org/r/[email protected]

Note that backporting this fix also requires the preceding patch.

Cc Michal Koutný <[email protected]>
Cc Oleg Nesterov <[email protected]>
Reported-by [email protected]
Fixes 5136f6365ce3 ("cgroup: implement "nsdelegate" mount option")
[mkoutny: v5.10: duplicate ns check in procs/threads write handler, adjust context]
---
 kernel/cgroup/cgroup-internal.h |  2 ++
 kernel/cgroup/cgroup.c          | 32 ++++++++++++++++++++++----------
 2 files changed, 24 insertions(+), 10 deletions(-)
kernel/cgroup/cgroup-internal.h +2 -0
index cf637bc4ab45..6e36e854b512 100644
@@ -68,6 +68,8 @@ static inline struct cgroup_fs_context *cgroup_fc2context(struct fs_context *fc)
 struct cgroup_pidlist;
 
 struct cgroup_file_ctx {
+	struct cgroup_namespace	*ns;
+
 	struct {
 		void			*trigger;
 	} psi;
kernel/cgroup/cgroup.c +22 -10
index ddfe6983ea7c..3f8447a5393e 100644
@@ -3700,14 +3700,19 @@ static int cgroup_file_open(struct kernfs_open_file *of)
 	ctx = kzalloc(sizeof(*ctx), GFP_KERNEL);
 	if (!ctx)
 		return -ENOMEM;
+
+	ctx->ns = current->nsproxy->cgroup_ns;
+	get_cgroup_ns(ctx->ns);
 	of->priv = ctx;
 
 	if (!cft->open)
 		return 0;
 
 	ret = cft->open(of);
-	if (ret)
+	if (ret) {
+		put_cgroup_ns(ctx->ns);
 		kfree(ctx);
+	}
 	return ret;
 }
 
@@ -3718,13 +3723,14 @@ static void cgroup_file_release(struct kernfs_open_file *of)
 
 	if (cft->release)
 		cft->release(of);
+	put_cgroup_ns(ctx->ns);
 	kfree(ctx);
 }
 
 static ssize_t cgroup_file_write(struct kernfs_open_file *of, char *buf,
 				 size_t nbytes, loff_t off)
 {
-	struct cgroup_namespace *ns = current->nsproxy->cgroup_ns;
+	struct cgroup_file_ctx *ctx = of->priv;
 	struct cgroup *cgrp = of->kn->parent->priv;
 	struct cftype *cft = of->kn->priv;
 	struct cgroup_subsys_state *css;
@@ -3741,7 +3747,7 @@ static ssize_t cgroup_file_write(struct kernfs_open_file *of, char *buf,
 	 */
 	if ((cgrp->root->flags & CGRP_ROOT_NS_DELEGATE) &&
 	    !(cft->flags & CFTYPE_NS_DELEGATABLE) &&
-	    ns != &init_cgroup_ns && ns->root_cset->dfl_cgrp == cgrp)
+	    ctx->ns != &init_cgroup_ns && ctx->ns->root_cset->dfl_cgrp == cgrp)
 		return -EPERM;
 
 	if (cft->write)
@@ -4726,9 +4732,9 @@ static int cgroup_may_write(const struct cgroup *cgrp, struct super_block *sb)
 
 static int cgroup_procs_write_permission(struct cgroup *src_cgrp,
 					 struct cgroup *dst_cgrp,
-					 struct super_block *sb)
+					 struct super_block *sb,
+					 struct cgroup_namespace *ns)
 {
-	struct cgroup_namespace *ns = current->nsproxy->cgroup_ns;
 	struct cgroup *com_cgrp = src_cgrp;
 	int ret;
 
@@ -4757,11 +4763,12 @@ static int cgroup_procs_write_permission(struct cgroup *src_cgrp,
 
 static int cgroup_attach_permissions(struct cgroup *src_cgrp,
 				     struct cgroup *dst_cgrp,
-				     struct super_block *sb, bool threadgroup)
+				     struct super_block *sb, bool threadgroup,
+				     struct cgroup_namespace *ns)
 {
 	int ret = 0;
 
-	ret = cgroup_procs_write_permission(src_cgrp, dst_cgrp, sb);
+	ret = cgroup_procs_write_permission(src_cgrp, dst_cgrp, sb, ns);
 	if (ret)
 		return ret;
 
@@ -4778,6 +4785,7 @@ static int cgroup_attach_permissions(struct cgroup *src_cgrp,
 static ssize_t cgroup_procs_write(struct kernfs_open_file *of,
 				  char *buf, size_t nbytes, loff_t off)
 {
+	struct cgroup_file_ctx *ctx = of->priv;
 	struct cgroup *src_cgrp, *dst_cgrp;
 	struct task_struct *task;
 	ssize_t ret;
@@ -4798,7 +4806,8 @@ static ssize_t cgroup_procs_write(struct kernfs_open_file *of,
 	spin_unlock_irq(&css_set_lock);
 
 	ret = cgroup_attach_permissions(src_cgrp, dst_cgrp,
-					of->file->f_path.dentry->d_sb, true);
+					of->file->f_path.dentry->d_sb, true,
+					ctx->ns);
 	if (ret)
 		goto out_finish;
 
@@ -4820,6 +4829,7 @@ static void *cgroup_threads_start(struct seq_file *s, loff_t *pos)
 static ssize_t cgroup_threads_write(struct kernfs_open_file *of,
 				    char *buf, size_t nbytes, loff_t off)
 {
+	struct cgroup_file_ctx *ctx = of->priv;
 	struct cgroup *src_cgrp, *dst_cgrp;
 	struct task_struct *task;
 	ssize_t ret;
@@ -4843,7 +4853,8 @@ static ssize_t cgroup_threads_write(struct kernfs_open_file *of,
 
 	/* thread migrations follow the cgroup.procs delegation rule */
 	ret = cgroup_attach_permissions(src_cgrp, dst_cgrp,
-					of->file->f_path.dentry->d_sb, false);
+					of->file->f_path.dentry->d_sb, false,
+					ctx->ns);
 	if (ret)
 		goto out_finish;
 
@@ -6023,7 +6034,8 @@ static int cgroup_css_set_fork(struct kernel_clone_args *kargs)
 		goto err;
 
 	ret = cgroup_attach_permissions(cset->dfl_cgrp, dst_cgrp, sb,
-					!(kargs->flags & CLONE_THREAD));
+					!(kargs->flags & CLONE_THREAD),
+					current->nsproxy->cgroup_ns);
 	if (ret)
 		goto err;
 
Signature · 1 line
2.35.1
maintainer
#5 lore raw

PATCH 3/3 cgroup-v1: Correct privileges check in release_agent writes

In reply to Michal Koutný

commit 467a726b754f474936980da793b4ff2ec3e382a7 upstream.

The idea is to check: a) the owning user_ns of cgroup_ns, b) capabilities in init_user_ns.

The commit 24f600856418 ("cgroup-v1: Require capabilities to set release_agent") got this wrong in the write handler of release_agent since it checked user_ns of the opener (may be different from the owning user_ns of cgroup_ns).
Secondly, to avoid possibly confused deputy, the capability of the opener must be checked.

Fixes 24f600856418 ("cgroup-v1: Require capabilities to set release_agent")
---
 kernel/cgroup/cgroup-v1.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)
kernel/cgroup/cgroup-v1.c +4 -2
index 40b8d4e4a810..8f0ea12d7cee 100644
@@ -544,6 +544,7 @@ static ssize_t cgroup_release_agent_write(struct kernfs_open_file *of,
 					  char *buf, size_t nbytes, loff_t off)
 {
 	struct cgroup *cgrp;
+	struct cgroup_file_ctx *ctx;
 
 	BUILD_BUG_ON(sizeof(cgrp->root->release_agent_path) < PATH_MAX);
 
@@ -551,8 +552,9 @@ static ssize_t cgroup_release_agent_write(struct kernfs_open_file *of,
 	 * Release agent gets called with all capabilities,
 	 * require capabilities to set release agent.
 	 */
-	if ((of->file->f_cred->user_ns != &init_user_ns) ||
-	    !capable(CAP_SYS_ADMIN))
+	ctx = of->priv;
+	if ((ctx->ns->user_ns != &init_user_ns) ||
+	    !file_ns_capable(of->file, &init_user_ns, CAP_SYS_ADMIN))
 		return -EPERM;
 
 	cgrp = cgroup_kn_lock_live(of->kn, false);
Signature · 1 line
2.35.1
author maintainer
#6 lore raw

Re: [PATCH 1/3] cgroup: Allocate cgroup_file_ctx for kernfs_open_file->priv

In reply to Michal Koutný
Quoted 21 lines · Michal Koutný

From: Tejun Heo <[email protected]>

commit 0d2b5955b36250a9428c832664f2079cbf723bec upstream.

of->priv is currently used by each interface file implementation to store private information. This patch collects the current two private data usages into struct cgroup_file_ctx which is allocated and freed by the common path. This allows generic private data which applies to multiple files, which will be used to in the following patch.

Note that cgroup_procs iterator is now embedded as procs.iter in the new cgroup_file_ctx so that it doesn't need to be allocated and freed separately.

v2: union dropped from cgroup_file_ctx and the procs iterator is embedded in
cgroup_file_ctx as suggested by Linus.

v3: Michal pointed out that cgroup1's procs pidlist uses of->priv too.
Converted. Didn't change to embedded allocation as cgroup1 pidlists get stored for caching.

Quoted 6 lines
Cc Linus Torvalds <[email protected]>
[mkoutny: v5.10: modify cgroup.pressure handlers, adjust context]

---

All now queued up, thanks.

greg k-h

maintainer
#7 lore raw
In reply to Greg KH
Quoted 1 line · Greg KH

Sorry, yes, it applies, but it breaks the build. Try typing 'make' now :)

Ah, you're right. There're the prerequisites from the other series. I've sent a group of patches for v5.10 to accomodate for "cgroup-v1: Correct privileges check in release_agent writes" (based on the original patches, I've not tested those).

HTH,
Michal