On 20/09/2016 02:30, Alexei Starovoitov wrote:

On Tue, Sep 20, 2016 at 12:49:13AM +0200, Mickaël Salaün wrote:

quoted

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.

quoted

-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().

quoted

-	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