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

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

[Mickaël Salaün]

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")
Signed-off-by: Mickaël Salaün <mic-WFhQfpSGs3bR7s880joybQ@public.gmane.org>
Cc: Alexei Starovoitov <ast-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
Cc: Andy Lutomirski <luto-kltTT9wpgjJwATOyAt5JVQ@public.gmane.org>
Cc: Daniel Borkmann <daniel-FeC+5ew28dpmcu3hnIyYJQ@public.gmane.org>
Cc: Daniel Mack <daniel-cYrQPVfZoowdnm+yROfE0A@public.gmane.org>
Cc: David S. Miller <davem-fT/PcQaiUtIeIZ0/mPfg9Q@public.gmane.org>
Cc: James Morris <james.l.morris-QHcLZuEGTsvQT0dZR+AlfA@public.gmane.org>
Cc: Kees Cook <keescook-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
Cc: Martin KaFai Lau <kafai-b10kYP2dOMg@public.gmane.org>
Cc: Tejun Heo <tj-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
---
 include/linux/cgroup.h |  2 +-
 kernel/bpf/arraymap.c  |  2 +-
 kernel/bpf/syscall.c   |  1 +
 kernel/cgroup.c        | 34 +++++++++++++++++++++++++++-------
 4 files changed, 30 insertions(+), 9 deletions(-)

diff --git a/include/linux/cgroup.h b/include/linux/cgroup.h
index c4688742ddc4..5767d471e292 100644
--- a/include/linux/cgroup.h
+++ b/include/linux/cgroup.h
@@ -87,7 +87,7 @@ struct cgroup_subsys_state *css_tryget_online_from_dir(struct dentry *dentry,
 						       struct cgroup_subsys *ss);
 
 struct cgroup *cgroup_get_from_path(const char *path);
-struct cgroup *cgroup_get_from_fd(int fd);
+struct cgroup *cgroup_get_from_fd(int fd, int access_mask);
 
 int cgroup_attach_task_all(struct task_struct *from, struct task_struct *);
 int cgroup_transfer_tasks(struct cgroup *to, struct cgroup *from);
diff --git a/kernel/bpf/arraymap.c b/kernel/bpf/arraymap.c
index a2ac051c342f..3d97c70134a0 100644
--- a/kernel/bpf/arraymap.c
+++ b/kernel/bpf/arraymap.c
@@ -543,7 +543,7 @@ static void *cgroup_fd_array_get_ptr(struct bpf_map *map,
 				     struct file *map_file /* not used */,
 				     int fd)
 {
-	return cgroup_get_from_fd(fd);
+	return cgroup_get_from_fd(fd, MAY_READ);
 }
 
 static void cgroup_fd_array_put_ptr(void *ptr)
diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
index 228f962447a5..cc7270eadcf7 100644
--- a/kernel/bpf/syscall.c
+++ b/kernel/bpf/syscall.c
@@ -17,6 +17,7 @@
 #include <linux/license.h>
 #include <linux/filter.h>
 #include <linux/version.h>
+#include <linux/fs.h>
 
 DEFINE_PER_CPU(int, bpf_prog_active);
 
diff --git a/kernel/cgroup.c b/kernel/cgroup.c
index b0d727d26fc7..e02e0a531be9 100644
--- a/kernel/cgroup.c
+++ b/kernel/cgroup.c
@@ -6236,34 +6236,54 @@ EXPORT_SYMBOL_GPL(cgroup_get_from_path);
 /**
  * cgroup_get_from_fd - get a cgroup pointer from a fd
  * @fd: fd obtained by open(cgroup2_dir)
+ * @access_mask: contains the permission mask
  *
  * Find the cgroup from a fd which should be obtained
  * by opening a cgroup directory.  Returns a pointer to the
  * cgroup on success. ERR_PTR is returned if the cgroup
- * cannot be found.
+ * cannot be found or its access is denied.
  */
-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);
-	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

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
> 

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

[Tejun Heo]

Hello,

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

Can you please explain why this change is beneficial?

Thanks.

-- 
tejun

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

[Mickaël Salaün]

[-- Attachment #1.1: Type: text/plain, Size: 3291 bytes --]


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 #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 455 bytes --]

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

[Tejun Heo]

On Tue, Sep 20, 2016 at 06:53:35PM +0200, Mickaël Salaün wrote:
> 
> 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.

Currently, bpf's access control and cgroup's are completely separate
and intentionally so.  I don't see why this matters given the current
model.

Thanks.

-- 
tejun