Re: [PATCH RFC v2 7/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 6 potential issue(s) to consider:
- [Critical] dma-buf: task->mm UAF and wrong cgroup fallback in dma_buf_xfer_charge()
- [Critical] mm: memcontrol: new_memcg UAF race in mem_cgroup_transfer_dmabuf_charge()
- [High] dma-buf: missing DAC permission checks in dma_buf_xfer_charge()
- [Medium] dma-buf: double charge and -ENOMEM race in mem_cgroup_transfer_dmabuf_charge()
- [Medium] dma-buf: broken runtime mem_cgroup_disabled() handling in dma_buf_xfer_charge()
- [Low] dma-buf: build failure in dma_buf_xfer_charge() when CONFIG_MEMCG=n
--

commit c7c3a18bea48fc8f09ccf4a84a6aa11e6f991c4c
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
mem_cgroup_transfer_dmabuf_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 f31f49ebd0af3..7fa607d9bf657 100644
> --- a/drivers/dma-buf/dma-buf.c
> +++ b/drivers/dma-buf/dma-buf.c
> @@ -540,6 +540,80 @@ static long dma_buf_import_sync_file(struct dma_buf *dmabuf,
>  }
>  #endif
>  
> +static long dma_buf_xfer_charge(struct dma_buf *dmabuf,
> +				const void __user *user_data)
> +{
> +	struct cgroup *cgrp;
> +	struct cgroup_subsys_state *css;
> +	struct dma_buf_transfer_charge arg;
> +	struct mem_cgroup *memcg;
> +	struct task_struct *task;
> +	const struct cred *tcred;
> +	unsigned int pidfd_flags;
> +	int ret = 0;
> +
> +	if (copy_from_user(&arg, user_data, sizeof(arg)))
> +		return -EFAULT;
> +
> +	if (!IS_ENABLED(CONFIG_MEMCG))
> +		return 0;

[Severity: Low]
Will drivers/dma-buf/dma-buf.c fail to build in dma_buf_xfer_charge() when
CONFIG_DMA_SHARED_BUFFER=y and CONFIG_MEMCG=n (or CONFIG_CGROUPS=n)?

Because if (!IS_ENABLED(CONFIG_MEMCG)) is a runtime C conditional rather
than a preprocessor guard, the compiler still resolves all symbols in the
rest of dma_buf_xfer_charge().

In include/linux/cgroup_subsys.h, memory_cgrp_subsys is only declared when
CONFIG_MEMCG is enabled:

    #if IS_ENABLED(CONFIG_MEMCG)
    SUBSYS(memory)
    #endif

And when CONFIG_CGROUPS=n, cgroup_get_from_fd(), cgroup_css(),
css_tryget_online(), and cgroup_put() in include/linux/cgroup.h have no
stub declarations. Should the body of dma_buf_xfer_charge() be guarded with
#if IS_ENABLED(CONFIG_MEMCG) instead?

> +
> +	if (arg.__padding != 0)
> +		return -EINVAL;
> +
> +	cgrp = cgroup_get_from_fd(arg.charge_fd);
> +	if (!IS_ERR(cgrp)) {
> +		/* charge_fd is a cgroupfd */
> +		rcu_read_lock();
> +		css = cgroup_css(cgrp, &memory_cgrp_subsys);
> +		if (!css || !css_tryget_online(css)) {
> +			rcu_read_unlock();
> +			cgroup_put(cgrp);
> +			return -ENOENT;
> +		}

[Severity: Medium]
What happens in dma_buf_xfer_charge() when CONFIG_MEMCG=y but memory cgroups
are disabled at boot via cgroup_disable=memory (so mem_cgroup_disabled() is
true)?

In that configuration, passing a valid cgroupfd causes
cgroup_css(cgrp, &memory_cgrp_subsys) to return NULL and fail with -ENOENT,
whereas passing a pidfd calls get_mem_cgroup_from_mm() in mm/memcontrol.c,
which returns NULL when mem_cgroup_disabled() is true, matches
dmabuf->memcg == NULL, and returns 0.

Similarly, in mm/memcontrol.c, mem_cgroup_transfer_dmabuf_charge() only
skips try_charge() when mem_cgroup_disabled() is true:

    if (!mem_cgroup_disabled() && try_charge(new_memcg, GFP_KERNEL, nr_pages))
            return false;

Unlike mem_cgroup_charge_dmabuf(), which returns true immediately when
mem_cgroup_disabled() is true, mem_cgroup_transfer_dmabuf_charge() still
proceeds to cmpxchg(), mod_memcg_state(), and refill_stock(). Should both
dma_buf_xfer_charge() and mem_cgroup_transfer_dmabuf_charge() return early
when mem_cgroup_disabled() is true?

> +
> +		memcg = mem_cgroup_from_css(css);
> +		rcu_read_unlock();
> +
> +		ret = security_dma_buf_charge_cgroup(current_cred(), cgrp);
> +		cgroup_put(cgrp);
> +		if (ret) {
> +			mem_cgroup_put(memcg);
> +			return ret;
> +		}
> +	} else {
> +		/* charge_fd is a pidfd  */
> +		task = pidfd_get_task(arg.charge_fd, &pidfd_flags);
> +		if (IS_ERR(task))
> +			return PTR_ERR(task);
> +
> +		tcred = get_task_cred(task);
> +		ret = security_dma_buf_charge_task(current_cred(), tcred);
> +		put_cred(tcred);
> +		if (ret) {
> +			put_task_struct(task);
> +			return ret;
> +		}

[Severity: High]
Is a DAC or cgroup permission check needed in dma_buf_xfer_charge() in
addition to the security_dma_buf_charge_cgroup() and
security_dma_buf_charge_task() LSM hooks?

In drivers/dma-buf/dma-buf.c, dma_buf_xfer_charge() relies solely on those
LSM hooks to authorize the transfer:

- In the cgroupfd path, cgroup_get_from_fd(arg.charge_fd) only checks that
  the file descriptor refers to a cgroup v2 directory, without verifying
  write access (such as inode_permission() with MAY_WRITE or
  cgroup_procs_write_permission()). Because /sys/fs/cgroup directories are
  world-readable (0755) by default, an unprivileged process can open
  /sys/fs/cgroup or another service's cgroup with O_RDONLY | O_DIRECTORY.
- In the pidfd path, pidfd_get_task() resolves any pidfd without checking
  whether current may act on that task (for example via ptrace_may_access()
  or credential/cgroup authorization).

When CONFIG_SECURITY=n, when no LSM implementing these hooks is active, when
a cgroup directory has no explicit security.selinux xattr, or when two
cgroups share the same SELinux domain, the hooks return 0. Could an
unprivileged holder of a dma-buf fd use DMA_BUF_IOCTL_XFER_CHARGE to shift
its charges to the root cgroup (where try_charge() in
mem_cgroup_transfer_dmabuf_charge() is a no-op, bypassing its own
memory.max), or transfer large charges into another cgroup to exhaust its
memory.max and trigger mem_cgroup_oom()?

> +
> +		memcg = get_mem_cgroup_from_mm(task->mm);
> +		put_task_struct(task);
> +	}

[Severity: Critical]
Can passing task->mm directly to get_mem_cgroup_from_mm(task->mm) in
dma_buf_xfer_charge() cause a use-after-free or charge the wrong cgroup?

In drivers/dma-buf/dma-buf.c, pidfd_get_task() pins task->usage, but it
does not pin task->mm or hold task_lock(task). If the target task
concurrently exits or execs:

    do_exit()
      exit_mm()
        task_lock(current);
        current->mm = NULL;
        ...
        task_unlock(current);
        ...
        mmput(mm); -> __mmdrop(mm) -> free_mm(mm)

or replaces its mm via execve() -> exec_mmap() -> exec_mm_put_old(), the
old mm_struct can be freed while get_mem_cgroup_from_mm() in mm/memcontrol.c
dereferences mm->owner:

    rcu_read_lock();
    do {
            memcg = mem_cgroup_from_task(rcu_dereference(mm->owner));

In addition, if task->mm is already NULL (such as a zombie task that has
completed exit_mm(), for which pidfd_get_task() still succeeds),
get_mem_cgroup_from_mm(NULL) in mm/memcontrol.c falls back to the caller's
context:

    if (unlikely(!mm)) {
            memcg = active_memcg();
            if (unlikely(memcg)) {
                    /* remote memcg must hold a ref */
                    css_get(&memcg->css);
                    return memcg;
            }
            mm = current->mm;
            if (unlikely(!mm))
                    return root_mem_cgroup;
    }

Instead of failing when the target task has no mm, this silently attributes
the dma-buf charge to the caller's own cgroup (or root_mem_cgroup). Should
dma_buf_xfer_charge() acquire a reference via get_task_mm(task), return an
error such as -ESRCH when mm is NULL, and call mmput(mm) after
get_mem_cgroup_from_mm(mm)?

> +
> +	if (dmabuf->memcg == memcg)
> +		goto skip_transfer;
> +
> +	if (!mem_cgroup_transfer_dmabuf_charge(dmabuf, memcg)) {
> +		ret = -ENOMEM;
> +		goto skip_transfer;
> +	}

[ ... ]

> diff --git a/mm/memcontrol.c b/mm/memcontrol.c
> index e94222237d31a..d994a4b2d35e4 100644
> --- a/mm/memcontrol.c
> +++ b/mm/memcontrol.c
> @@ -5688,6 +5688,46 @@ bool mem_cgroup_charge_dmabuf(struct dma_buf *dmabuf, struct mem_cgroup *memcg,
>  	return true;
>  }
>  
> +/**
> + * mem_cgroup_transfer_dmabuf_charge - move a dma-buf charge
> + * @dmabuf: buffer to transfer charge
> + * @new_memcg: target memcg; caller must hold a reference
> + *
> + * Transfers the charge for @dmabuf to @new_memcg and, on
> + * success, stores @new_memcg in @dmabuf->memcg.
> + * Returns %true on success, %false if the charge against @new_memcg
> + * failed or a concurrent transfer raced on @dmabuf->memcg.
> + * On failure the caller retains its reference to @new_memcg.
> + */
> +bool mem_cgroup_transfer_dmabuf_charge(struct dma_buf *dmabuf,
> +				       struct mem_cgroup *new_memcg)
> +{
> +	struct mem_cgroup *old_memcg = READ_ONCE(dmabuf->memcg);
> +	unsigned int nr_pages = PAGE_ALIGN(dmabuf->size) >> PAGE_SHIFT;
> +
> +	if (!mem_cgroup_disabled() && try_charge(new_memcg, GFP_KERNEL, nr_pages))
> +		return false;
> +
> +	if (cmpxchg(&dmabuf->memcg, old_memcg, new_memcg) != old_memcg) {
> +		if (!mem_cgroup_is_root(new_memcg))
> +			refill_stock(new_memcg, nr_pages);
> +		return false;
> +	}

[Severity: Medium]
How does mem_cgroup_transfer_dmabuf_charge() behave when two threads race
to transfer the same dmabuf?

First, dma_buf_xfer_charge() in drivers/dma-buf/dma-buf.c checks
dmabuf->memcg == memcg before calling mem_cgroup_transfer_dmabuf_charge(),
which then reloads old_memcg = READ_ONCE(dmabuf->memcg) without rechecking
old_memcg == new_memcg. If two threads concurrently transfer dmabuf to the
same target cgroup, both can pass the check in dma_buf_xfer_charge(); after
the first transfer finishes, the second thread reads
old_memcg == new_memcg and calls try_charge(new_memcg, GFP_KERNEL, nr_pages)
before uncharging old_memcg, transiently charging 2 * nr_pages to new_memcg
and potentially triggering unnecessary reclaim, OOM, or -ENOMEM even though
dmabuf is already charged to new_memcg.

Second, old_memcg is read before try_charge(new_memcg, GFP_KERNEL, nr_pages),
which can sleep in direct reclaim. If another transfer updates dmabuf->memcg
while try_charge() is sleeping, cmpxchg(&dmabuf->memcg, old_memcg, new_memcg)
fails without retrying, and dma_buf_xfer_charge() returns -ENOMEM to
userspace even though charging new_memcg succeeded. Should the cmpxchg loop
retry with the updated old_memcg (and treat old_memcg == new_memcg as a
no-op)?

> +
> +	mod_memcg_state(new_memcg, MEMCG_DMABUF, nr_pages);
> +
> +	if (old_memcg) {
> +		/* Uncharge old cgroup */
> +		mod_memcg_state(old_memcg, MEMCG_DMABUF, -nr_pages);
> +		if (!mem_cgroup_is_root(old_memcg))
> +			refill_stock(old_memcg, nr_pages);
> +		/* unref from buffer - buffer keeps new ref to new_memcg */
> +		mem_cgroup_put(old_memcg);
> +	}

[Severity: Critical]
Could publishing new_memcg into dmabuf->memcg via cmpxchg() before calling
mod_memcg_state(new_memcg, MEMCG_DMABUF, nr_pages) lead to a use-after-free
on new_memcg when two DMA_BUF_IOCTL_XFER_CHARGE calls race?

In mm/memcontrol.c, dma_buf_xfer_charge() holds a single reference to
new_memcg that is transferred to dmabuf->memcg as soon as cmpxchg()
succeeds:

Thread A                              Thread B
mem_cgroup_transfer_dmabuf_charge()
  cmpxchg(&dmabuf->memcg,
          old_memcg, new_memcg)
                                      mem_cgroup_transfer_dmabuf_charge()
                                        old_memcg = READ_ONCE(dmabuf->memcg);
                                        /* old_memcg is new_memcg */
                                        cmpxchg(&dmabuf->memcg,
                                                new_memcg, memcg_C)
                                        mod_memcg_state(new_memcg,
                                                        MEMCG_DMABUF,
                                                        -nr_pages);
                                        refill_stock(new_memcg, nr_pages);
                                        mem_cgroup_put(new_memcg);
  mod_memcg_state(new_memcg,
                  MEMCG_DMABUF,
                  nr_pages);

If new_memcg was concurrently offlined after cgroup_put(cgrp) in
dma_buf_xfer_charge(), and refill_stock() uncharges directly (for example
when nr_pages > MEMCG_CHARGE_BATCH), Thread B's mem_cgroup_put(new_memcg)
can drop the last reference and free new_memcg before Thread A executes
mod_memcg_state(new_memcg, MEMCG_DMABUF, nr_pages). Even when new_memcg is
not freed, Thread B decrements MEMCG_DMABUF before Thread A increments it.

Would updating mod_memcg_state(new_memcg, MEMCG_DMABUF, nr_pages) before the
cmpxchg() (and reverting it if cmpxchg() fails), or serializing transfers,
avoid exposing new_memcg before its state update completes?

[ ... ]

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