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

[sashiko-bot@kernel.org]

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 <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 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-78d2c4cb8647@redhat.com?part=2