Re: [PATCH] dma-buf: udmabuf: validate create-list count before copying

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

Pre-existing issues:
- [Critical] Integer overflow in `udmabuf_create()` leads to undersized array allocation and a subsequent heap buffer overflow when pinning folios on 32-bit systems.
- [High] Unprotected initialization of `ubuf->sg` in `begin_cpu_udmabuf` leads to memory leaks and data races.
--

commit a036fc929a16b1c94a5ac12cbef23efece456576
Author: Samuel Moelius <sam.moelius@trailofbits.com>

dma-buf: udmabuf: validate create-list count before copying
    
UDMABUF_CREATE_LIST copies a variable-length list using a byte count
derived from head.count. The list_limit module parameter is signed and
writable, so setting it negative lets a large unsigned count bypass the
limit check. The u32 byte-count calculation can then wrap, causing only
a small list to be copied while udmabuf_create() still iterates over the
large count.

Reject negative list_limit values and use checked size_t multiplication
before copying the list.

> diff --git a/drivers/dma-buf/udmabuf.c b/drivers/dma-buf/udmabuf.c
> index bced421c0d658..264718676d485 100644
> --- a/drivers/dma-buf/udmabuf.c
> +++ b/drivers/dma-buf/udmabuf.c

[ ... ]

> @@ -469,13 +470,14 @@ static long udmabuf_ioctl_create_list(struct file *filp, unsigned long arg)
>  	struct udmabuf_create_list head;
>  	struct udmabuf_create_item *list;
>  	int ret = -EINVAL;
> -	u32 lsize;
> +	size_t lsize;
>  
>  	if (copy_from_user(&head, (void __user *)arg, sizeof(head)))
>  		return -EFAULT;
> -	if (head.count > list_limit)
> +	if (list_limit < 0 || head.count > list_limit)
> +		return -EINVAL;
> +	if (check_mul_overflow(sizeof(struct udmabuf_create_item), head.count, &lsize))
>  		return -EINVAL;
> -	lsize = sizeof(struct udmabuf_create_item) * head.count;
>  	list = memdup_user((void __user *)(arg + sizeof(head)), lsize);
>  	if (IS_ERR(list))
>  		return PTR_ERR(list);

[Severity: Critical]
This is a pre-existing issue, but can the 32-bit addition of subpgcnt wrap
pgcnt?

In udmabuf_create(), list[i].size is a user-controlled __u64 value.
On 32-bit architectures, pgoff_t is a 32-bit unsigned integer:

drivers/dma-buf/udmabuf.c:udmabuf_create() {
    ...
		subpgcnt = list[i].size >> PAGE_SHIFT;
		pgcnt += subpgcnt;
		if (pgcnt > pglimit)
			goto err_noinit;
    ...
}

If user space provides two items—one small (e.g., 2 pages) and one huge
(e.g., size = 0xffffffff000ULL)—subpgcnt truncates to 32 bits and pgcnt can
wrap around to a small positive value. This bypasses the pgcnt > pglimit
check.

init_udmabuf() then allocates arrays sized by the wrapped pgcnt:

drivers/dma-buf/udmabuf.c:init_udmabuf() {
    ...
	ubuf->pages = kvmalloc_objs(*ubuf->pages, pgcnt);
    ...
}

Later, udmabuf_pin_folios() calculates a local 32-bit pgcnt = 0xffffffff and
asks memfd_pin_folios() to pin up to that many folios. If memfd_pin_folios()
returns a large number of folios, udmabuf_pin_folios() unconditionally writes
these kernel pointers into the undersized arrays, resulting in an out-of-bounds
write:

drivers/dma-buf/udmabuf.c:udmabuf_pin_folios() {
    ...
		ubuf->pinned_folios[nr_pinned++] = folios[cur_folio];
    ...
			ubuf->pages[upgcnt] = folio_page(folios[cur_folio],
						subpgoff >> PAGE_SHIFT);
    ...
}

Could we check for overflow when accumulating pgcnt?

[Severity: High]
This isn't a bug introduced by this patch, but does begin_cpu_udmabuf() have a
race condition when allocating ubuf->sg?

drivers/dma-buf/udmabuf.c:begin_cpu_udmabuf() {
	if (!ubuf->sg) {
		ubuf->sg = get_sg_table(dev, buf, direction);
		if (IS_ERR(ubuf->sg)) {
			ret = PTR_ERR(ubuf->sg);
			ubuf->sg = NULL;
		} else {
			ubuf->sg_dir = direction;
		}
	} else {
    ...
}

The DMA_BUF_IOCTL_SYNC ioctl triggers dma_buf_begin_cpu_access(), which calls
the exporter's .begin_cpu_access hook (begin_cpu_udmabuf()) with the dma-buf
reservation lock intentionally unlocked (as documented in dma-buf.h).

If two user-space threads invoke DMA_BUF_IOCTL_SYNC concurrently on the same
file descriptor, both can observe !ubuf->sg, allocate separate SG tables, and
establish duplicate DMA mappings.

The second thread will overwrite the ubuf->sg pointer, leaking the first
allocated SG table and its DMA resources. Concurrent readers could also operate
on partially written pointers.

Is there a lock we can hold while checking and assigning ubuf->sg?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260605154252.2125942-1-sam.moelius@trailofbits.com?part=1