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;
>
