Re: [lvc-project] [PATCH] drm/amdgpu: check a user-provided number of BOs in list

["Christian König" <ckoenig.leichtzumerken@gmail.com>]

Am 13.04.25 um 13:31 schrieb Fedor Pchelkin:
> On Thu, 10. Apr 11:07, Christian König wrote:
>> Am 09.04.25 um 19:27 schrieb Linus Torvalds:
>>> The VM layer allows larger allocations. But the "this is a simple
>>> allocation, choose kmalloc or vmalloc automatically based on size"
>>> helper says "you are being simple, I'm going to check your arguments
>>> are actually sane".
>>>
>>> So the drm code can easily have a function that validates the input
>>> for your specific cases, and then you (a) don't need the helper
>>> function that does the overflow protection and (b) don't want it.
>>>
>>> But it should actually validate arguments for real sanity at that
>>> point. Not just open-code kvmalloc() without the sanity check.
>> Yeah, exactly that has been proposed by driver maintainers before and we just rejected it on the subsystem maintainers level.
>>
>> For this particular use case here I will propose some hopefully high enough hard coded limit, but I can't guarantee that this will work for all use cases.
> FWIW, the current code anyway has this limit being some sort of 4Gb, not
> more.
>
> The resulting calculation of `bytes` wraps at 32 bits albeit itself being
> of type *unsigned long*.

Yeah that is a *much* more serious bug. Thanks for pointing that out.

This should probably be using size_t here and applying the limit to the bo_number before the calculation.

And a bo_info_size which doesn't match the expected size should be rejected and not worked around like it currently is.

Thanks,
Christian.

>
> 	/* copy the handle array from userspace to a kernel buffer */
> 	r = -EFAULT;
> 	if (likely(info_size == in->bo_info_size)) {
> 		unsigned long bytes = in->bo_number *
> 			in->bo_info_size;
>
> 		if (copy_from_user(info, uptr, bytes))
> 			goto error_free;
>