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

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