Re: [PATCH v1] cgroup,bpf: Add access check for cgroup_get_from_fd()

[MickaÃl SalaÃn]

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