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

[Alexei Starovoitov]

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.

> -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?

> -	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
>