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
