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

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

[Denis Arefev]

The user can set any value to the variable ‘bo_number’, via the ioctl
command DRM_IOCTL_AMDGPU_BO_LIST. This will affect the arithmetic
expression ‘in->bo_number * in->bo_info_size’, which is prone to
overflow. Add a valid value check.

Found by Linux Verification Center (linuxtesting.org) with SVACE.

Fixes: 964d0fbf6301 ("drm/amdgpu: Allow to create BO lists in CS ioctl v3")
Cc: stable@vger.kernel.org
Signed-off-by: Denis Arefev <arefev@swemel.ru>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_bo_list.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_bo_list.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_bo_list.c
index 702f6610d024..dd30d2426ff7 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_bo_list.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_bo_list.c
@@ -189,6 +189,9 @@ int amdgpu_bo_create_list_entry_array(struct drm_amdgpu_bo_list_in *in,
 	struct drm_amdgpu_bo_list_entry *info;
 	int r;
 
+	if (!in->bo_number || in->bo_number > UINT_MAX / info_size)
+		return -EINVAL;
+
 	info = kvmalloc_array(in->bo_number, info_size, GFP_KERNEL);
 	if (!info)
 		return -ENOMEM;
-- 
2.43.0

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

[Christian König]

Am 08.04.25 um 11:17 schrieb Denis Arefev:
> The user can set any value to the variable ‘bo_number’, via the ioctl
> command DRM_IOCTL_AMDGPU_BO_LIST. This will affect the arithmetic
> expression ‘in->bo_number * in->bo_info_size’, which is prone to
> overflow. Add a valid value check.

As far as I can see that is already checked by kvmalloc_array().

So adding this additional check manually is completely superfluous.

Regards,
Christian.

>
> Found by Linux Verification Center (linuxtesting.org) with SVACE.
>
> Fixes: 964d0fbf6301 ("drm/amdgpu: Allow to create BO lists in CS ioctl v3")
> Cc: stable@vger.kernel.org
> Signed-off-by: Denis Arefev <arefev@swemel.ru>
> ---
>  drivers/gpu/drm/amd/amdgpu/amdgpu_bo_list.c | 3 +++
>  1 file changed, 3 insertions(+)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_bo_list.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_bo_list.c
> index 702f6610d024..dd30d2426ff7 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_bo_list.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_bo_list.c
> @@ -189,6 +189,9 @@ int amdgpu_bo_create_list_entry_array(struct drm_amdgpu_bo_list_in *in,
>  	struct drm_amdgpu_bo_list_entry *info;
>  	int r;
>  
> +	if (!in->bo_number || in->bo_number > UINT_MAX / info_size)
> +		return -EINVAL;
> +
>  	info = kvmalloc_array(in->bo_number, info_size, GFP_KERNEL);
>  	if (!info)
>  		return -ENOMEM;

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

[Fedor Pchelkin]

On Tue, 08. Apr 11:26, Christian König wrote:
> Am 08.04.25 um 11:17 schrieb Denis Arefev:
> > The user can set any value to the variable ‘bo_number’, via the ioctl
> > command DRM_IOCTL_AMDGPU_BO_LIST. This will affect the arithmetic
> > expression ‘in->bo_number * in->bo_info_size’, which is prone to
> > overflow. Add a valid value check.
> 
> As far as I can see that is already checked by kvmalloc_array().
> 
> So adding this additional check manually is completely superfluous.

Note that in->bo_number is of type 'u32' while kvmalloc_array() checks for
an overflow in 'size_t', usually 64-bit.

So it looks possible to pass some large 32-bit number, then multiply it by
(comparatively small) in->bo_info_size and still remain in 64-bit bounds.

And later that would likely result in a WARNING in

void *__kvmalloc_node_noprof(DECL_BUCKET_PARAMS(size, b), gfp_t flags, int node)
{
...
	/* Don't even allow crazy sizes */
	if (unlikely(size > INT_MAX)) {
		WARN_ON_ONCE(!(flags & __GFP_NOWARN));
		return NULL;
	}

But the commit description lacks such details, I admit.

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

[Christian König]

Am 08.04.25 um 11:39 schrieb Fedor Pchelkin:
> On Tue, 08. Apr 11:26, Christian König wrote:
>> Am 08.04.25 um 11:17 schrieb Denis Arefev:
>>> The user can set any value to the variable ‘bo_number’, via the ioctl
>>> command DRM_IOCTL_AMDGPU_BO_LIST. This will affect the arithmetic
>>> expression ‘in->bo_number * in->bo_info_size’, which is prone to
>>> overflow. Add a valid value check.
>> As far as I can see that is already checked by kvmalloc_array().
>>
>> So adding this additional check manually is completely superfluous.
> Note that in->bo_number is of type 'u32' while kvmalloc_array() checks for
> an overflow in 'size_t', usually 64-bit.
>
> So it looks possible to pass some large 32-bit number, then multiply it by
> (comparatively small) in->bo_info_size and still remain in 64-bit bounds.
>
> And later that would likely result in a WARNING in
>
> void *__kvmalloc_node_noprof(DECL_BUCKET_PARAMS(size, b), gfp_t flags, int node)
> {
> ...
> 	/* Don't even allow crazy sizes */
> 	if (unlikely(size > INT_MAX)) {
> 		WARN_ON_ONCE(!(flags & __GFP_NOWARN));
> 		return NULL;
> 	}
>
> But the commit description lacks such details, I admit.

Yeah, so what? I'm perfectly aware that this can result in a warning, but that is just not something worth fixing.

Christian.

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

[Fedor Pchelkin]

On Tue, 08. Apr 13:37, Christian König wrote:
> Am 08.04.25 um 11:39 schrieb Fedor Pchelkin:
> > On Tue, 08. Apr 11:26, Christian König wrote:
> >> Am 08.04.25 um 11:17 schrieb Denis Arefev:
> >>> The user can set any value to the variable ‘bo_number’, via the ioctl
> >>> command DRM_IOCTL_AMDGPU_BO_LIST. This will affect the arithmetic
> >>> expression ‘in->bo_number * in->bo_info_size’, which is prone to
> >>> overflow. Add a valid value check.
> >> As far as I can see that is already checked by kvmalloc_array().
> >>
> >> So adding this additional check manually is completely superfluous.
> > Note that in->bo_number is of type 'u32' while kvmalloc_array() checks for
> > an overflow in 'size_t', usually 64-bit.
> >
> > So it looks possible to pass some large 32-bit number, then multiply it by
> > (comparatively small) in->bo_info_size and still remain in 64-bit bounds.
> >
> > And later that would likely result in a WARNING in
> >
> > void *__kvmalloc_node_noprof(DECL_BUCKET_PARAMS(size, b), gfp_t flags, int node)
> > {
> > ...
> > 	/* Don't even allow crazy sizes */
> > 	if (unlikely(size > INT_MAX)) {
> > 		WARN_ON_ONCE(!(flags & __GFP_NOWARN));
> > 		return NULL;
> > 	}
> >
> > But the commit description lacks such details, I admit.
> 
> Yeah, so what? I'm perfectly aware that this can result in a warning, but that is just not something worth fixing.

It's a warning directly trigerrable by userspace. It's not the purpose of
kernel warnings. The WARN checks inside the allocator imply that the
in-kernel caller should be aware of what sizes he requests.

If user can request an arbitrary size value then we should use __GFP_NOWARN
and back on the allocator to return NULL in case it doesn't even try to
satisfy an enormous memory allocation request (in which case it yells in
the log without __GFP_NOWARN being passed). Maybe that would be a more
appropriate thing here?

Please see:
https://lore.kernel.org/dm-devel/CAHk-=wi8Zer6tnqO-bz+WxFpMv9sPc-LxGRm_3poOtZegjhfrg@mail.gmail.com/

On Wed, 3 Jan 2024 at 11:21, Linus Torvalds <torvalds@linux-foundation.org> wrote:
> On Wed, 3 Jan 2024 at 11:15, Mikulas Patocka <mpatocka@redhat.com> wrote:
> >
> > Should we use __GFP_NOWARN? (but this would shut up also genuine
> > warnings).
> 
> This can only be fixed in the *caller*, which need to either
> 
>  (a) have saen limit checking that checks for an obviously safe limit
> (please don't just make it INT_MAX to handle this one case - make it
> something *reasonable*)
> 
> _or_
> 
>  (b) the __GPF_NOWARN with a very obvious "I handle a failed return
> gracefully" handling all the way out to user space (error returns
> and/or things like "fall back to smaller sizes")./
> 
> because a caller that just passes in a random value to kmalloc()
> should continue to warn if that random value is unreasonable.
> 
> Exactly *because* we want all those crazy random tester robots to
> actually find cases where people just randomly take untrusted lengths
> from user space.

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

[Fedor Pchelkin]

On Tue, 08. Apr 14:22, Christian König wrote:
> Am 08.04.25 um 13:54 schrieb Fedor Pchelkin:
> > If user can request an arbitrary size value then we should use __GFP_NOWARN
> > and back on the allocator to return NULL in case it doesn't even try to
> > satisfy an enormous memory allocation request (in which case it yells in
> > the log without __GFP_NOWARN being passed). Maybe that would be a more
> > appropriate thing here?
> 
> Using __GFP_NOWARN is most likely an incorrect approach as well, this
> might disable all warnings. E.g. also OOM if I'm not completely mistaken
> and we clearly do want those.

Okay, that sounds reasonable.

> 
> > Please see:
> > https://lore.kernel.org/dm-devel/CAHk-=wi8Zer6tnqO-bz+WxFpMv9sPc-LxGRm_3poOtZegjhfrg@mail.gmail.com/
> 
> 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.

[27651.163357] ------------[ cut here ]------------
[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:
[27651.163426]  <TASK>
[27651.163429]  ? show_trace_log_lvl+0x1b0/0x2f0
[27651.163431]  ? show_trace_log_lvl+0x1b0/0x2f0
[27651.163434]  ? amdgpu_bo_create_list_entry_array+0x3d/0x150 [amdgpu]
[27651.163579]  ? __kvmalloc_node_noprof+0xc1/0xd0
[27651.163581]  ? __warn.cold+0x93/0xfa
[27651.163582]  ? __kvmalloc_node_noprof+0xc1/0xd0
[27651.163584]  ? report_bug+0xff/0x140
[27651.163586]  ? handle_bug+0x58/0x90
[27651.163588]  ? exc_invalid_op+0x17/0x70
[27651.163589]  ? asm_exc_invalid_op+0x1a/0x20
[27651.163591]  ? __kmalloc_node_noprof+0x3f9/0x590
[27651.163592]  ? __kvmalloc_node_noprof+0xc1/0xd0
[27651.163594]  ? __kvmalloc_node_noprof+0x37/0xd0
[27651.163595]  amdgpu_bo_create_list_entry_array+0x3d/0x150 [amdgpu]
[27651.163704]  amdgpu_bo_list_ioctl+0x55/0x350 [amdgpu]
[27651.163805]  ? __pfx_amdgpu_bo_list_ioctl+0x10/0x10 [amdgpu]
[27651.163909]  drm_ioctl_kernel+0xad/0x100
[27651.163911]  drm_ioctl+0x288/0x530
[27651.163912]  ? __pfx_amdgpu_bo_list_ioctl+0x10/0x10 [amdgpu]
[27651.164010]  amdgpu_drm_ioctl+0x4b/0x80 [amdgpu]
[27651.164106]  __x64_sys_ioctl+0x94/0xc0
[27651.164108]  do_syscall_64+0x82/0x160
[27651.164110]  ? handle_mm_fault+0x20d/0x330
[27651.164111]  ? do_user_addr_fault+0x55a/0x7b0
[27651.164113]  ? exc_page_fault+0x7e/0x180
[27651.164114]  entry_SYSCALL_64_after_hwframe+0x76/0x7e
[27651.164116] RIP: 0033:0x7f271589d4ad
[27651.164138]  </TASK>
[27651.164138] ---[ end trace 0000000000000000 ]---


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


> 
> So pre-checking those parameters in the caller once more is a bit
> questionable, especially since we need to spread that around to all
> callers of kvmalloc_array() which looks backwards considering the
> purpose of kvmalloc_array().

kvmalloc_array() performs an extra check only for that the `size * n`
thing doesn't overflow the (generally) 64-bit arithmetic. Otherwise, it's
actually like an ordinary kvmalloc() with a request of `size * n`.

Performing that extra size_t-overflow check is not the same as checking
that `size * n` cannot take enormous and possibly unverified values.
That's what the logic of the checks inside the allocator implies IMO.

> 
> Maybe we should reduce the warning to info level for kvmalloc_array()
> since returning NULL when incorrect parameters are given can be
> perfectly handled by the caller.

That would require running through and inspecting all of its callers in
the source tree.

And it would then differentiate from the underlying kvmalloc(), I'd say.

The warning was added deliberately.

commit 7661809d493b426e979f39ab512e3adf41fbcc69
Author: Linus Torvalds <torvalds@linux-foundation.org>
Date:   Wed Jul 14 09:45:49 2021 -0700

    mm: don't allow oversized kvmalloc() calls
    
    'kvmalloc()' is a convenience function for people who want to do a
    kmalloc() but fall back on vmalloc() if there aren't enough physically
    contiguous pages, or if the allocation is larger than what kmalloc()
    supports.
    
    However, let's make sure it doesn't get _too_ easy to do crazy things
    with it.  In particular, don't allow big allocations that could be due
    to integer overflow or underflow.  So make sure the allocation size fits
    in an 'int', to protect against trivial integer conversion issues.
    
    Acked-by: Willy Tarreau <w@1wt.eu>
    Cc: Kees Cook <keescook@chromium.org>
    Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org>

diff --git a/mm/util.c b/mm/util.c
index db3091116b7c..499b6b5767ed 100644
--- a/mm/util.c
+++ b/mm/util.c
@@ -593,6 +593,10 @@ void *kvmalloc_node(size_t size, gfp_t flags, int node)
        if (ret || size <= PAGE_SIZE)
                return ret;
 
+       /* Don't even allow crazy sizes */
+       if (WARN_ON_ONCE(size > INT_MAX))
+               return NULL;
+
        return __vmalloc_node(size, 1, flags, node,
                        __builtin_return_address(0));
 }

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

[Linus Torvalds]

On Tue, 8 Apr 2025 at 09:07, Fedor Pchelkin <pchelkin@ispras.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.

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

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

[Christian König]

Am 09.04.25 um 04:39 schrieb Linus Torvalds:
> On Tue, 8 Apr 2025 at 09:07, Fedor Pchelkin <pchelkin@ispras.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

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

[Linus Torvalds]

On Wed, 9 Apr 2025 at 00:29, Christian König <christian.koenig@amd.com> wrote:
>
> 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().

No.

If we add some limit to kvmalloc_array(), I guarantee that people will
just then call it with ~0UL.

Making it all entirely pointless.

So thus the "kvmalloc_array() warns about unreasonable uses
unconditionally, at a limit that is high enough to be useful, and low
enough that the automated code randomization tools will hopefully
trigger it and find bad code that doesn't validate kernel input".

> BTW we have been running into the kvmalloc() check on valid use cases as well.

*IF* you do proper validation, you can just use the raw vmalloc()
interfaces by hand and avoid it all.

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.

             Linus

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

[Christian König]

Am 09.04.25 um 19:27 schrieb Linus Torvalds:
> On Wed, 9 Apr 2025 at 00:29, Christian König <christian.koenig@amd.com> wrote:
>> 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().
> No.
>
> If we add some limit to kvmalloc_array(), I guarantee that people will
> just then call it with ~0UL.
>
> Making it all entirely pointless.

Hehe, yeah good point as well.

> So thus the "kvmalloc_array() warns about unreasonable uses
> unconditionally, at a limit that is high enough to be useful, and low
> enough that the automated code randomization tools will hopefully
> trigger it and find bad code that doesn't validate kernel input".
>
>> BTW we have been running into the kvmalloc() check on valid use cases as well.
> *IF* you do proper validation, you can just use the raw vmalloc()
> interfaces by hand and avoid it all.

The problem is that we have use cases where the same code needs to work from the low end mobile with just a few gigabytes all the way to the HPC data center with up to petabytes of system memory. The kvmalloc approach is just very convenient to handle that since it scales.

To summarize for some use cases I can't come up with good hard coded limits since they basically just depend on the system.

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

Thanks,
Christian.

>
>              Linus

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

[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*.

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

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

[Christian König]

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

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

[Christian König]

Coming back to the original patch.

Am 08.04.25 um 11:17 schrieb Denis Arefev:
> The user can set any value to the variable ‘bo_number’, via the ioctl
> command DRM_IOCTL_AMDGPU_BO_LIST. This will affect the arithmetic
> expression ‘in->bo_number * in->bo_info_size’, which is prone to
> overflow. Add a valid value check.
>
> Found by Linux Verification Center (linuxtesting.org) with SVACE.
>
> Fixes: 964d0fbf6301 ("drm/amdgpu: Allow to create BO lists in CS ioctl v3")
> Cc: stable@vger.kernel.org
> Signed-off-by: Denis Arefev <arefev@swemel.ru>
> ---
>  drivers/gpu/drm/amd/amdgpu/amdgpu_bo_list.c | 3 +++
>  1 file changed, 3 insertions(+)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_bo_list.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_bo_list.c
> index 702f6610d024..dd30d2426ff7 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_bo_list.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_bo_list.c
> @@ -189,6 +189,9 @@ int amdgpu_bo_create_list_entry_array(struct drm_amdgpu_bo_list_in *in,
>  	struct drm_amdgpu_bo_list_entry *info;
>  	int r;
>  
> +	if (!in->bo_number || in->bo_number > UINT_MAX / info_size)
> +		return -EINVAL;
> +

As discussed with Linus the goal here is not to avoid the warning, but rather to apply a reasonable limit.

Since we already use an u16 for the number of BOs in other cases it is probably reasonable to assume that we will never have more than USHRT_MAX number of BOs here as well.

So please change the patch accordingly and hopefully nobody will complain.

Regards,
Christian.

>  	info = kvmalloc_array(in->bo_number, info_size, GFP_KERNEL);
>  	if (!info)
>  		return -ENOMEM;