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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] selinux: selinux_dma_buf_charge_cgroup() bypasses check for genfscon cgroups
- [Medium] selinux: stale xattr length in selinux_dma_buf_charge_cgroup() on race
- [Low] selinux: build failure in selinux_dma_buf_charge_cgroup() if !CONFIG_CGROUPS
--

commit 8979483cce99c621c0be48bae510af01c958c235
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 control cross-cgroup
dma-buf charging. It derives the target security identifier from the
security.selinux extended attribute on the cgroup kernfs node and checks
access via avc_has_perm().

> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> index 8f39d91eabc97..8de5e16951dec 100644
> --- a/security/selinux/hooks.c
> +++ b/security/selinux/hooks.c
> @@ -2210,6 +2210,38 @@ static int selinux_dma_buf_charge_task(const struct cred *from, const struct cre
>  			    SECCLASS_DMA_BUF, DMA_BUF__CHARGE_TASK, NULL);
>  }
>  
> +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);

[Severity: Low]
Will this fail to build when CONFIG_SECURITY_SELINUX is enabled and
CONFIG_CGROUPS is disabled?

In security/selinux/hooks.c, selinux_dma_buf_charge_cgroup() is compiled
unconditionally and dereferences cgrp->kn. However, the full definition of
struct cgroup in include/linux/cgroup-defs.h is guarded by CONFIG_CGROUPS:

include/linux/cgroup-defs.h:
#ifdef CONFIG_CGROUPS

struct cgroup;

When CONFIG_CGROUPS is disabled, include/linux/cgroup.h only forward-declares
struct cgroup as an incomplete type:

include/linux/cgroup.h:
#else /* !CONFIG_CGROUPS */

struct cgroup_subsys_state;
struct cgroup;

Since CONFIG_SECURITY_SELINUX does not depend on CONFIG_CGROUPS, dereferencing
cgrp->kn without a CONFIG_CGROUPS guard will reference an incomplete type.

> +	if (rc == -ENODATA)
> +		return 0;

[Severity: High]
Does returning 0 on -ENODATA in selinux_dma_buf_charge_cgroup() bypass the
permission check for all cgroups that use default genfscon labeling (such as
cgroup_t)?

When a process calls dma_heap_ioctl_allocate() or dma_buf_xfer_charge() ->
security_dma_buf_charge_cgroup() -> selinux_dma_buf_charge_cgroup(), this code
queries XATTR_NAME_SELINUX directly on cgrp->kn.

However, cgroup and cgroup2 filesystems are configured with SE_SBGENFS |
SE_SBGENFS_XATTR. When a cgroup inode is initialized in
security/selinux/hooks.c:inode_doinit_with_dentry(), its default label from
genfscon (for example, cgroup_t) is resolved via selinux_genfs_get_sid() and
passed as def_sid to 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);
    ...
}

In security/selinux/hooks.c:inode_doinit_use_xattr(), when no explicit
security.selinux xattr has been set on the kernfs node, getxattr returns
-ENODATA and the inode falls back to def_sid in memory without writing
XATTR_NAME_SELINUX 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 XATTR_NAME_SELINUX on child cgroup nodes so that
they also fall back to genfscon.

As a result, unless a cgroup directory has been explicitly relabeled via
setxattr(), kernfs_xattr_get(cgrp->kn, XATTR_NAME_SELINUX, NULL, 0) returns
-ENODATA and selinux_dma_buf_charge_cgroup() returns 0 without calling
avc_has_perm(), leaving default-labeled cgroup_t cgroups unrestricted.

> +	else if (rc < 0)
> +		return rc;
> +
> +	clen = (u32)rc;
> +	context = kmalloc(clen, GFP_KERNEL);
> +	if (!context)
> +		return -ENOMEM;
> +
> +	rc = kernfs_xattr_get(cgrp->kn, XATTR_NAME_SELINUX, context, clen);
> +	if (rc < 0) {
> +		kfree(context);
> +		return rc;
> +	}
> +
> +	rc = security_context_to_sid(context, clen, &cgrp_sid, GFP_KERNEL);

[Severity: Medium]
Could passing clen instead of rc to security_context_to_sid() read
uninitialized heap memory if the security.selinux xattr is concurrently
shortened via setxattr()?

In selinux_dma_buf_charge_cgroup(), context is allocated with
kmalloc(clen, GFP_KERNEL) using the length from the first kernfs_xattr_get()
call. In fs/xattr.c:simple_xattr_get(), the lookup is only protected by a
local rcu read lock per call:

fs/xattr.c:simple_xattr_get() {
    ...
	guard(rcu)();
	xattr = rhashtable_lookup(ht, &key, simple_xattr_params);
	if (xattr) {
		ret = xattr->size;
		if (buffer) {
			if (size < xattr->size)
				ret = -ERANGE;
			else
				memcpy(buffer, xattr->value, xattr->size);
		}
	}
	return ret;
}

If a concurrent setxattr() replaces security.selinux on the cgroup with a
shorter, non-nul-terminated context string between the two kernfs_xattr_get()
calls, the second kernfs_xattr_get() copies only the new rc bytes into context
and returns rc < clen, leaving context[rc .. clen - 1] uninitialized.

Passing the stale clen to security_context_to_sid() then causes
security/selinux/ss/services.c:security_context_to_sid_core() to copy all clen
bytes and place the terminator at index clen:

security/selinux/ss/services.c:security_context_to_sid_core() {
    ...
	/* Copy the string to allow changes and ensure a NUL terminator */
	scontext2 = kmemdup_nul(scontext, scontext_len, gfp_flags);
	if (!scontext2)
		return -ENOMEM;
    ...
}

Should security_context_to_sid() be passed rc from the second
kernfs_xattr_get() call instead of clen?

> +	kfree(context);
> +	if (rc)
> +		return rc;
> +
> +	return avc_has_perm(cred_sid(cred), cgrp_sid,
> +			    SECCLASS_DMA_BUF, DMA_BUF__CHARGE_CGROUP, NULL);
> +}

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261021-v2_20230123_tjmercier_google_com-v2-0-ef6ee2dd8c5c@redhat.com?part=6