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;
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_3poOt...
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.
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;
linux-stable-mirror@lists.linaro.org
-
Christian König -
Denis Arefev -
Fedor Pchelkin