FAILED: patch "[PATCH] cgroup-v1: Correct privileges check in release_agent writes" failed to apply to 5.10-stable tree
Subsystems (2)
[email protected]
maintainerThe 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.
kernel/cgroup/cgroup-v1.c +4 -2
@@ -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);
Michal Koutný
maintainerHello.
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
9940314ebfc6Linux 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
Greg KH
maintainerQuoted 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
9940314ebfc6Linux 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 :)
Michal Koutný
maintainerPATCH 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.
--- 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
@@ -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
@@ -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
Michal Koutný
maintainerPATCH 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.
--- 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
@@ -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
@@ -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
Michal Koutný
maintainerPATCH 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.
--- kernel/cgroup/cgroup-v1.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-)
kernel/cgroup/cgroup-v1.c +4 -2
@@ -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
Greg KH
maintainerRe: [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
0d2b5955b36250a9428c832664f2079cbf723becupstream.
of->privis currently used by each interface file implementation to store private information. This patch collects the current two private data usages into structcgroup_file_ctxwhich 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_procsiterator is now embedded as procs.iter in the newcgroup_file_ctxso that it doesn't need to be allocated and freed separately.v2: union dropped from
cgroup_file_ctxand the procs iterator is embedded in
cgroup_file_ctxas suggested by Linus.v3: Michal pointed out that cgroup1's procs pidlist uses
of->privtoo.
Converted. Didn't change to embedded allocation as cgroup1 pidlists get stored for caching.
Quoted 6 lines
Signed-off-by Tejun Heo <[email protected]>Cc Linus Torvalds <[email protected]>Reviewed-by Michal Koutný <[email protected]>[mkoutny: v5.10: modify cgroup.pressure handlers, adjust context]Signed-off-by Michal Koutný <[email protected]>---
All now queued up, thanks.
greg k-h
Michal Koutný
maintainerQuoted 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