+Jann Horn for his thoughts
On Thu, Apr 11, 2024 at 12:25 PM Christian König <christian.koenig@xxxxxxx> wrote:
Am 11.04.24 um 05:28 schrieb xinhui pan:
> Ensure there is no address overlapping.
>
> Reported-by: Vlad Stolyarov <hexed@xxxxxxxxxx>
> Signed-off-by: xinhui pan <xinhui.pan@xxxxxxx>
> ---
> drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 6 ++++++
> 1 file changed, 6 insertions(+)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> index 8af3f0fd3073..f1315a854192 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> @@ -1852,6 +1852,12 @@ int amdgpu_vm_bo_clear_mappings(struct amdgpu_device *adev,
> LIST_HEAD(removed);
> uint64_t eaddr;
>
> + /* validate the parameters */
> + if (saddr & ~PAGE_MASK || size & ~PAGE_MASK)
> + return -EINVAL;
Well as general rule: *never* use PAGE_MASK and other PAGE_* macros
here. This is GPUVM and not related to the CPUVM space.
> + if (saddr + size <= saddr)
> + return -EINVAL;
> +
Mhm, so basically size is not checked for a wraparound?
> eaddr = saddr + size - 1;
> saddr /= AMDGPU_GPU_PAGE_SIZE;
> eaddr /= AMDGPU_GPU_PAGE_SIZE;
If that's the case then I would rather check for saddr < eaddr here.
But that actually shouldn't matter since this code here:
/* Now gather all removed mappings */
tmp = amdgpu_vm_it_iter_first(&vm->va, saddr, eaddr);
while (tmp) {
Then shouldn't return anything, so the operation is basically a NO-OP then.
Regards,
Christian.
