[PATCH v2 0/2] udmabuf: Improve list length limit handling

[PATCH v2 0/2] udmabuf: Improve list length limit handling

[Val Packett]

v2: Set a reasonable size, and fix size overflow check (Christian)
    I've put the check into a separate patch that's Cc'ed to stable
    and has a Fixes tag since it has been an issue since the size
    has been allowed to be changed via tunables and the condition
    has been reachable like that, independent of me changing the
    default value..
v1: https://lore.kernel.org/all/20261006005053.2245156-1-val@invisiblethingslab.com/

Val Packett (2):
  udmabuf: Fix list length overflow check
  udmabuf: Increase the default list length limit

 drivers/dma-buf/udmabuf.c | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

-- 
2.55.0

[PATCH v2 1/2] udmabuf: Fix list length overflow check

[Val Packett]

The udmabuf list length limit was made configurable, creating the
possibility for integer overflow that was not properly detected.
If the system administrator set the udmabuf.list_limit tunable to
a large value like INT_MAX, that would allow udmabuf users to submit
a list whose length would overflow the u32 length variable.

Use memdup_array_user() instead of raw memdup_user() to make the length
calculation an overflow-checked multiplication.

Cc: stable@vger.kernel.org
Fixes: 2e717a55982a ("udmabuf: configurable list_limit and size_limit_mb")
Suggested-by: Christian König <christian.koenig@amd.com>
Signed-off-by: Val Packett <val@invisiblethingslab.com>
---
 drivers/dma-buf/udmabuf.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/dma-buf/udmabuf.c b/drivers/dma-buf/udmabuf.c
index df6dd0046242..654ebe7f6b3f 100644
--- a/drivers/dma-buf/udmabuf.c
+++ b/drivers/dma-buf/udmabuf.c
@@ -470,14 +470,13 @@ 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;
 
 	if (copy_from_user(&head, (void __user *)arg, sizeof(head)))
 		return -EFAULT;
 	if (head.count > list_limit)
 		return -EINVAL;
-	lsize = sizeof(struct udmabuf_create_item) * head.count;
-	list = memdup_user((void __user *)(arg + sizeof(head)), lsize);
+	list = memdup_array_user((void __user *)(arg + sizeof(head)),
+				head.count, sizeof(struct udmabuf_create_item));
 	if (IS_ERR(list))
 		return PTR_ERR(list);
 
-- 
2.55.0

[PATCH v2 2/2] udmabuf: Increase the default list length limit

[Val Packett]

Shared memory buffers for software-rendered GUI applications are often
large and highly fragmented in memory. To get a shareable handle to such
a buffer from a guest VM, a virtio-gpu device backend can use the
UDMABUF_CREATE_LIST ioctl with the memfds backing guest RAM and the list
of guest physical pages sent by the guest driver. That list reaches 80K
entries for a maximized window on a 4K display!

Increase the default limit to 131072.

Signed-off-by: Val Packett <val@invisiblethingslab.com>
---
 drivers/dma-buf/udmabuf.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/dma-buf/udmabuf.c b/drivers/dma-buf/udmabuf.c
index 654ebe7f6b3f..dd913f9f3b06 100644
--- a/drivers/dma-buf/udmabuf.c
+++ b/drivers/dma-buf/udmabuf.c
@@ -16,9 +16,9 @@
 #include <linux/vmalloc.h>
 #include <linux/iosys-map.h>
 
-static int list_limit = 1024;
+static int list_limit = 131072;
 module_param(list_limit, int, 0644);
-MODULE_PARM_DESC(list_limit, "udmabuf_create_list->count limit. Default is 1024.");
+MODULE_PARM_DESC(list_limit, "udmabuf_create_list->count limit. Default is 131072.");
 
 static int size_limit_mb = INT_MAX;
 module_param(size_limit_mb, int, 0644);
-- 
2.55.0

Re: [PATCH v2 2/2] udmabuf: Increase the default list length limit

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] udmabuf: order-10 contiguous allocation in udmabuf_ioctl_create_list()
- [Low] udmabuf: missing kernel-parameters.txt update for list_limit parameter
--

commit 70d859f4b270b1a12dc2cb5cc01faf794efe4700
Author: Val Packett <val@invisiblethingslab.com>

udmabuf: Increase the default list length limit

This commit increases the default list_limit module parameter in
drivers/dma-buf/udmabuf.c from 1024 to 131072 to support larger page lists
for fragmented guest memory buffers.

> diff --git a/drivers/dma-buf/udmabuf.c b/drivers/dma-buf/udmabuf.c
> index 6fb69d8399c18..9aec0e3a1e49b 100644
> --- a/drivers/dma-buf/udmabuf.c
> +++ b/drivers/dma-buf/udmabuf.c
> @@ -16,9 +16,9 @@
>  #include <linux/vmalloc.h>
>  #include <linux/iosys-map.h>
>  
> -static int list_limit = 1024;
> +static int list_limit = 131072;

[Severity: Medium]
With list_limit raised to 131072, udmabuf_ioctl_create_list() in
drivers/dma-buf/udmabuf.c copies the user item array using
memdup_array_user():

	if (head.count > list_limit)
		return -EINVAL;
	list = memdup_array_user((void __user *)(arg + sizeof(head)),
				 head.count, sizeof(struct udmabuf_create_item));
	if (IS_ERR(list))
		return PTR_ERR(list);

	ret = udmabuf_create(filp->private_data, &head, list);
	kfree(list);

which calls memdup_user() in mm/util.c:

	p = kmem_buckets_alloc_track_caller(user_buckets, len, GFP_USER | __GFP_NOWARN);
	if (!p)
		return ERR_PTR(-ENOMEM);

Each struct udmabuf_create_item is 24 bytes, so a head.count of 80,000 to
131,072 entries requests roughly 1.83 MiB to 3 MiB (order 9 to order 10 on
4 KiB page systems).

Because memdup_array_user() -> memdup_user() ->
kmem_buckets_alloc_track_caller() allocates physically contiguous memory
without a vmalloc fallback, could this fail with -ENOMEM or stall in direct
compaction on hosts with fragmented physical memory (or fail unconditionally
when the maximum page allocation order is less than 10)?

Should udmabuf_ioctl_create_list() use vmemdup_array_user() and kvfree() for
list instead?

>  module_param(list_limit, int, 0644);
> -MODULE_PARM_DESC(list_limit, "udmabuf_create_list->count limit. Default is 1024.");
> +MODULE_PARM_DESC(list_limit, "udmabuf_create_list->count limit. Default is 131072.");

[Severity: Low]
Since list_limit in drivers/dma-buf/udmabuf.c is exposed as a kernel
command-line parameter via module_param(list_limit, int, 0644) (as
udmabuf.list_limit), should Documentation/admin-guide/kernel-parameters.txt
also be updated to document this parameter and its new default?

>  
>  static int size_limit_mb = INT_MAX;
>  module_param(size_limit_mb, int, 0644);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261021083933.1983894-1-val@invisiblethingslab.com?part=2