* [PATCH for 4.4.y-cip] cgroup-v1: Require capabilities to set release_agent
@ 2022-02-15 23:40 ` Masami Ichikawa
0 siblings, 0 replies; 5+ messages in thread
From: Masami Ichikawa @ 2022-02-15 23:40 UTC (permalink / raw)
To: cip-dev
Cc: Eric W. Biederman, Tabitha Sable, stable, Tejun Heo,
Michal Koutný,
Greg Kroah-Hartman, Masami Ichikawa
From: "Eric W. Biederman" <ebiederm@xmission.com>
commit 24f6008564183aa120d07c03d9289519c2fe02af upstream.
The cgroup release_agent is called with call_usermodehelper. The function
call_usermodehelper starts the release_agent with a full set fo capabilities.
Therefore require capabilities when setting the release_agaent.
Reported-by: Tabitha Sable <tabitha.c.sable@gmail.com>
Tested-by: Tabitha Sable <tabitha.c.sable@gmail.com>
Fixes: 81a6a5cdd2c5 ("Task Control Groups: automatic userspace notification of idle cgroups")
Cc: stable@vger.kernel.org # v2.6.24+
Signed-off-by: "Eric W. Biederman" <ebiederm@xmission.com>
Signed-off-by: Tejun Heo <tj@kernel.org>
[mkoutny: Adjust for pre-fs_context, duplicate mount/remount check, drop log messages.]
Acked-by: Michal Koutný <mkoutny@suse.com>
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
[masami: Backport patch from 4.9. Adjust to use current_user_ns() to get current user_ns.
Fix conflict in cgroup_release_agent_write().]
Reference: CVE-2022-0492
Signed-off-by: Masami Ichikawa(CIP) <masami.ichikawa@cybertrust.co.jp>
---
kernel/cgroup.c | 25 +++++++++++++++++++++++++
1 file changed, 25 insertions(+)
diff --git a/kernel/cgroup.c b/kernel/cgroup.c
index 1f5e7dcbfd40..af521a3da21c 100644
--- a/kernel/cgroup.c
+++ b/kernel/cgroup.c
@@ -1786,6 +1786,13 @@ static int cgroup_remount(struct kernfs_root *kf_root, int *flags, char *data)
pr_warn("option changes via remount are deprecated (pid=%d comm=%s)\n",
task_tgid_nr(current), current->comm);
+ /* See cgroup_mount release_agent handling */
+ if (opts.release_agent &&
+ ((current_user_ns() != &init_user_ns) || !capable(CAP_SYS_ADMIN))) {
+ ret = -EINVAL;
+ goto out_unlock;
+ }
+
added_mask = opts.subsys_mask & ~root->subsys_mask;
removed_mask = root->subsys_mask & ~opts.subsys_mask;
@@ -2135,6 +2142,16 @@ static struct dentry *cgroup_mount(struct file_system_type *fs_type,
goto out_unlock;
}
+ /*
+ * Release agent gets called with all capabilities,
+ * require capabilities to set release agent.
+ */
+ if (opts.release_agent &&
+ ((current_user_ns() != &init_user_ns) || !capable(CAP_SYS_ADMIN))) {
+ ret = -EINVAL;
+ goto out_unlock;
+ }
+
root = kzalloc(sizeof(*root), GFP_KERNEL);
if (!root) {
ret = -ENOMEM;
@@ -2839,6 +2856,14 @@ static ssize_t cgroup_release_agent_write(struct kernfs_open_file *of,
BUILD_BUG_ON(sizeof(cgrp->root->release_agent_path) < PATH_MAX);
+ /*
+ * 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))
+ return -EPERM;
+
cgrp = cgroup_kn_lock_live(of->kn);
if (!cgrp)
return -ENODEV;
--
2.35.1
^ permalink raw reply related [flat|nested] 5+ messages in thread
* [PATCH for 4.4.y-cip] cgroup-v1: Require capabilities to set release_agent
@ 2022-02-15 23:40 ` Masami Ichikawa
0 siblings, 0 replies; 5+ messages in thread
From: Masami Ichikawa @ 2022-02-15 23:40 UTC (permalink / raw)
To: cip-dev
Cc: Eric W. Biederman, Tabitha Sable, stable, Tejun Heo,
Michal Koutný,
Greg Kroah-Hartman, Masami Ichikawa
From: "Eric W. Biederman" <ebiederm@xmission.com>
commit 24f6008564183aa120d07c03d9289519c2fe02af upstream.
The cgroup release_agent is called with call_usermodehelper. The function
call_usermodehelper starts the release_agent with a full set fo capabilities.
Therefore require capabilities when setting the release_agaent.
Reported-by: Tabitha Sable <tabitha.c.sable@gmail.com>
Tested-by: Tabitha Sable <tabitha.c.sable@gmail.com>
Fixes: 81a6a5cdd2c5 ("Task Control Groups: automatic userspace notification of idle cgroups")
Cc: stable@vger.kernel.org # v2.6.24+
Signed-off-by: "Eric W. Biederman" <ebiederm@xmission.com>
Signed-off-by: Tejun Heo <tj@kernel.org>
[mkoutny: Adjust for pre-fs_context, duplicate mount/remount check, drop log messages.]
Acked-by: Michal Koutný <mkoutny@suse.com>
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
[masami: Backport patch from 4.9. Adjust to use current_user_ns() to get current user_ns.
Fix conflict in cgroup_release_agent_write().]
Reference: CVE-2022-0492
Signed-off-by: Masami Ichikawa(CIP) <masami.ichikawa@cybertrust.co.jp>
---
kernel/cgroup.c | 25 +++++++++++++++++++++++++
1 file changed, 25 insertions(+)
diff --git a/kernel/cgroup.c b/kernel/cgroup.c
index 1f5e7dcbfd40..af521a3da21c 100644
--- a/kernel/cgroup.c
+++ b/kernel/cgroup.c
@@ -1786,6 +1786,13 @@ static int cgroup_remount(struct kernfs_root *kf_root, int *flags, char *data)
pr_warn("option changes via remount are deprecated (pid=%d comm=%s)\n",
task_tgid_nr(current), current->comm);
+ /* See cgroup_mount release_agent handling */
+ if (opts.release_agent &&
+ ((current_user_ns() != &init_user_ns) || !capable(CAP_SYS_ADMIN))) {
+ ret = -EINVAL;
+ goto out_unlock;
+ }
+
added_mask = opts.subsys_mask & ~root->subsys_mask;
removed_mask = root->subsys_mask & ~opts.subsys_mask;
@@ -2135,6 +2142,16 @@ static struct dentry *cgroup_mount(struct file_system_type *fs_type,
goto out_unlock;
}
+ /*
+ * Release agent gets called with all capabilities,
+ * require capabilities to set release agent.
+ */
+ if (opts.release_agent &&
+ ((current_user_ns() != &init_user_ns) || !capable(CAP_SYS_ADMIN))) {
+ ret = -EINVAL;
+ goto out_unlock;
+ }
+
root = kzalloc(sizeof(*root), GFP_KERNEL);
if (!root) {
ret = -ENOMEM;
@@ -2839,6 +2856,14 @@ static ssize_t cgroup_release_agent_write(struct kernfs_open_file *of,
BUILD_BUG_ON(sizeof(cgrp->root->release_agent_path) < PATH_MAX);
+ /*
+ * 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))
+ return -EPERM;
+
cgrp = cgroup_kn_lock_live(of->kn);
if (!cgrp)
return -ENODEV;
--
2.35.1
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH for 4.4.y-cip] cgroup-v1: Require capabilities to set release_agent
2022-02-15 23:40 ` Masami Ichikawa
(?)
@ 2022-02-16 12:11 ` Michal Koutný
2022-02-17 2:17 ` Masami Ichikawa
-1 siblings, 1 reply; 5+ messages in thread
From: Michal Koutný @ 2022-02-16 12:11 UTC (permalink / raw)
To: Masami Ichikawa
Cc: cip-dev, Eric W. Biederman, Tabitha Sable, stable, Tejun Heo,
Greg Kroah-Hartman, Masami Ichikawa
Thanks for sharing this Masami, I've been looking into pre-cgroup_ns
backport too.
On Wed, Feb 16, 2022 at 08:40:37AM +0900, Masami Ichikawa <masami256@gmail.com> wrote:
> [masami: Backport patch from 4.9. Adjust to use current_user_ns() to get current user_ns.
> Fix conflict in cgroup_release_agent_write().]
The condition to allow modifying release_agent is two-fold:
a) caller is capabable(CAP_SYS_ADMIN),
b) cgroup_ns is owned by init_user_ns.
In pre-cgroup_ns kernels, it is IMO safer to consider all (=the only)
cgroup_ns owned by init_user_ns.
So the (positive) condition translates into capable(CAP_SYS_ADMIN) only.
[
Additionally, there's invariant/implication
capable(CAP_XXX) -> (current_user_ns() == &init_user_ns) ,
so the expression
(current_user_ns() != &init_user_ns) || !capable(CAP_SYS_ADMIN)
simplifies to
!capable(CAP_SYS_ADMIN) .
]
> @@ -2839,6 +2856,14 @@ static ssize_t cgroup_release_agent_write(struct kernfs_open_file *of,
>
> BUILD_BUG_ON(sizeof(cgrp->root->release_agent_path) < PATH_MAX);
>
> + /*
> + * 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))
> + return -EPERM;
> +
Following the reasoning above, the check simplifies too but it should be
be against the opener, not the writer:
file_ns_capable(of->file, &init_user_ns, CAP_SYS_ADMIN)
(It seems to be to be incorrect even in the original commit.
So I'll send a patch there to rectify that.)
Michal
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH for 4.4.y-cip] cgroup-v1: Require capabilities to set release_agent
2022-02-16 12:11 ` Michal Koutný
@ 2022-02-17 2:17 ` Masami Ichikawa
0 siblings, 0 replies; 5+ messages in thread
From: Masami Ichikawa @ 2022-02-17 2:17 UTC (permalink / raw)
To: Michal Koutný
Cc: cip-dev, Eric W. Biederman, Tabitha Sable, stable, Tejun Heo,
Greg Kroah-Hartman, Masami Ichikawa
On Wed, Feb 16, 2022 at 9:11 PM Michal Koutný <mkoutny@suse.com> wrote:
>
> Thanks for sharing this Masami, I've been looking into pre-cgroup_ns
> backport too.
>
Thank you for the review.
> On Wed, Feb 16, 2022 at 08:40:37AM +0900, Masami Ichikawa <masami256@gmail.com> wrote:
> > [masami: Backport patch from 4.9. Adjust to use current_user_ns() to get current user_ns.
> > Fix conflict in cgroup_release_agent_write().]
>
> The condition to allow modifying release_agent is two-fold:
> a) caller is capabable(CAP_SYS_ADMIN),
> b) cgroup_ns is owned by init_user_ns.
>
> In pre-cgroup_ns kernels, it is IMO safer to consider all (=the only)
> cgroup_ns owned by init_user_ns.
>
> So the (positive) condition translates into capable(CAP_SYS_ADMIN) only.
>
I see. Thank you for the details.
If we treat cgroup_ns as only cgroup_ns that are owned by
init_user_ns, we don't have to have an extra check.
> [
> Additionally, there's invariant/implication
> capable(CAP_XXX) -> (current_user_ns() == &init_user_ns) ,
> so the expression
> (current_user_ns() != &init_user_ns) || !capable(CAP_SYS_ADMIN)
> simplifies to
> !capable(CAP_SYS_ADMIN) .
> ]
>
>
> > @@ -2839,6 +2856,14 @@ static ssize_t cgroup_release_agent_write(struct kernfs_open_file *of,
> >
> > BUILD_BUG_ON(sizeof(cgrp->root->release_agent_path) < PATH_MAX);
> >
> > + /*
> > + * 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))
> > + return -EPERM;
> > +
>
> Following the reasoning above, the check simplifies too but it should be
> be against the opener, not the writer:
> file_ns_capable(of->file, &init_user_ns, CAP_SYS_ADMIN)
>
Thanks. I got it.
> (It seems to be to be incorrect even in the original commit.
> So I'll send a patch there to rectify that.)
>
>
> Michal
>
>
Regards,
--
/**
* Masami Ichikawa
* personal: masami256@gmail.com
* fedora project: masami@fedoraproject.org
*/
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH for 4.4.y-cip] cgroup-v1: Require capabilities to set release_agent
@ 2022-02-17 2:17 ` Masami Ichikawa
0 siblings, 0 replies; 5+ messages in thread
From: Masami Ichikawa @ 2022-02-17 2:17 UTC (permalink / raw)
To: Michal Koutný
Cc: cip-dev, Eric W. Biederman, Tabitha Sable, stable, Tejun Heo,
Greg Kroah-Hartman, Masami Ichikawa
On Wed, Feb 16, 2022 at 9:11 PM Michal Koutný <mkoutny@suse.com> wrote:
>
> Thanks for sharing this Masami, I've been looking into pre-cgroup_ns
> backport too.
>
Thank you for the review.
> On Wed, Feb 16, 2022 at 08:40:37AM +0900, Masami Ichikawa <masami256@gmail.com> wrote:
> > [masami: Backport patch from 4.9. Adjust to use current_user_ns() to get current user_ns.
> > Fix conflict in cgroup_release_agent_write().]
>
> The condition to allow modifying release_agent is two-fold:
> a) caller is capabable(CAP_SYS_ADMIN),
> b) cgroup_ns is owned by init_user_ns.
>
> In pre-cgroup_ns kernels, it is IMO safer to consider all (=the only)
> cgroup_ns owned by init_user_ns.
>
> So the (positive) condition translates into capable(CAP_SYS_ADMIN) only.
>
I see. Thank you for the details.
If we treat cgroup_ns as only cgroup_ns that are owned by
init_user_ns, we don't have to have an extra check.
> [
> Additionally, there's invariant/implication
> capable(CAP_XXX) -> (current_user_ns() == &init_user_ns) ,
> so the expression
> (current_user_ns() != &init_user_ns) || !capable(CAP_SYS_ADMIN)
> simplifies to
> !capable(CAP_SYS_ADMIN) .
> ]
>
>
> > @@ -2839,6 +2856,14 @@ static ssize_t cgroup_release_agent_write(struct kernfs_open_file *of,
> >
> > BUILD_BUG_ON(sizeof(cgrp->root->release_agent_path) < PATH_MAX);
> >
> > + /*
> > + * 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))
> > + return -EPERM;
> > +
>
> Following the reasoning above, the check simplifies too but it should be
> be against the opener, not the writer:
> file_ns_capable(of->file, &init_user_ns, CAP_SYS_ADMIN)
>
Thanks. I got it.
> (It seems to be to be incorrect even in the original commit.
> So I'll send a patch there to rectify that.)
>
>
> Michal
>
>
Regards,
--
/**
* Masami Ichikawa
* personal: masami256@gmail.com
* fedora project: masami@fedoraproject.org
*/
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2022-02-17 2:17 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-02-15 23:40 [PATCH for 4.4.y-cip] cgroup-v1: Require capabilities to set release_agent Masami Ichikawa
2022-02-15 23:40 ` Masami Ichikawa
2022-02-16 12:11 ` Michal Koutný
2022-02-17 2:17 ` Masami Ichikawa
2022-02-17 2:17 ` Masami Ichikawa
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.