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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] dma-buf: heaps: build failure in dma_heap_ioctl_allocate() when CONFIG_MEMCG=n
- [Medium] dma-heap: missing __padding zero check in dma_heap_ioctl_allocate()
--

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

security: dma-buf: Add dma_buf_charge_cgroup LSM hook

This patch adds the security_dma_buf_charge_cgroup() LSM hook and calls it
in dma_heap_ioctl_allocate() when charge_fd refers to a cgroupfd. When
CONFIG_SECURITY is disabled, the hook compiles to a no-op inline returning 0.

> diff --git a/drivers/dma-buf/dma-heap.c b/drivers/dma-buf/dma-heap.c
> index c315739178867..e06131f4655d8 100644
> --- a/drivers/dma-buf/dma-heap.c
> +++ b/drivers/dma-buf/dma-heap.c
> @@ -115,7 +115,13 @@ static struct dma_buf *dma_heap_ioctl_allocate(struct file *file, void *data)
>  
>  			memcg = mem_cgroup_from_css(css);
>  			rcu_read_unlock();
> +
> +			ret = security_dma_buf_charge_cgroup(current_cred(), cgrp);
>  			cgroup_put(cgrp);
> +			if (ret) {
> +				mem_cgroup_put(memcg);
> +				return ERR_PTR(ret);
> +			}

[Severity: Medium]
Will drivers/dma-buf/dma-heap.c fail to build when CONFIG_DMABUF_HEAPS=y and
CONFIG_MEMCG=n (or CONFIG_CGROUPS=n)?

In drivers/dma-buf/Kconfig, DMABUF_HEAPS only selects DMA_SHARED_BUFFER and
does not depend on CONFIG_MEMCG or CONFIG_CGROUPS:

    menuconfig DMABUF_HEAPS
    	bool "DMA-BUF Userland Memory Heaps"
    	select DMA_SHARED_BUFFER

In dma_heap_ioctl_allocate(), the cgroupfd path introduced earlier in this
series ("dma-heap: charge dma-buf memory via explicit memcg") and updated
here unconditionally references cgroup_get_from_fd(), cgroup_css(),
memory_cgrp_subsys, css_tryget_online(), and cgroup_put():

		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 include/linux/cgroup_subsys.h, SUBSYS(memory) (which declares
memory_cgrp_subsys in include/linux/cgroup.h) is only emitted when
CONFIG_MEMCG is enabled:

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

Additionally, in include/linux/cgroup.h, cgroup_get_from_fd(),
css_tryget_online(), cgroup_css(), and cgroup_put() are only declared inside
#ifdef CONFIG_CGROUPS and have no stubs when CONFIG_CGROUPS=n, unlike the
helpers in include/linux/memcontrol.h.

Should the cgroupfd resolution block in dma_heap_ioctl_allocate() be guarded
with #ifdef CONFIG_MEMCG?

>  		} else {
>  			/* charge_fd is a pidfd  */
>  			task = pidfd_get_task(heap_allocation->charge_fd, &pidfd_flags);

[Severity: Medium]
Also in dma_heap_ioctl_allocate(), should non-zero values in
heap_allocation->__padding be rejected with ERR_PTR(-EINVAL)?

Earlier in this series ("dma-heap: charge dma-buf memory via explicit
memcg"), struct dma_heap_allocation_data in include/uapi/linux/dma-heap.h was
extended with charge_fd and __padding:

     * @__padding:		reserved, must be zero
     *
     * Provided by userspace as an argument to the ioctl
     */
    struct dma_heap_allocation_data {
    	__u64 len;
    	__u32 fd;
    	__u32 fd_flags;
    	__u64 heap_flags;
    	__u32 charge_fd;
    	__u32 __padding;
    };

When a userspace process issues ioctl(heap_fd, DMA_HEAP_IOCTL_ALLOC, &alloc)
with a 32-byte struct dma_heap_allocation_data where alloc.__padding != 0,
dma_heap_ioctl() copies all 32 bytes into kdata and calls
dma_heap_ioctl_allocate(file, kdata). However, dma_heap_ioctl_allocate() only
checks fd, fd_flags, and heap_flags before processing charge_fd:

	if (heap_allocation->fd)
		return ERR_PTR(-EINVAL);

	if (heap_allocation->fd_flags & ~DMA_HEAP_VALID_FD_FLAGS)
		return ERR_PTR(-EINVAL);

	if (heap_allocation->heap_flags & ~DMA_HEAP_VALID_HEAP_FLAGS)
		return ERR_PTR(-EINVAL);

Because dma_heap_ioctl() zeroes trailing bytes when ksize > in_size:

	if (ksize > in_size)
		memset(kdata + in_size, 0, ksize - in_size);

legacy 24-byte ioctl callers already have __padding zeroed, whereas 32-byte
callers can currently pass arbitrary non-zero values in __padding without
receiving -EINVAL, preventing __padding from being safely repurposed in the
future.

[ ... ]

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