Re: [PATCH RFC v2 3/8] security: dma-buf: Add dma_buf_charge_task LSM hook

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Critical] dma-buf: heaps: UAF on remote task->mm in dma_heap_ioctl_allocate()
- [High] dma-buf: allocate() runs before memcg charge in dma_heap_ioctl_allocate()
- [Medium] dma-buf: heaps: unguarded cgroup symbols in dma_heap_ioctl_allocate()
--

commit 4f97d3e850a29378e70a6ab0cb15c71860e1fcea
Author: Albert Esteve <aesteve@redhat.com>

security: dma-buf: Add dma_buf_charge_task LSM hook

This commit adds the security_dma_buf_charge_task() LSM hook and calls it
in dma_heap_ioctl_allocate() when charge_fd refers to a process via pidfd,
allowing security modules to control cross-cgroup dma-buf charging.

> diff --git a/drivers/dma-buf/dma-heap.c b/drivers/dma-buf/dma-heap.c
> index be77a4c168ab0..c315739178867 100644
> --- a/drivers/dma-buf/dma-heap.c
> +++ b/drivers/dma-buf/dma-heap.c

[ ... ]

> @@ -81,13 +82,14 @@ static struct dma_buf *dma_heap_ioctl_allocate(struct file *file, void *data)
>  {
>  	struct cgroup *cgrp;
>  	struct cgroup_subsys_state *css;
> +	const struct cred *tcred;
>  	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;
> +	int fd, ret;
>  	size_t len;
>  
>  	if (heap_allocation->fd)

[Severity: Medium]
Will this cause a build regression in dma_heap_ioctl_allocate() when
CONFIG_DMABUF_HEAPS=y is built with CONFIG_MEMCG=n or CONFIG_CGROUPS=n?

In drivers/dma-buf/dma-heap.c, dma_heap_ioctl_allocate() unconditionally
references cgroup_get_from_fd(), cgroup_css(), memory_cgrp_subsys,
css_tryget_online(), and cgroup_put() in the charge_fd resolution block
(introduced earlier in this series by commit "dma-heap: charge dma-buf
memory via explicit memcg" and updated here):

	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);
			}

In drivers/dma-buf/Kconfig, DMABUF_HEAPS only selects DMA_SHARED_BUFFER and
does not depend on CONFIG_MEMCG or CONFIG_CGROUPS. However, in
include/linux/cgroup_subsys.h, SUBSYS(memory) (which declares
memory_cgrp_subsys) is guarded by:

#if IS_ENABLED(CONFIG_MEMCG)
SUBSYS(memory)
#endif

Furthermore, in include/linux/cgroup.h, the SUBSYS() declarations along
with cgroup_get_from_fd(), cgroup_css(), css_tryget_online(), and
cgroup_put() are only declared inside #ifdef CONFIG_CGROUPS without stub
definitions when CONFIG_CGROUPS is disabled. Should this path be guarded by
IS_ENABLED(CONFIG_MEMCG), or should Kconfig dependencies/stubs be added?

> @@ -120,6 +122,14 @@ static struct dma_buf *dma_heap_ioctl_allocate(struct file *file, void *data)
>  			if (IS_ERR(task))
>  				return ERR_CAST(task);
>  
> +			tcred = get_task_cred(task);
> +			ret = security_dma_buf_charge_task(current_cred(), tcred);
> +			put_cred(tcred);
> +			if (ret) {
> +				put_task_struct(task);
> +				return ERR_PTR(ret);
> +			}
> +
>  			memcg = get_mem_cgroup_from_mm(task->mm);

[Severity: Critical]
Can passing task->mm directly to get_mem_cgroup_from_mm(task->mm) in
dma_heap_ioctl_allocate() trigger a use-after-free on struct mm_struct, or
silently charge the wrong memcg when task->mm is NULL?

When resolving heap_allocation->charge_fd via pidfd_get_task(), the caller
only holds a reference on task->usage, which does not pin task->mm. After
security_dma_buf_charge_task() returns 0, dma_heap_ioctl_allocate() reads
task->mm and passes it to get_mem_cgroup_from_mm(task->mm) without holding
task_lock(task) or acquiring an mm reference via get_task_mm(task) (a
pattern introduced in commit "dma-heap: charge dma-buf memory via explicit
memcg", modified here, and duplicated in commit "dma-buf: transfer memcg
charges via DMA_BUF_IOCTL_XFER_CHARGE").

If the target task concurrently exits via do_exit() -> exit_mm() in
kernel/exit.c (or replaces its mm during exec via exec_mmap() ->
exec_mm_put_old() in fs/exec.c), it clears task->mm under task_lock() and
drops the last reference via mmput() -> __mmdrop() -> free_mm():

	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 and __mmdrop() frees the
mm_struct immediately without an RCU grace period, get_mem_cgroup_from_mm()
in mm/memcontrol.c can dereference freed memory at mm->owner:

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

In addition, if the target task is already a zombie (having completed
exit_mm() while still reachable via pidfd_get_task()) or a kernel thread,
task->mm is NULL. Even though get_task_cred(task) and
security_dma_buf_charge_task() authorized charging the target task,
get_mem_cgroup_from_mm(NULL) in mm/memcontrol.c takes the !mm fallback:

	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;
	}

which silently charges current->mm (the allocating caller's own memcg) or
root_mem_cgroup instead of charging the target task's cgroup or returning
an error. Should this pin the mm with get_task_mm(task) / mmput(mm) (or
resolve the memcg from the task under rcu_read_lock() via
mem_cgroup_from_task())?

>  			put_task_struct(task);
>  		}

[Severity: High]
Should the memcg charge happen before heap->ops->allocate() allocates the
physical backing pages in dma_heap_ioctl_allocate()?

Immediately following the charge_fd resolution in drivers/dma-buf/dma-heap.c,
dma_heap_ioctl_allocate() invokes heap->ops->allocate() for the full
requested len before charging memcg via mem_cgroup_charge_dmabuf():

	dmabuf = heap->ops->allocate(heap, len, heap_allocation->fd_flags,
				     heap_allocation->heap_flags);

	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);
		}
	}

Because commit "memcg: Add MEMCG_DMABUF accounting infrastructure" removed
__GFP_ACCOUNT from alloc_largest_available() in
drivers/dma-buf/heaps/system_heap.c:

		flags = order_flags[i];
		page = alloc_pages(flags, orders[i]);

system_heap_allocate() allocates and zeroes the entire buffer of size len
using uncharged alloc_pages() calls before try_charge() is evaluated in
mem_cgroup_charge_dmabuf(). Can a process inside a memory-limited cgroup
(or targeting one via charge_fd in DMA_HEAP_IOCTL_ALLOC) use this window to
exhaust host physical memory and trigger the global OOM killer before the
cgroup limit check runs?

[ ... ]

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