linux-kernel - Re: [PATCH v1] cgroup,bpf: Add access check for cgroup_get_from

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20160920003007.GB91808@ast-mbp.thefacebook.com>
Date:   Mon, 19 Sep 2016 17:30:08 -0700
From:   Alexei Starovoitov <alexei.starovoitov@...il.com>
To:     Mickaël Salaün <mic@...ikod.net>
Cc:     linux-kernel@...r.kernel.org, Alexei Starovoitov <ast@...nel.org>,
        Andy Lutomirski <luto@...capital.net>,
        Daniel Borkmann <daniel@...earbox.net>,
        Daniel Mack <daniel@...que.org>,
        "David S . Miller" <davem@...emloft.net>,
        James Morris <james.l.morris@...cle.com>,
        Kees Cook <keescook@...omium.org>,
        Martin KaFai Lau <kafai@...com>, Tejun Heo <tj@...nel.org>,
        cgroups@...r.kernel.org
Subject: Re: [PATCH v1] cgroup,bpf: Add access check for cgroup_get_from_fd()

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
>