On 20/09/2016 02:30, Alexei Starovoitov wrote:
> On Tue, Sep 20, 2016 at 12:49:13AM +0200, Mickaël Salaün wrote:
>> Add security access check for cgroup backed FD. The "cgroup.procs" file
>> of the corresponding cgroup should be readable to identify the cgroup,
>> and writable to prove that the current process can manage this cgroup
>> (e.g. through delegation). This is similar to the check done by
>> cgroup_procs_write_permission().
>>
>> Fixes: 4ed8ec521ed5 ("cgroup: bpf: Add BPF_MAP_TYPE_CGROUP_ARRAY")
> 
> I don't understand what 'fixes' is about.
> Looks like new feature or tightening?
> Since cgroup was opened by the process and it got an fd,
> it had an access, so extra check here looks unnecessary.

It may not be a "fix", but this patch tighten the access control. The
current cgroup_get_from_fd() only rely on the access check done on the
passed FD. However, this FD come from a cgroup directory, not a
"cgroup.procs" (in this directory). The "cgroup.procs" is used for
cgroup delegation by cgroup_procs_write_permission(). Checking
"cgroup.procs" is then more consistent with access checks done by other
part of the cgroup code. Being able to open a cgroup directory only
means that the current process is able to list the cgroup hierarchy, not
necessarily to list the tasks in this cgroups.

A BPF_MAP_TYPE_CGROUP_ARRAY should then only contains cgroups readable
by the process that filled the map. It is currently possible to call
bpf_skb_in_cgroup() and know if a packet come from a task in a cgroup,
whereas the loading process may not be able to list this tasks.

Write access to a cgroup directory means to be able to create
sub-cgroups, not to add or remove tasks from that cgroup. This will be
important for future use like the Daniel Mack's patch (attach an eBPF
program to a cgroup). Indeed, with the current code, a process with
CAP_NET_ADMIN (but without the right to manage a cgroup) would be able
to attach programs to a cgroup. Similar thing goes for Landlock.

> 
>> -struct cgroup *cgroup_get_from_fd(int fd)
>> +struct cgroup *cgroup_get_from_fd(int fd, int access_mask)
>>  {
>>      struct cgroup_subsys_state *css;
>>      struct cgroup *cgrp;
>>      struct file *f;
>> +    struct inode *inode;
>> +    int ret;
>>  
>>      f = fget_raw(fd);
>>      if (!f)
>>              return ERR_PTR(-EBADF);
>>  
>>      css = css_tryget_online_from_dir(f->f_path.dentry, NULL);
>> -    fput(f);
> 
> why move it down?

Because it is used by kernfs_get_inode().

> 
>> -    if (IS_ERR(css))
>> -            return ERR_CAST(css);
>> +    if (IS_ERR(css)) {
>> +            ret = PTR_ERR(css);
>> +            goto put_f;
>> +    }
>>  
>>      cgrp = css->cgroup;
>>      if (!cgroup_on_dfl(cgrp)) {
>> -            cgroup_put(cgrp);
>> -            return ERR_PTR(-EBADF);
>> +            ret = -EBADF;
>> +            goto put_cgrp;
>> +    }
>> +
>> +    ret = -ENOMEM;
>> +    inode = kernfs_get_inode(f->f_path.dentry->d_sb, cgrp->procs_file.kn);
>> +    if (inode) {
>> +            ret = inode_permission(inode, access_mask);
>> +            iput(inode);
>>      }
>> +    if (ret)
>> +            goto put_cgrp;
>>  
>> +    fput(f);
>>      return cgrp;
>> +
>> +put_cgrp:
>> +    cgroup_put(cgrp);
>> +put_f:
>> +    fput(f);
>> +    return ERR_PTR(ret);
>>  }
>>  EXPORT_SYMBOL_GPL(cgroup_get_from_fd);
>>  
>> -- 
>> 2.9.3
>>
> 

Attachment: signature.asc
Description: OpenPGP digital signature

Reply via email to