On Wed, Feb 16, 2022 at 9:11 PM Michal Koutný <mkoutny@xxxxxxxx> 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@xxxxxxxxx> 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@xxxxxxxxx * fedora project: masami@xxxxxxxxxxxxxxxxx */
