Re: [PATCH v3 6/8] selinux: Restrict dma-buf direct cgroup charging

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] selinux: selinux_dma_buf_charge_cgroup() bypasses check on genfscon cgroups
--

commit 6b412783b17727c382f1a67078326c58c39264e7
Author: Albert Esteve <aesteve@redhat.com>

selinux: Restrict dma-buf direct cgroup charging

This commit implements the selinux_dma_buf_charge_cgroup() LSM hook and adds
the charge_cgroup permission to the dma_buf security class to control which
processes may charge dma-buf memory to a cgroup. It reads the target security
context from the cgroup kernfs node's security.selinux extended attribute and
checks permission via avc_has_perm().

> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> index 8f39d91eabc97..2f99eaa910af4 100644
> --- a/security/selinux/hooks.c
> +++ b/security/selinux/hooks.c
> @@ -2210,6 +2210,40 @@ static int selinux_dma_buf_charge_task(const struct cred *from, const struct cre
>  			    SECCLASS_DMA_BUF, DMA_BUF__CHARGE_TASK, NULL);
>  }
>  
> +#ifdef CONFIG_CGROUPS
> +static int selinux_dma_buf_charge_cgroup(const struct cred *cred, struct cgroup *cgrp)
> +{
> +	u32 cgrp_sid, clen;
> +	int rc;
> +	char *context;
> +
> +	rc = kernfs_xattr_get(cgrp->kn, XATTR_NAME_SELINUX, NULL, 0);
> +	if (rc == -ENODATA)
> +		return 0;

[Severity: High]
Does returning 0 on -ENODATA here bypass the permission check for all cgroups
that carry their default genfscon label (such as cgroup_t)?

In selinux_set_mnt_opts(), cgroup and cgroup2 filesystems are configured with
SE_SBGENFS | SE_SBGENFS_XATTR. When a cgroup VFS inode is initialized in
inode_doinit_with_dentry(), SELinux obtains the default SID from policy via
selinux_genfs_get_sid() and then checks for an optional xattr override via
inode_doinit_use_xattr():

security/selinux/hooks.c:inode_doinit_with_dentry() {
	...
	rc = selinux_genfs_get_sid(dentry, sclass,
				   sbsec->flags, &sid);
	if (rc) {
		dput(dentry);
		goto out;
	}

	if ((sbsec->flags & SE_SBGENFS_XATTR) &&
	    (inode->i_opflags & IOP_XATTR)) {
		rc = inode_doinit_use_xattr(inode, dentry,
					    sid, &sid);
	...
}

When no explicit setxattr() has been performed on the cgroup directory,
inode_doinit_use_xattr() sees -ENODATA from the underlying kernfs node and
falls back to def_sid in memory without writing a security.selinux xattr to
cgrp->kn:

security/selinux/hooks.c:inode_doinit_use_xattr() {
	...
	if (rc < 0) {
		kfree(context);
		if (rc != -ENODATA) {
			pr_warn("SELinux: %s:  getxattr returned %d for dev=%s ino=%llu\n",
				__func__, -rc, inode->i_sb->s_id, inode->i_ino);
			return rc;
		}
		*sid = def_sid;
		return 0;
	}
	...
}

Likewise, selinux_kernfs_init_security() sees -ENODATA on the parent kernfs
node and returns 0 without storing an xattr on newly created child cgroups.

As a result, unless a cgroup directory has been explicitly relabeled with
setxattr(), kernfs_xattr_get(cgrp->kn, XATTR_NAME_SELINUX, NULL, 0) will
return -ENODATA even though the cgroup is labeled cgroup_t via genfscon.
When userspace passes a cgroup fd via DMA_HEAP_IOCTL_ALLOC or
DMA_BUF_IOCTL_XFER_CHARGE, mem_cgroup_get_from_fd() invokes
security_dma_buf_charge_cgroup() -> selinux_dma_buf_charge_cgroup(), which
will return 0 without calling avc_has_perm(), leaving default-labeled
cgroup_t cgroups unrestricted?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261027-v2_20230123_tjmercier_google_com-v3-0-78d2c4cb8647@redhat.com?part=6