Message-ID: <437e12e2-ac0d-4a97-bd55-39ee03979526@amd.com>
Date: Wed, 9 Apr 2025 09:29:47 +0200
From: Christian König <christian.koenig@....com>
To: Linus Torvalds <torvalds@...ux-foundation.org>,
 Fedor Pchelkin <pchelkin@...ras.ru>
Cc: 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 09.04.25 um 04:39 schrieb Linus Torvalds:
> On Tue, 8 Apr 2025 at 09:07, Fedor Pchelkin <pchelkin@...ras.ru> wrote:
>>> Linus comment is about kvmalloc(), but the code here is using
>>> kvmalloc_array() which as far as I know is explicitly made to safely
>>> allocate arrays with parameters provided by userspace.
> No.
>
> ABSOLUTELY NOTHING CAN ALLOCATE ARRAYS WITH PARAMETERS PROVIDED BY USER SPACE.
>
> All kvmalloc_array() does is to check for overflow on the multiplication.
>
> That does *not* mean that you can then just blindly take user space
> input and pass it to kvmalloc_array().
>
> That could easily cause the machine to run out of memory immediately,
> for example. Or just cause huge latency issues. Or any number of other
> things.
>
>> [27651.163361] WARNING: CPU: 3 PID: 183060 at mm/util.c:657 __kvmalloc_node_noprof+0xc1/0xd0
>> [27651.163411] CPU: 3 UID: 0 PID: 183060 Comm: a.out Not tainted 6.13.9-200.fc41.x86_64 #1
>> [27651.163412] Hardware name: ASUS System Product Name/PRIME X670E-PRO WIFI, BIOS 3035 09/05/2024
>> [27651.163413] RIP: 0010:__kvmalloc_node_noprof+0xc1/0xd0
>> [27651.163424] Call Trace:
>> That's just
>>
>>     union drm_amdgpu_bo_list bo_list;
>>     int fd, ret;
>>
>>     memset(&bo_list, 0, sizeof(bo_list));
>>
>>     fd = open(DEVICE_PATH, O_RDWR);
>>
>>     bo_list.in.bo_number = 1 << 31;
>>     ret = ioctl(fd, DRM_IOCTL_AMDGPU_BO_LIST, &bo_list);
> Yes, exactly, and that's bogus code in the DRM layer to just blindly
> trust user space.
>
> User space input absolutely has to be validated for sanity.

That's what I totally agree on. My question is rather is it better to do this in a centralized function or spread out over tons of different use cases?

I mean open coding the limit checks everywhere certainly works, but as far as I can see it would be more defensive if we do that inside kvmalloc_array().

Or maybe a separate function which takes a maximum as parameter?

BTW we have been running into the kvmalloc() check on valid use cases as well. For example on a system with a >256TiB of system memory having a single descriptor array >4GiB is perfectly valid. It's just not valid in any possible way if you only have 8GiB in total.

In the past we worked around that by switching to multiple smaller allocations or different container types, but that is actually not the most optimal way of doing it.

Regards,
Christian.

>
> There's a very real reason why we have things like PATH_MAX.
>
> Could we allocate any amount of memory for user paths, with the
> argument that path length shouldn't be limited to some (pretty small)
> number?
>
> Sure. We *could* do that.
>
> And that would be a huge mistake. Limiting and sanity-checking user
> space arguments isn't just a good idea - it's an absolute requirement.
>
> So that kvmalloc warning exists *exactly* so that you will get a
> warning if you do something stupid like just blindly trust user space.
>
> Because no, "doesn't overflow" isn't even remotely a valid limit. A
> real limit on memory allocations - and most other things, for that
> matter - needs to be about practical real issues, not about something
> like  "this doesn't overflow".
>
>             Linus

Powered by blists - more mailing lists