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 <[email protected]> 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-78d2c4cb8...@redhat.com?part=6
