Re: [PATCH v4 3/8] dma-buf: transfer memcg charges via DMA_BUF_IOCTL_XFER_CHARGE

[sashiko-bot@kernel.org]

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-78d2c4cb8647@redhat.com
[2] https://lore.kernel.org/r/20261027-v2_20230123_tjmercier_google_com-v3-7-78d2c4cb8647@redhat.com

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