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] [thread-next>] [day] [month] [year] [list]

Message-ID: <qc72y52kt7vuwox4lhk42zligy5bslttselfoexse42mywtpps@ebqijs2tap2t>
Date: Tue, 8 Apr 2025 14:54:06 +0300
From: Fedor Pchelkin <pchelkin@...ras.ru>
To: Christian König <christian.koenig@....com>
Cc: Denis Arefev <arefev@...mel.ru>, 
	Alex Deucher <alexander.deucher@....com>, Simona Vetter <simona@...ll.ch>, 
	Andrey Grodzovsky <andrey.grodzovsky@....com>, lvc-project@...uxtesting.org, 
	Chunming Zhou <david1.zhou@....com>, linux-kernel@...r.kernel.org, dri-devel@...ts.freedesktop.org, 
	amd-gfx@...ts.freedesktop.org, stable@...r.kernel.org, David Airlie <airlied@...il.com>
Subject: Re: [lvc-project] [PATCH] drm/amdgpu: check a user-provided number
 of BOs in list

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@...ux-foundation.org> wrote:
> On Wed, 3 Jan 2024 at 11:15, Mikulas Patocka <mpatocka@...hat.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.