Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider: - [Critical] dma-heap: missing permission check in mem_cgroup_get_from_fd() - [High] dma-buf: system_heap: 32-bit nr_pages truncation in system_heap_allocate() - [Medium] mm: memcontrol: missing EXPORT_SYMBOL_GPL() on dmabuf memcg helpers - [Medium] mm: memcontrol: mem_cgroup_get_from_fd() returns NULL when memcg is disabled --
commit 961ee31303129349ee7fa6988bef4bc244f22a61 Author: Albert Esteve <[email protected]> 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 pidfd or cgroupfd via mem_cgroup_get_from_fd() and charge system heap allocations to that memcg. [ ... ] > diff --git a/drivers/dma-buf/heaps/system_heap.c > b/drivers/dma-buf/heaps/system_heap.c > index c8959eadc71d1..adeea8cd328b8 100644 > --- a/drivers/dma-buf/heaps/system_heap.c > +++ b/drivers/dma-buf/heaps/system_heap.c > @@ -17,6 +17,7 @@ > #include <linux/err.h> > #include <linux/highmem.h> > #include <linux/mem_encrypt.h> > +#include <linux/memcontrol.h> > #include <linux/mm.h> > #include <linux/set_memory.h> > #include <linux/module.h> > @@ -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; [Severity: High] Can nr_pages truncate on 64-bit systems when a caller passes a large heap_allocation->len via DMA_HEAP_IOCTL_ALLOC? In dma_heap_ioctl_allocate(), heap_allocation->len is page-aligned into a size_t len and only checked for zero before being passed to system_heap_allocate() as an unsigned long: 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 keeps the full 64-bit len, while nr_pages is stored in a 32-bit unsigned int. Because __GFP_ACCOUNT is removed from alloc_largest_available(), cgroup enforcement now relies solely on mem_cgroup_reserve_dmabuf(memcg, GFP_KERNEL, nr_pages). If userspace passes len >= (1ULL << (32 + PAGE_SHIFT)) (for example, 16 TiB with 4 KiB pages), nr_pages wraps to 0 (or a small value), so mem_cgroup_reserve_dmabuf() succeeds without charging the cgroup while the while (size_remaining > 0) loop continues allocating uncharged physical pages until global memory exhaustion. In addition, values of nr_pages in [0x80000000U, 0xffffffffU] are passed by mem_cgroup_commit_dmabuf() to mod_memcg_state(), which takes a signed int val and would corrupt MEMCG_DMABUF statistics. > 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); > > + buffer = kzalloc_obj(*buffer); > + if (!buffer) { > + ret = -ENOMEM; > + goto cancel_charge; > + } > + > INIT_LIST_HEAD(&buffer->attachments); > mutex_init(&buffer->lock); > buffer->heap = heap; > @@ -478,6 +484,9 @@ static struct dma_buf *system_heap_allocate(struct > dma_heap *heap, > ret = PTR_ERR(dmabuf); > goto free_pages; > } > + > + mem_cgroup_commit_dmabuf(dmabuf, memcg, nr_pages); > + > return dmabuf; > > free_pages: > @@ -498,6 +507,8 @@ static struct dma_buf *system_heap_allocate(struct > dma_heap *heap, > list_for_each_entry_safe(page, tmp_page, &pages, lru) > __free_pages(page, compound_order(page)); > kfree(buffer); > +cancel_charge: > + mem_cgroup_cancel_dmabuf(memcg, nr_pages); [Severity: Medium] Will building with CONFIG_DMABUF_HEAPS_SYSTEM=m and CONFIG_MEMCG=y fail at modpost with undefined symbol errors for mem_cgroup_reserve_dmabuf(), mem_cgroup_commit_dmabuf(), and mem_cgroup_cancel_dmabuf()? In drivers/dma-buf/heaps/Kconfig, DMABUF_HEAPS_SYSTEM is tristate, so drivers/dma-buf/heaps/system_heap.c can be compiled as a loadable module (system_heap.ko). However, mem_cgroup_reserve_dmabuf(), mem_cgroup_commit_dmabuf(), and mem_cgroup_cancel_dmabuf() in mm/memcontrol.c do not have EXPORT_SYMBOL_GPL() annotations. Should those symbols be exported (or should the reservation/commit/cancel calls live in the built-in drivers/dma-buf/dma-heap.c layer as described in the commit message)? [ ... ] > diff --git a/mm/memcontrol.c b/mm/memcontrol.c > index bbd46ba40350b..f66254f52b999 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; > +} [Severity: Critical] What prevents an unprivileged user with access to a dma-heap device from passing a cgroupfd (opened with O_RDONLY | O_DIRECTORY or O_PATH on any world-readable /sys/fs/cgroup directory) or a pidfd (via pidfd_open() on PID 1 or another user's process) as charge_fd to DMA_HEAP_IOCTL_ALLOC? Through dma_heap_ioctl_allocate() -> mem_cgroup_get_from_fd() and system_heap_allocate() -> mem_cgroup_reserve_dmabuf() -> try_charge_memcg(), charging to the root cgroup (where mem_cgroup_is_root(memcg) skips page counter charging) allows a restricted process to bypass its own memory.max and memory.high limits, while charging to another cgroup allows exhausting that victim cgroup's quota and triggering mem_cgroup_oom() kills inside it. Later patches 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 checks, but there are no baseline DAC, capability, ptrace_may_access(), or cgroup write permission checks when CONFIG_SECURITY is disabled, when SELinux is not active, or when a cgroup directory has no explicit security.selinux xattr (where selinux_dma_buf_charge_cgroup() returns 0 on -ENODATA). Also, when charging a remote memcg via try_charge_memcg() breaches that target cgroup's memory.high limit, try_charge_memcg() increments current->memcg_nr_pages_over_high and calls __mem_cgroup_handle_over_high(), which resolves get_mem_cgroup_from_mm(current->mm) and reclaims from the allocating process's own cgroup rather than the remotely charged memcg. Should remote charging require appropriate permissions over the target task or cgroup, and how should memory.high reclaim be handled when charging a foreign memcg? [Severity: Medium] In mem_cgroup_get_from_fd(), what happens on the pidfd path when CONFIG_MEMCG=y and memory cgroups are disabled at boot via cgroup_disable=memory? When mem_cgroup_disabled() is true, get_mem_cgroup_from_mm(mm) returns NULL, so mem_cgroup_get_from_fd() returns NULL instead of an ERR_PTR as documented in its kernel-doc comment. In dma_heap_ioctl_allocate() (and in dma_buf_ioctl_xfer_charge() later in the series), IS_ERR(memcg) is false for NULL, so a pidfd charge_fd silently falls through while a cgroupfd charge_fd fails with -ENOENT (because cgroup_css() returns NULL when the subsystem is disabled). Should the pidfd path also return an ERR_PTR when get_mem_cgroup_from_mm(mm) returns NULL? -- Sashiko AI review · https://sashiko.dev/#/patchset/20261027-v2_20230123_tjmercier_google_com-v3-0-78d2c4cb8...@redhat.com?part=2
