Re: [PATCH v4 7/8] selinux: Restrict dma-buf direct cgroup charging

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] selinux: selinux_dma_buf_charge_cgroup() bypasses check on genfscon cgroups
- [Medium] selinux: one-off dma_buf charge_cgroup lacks broader cgroup control design
--

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

selinux: Restrict dma-buf direct cgroup charging

This commit implements the selinux_dma_buf_charge_cgroup() hook and adds the
charge_cgroup permission to the dma_buf security class to restrict which
processes can charge dma-buf memory to a target cgroup.

> 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]
As reported by Sashiko on v3 [1], in security/selinux/hooks.c, does returning
0 in selinux_dma_buf_charge_cgroup() when kernfs_xattr_get() returns -ENODATA
bypass the permission check for all default-labeled cgroups?

When a process calls ioctl(DMA_HEAP_IOCTL_ALLOC) or
ioctl(DMA_BUF_IOCTL_XFER_CHARGE) with a charge_fd referencing a cgroup
directory, mem_cgroup_get_from_fd() invokes security_dma_buf_charge_cgroup()
-> selinux_dma_buf_charge_cgroup().

In SELinux, cgroup and cgroup2 filesystems are configured with
SE_SBGENFS | SE_SBGENFS_XATTR in selinux_set_mnt_opts() and obtain their
default label (such as cgroup_t) from policy genfscon rules. When a cgroupfs
VFS inode is initialized in inode_doinit_with_dentry(), SELinux computes the
default SID via selinux_genfs_get_sid() and only uses kernfs xattrs as an
optional override:

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() gets -ENODATA and falls back to def_sid in memory
without writing any 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;
	}
    ...
}

Similarly, selinux_kernfs_init_security() sees -ENODATA on the parent node
and returns 0 without setting an xattr on child cgroup nodes, and if the
cgroup_seclabel policy capability is not enabled, security.selinux xattrs
cannot be set on cgroup directories at all.

Because kernfs_xattr_get(cgrp->kn, XATTR_NAME_SELINUX, NULL, 0) returns
-ENODATA for every cgroup that has not been manually relabeled via
setxattr(), won't selinux_dma_buf_charge_cgroup() skip avc_has_perm() for all
default-labeled cgroup_t cgroups and render policy rules like
"allow allocator_app_t cgroup_t:dma_buf charge_cgroup;" ineffective?

[ ... ]

> diff --git a/security/selinux/include/classmap.h b/security/selinux/include/classmap.h
> index b4ebedc6e4240..90d1e04c3d27c 100644
> --- a/security/selinux/include/classmap.h
> +++ b/security/selinux/include/classmap.h
> @@ -181,7 +181,7 @@ const struct security_class_mapping secclass_map[] = {
>  	{ "user_namespace", { "create", NULL } },
>  	{ "memfd_file",
>  	  { COMMON_FILE_PERMS, "execute_no_trans", "entrypoint", NULL } },
> -	{ "dma_buf", { "charge_task", NULL } },
> +	{ "dma_buf", { "charge_task", "charge_cgroup", NULL } },

[Severity: Medium]
As reported by Paul Moore on RFC [2], in security/selinux/include/classmap.h
(secclass_map) and security/selinux/hooks.c (selinux_dma_buf_charge_cgroup()),
this adds a narrow dma_buf class permission for cgroup charging invoked from
DMA_HEAP_IOCTL_ALLOC and DMA_BUF_IOCTL_XFER_CHARGE via
mem_cgroup_get_from_fd() -> security_dma_buf_charge_cgroup().

Rather than introducing a one-off dma_buf cgroup charging permission, should
this be part of a broader SELinux access control design covering existing
cgroup control points so that the object class and permission model is
consistent across cgroup operations?

[1] https://lore.kernel.org/r/20261027-v2_20230123_tjmercier_google_com-v3-6-78d2c4cb8647@redhat.com
[2] https://lore.kernel.org/r/16093a0278a6d7d1a0a8bc055c228bed@paul-moore.com

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261104-v2_20230123_tjmercier_google_com-v4-0-1819a017000b@redhat.com?part=7