Re: [PATCH RFC v2 2/8] dma-heap: charge dma-buf memory via explicit memcg

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [Critical] dma-heap: UAF and wrong memcg fallback on task->mm in dma_heap_ioctl_allocate()
- [High] dma-heap: missing permission check on charge_fd in dma_heap_ioctl_allocate()
- [Medium] dma-heap: missing __padding validation in dma_heap_ioctl_allocate()
- [Medium] dma-heap: physical pages allocated before memcg charge in dma_heap_ioctl_allocate()
- [Low] dma-heap: build failure in dma_heap_ioctl_allocate() when CONFIG_MEMCG=n
--

commit 662cb8c6d2fd1fbf166b582f9ede716df0713b4a
Author: Albert Esteve <aesteve@redhat.com>

dma-heap: charge dma-buf memory via explicit memcg

This commit adds a charge_fd field to struct dma_heap_allocation_data so
DMA_HEAP_IOCTL_ALLOC can resolve a target cgroup from a cgroupfd or pidfd
and charge the allocated dma-buf to that memcg via
mem_cgroup_charge_dmabuf(). When charge_fd is unset and mem_accounting is
enabled, the allocation is charged to the calling process's memcg.

> diff --git a/drivers/dma-buf/dma-heap.c b/drivers/dma-buf/dma-heap.c
> index 8a6c2b6e0cc60..be77a4c168ab0 100644
> --- a/drivers/dma-buf/dma-heap.c
> +++ b/drivers/dma-buf/dma-heap.c
> @@ -7,14 +7,18 @@
>   */
>  
>  #include <linux/cdev.h>
> +#include <linux/cgroup.h>
>  #include <linux/device.h>
>  #include <linux/dma-buf.h>
>  #include <linux/dma-heap.h>
> +#include <linux/memcontrol.h>
> +#include <linux/sched/mm.h>
>  #include <linux/err.h>
>  #include <linux/export.h>
>  #include <linux/list.h>
>  #include <linux/minmax.h>
>  #include <linux/nospec.h>
> +#include <linux/pidfd.h>
>  #include <linux/syscalls.h>
>  #include <linux/uaccess.h>
>  #include <linux/xarray.h>
> @@ -75,9 +79,14 @@ static int dma_heap_open(struct inode *inode, struct file *file)
>  
>  static struct dma_buf *dma_heap_ioctl_allocate(struct file *file, void *data)
>  {
> +	struct cgroup *cgrp;
> +	struct cgroup_subsys_state *css;
>  	struct dma_heap_allocation_data *heap_allocation = data;
>  	struct dma_heap *heap = file->private_data;
>  	struct dma_buf *dmabuf;
> +	struct mem_cgroup *memcg = NULL;
> +	struct task_struct *task;
> +	unsigned int pidfd_flags;
>  	int fd;
>  	size_t len;
>  
> @@ -90,15 +99,56 @@ static struct dma_buf *dma_heap_ioctl_allocate(struct file *file, void *data)
>  	if (heap_allocation->heap_flags & ~DMA_HEAP_VALID_HEAP_FLAGS)
>  		return ERR_PTR(-EINVAL);
>  
> +	if (heap_allocation->charge_fd) {
> +		cgrp = cgroup_get_from_fd(heap_allocation->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 ERR_PTR(-ENOENT);
> +			}
> +
> +			memcg = mem_cgroup_from_css(css);
> +			rcu_read_unlock();
> +			cgroup_put(cgrp);

[Severity: Low]
Will this build when CONFIG_DMABUF_HEAPS=y and CONFIG_MEMCG=n (or
CONFIG_CGROUPS=n)?

In drivers/dma-buf/Kconfig, DMABUF_HEAPS only selects DMA_SHARED_BUFFER and
does not depend on CONFIG_MEMCG or CONFIG_CGROUPS:

drivers/dma-buf/Kconfig:
    menuconfig DMABUF_HEAPS
        bool "DMA-BUF Userland Memory Heaps"
        select DMA_SHARED_BUFFER

In include/linux/cgroup_subsys.h, SUBSYS(memory) is guarded by
#if IS_ENABLED(CONFIG_MEMCG), so memory_cgrp_subsys is only declared when
both CONFIG_CGROUPS and CONFIG_MEMCG are enabled:

include/linux/cgroup_subsys.h:
    #if IS_ENABLED(CONFIG_MEMCG)
    SUBSYS(memory)
    #endif

Additionally, when CONFIG_CGROUPS=n, include/linux/cgroup.h does not provide
declarations or stubs for cgroup_get_from_fd(), cgroup_css(),
css_tryget_online(), or cgroup_put(). Referencing memory_cgrp_subsys and
those helpers unconditionally in drivers/dma-buf/dma-heap.c:
dma_heap_ioctl_allocate() breaks the build when CONFIG_MEMCG=n. Should this
resolution block be guarded by IS_ENABLED(CONFIG_MEMCG) or moved into a
memcontrol helper with a !CONFIG_MEMCG stub?

> +		} else {
> +			/* charge_fd is a pidfd  */
> +			task = pidfd_get_task(heap_allocation->charge_fd, &pidfd_flags);
> +			if (IS_ERR(task))
> +				return ERR_CAST(task);
> +
> +			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
drivers/dma-buf/dma-heap.c:dma_heap_ioctl_allocate() trigger a
use-after-free on struct mm_struct or silently charge the wrong memcg?

pidfd_get_task() only increments task->usage and does not hold
task_lock(task) or pin task->mm via get_task_mm(task). If the target task
concurrently exits in kernel/exit.c:exit_mm() or execs in
fs/exec.c:exec_mmap() -> exec_mm_put_old(), it clears or replaces task->mm
under task_lock() and drops its reference via mmput(mm) -> __mmdrop(mm) ->
free_mm(mm):

kernel/exit.c:exit_mm() {
    ...
    task_lock(current);
    smp_mb__after_spinlock();
    local_irq_disable();
    current->mm = NULL;
    membarrier_update_current_mm(NULL);
    enter_lazy_tlb(mm, current);
    local_irq_enable();
    task_unlock(current);
    mmap_read_unlock(mm);
    mm_update_next_owner(mm);
    mmput(mm);
}

Because mm_cachep is not SLAB_TYPESAFE_BY_RCU, free_mm() frees the mm_struct
immediately while mm/memcontrol.c:get_mem_cgroup_from_mm() dereferences
rcu_dereference(mm->owner):

mm/memcontrol.c:get_mem_cgroup_from_mm() {
    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;
    }

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

In addition, if task->mm is already NULL when charge_fd is resolved (for
instance, a zombie task after exit_mm() whose pidfd is still valid, a
thread-group leader that exited via pthread_exit() while other threads
remain alive, or a kernel thread), get_mem_cgroup_from_mm(NULL) falls back
to current->mm and silently charges the central allocator's own cgroup
(even when mem_accounting is disabled) instead of failing with an error
such as -ESRCH. Should this use get_task_mm(task) / mmput(mm) and return an
error if mm is NULL, or resolve the memcg directly from task under RCU?

[Severity: High]
What prevents an unprivileged process with access to /dev/dma_heap/* from
passing an arbitrary pidfd or read-only cgroupfd in charge_fd to escape its
own memory.max limit or trigger OOM kills in another cgroup?

In drivers/dma-buf/dma-heap.c:dma_heap_ioctl_allocate(), charge_fd is
resolved via cgroup_get_from_fd() or pidfd_get_task() and charged via
mem_cgroup_charge_dmabuf() without checking whether the caller has
permission to charge the target cgroup or task:

1. sys_pidfd_open() in kernel/pid.c allows opening a pidfd for any task
   visible in the caller's PID namespace (such as PID 1 in the root cgroup
   or a task in a peer cgroup) without permission checks.
2. cgroup_get_from_fd() in kernel/cgroup/cgroup.c only checks that the file
   descriptor refers to a cgroup v2 directory and does not check write
   permission (unlike cgroup_css_set_fork(), which checks
   cgroup_may_write()), so any world-readable /sys/fs/cgroup directory
   opened with O_RDONLY | O_DIRECTORY or O_PATH can be charged.

While later commits in this series ("security: dma-buf: Add
dma_buf_charge_task LSM hook", "security: dma-buf: Add
dma_buf_charge_cgroup LSM hook", "selinux: Restrict cross-cgroup dma-buf
charging", and "selinux: Restrict dma-buf direct cgroup charging") add LSM
hooks and SELinux policy checks, there is no baseline DAC,
cgroup_may_write(), ptrace_may_access(), or capability check (such as
CAP_SYS_RESOURCE). On systems without SELinux enabled (or with
CONFIG_SECURITY=n or other LSMs), or on SELinux systems where cgroup
directories lack explicit security.selinux xattrs (-ENODATA in
selinux_dma_buf_charge_cgroup()) or tasks share a domain, unprivileged
cross-cgroup charging remains unrestricted. Should dma_heap_ioctl_allocate()
enforce baseline permission checks on the target cgroup and task in addition
to the LSM hooks?

> +
>  	len = PAGE_ALIGN(heap_allocation->len);
> -	if (!len)
> +	if (!len) {
> +		mem_cgroup_put(memcg);
>  		return ERR_PTR(-EINVAL);
> +	}
>  
>  	dmabuf = heap->ops->allocate(heap, len, heap_allocation->fd_flags,
>  				     heap_allocation->heap_flags);
>  
> -	if (IS_ERR(dmabuf))
> +	if (IS_ERR(dmabuf)) {
> +		mem_cgroup_put(memcg);
>  		return dmabuf;
> +	}
> +
> +	if (!memcg && mem_accounting)
> +		memcg = get_mem_cgroup_from_mm(current->mm);
> +
> +	if (memcg) {
> +		if (!mem_cgroup_charge_dmabuf(dmabuf, memcg, GFP_KERNEL)) {
> +			mem_cgroup_put(memcg);
> +			dma_buf_put(dmabuf);
> +			return ERR_PTR(-ENOMEM);
> +		}
> +	}

[Severity: Medium]
Could calling heap->ops->allocate() before mem_cgroup_charge_dmabuf() in
drivers/dma-buf/dma-heap.c:dma_heap_ioctl_allocate() allow a process in a
memory-limited cgroup to allocate and pin physical memory far beyond its
memory.max limit before the charge check happens?

Prior to commit "memcg: Add MEMCG_DMABUF accounting infrastructure" in this
series, system_heap_allocate() passed __GFP_ACCOUNT to alloc_pages() when
mem_accounting was enabled, charging the memcg incrementally as each page
was allocated and aborting as soon as memory.max was reached.

With __GFP_ACCOUNT removed from system_heap_allocate(),
dma_heap_ioctl_allocate() now allocates and zeroes the entire requested len
of physical backing pages inside heap->ops->allocate() before
mem_cgroup_charge_dmabuf() calls try_charge(). A task confined to a small
memory.max limit can request a multi-gigabyte buffer and force the kernel to
allocate all physical pages (potentially triggering global reclaim or global
OOM) before try_charge() rejects the charge. In addition, if the allocation
would only fit after reclaiming from the target cgroup, allocating the
physical pages before try_charge() prevents cgroup reclaim from freeing the
target cgroup's pages prior to the physical allocation. Could the memcg be
charged before invoking heap->ops->allocate() and uncharged if the
allocation fails?

[ ... ]

> diff --git a/include/uapi/linux/dma-heap.h b/include/uapi/linux/dma-heap.h
> index a4cf716a49fa6..bfcd1c25bc7ea 100644
> --- a/include/uapi/linux/dma-heap.h
> +++ b/include/uapi/linux/dma-heap.h
> @@ -29,6 +29,11 @@
>   *			handle to the allocated dma-buf
>   * @fd_flags:		file descriptor flags used when allocating
>   * @heap_flags:		flags passed to heap
> + * @charge_fd:		optional pidfd or cgroupfd whose cgroup should be
> + *			charged for this allocation; 0 means charge the calling
> + *			process's cgroup when the mem_accounting module parameter
> + *			is enabled, or skip charging otherwise
> + * @__padding:		reserved, must be zero
>   *
>   * Provided by userspace as an argument to the ioctl
>   */
> @@ -37,6 +42,8 @@ struct dma_heap_allocation_data {
>  	__u32 fd;
>  	__u32 fd_flags;
>  	__u64 heap_flags;
> +	__u32 charge_fd;
> +	__u32 __padding;
>  };

[Severity: Medium]
Should drivers/dma-buf/dma-heap.c:dma_heap_ioctl_allocate() reject non-zero
values in heap_allocation->__padding with -EINVAL?

include/uapi/linux/dma-heap.h documents @__padding as "reserved, must be
zero", and dma_heap_ioctl_allocate() validates heap_allocation->fd,
heap_allocation->fd_flags, and heap_allocation->heap_flags, but never checks
heap_allocation->__padding:

drivers/dma-buf/dma-heap.c:dma_heap_ioctl_allocate() {
    if (heap_allocation->fd)
        return ERR_PTR(-EINVAL);

    if (heap_allocation->fd_flags & ~DMA_HEAP_VALID_FD_FLAGS)
        return ERR_PTR(-EINVAL);

    if (heap_allocation->heap_flags & ~DMA_HEAP_VALID_HEAP_FLAGS)
        return ERR_PTR(-EINVAL);

    if (heap_allocation->charge_fd) {
    ...
}

Whereas a later commit in this series validates arg.__padding != 0 in
dma_buf_xfer_charge(), dma_heap_ioctl_allocate() leaves __padding unchecked
throughout the series. Also, using charge_fd == 0 as the unset sentinel
without a flag in heap_flags makes it impossible to pass valid file
descriptor 0 as charge_fd and causes older kernels to silently ignore
charge_fd. Note that extending struct dma_heap_allocation_data here also
causes tools/testing/selftests/dmabuf-heaps/dmabuf-heap.c:
dmabuf_heap_alloc_newer() (which places .garbage1 = 0xffffffff at this
offset) to fail with -EBADF at this commit until the selftest struct layout
is updated in commit 0809a03b2fbc81e6f2b637ef5c0744b2c8ae99f1
("selftests/dmabuf-heaps: Add dma-buf memcg accounting tests").

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