Re: [PATCH] drm/lease: use kvmalloc for temporary ioctl arrays

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Unbounded user-controlled array allocation and unbounded loop iteration leading to soft lockup (Denial of Service).
--

commit 7fbca73c75b2d8380009850496a05eb9659b0cab
Author: Aleksandr Nogikh <[email protected]>

drm/lease: use kvmalloc for temporary ioctl arrays

This commit replaces kzalloc_objs() and memdup_array_user() with
kvcalloc() and vmemdup_array_user() in drm_mode_create_lease_ioctl()
to prevent warnings when userspace provides a large object_count.

Link: https://syzkaller.appspot.com/ai_job?id=22f7a1b3-b0e0-4e5d-abf2-e7119094823a

> diff --git a/drivers/gpu/drm/drm_lease.c b/drivers/gpu/drm/drm_lease.c
> index 5d2cf724cbd7b..240e0ea6c5c3d 100644
> --- a/drivers/gpu/drm/drm_lease.c
> +++ b/drivers/gpu/drm/drm_lease.c
> @@ -386,7 +386,8 @@ static int fill_object_idr(struct drm_device *dev,
>  	int ret;
>  	bool universal_planes = READ_ONCE(lessor_priv->universal_planes);
>  
> -	objects = kzalloc_objs(struct drm_mode_object *, object_count);
> +	objects = kvcalloc(object_count, sizeof(struct drm_mode_object *),
> +			   GFP_KERNEL);

[Severity: High]
Does removing the implicit size limit of kmalloc allow object_count to become
arbitrarily large?

If object_count is massive, will the subsequent unbounded loop in
fill_object_idr() cause soft lockups?

	for (o = 0; o < object_count; o++) {
		objects[o] = drm_mode_object_find(dev, lessor_priv,
						  object_ids[o],
						  DRM_MODE_OBJECT_ANY);
		...

Since drm_mode_object_find() acquires and releases a subsystem mutex on each
iteration, and there is no cond_resched() in this loop, could a large
object_count cause the kernel to spin for an excessive amount of time?

[ ... ]

> @@ -509,8 +510,8 @@ int drm_mode_create_lease_ioctl(struct drm_device *dev,
>  	/* Handle leased objects, if any */
>  	idr_init(&leases);
>  	if (object_count != 0) {
> -		object_ids = memdup_array_user(u64_to_user_ptr(cl->object_ids),
> -					       object_count, sizeof(__u32));
> +		object_ids = vmemdup_array_user(u64_to_user_ptr(cl->object_ids),
> +						object_count, sizeof(__u32));

[Severity: High]
Similar to the objects array above, does changing to vmemdup_array_user()
remove the implicit bounds check on object_count that was previously enforced
by the slab allocator limits?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1