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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Critical] mm: memcontrol: missing permission checks in mem_cgroup_get_from_fd()
- [High] dma-buf: system_heap: 32-bit nr_pages truncation in system_heap_allocate()
--

commit e390a33c5307a75778a81c2ca5beff0530617904
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
that DMA_HEAP_IOCTL_ALLOC can resolve a target pidfd or cgroupfd and charge
the exported dma-buf memory to that memcg via mem_cgroup_reserve_dmabuf().
It also removes __GFP_ACCOUNT from system heap page allocations to avoid
double counting.

> diff --git a/drivers/dma-buf/heaps/system_heap.c b/drivers/dma-buf/heaps/system_heap.c
> index c8959eadc71d1..2bd245e1eab8f 100644
> --- a/drivers/dma-buf/heaps/system_heap.c
> +++ b/drivers/dma-buf/heaps/system_heap.c
[ ... ]
> @@ -388,8 +389,6 @@ static struct page *alloc_largest_available(unsigned long size,
>  		if (max_order < orders[i])
>  			continue;
>  		flags = order_flags[i];
> -		if (mem_accounting)
> -			flags |= __GFP_ACCOUNT;
>  		page = alloc_pages(flags, orders[i]);
>  		if (!page)
>  			continue;
> @@ -401,11 +400,13 @@ static struct page *alloc_largest_available(unsigned long size,
>  static struct dma_buf *system_heap_allocate(struct dma_heap *heap,
>  					    unsigned long len,
>  					    u32 fd_flags,
> -					    u64 heap_flags)
> +					    u64 heap_flags,
> +					    struct mem_cgroup *memcg)
>  {
>  	struct system_heap_buffer *buffer;
>  	DEFINE_DMA_BUF_EXPORT_INFO(exp_info);
>  	unsigned long size_remaining = len;
> +	unsigned int nr_pages = PAGE_ALIGN(len) >> PAGE_SHIFT;
>  	unsigned int max_order = orders[0];
>  	struct system_heap_priv *priv = dma_heap_get_drvdata(heap);
>  	bool cc_shared = priv->cc_shared;
> @@ -416,10 +417,15 @@ static struct dma_buf *system_heap_allocate(struct dma_heap *heap,
>  	struct page *page, *tmp_page;
>  	int i, ret = -ENOMEM;
>  
> -	buffer = kzalloc_obj(*buffer);
> -	if (!buffer)
> +	if (!mem_cgroup_reserve_dmabuf(memcg, GFP_KERNEL, nr_pages))
>  		return ERR_PTR(-ENOMEM);

[Severity: High]
As reported by Sashiko on v3 [1], can truncating nr_pages to a 32-bit
unsigned int in system_heap_allocate() allow a 64-bit allocation length to
bypass memcg memory.max limits?

In dma_heap_ioctl_allocate(), heap_allocation->len (__u64) is page-aligned
into a 64-bit size_t len and only checked for wrap-around to zero before
being passed as an unsigned long len to system_heap_allocate():

drivers/dma-buf/dma-heap.c:dma_heap_ioctl_allocate() {
	len = PAGE_ALIGN(heap_allocation->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, memcg);
}

In system_heap_allocate(), size_remaining retains the full 64-bit len while
nr_pages is truncated to unsigned int. Because __GFP_ACCOUNT is removed from
alloc_largest_available(), mem_cgroup_reserve_dmabuf() becomes the only
memcg limit check.

On a 64-bit system with 4 KiB pages (PAGE_SHIFT == 12), if a user passes
len = (1ULL << 44) (or (1ULL << 44) + PAGE_SIZE), PAGE_ALIGN(len) >>
PAGE_SHIFT truncates in 32-bit unsigned int nr_pages to 0 (or 1).
mem_cgroup_reserve_dmabuf() then charges 0 (or 1) pages and succeeds inside
a restricted cgroup, while the while (size_remaining > 0) loop allocates
uncharged physical pages for the full 64-bit size_remaining until host RAM
is exhausted and the global OOM killer is triggered.

Similarly, in dma_buf_export() and mem_cgroup_uncharge_dmabuf(),
PAGE_ALIGN(size) >> PAGE_SHIFT is stored in an unsigned int nr_pages and
passed to mod_memcg_state(), which takes a signed int val and can overflow
to a negative delta when bit 31 of nr_pages is set. Should nr_pages be
unsigned long (or should len be bounds-checked before converting to page
counts)?

[ ... ]
> diff --git a/mm/memcontrol.c b/mm/memcontrol.c
> index e53dd7eebc3b5..17fdefc56b97f 100644
> --- a/mm/memcontrol.c
> +++ b/mm/memcontrol.c
> @@ -5661,6 +5661,53 @@ void mem_cgroup_flush_workqueue(void)
>  	flush_workqueue(memcg_wq);
>  }
>  
> +/**
> + * mem_cgroup_get_from_fd - resolve a memcg from a cgroupfd or pidfd
> + * @fd: file descriptor (cgroupfd or pidfd)
> + *
> + * Tries @fd as a cgroupfd first; if that fails, tries it as a pidfd.
> + * On success returns a referenced memcg pointer. On failure returns
> + * an ERR_PTR.
> + */
> +struct mem_cgroup *mem_cgroup_get_from_fd(int fd)
> +{
> +	struct cgroup_subsys_state *css;
> +	struct mem_cgroup *memcg;
> +	struct cgroup *cgrp;
> +	struct task_struct *task;
> +	struct mm_struct *mm;
> +	unsigned int pidfd_flags;
> +
> +	cgrp = cgroup_get_from_fd(fd);
> +	if (!IS_ERR(cgrp)) {
> +		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);
> +		return memcg;
> +	}
> +
> +	task = pidfd_get_task(fd, &pidfd_flags);
> +	if (IS_ERR(task))
> +		return ERR_CAST(task);
> +
> +	mm = get_task_mm(task);
> +	put_task_struct(task);
> +	if (!mm)
> +		return ERR_PTR(-ESRCH);
> +
> +	memcg = get_mem_cgroup_from_mm(mm);
> +	mmput(mm);
> +	return memcg ?: ERR_PTR(-ENOENT);
> +}

[Severity: Critical]
As reported by Sashiko on v3 [2], does mem_cgroup_get_from_fd() allow an
unprivileged caller to charge dma-buf allocations to arbitrary cgroups
without permission checks?

When dma_heap_ioctl_allocate() calls mem_cgroup_get_from_fd() with a
user-supplied charge_fd, the target mem_cgroup is resolved either from a
cgroup v2 directory fd (which can be opened with O_RDONLY | O_DIRECTORY or
O_PATH on any world-readable /sys/fs/cgroup directory, including the root
cgroup) or from a pidfd obtained via pidfd_open() for any visible process
such as PID 1.

Neither path verifies DAC permissions, cgroup write access (such as
cgroup_may_write() or cgroup_attach_permissions() used by CLONE_INTO_CGROUP
in cgroup_css_set_fork()), ptrace_may_access(), or capabilities such as
CAP_SYS_ADMIN / CAP_SYS_RESOURCE.

When system_heap_allocate() then calls mem_cgroup_reserve_dmabuf(memcg,
GFP_KERNEL, nr_pages) -> try_charge(), targeting root_mem_cgroup returns 0
immediately and bypasses the caller's memory.max and memory.high limits,
while targeting another cgroup can trigger reclaim and OOM kills in that
victim cgroup. In addition, when a remote target memcg exceeds memory.high
during try_charge_memcg(), the over-high penalty and reclaim in
__mem_cgroup_handle_over_high() are applied against current->mm's memcg
rather than the remotely charged memcg.

Later commits in the 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, but still
leave no baseline DAC, capability, cgroup write permission, or
ptrace_may_access() checks when CONFIG_SECURITY is disabled, when SELinux is
not the active LSM, or when a cgroup v2 directory uses default genfscon
labeling without an explicit security.selinux kernfs xattr (where
selinux_dma_buf_charge_cgroup() sees kernfs_xattr_get() == -ENODATA and
returns 0).

Should baseline permission checks be enforced before allowing a process to
charge memory to another task's or cgroup's memcg?

[1] https://lore.kernel.org/r/20261027-v2_20230123_tjmercier_google_com-v3-2-78d2c4cb8647@redhat.com
[2] https://lore.kernel.org/r/20261027-v2_20230123_tjmercier_google_com-v3-2-78d2c4cb8647@redhat.com

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