Re: [RFC PATCH 1/2] dma-buf/udmabuf: Introduce CONFIG_UDMABUF_SIZE_LIMIT_MBYTES

[sashiko-bot@kernel.org]

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

New issues:
- [Low] CONFIG_UDMABUF_SIZE_LIMIT_MBYTES lacks a dependency on CONFIG_UDMABUF.
- [Low] Missing range constraint on UDMABUF_SIZE_LIMIT_MBYTES allows negative values, bypassing size validation.

Pre-existing issues:
- [High] Concurrent calls to DMA_BUF_IOCTL_SYNC trigger a race condition in begin_cpu_udmabuf, leading to a memory leak.
--

--- Patch [1]: [RFC PATCH 1/2] dma-buf/udmabuf: Introduce CONFIG_UDMABUF_SIZE_LIMIT_MBYTES ---
commit 258354f822e15700d93d45826a3e51b308dd35b8
Author: Robert Mader <robert.mader@collabora.com>

dma-buf/udmabuf: Introduce CONFIG_UDMABUF_SIZE_LIMIT_MBYTES

This patch introduces a new Kconfig option, CONFIG_UDMABUF_SIZE_LIMIT_MBYTES,
to allow distributions to configure the maximum size limit for udmabufs at 
compile time. The limit defaults to 64MB and was previously only configurable 
via the kernel command line.

> diff --git a/drivers/dma-buf/Kconfig b/drivers/dma-buf/Kconfig
> index 7efc0f0d07126..35f0779cdc801 100644
> --- a/drivers/dma-buf/Kconfig
> +++ b/drivers/dma-buf/Kconfig
> @@ -40,6 +40,12 @@ config UDMABUF
>  	  A driver to let userspace turn memfd regions into dma-bufs.
>  	  Qemu can use this to create host dmabufs for guest framebuffers.
>  
> +config UDMABUF_SIZE_LIMIT_MBYTES
> +	int "Size limit in Mega Bytes"
> +	default 64

[Severity: Low]
Should this config have a 'depends on UDMABUF' directive?

Without it, users might be prompted to configure the size limit even
when the UDMABUF driver itself is disabled, polluting the global config.

[Severity: Low]
Could a user configure a negative value here and bypass the size limits?

If this is set to a negative value, it appears it will wrap around to a 
massive value when cast to u64 in udmabuf_create():

drivers/dma-buf/udmabuf.c:udmabuf_create() {
...
	pglimit = ((u64)size_limit_mb * 1024 * 1024) >> PAGE_SHIFT;
...
}

Would it be safer to add a range constraint to the Kconfig definition?

> +	help
> +	  Maximum size of a udmabuf, in megabytes. Default is 64.
> +
>  config DMABUF_DEBUG
>  	bool "DMA-BUF debug checks"
>  	depends on DMA_SHARED_BUFFER

[ ... ]

> diff --git a/drivers/dma-buf/udmabuf.c b/drivers/dma-buf/udmabuf.c
> index bced421c0d658..a831533263621 100644
> --- a/drivers/dma-buf/udmabuf.c
> +++ b/drivers/dma-buf/udmabuf.c
> @@ -20,7 +20,11 @@ static int list_limit = 1024;
>  module_param(list_limit, int, 0644);
>  MODULE_PARM_DESC(list_limit, "udmabuf_create_list->count limit. Default is 1024.");
>  
> +#ifdef CONFIG_UDMABUF_SIZE_LIMIT_MBYTES
> +static int size_limit_mb = CONFIG_UDMABUF_SIZE_LIMIT_MBYTES;
> +#else
>  static int size_limit_mb = 64;
> +#endif
>  module_param(size_limit_mb, int, 0644);
>  MODULE_PARM_DESC(size_limit_mb, "Max size of a dmabuf, in megabytes. Default is 64.");
>  

[Severity: High]
This is a pre-existing issue, but can concurrent calls to 
DMA_BUF_IOCTL_SYNC cause a memory leak?

Looking at begin_cpu_udmabuf(), the scatter-gather table pointer is 
checked and assigned without locks:

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

If multiple threads invoke the ioctl simultaneously, could they bypass 
the check, allocate multiple tables, and overwrite ubuf->sg, permanently
leaking the previously allocated tables and mappings?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260711144814.8205-1-robert.mader@collabora.com?part=1