Hello, Andy. On Mon, Oct 17, 2016 at 03:40:37PM -0700, Andy Lutomirski wrote: > > @@ -2856,7 +2856,8 @@ static int cgroup_procs_write_permission(struct > > task_struct *task, > > */ > > if (!uid_eq(cred->euid, GLOBAL_ROOT_UID) && > > !uid_eq(cred->euid, tcred->uid) && > > - !uid_eq(cred->euid, tcred->suid)) > > + !uid_eq(cred->euid, tcred->suid) && > > + !ns_capable(tcred->user_ns, CAP_CGROUP_MIGRATE)) > > ret = -EACCES; > > This logic seems rather confused to me. Without this patch, a user > can write to procs if it's root *or* it matches the target uid *or* it > matches the target suid. How does this make sense? How about > ptrace_may_access(...) || ns_capable(tcred->user_ns, > CAP_CGROUP_MIGRATE)?
Yeah, it's weird. The problem is that there was no delegation model defined on v1 and it used a hybrid of file + ptracey access checks. The goal, I think, was disallowing !root user from pulling in random tasks into a cgroup it has write access to, which was possible because there was no isolation on the delegation boundary. Given how long it has been out in the wild, I don't think changing the logic is a good idea. We should simply replace GLOBAL_ROOT_UID test with CAT_WHATEVER_WE_PICK test and just ignore the whole thing on v2. Thanks. -- tejun
