| Message ID | 20220215234036.19800-1-masami256@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | [for,4.4.y-cip] cgroup-v1: Require capabilities to set release_agent | expand |
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
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;