Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider: - [High] dma-buf: unprivileged cgroup escape and OOM via dma_buf_xfer_charge() - [Medium] dma-buf: broken !CONFIG_MEMCG and disabled-memcg handling in dma_buf_xfer_charge() --
commit 3f75f95a079fdfc2b4d2659ae4dbea6726be45ad Author: Albert Esteve aesteve@redhat.com
dma-buf: transfer memcg charges via DMA_BUF_IOCTL_XFER_CHARGE
This commit adds the DMA_BUF_IOCTL_XFER_CHARGE ioctl and a generic mem_cgroup_transfer_charge() helper to transfer a dma-buf's memory cgroup charge to a target cgroup identified by a cgroupfd or pidfd.
diff --git a/drivers/dma-buf/dma-buf.c b/drivers/dma-buf/dma-buf.c index 8376662735502..3bd26fbb11092 100644 --- a/drivers/dma-buf/dma-buf.c +++ b/drivers/dma-buf/dma-buf.c @@ -540,6 +540,45 @@ static long dma_buf_import_sync_file(struct dma_buf *dmabuf, } #endif +/* Transfer a dma-buf's memcg charge; see struct dma_buf_transfer_charge */ +static long dma_buf_xfer_charge(struct dma_buf *dmabuf,
const void __user *user_data)+{
- struct dma_buf_transfer_charge arg;
- struct mem_cgroup *memcg;
- unsigned int nr_pages = PAGE_ALIGN(dmabuf->size) >> PAGE_SHIFT;
- int ret = 0;
- if (copy_from_user(&arg, user_data, sizeof(arg)))
return -EFAULT;- if (!IS_ENABLED(CONFIG_MEMCG))
return 0;
[Severity: Medium] As reported by Sashiko on v3 [1], when CONFIG_MEMCG is disabled, dma_buf_xfer_charge() returns 0 before checking arg.flags, mem_cgroup_get_from_fd(arg.charge_fd), or dmabuf->memcg.
On a CONFIG_MEMCG=n kernel, won't this accept non-zero arg.flags instead of returning -EINVAL, and return 0 instead of -EOPNOTSUPP on buffers that have no existing memcg charge?
In include/uapi/linux/dma-buf.h, struct dma_buf_transfer_charge documents that flags must be zero and that the ioctl returns -EOPNOTSUPP if the buffer has no existing memcg charge. In include/linux/memcontrol.h, the !CONFIG_MEMCG stub for mem_cgroup_get_from_fd() already returns ERR_PTR(-EOPNOTSUPP):
include/linux/memcontrol.h: static inline struct mem_cgroup *mem_cgroup_get_from_fd(int fd) { return ERR_PTR(-EOPNOTSUPP); }
Returning 0 early here bypasses that stub and leaves the !CONFIG_MEMCG stub for mem_cgroup_transfer_charge() in include/linux/memcontrol.h unreachable.
In addition, in mm/memcontrol.c, mem_cgroup_transfer_charge() has:
if (!new_memcg || mem_cgroup_disabled()) return true;
which returns true without updating *ptr_memcg or uncharging and dropping the reference to old_memcg, even though the function comment states that on success it stores new_memcg in *ptr_memcg. Should the early CONFIG_MEMCG check in dma_buf_xfer_charge() be dropped so it falls through to arg.flags validation and mem_cgroup_get_from_fd()?
- if (arg.flags != 0)
return -EINVAL;- memcg = mem_cgroup_get_from_fd(arg.charge_fd);
- if (IS_ERR(memcg))
return PTR_ERR(memcg);- dma_resv_lock(dmabuf->resv, NULL);
- if (!dmabuf->memcg) {
ret = -EOPNOTSUPP;goto out;- }
- if (dmabuf->memcg != memcg &&
!mem_cgroup_transfer_charge(&dmabuf->memcg, memcg, MEMCG_DMABUF, nr_pages))ret = -ENOMEM;
[Severity: High] As reported by Sashiko on v3 [2], can an unprivileged holder of a dma-buf file descriptor use dma_buf_xfer_charge() to escape its own memory cgroup limits or trigger reclaim and OOM kills in another cgroup?
In dma_buf_ioctl() -> dma_buf_xfer_charge() -> mem_cgroup_get_from_fd(), neither dma_buf_xfer_charge() nor mem_cgroup_get_from_fd() checks DAC permissions, capabilities (such as CAP_SYS_RESOURCE or CAP_SYS_ADMIN), cgroup v2 write/delegation permissions (such as cgroup_procs_write_permission() used by CLONE_INTO_CGROUP), or task access permissions (such as ptrace_may_access()) on either the source cgroup (dmabuf->memcg) or the target cgroup/task (memcg):
mm/memcontrol.c:mem_cgroup_get_from_fd() { cgrp = cgroup_get_from_fd(fd); if (!IS_ERR(cgrp)) { ... return memcg; }
task = pidfd_get_task(fd, &pidfd_flags); ... }
Because /sys/fs/cgroup directories are world-readable (0755) by default and pidfd_open() requires no privileges for visible PIDs, any unprivileged process can open /sys/fs/cgroup (root_mem_cgroup) or another cgroup's directory or pidfd.
Even with the subsequent LSM and SELinux patches in this series: 1. On systems without SELinux (such as AppArmor, Smack, or CONFIG_SECURITY=n), security_dma_buf_charge_cgroup() and security_dma_buf_charge_task() return 0 with no baseline DAC, cgroup permission, or capability checks. 2. On SELinux systems, selinux_dma_buf_charge_cgroup() checks kernfs_xattr_get(cgrp->kn, XATTR_NAME_SELINUX, NULL, 0) and returns 0 on -ENODATA whenever a cgroup directory lacks an explicit security.selinux xattr (including /sys/fs/cgroup and cgroups labeled via default genfscon policy rules). 3. Neither dma_buf_xfer_charge() nor the LSM hooks check whether the caller is authorized to uncharge the source cgroup (dmabuf->memcg).
When mem_cgroup_transfer_charge() runs:
mm/memcontrol.c:mem_cgroup_transfer_charge() { ... if (try_charge(new_memcg, GFP_KERNEL, nr_pages)) return false; ... if (old_memcg) { /* Uncharge old cgroup */ mod_memcg_state(old_memcg, item, -nr_pages); if (!mem_cgroup_is_root(old_memcg)) refill_stock(old_memcg, nr_pages); mem_cgroup_put(old_memcg); } }
A confined process can pass an fd for /sys/fs/cgroup (where try_charge() on root_mem_cgroup is a no-op) to uncharge its own cgroup and bypass memory.max, or pass an fd for another cgroup to trigger GFP_KERNEL direct reclaim and mem_cgroup_oom() in that victim cgroup. Should dma_buf_xfer_charge() or mem_cgroup_get_from_fd() enforce baseline cgroup write/task permissions and authorization to uncharge dmabuf->memcg?
[1] https://lore.kernel.org/r/20261027-v2_20230123_tjmercier_google_com-v3-7-78d... [2] https://lore.kernel.org/r/20261027-v2_20230123_tjmercier_google_com-v3-7-78d...