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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <44f0bf71-c689-46de-bfdc-e4a24b9fb921@gmail.com>
Date: Mon, 14 Apr 2025 11:35:24 +0200
From: Christian König <ckoenig.leichtzumerken@...il.com>
To: Fedor Pchelkin <pchelkin@...ras.ru>,
 Christian König <christian.koenig@....com>
Cc: Linus Torvalds <torvalds@...ux-foundation.org>,
 Denis Arefev <arefev@...mel.ru>, Alex Deucher <alexander.deucher@....com>,
 Simona Vetter <simona@...ll.ch>, lvc-project@...uxtesting.org,
 linux-kernel@...r.kernel.org, dri-devel@...ts.freedesktop.org,
 amd-gfx@...ts.freedesktop.org, David Airlie <airlied@...il.com>
Subject: Re: [lvc-project] [PATCH] drm/amdgpu: check a user-provided number of
 BOs in list

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