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-78d... [2] https://lore.kernel.org/r/16093a0278a6d7d1a0a8bc055c228bed@paul-moore.com