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

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;

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;

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.

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.

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.