On 3/12/26 10:48, Zhang, Jesse(Jie) wrote: > [AMD Official Use Only - AMD Internal Distribution Only] > >> -----Original Message----- >> From: Koenig, Christian <[email protected]> >> Sent: Thursday, March 12, 2026 5:31 PM >> To: Zhang, Jesse(Jie) <[email protected]>; [email protected] >> Cc: Deucher, Alexander <[email protected]> >> Subject: Re: [PATCH] drm/amdgpu: add overflow check for BO list array >> allocation >> >> On 3/12/26 10:27, Zhang, Jesse(Jie) wrote: >>> [AMD Official Use Only - AMD Internal Distribution Only] >>> >>>> -----Original Message----- >>>> From: Koenig, Christian <[email protected]> >>>> Sent: Thursday, March 12, 2026 5:18 PM >>>> To: Zhang, Jesse(Jie) <[email protected]>; >>>> [email protected] >>>> Cc: Deucher, Alexander <[email protected]> >>>> Subject: Re: [PATCH] drm/amdgpu: add overflow check for BO list array >>>> allocation >>>> >>>> On 3/12/26 09:33, Zhang, Jesse(Jie) wrote: >>>>> [AMD Official Use Only - AMD Internal Distribution Only] >>>>> >>>>>> -----Original Message----- >>>>>> From: Koenig, Christian <[email protected]> >>>>>> Sent: Thursday, March 12, 2026 4:23 PM >>>>>> To: Zhang, Jesse(Jie) <[email protected]>; >>>>>> [email protected] >>>>>> Cc: Deucher, Alexander <[email protected]> >>>>>> Subject: Re: [PATCH] drm/amdgpu: add overflow check for BO list >>>>>> array allocation >>>>>> >>>>>> On 3/12/26 09:18, Jesse.Zhang wrote: >>>>>>> When allocating memory for a BO list array, the multiplication >>>>>>> bo_number * info_size may overflow on 32-bit systems if userspace >>>>>>> supplies large values. This could lead to allocating a smaller >>>>>>> buffer than expected, followed by a memset or copy_from_user that >>>>>>> writes beyond the allocated memory, potentially causing memory >>>>>>> corruption or information disclosure. >>>>>>> >>>>>>> Add an overflow check using check_mul_overflow to detect such cases. >>>>>>> Also ensure the resulting allocation size does not exceed INT_MAX, >>>>>>> as the subsequent user copy operations may rely on this limit. >>>>>>> Return -EINVAL if either condition fails. >>>>>> >>>>>> That is completely unnecessary, vmemdup_array_user() already does >>>>>> that >>>> check. >>>>>> >>>>>>> >>>>>>> A crash log illustrating the issue: >>>>>>> >>>>>>> [ 2943.053706] RIP: 0010:__kvmalloc_node_noprof+0x5be/0x8a0 >>>>>>> ... >>>>>>> [ 2943.053725] Call Trace: >>>>>>> [ 2943.053728] amdgpu_bo_create_list_entry_array+0x42/0x130 >>>>>>> [amdgpu] [ 2943.053947] amdgpu_bo_list_ioctl+0x51/0x300 [amdgpu] [ >>>>>>> 2943.054277] >>>>>>> drm_ioctl+0x2cb/0x5a0 [drm] [ 2943.054379] >>>>>>> __x64_sys_ioctl+0x9e/0xf0 >>>>>>> >>>>>>> The overflow occurs in the allocation inside >>>>>>> amdgpu_bo_create_list_entry_array, leading to a crash in >>>>>>> vmemdup_user (via __kvmalloc_node_noprof). >>>>>> >>>>>> How and on which kernel can you reproduce that? >>>>> We are developing some fuzz tests for the unified project. >>>>> The tests involve passing different levels of garbage data and >>>>> ensuring the kernel >>>> can handle this data correctly. >>>>> This issue can be reproduced on the amd-staging-drm-next branch. >>>> >>>> Do you have the full backtrace? >>> Yes, >>> [ 2943.053649] WARNING: mm/slub.c:7152 at >>> __kvmalloc_node_noprof+0x5be/0x8a0, CPU#13: amd_fuzzing/2765 >> >> Ah, yes. That problem came up before. >> >> The maximum number of BOs in a BO list should be limited and not the result >> of the >> multiplication checked. >> >> The problem is that we couldn't give a good number on the maximum BOs we can >> have in a BO list. > Thanks Chritian, agreed. v2 switches from pure multiplication-overflow > wording to a BO-count limit. We now bound bo_number by INT_MAX / > sizeof(drm_amdgpu_bo_list_entry) before allocation/copy. This keeps behavior > deterministic for fuzzed input and avoids warning-prone huge allocation paths
Yeah, but I've rejected that before as well. INT_MAX / sizeof(drm_amdgpu_bo_list_entry) is just not a good maximum limit. This needs to be a fixed constant. Regards, Christian. > > Thanks > Jesse > >> >> Regards, >> Christian.
