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,