UDMABUF_CREATE_LIST copies an array whose element count comes from userspace. The count is compared against list_limit, but list_limit is a signed module parameter while the count is u32.
If the limit is raised too far or made negative, that comparison no longer bounds the count to a range where sizeof(*list) * count fits in the u32 temporary used for the copy length. A wrapped copy length lets memdup_user() copy fewer entries than udmabuf_create() subsequently walks, leading to out-of-bounds reads from the copied list.
Take a positive snapshot of the module limit and use memdup_array_user() so the multiplication is checked before copying.
Signed-off-by: Yousef Alhouseen alhouseenyousef@gmail.com --- drivers/dma-buf/udmabuf.c | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-)
diff --git a/drivers/dma-buf/udmabuf.c b/drivers/dma-buf/udmabuf.c index bced421c0..b4078ec84 100644 --- a/drivers/dma-buf/udmabuf.c +++ b/drivers/dma-buf/udmabuf.c @@ -469,14 +469,15 @@ 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; + int limit;
if (copy_from_user(&head, (void __user *)arg, sizeof(head))) return -EFAULT; - if (head.count > list_limit) + limit = READ_ONCE(list_limit); + if (!head.count || limit <= 0 || head.count > 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(*list)); if (IS_ERR(list)) return PTR_ERR(list);
On 6/24/26 14:52, Yousef Alhouseen wrote:
UDMABUF_CREATE_LIST copies an array whose element count comes from userspace. The count is compared against list_limit, but list_limit is a signed module parameter while the count is u32.
We should probably just drop the sign from the module parameter instead.
I don't see an use case for negative values here.
Regards, Christian.
If the limit is raised too far or made negative, that comparison no longer bounds the count to a range where sizeof(*list) * count fits in the u32 temporary used for the copy length. A wrapped copy length lets memdup_user() copy fewer entries than udmabuf_create() subsequently walks, leading to out-of-bounds reads from the copied list.
Take a positive snapshot of the module limit and use memdup_array_user() so the multiplication is checked before copying.
Signed-off-by: Yousef Alhouseen alhouseenyousef@gmail.com
drivers/dma-buf/udmabuf.c | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-)
diff --git a/drivers/dma-buf/udmabuf.c b/drivers/dma-buf/udmabuf.c index bced421c0..b4078ec84 100644 --- a/drivers/dma-buf/udmabuf.c +++ b/drivers/dma-buf/udmabuf.c @@ -469,14 +469,15 @@ 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;
int limit; if (copy_from_user(&head, (void __user *)arg, sizeof(head))) return -EFAULT;
if (head.count > list_limit)
limit = READ_ONCE(list_limit);if (!head.count || limit <= 0 || head.count > 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(*list)); if (IS_ERR(list)) return PTR_ERR(list);-- 2.54.0
Hi Christian,
Agreed. I sent a follow-up that makes list_limit unsigned and keeps the checked array copy path.
Thanks, Yousef
On Wed, 24 Jun 2026 14:58:58 +0200, "Christian König" christian.koenig@amd.com wrote:
On 6/24/26 14:52, Yousef Alhouseen wrote:
UDMABUF_CREATE_LIST copies an array whose element count comes from userspace. The count is compared against list_limit, but list_limit is a signed module parameter while the count is u32.
We should probably just drop the sign from the module parameter instead.
I don't see an use case for negative values here.
Regards, Christian.
If the limit is raised too far or made negative, that comparison no longer bounds the count to a range where sizeof(*list) * count fits in the u32 temporary used for the copy length. A wrapped copy length lets memdup_user() copy fewer entries than udmabuf_create() subsequently walks, leading to out-of-bounds reads from the copied list.
Take a positive snapshot of the module limit and use memdup_array_user() so the multiplication is checked before copying.
Signed-off-by: Yousef Alhouseen alhouseenyousef@gmail.com
drivers/dma-buf/udmabuf.c | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-)
diff --git a/drivers/dma-buf/udmabuf.c b/drivers/dma-buf/udmabuf.c index bced421c0..b4078ec84 100644 --- a/drivers/dma-buf/udmabuf.c +++ b/drivers/dma-buf/udmabuf.c @@ -469,14 +469,15 @@ 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;
- int limit;
if (copy_from_user(&head, (void __user *)arg, sizeof(head))) return -EFAULT;
- if (head.count > list_limit)
- limit = READ_ONCE(list_limit);
- if (!head.count || limit <= 0 || head.count > 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(*list));
if (IS_ERR(list)) return PTR_ERR(list);
-- 2.54.0
On Wed, 24 Jun 2026 14:58:58 +0200 Christian König christian.koenig@amd.com wrote:
On 6/24/26 14:52, Yousef Alhouseen wrote:
UDMABUF_CREATE_LIST copies an array whose element count comes from userspace. The count is compared against list_limit, but list_limit is a signed module parameter while the count is u32.
We should probably just drop the sign from the module parameter instead.
Does anything sanity-check the module parameter?
David
I don't see an use case for negative values here.
Regards, Christian.
If the limit is raised too far or made negative, that comparison no longer bounds the count to a range where sizeof(*list) * count fits in the u32 temporary used for the copy length. A wrapped copy length lets memdup_user() copy fewer entries than udmabuf_create() subsequently walks, leading to out-of-bounds reads from the copied list.
Take a positive snapshot of the module limit and use memdup_array_user() so the multiplication is checked before copying.
Signed-off-by: Yousef Alhouseen alhouseenyousef@gmail.com
drivers/dma-buf/udmabuf.c | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-)
diff --git a/drivers/dma-buf/udmabuf.c b/drivers/dma-buf/udmabuf.c index bced421c0..b4078ec84 100644 --- a/drivers/dma-buf/udmabuf.c +++ b/drivers/dma-buf/udmabuf.c @@ -469,14 +469,15 @@ 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;
int limit; if (copy_from_user(&head, (void __user *)arg, sizeof(head))) return -EFAULT;
if (head.count > list_limit)
limit = READ_ONCE(list_limit);if (!head.count || limit <= 0 || head.count > 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(*list)); if (IS_ERR(list)) return PTR_ERR(list);-- 2.54.0
On 6/26/26 13:31, David Laight wrote:
On Wed, 24 Jun 2026 14:58:58 +0200 Christian König christian.koenig@amd.com wrote:
On 6/24/26 14:52, Yousef Alhouseen wrote:
UDMABUF_CREATE_LIST copies an array whose element count comes from userspace. The count is compared against list_limit, but list_limit is a signed module parameter while the count is u32.
We should probably just drop the sign from the module parameter instead.
Does anything sanity-check the module parameter?
Do we need to? I mean shooting into your own foot is supposed to hurt.
Christian.
David
I don't see an use case for negative values here.
Regards, Christian.
If the limit is raised too far or made negative, that comparison no longer bounds the count to a range where sizeof(*list) * count fits in the u32 temporary used for the copy length. A wrapped copy length lets memdup_user() copy fewer entries than udmabuf_create() subsequently walks, leading to out-of-bounds reads from the copied list.
Take a positive snapshot of the module limit and use memdup_array_user() so the multiplication is checked before copying.
Signed-off-by: Yousef Alhouseen alhouseenyousef@gmail.com
drivers/dma-buf/udmabuf.c | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-)
diff --git a/drivers/dma-buf/udmabuf.c b/drivers/dma-buf/udmabuf.c index bced421c0..b4078ec84 100644 --- a/drivers/dma-buf/udmabuf.c +++ b/drivers/dma-buf/udmabuf.c @@ -469,14 +469,15 @@ 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;
int limit; if (copy_from_user(&head, (void __user *)arg, sizeof(head))) return -EFAULT;
if (head.count > list_limit)
limit = READ_ONCE(list_limit);if (!head.count || limit <= 0 || head.count > 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(*list)); if (IS_ERR(list)) return PTR_ERR(list);-- 2.54.0
linaro-mm-sig@lists.linaro.org
-
Christian König -
David Laight -
Yousef Alhouseen