On 6/26/26 13:31, David Laight wrote: > On Wed, 24 Jun 2026 14:58:58 +0200 > Christian König <[email protected]> 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 <[email protected]> >>> --- >>> 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 >>> >> >> >
