Re: [PATCH] drm/amdgpu: validate the parameters of bo mapping operations more clearly

[Christian König <christian.koenig@xxxxxxx>]

Am 12.04.24 um 09:35 schrieb xinhui pan:
> Verify the parameters of
> amdgpu_vm_bo_(map/replace_map/clearing_mappings) in one common place.
>
> Reported-by: Vlad Stolyarov <hexed@xxxxxxxxxx>
> Suggested-by: Christian König <christian.koenig@xxxxxxx>
> Signed-off-by: xinhui pan <xinhui.pan@xxxxxxx>

Reviewed-by: Christian König <christian.koenig@xxxxxxx>

> ---
>   drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 72 ++++++++++++++++----------
>   1 file changed, 46 insertions(+), 26 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> index 8af3f0fd3073..4e2391c83d7c 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> @@ -1647,6 +1647,37 @@ static void amdgpu_vm_bo_insert_map(struct amdgpu_device *adev,
>   	trace_amdgpu_vm_bo_map(bo_va, mapping);
>   }
>   
> +/* Validate operation parameters to prevent potential abuse */
> +static int amdgpu_vm_verify_parameters(struct amdgpu_device *adev,
> +					  struct amdgpu_bo *bo,
> +					  uint64_t saddr,
> +					  uint64_t offset,
> +					  uint64_t size)
> +{
> +	uint64_t tmp, lpfn;
> +
> +	if (saddr & AMDGPU_GPU_PAGE_MASK
> +	    || offset & AMDGPU_GPU_PAGE_MASK
> +	    || size & AMDGPU_GPU_PAGE_MASK)
> +		return -EINVAL;
> +
> +	if (check_add_overflow(saddr, size, &tmp)
> +	    || check_add_overflow(offset, size, &tmp)
> +	    || size == 0 /* which also leads to end < begin */)
> +		return -EINVAL;
> +
> +	/* make sure object fit at this offset */
> +	if (bo && offset + size > amdgpu_bo_size(bo))
> +		return -EINVAL;
> +
> +	/* Ensure last pfn not exceed max_pfn */
> +	lpfn = (saddr + size - 1) >> AMDGPU_GPU_PAGE_SHIFT;
> +	if (lpfn >= adev->vm_manager.max_pfn)
> +		return -EINVAL;
> +
> +	return 0;
> +}
> +
>   /**
>    * amdgpu_vm_bo_map - map bo inside a vm
>    *
> @@ -1673,21 +1704,14 @@ int amdgpu_vm_bo_map(struct amdgpu_device *adev,
>   	struct amdgpu_bo *bo = bo_va->base.bo;
>   	struct amdgpu_vm *vm = bo_va->base.vm;
>   	uint64_t eaddr;
> +	int r;
>   
> -	/* validate the parameters */
> -	if (saddr & ~PAGE_MASK || offset & ~PAGE_MASK || size & ~PAGE_MASK)
> -		return -EINVAL;
> -	if (saddr + size <= saddr || offset + size <= offset)
> -		return -EINVAL;
> -
> -	/* make sure object fit at this offset */
> -	eaddr = saddr + size - 1;
> -	if ((bo && offset + size > amdgpu_bo_size(bo)) ||
> -	    (eaddr >= adev->vm_manager.max_pfn << AMDGPU_GPU_PAGE_SHIFT))
> -		return -EINVAL;
> +	r = amdgpu_vm_verify_parameters(adev, bo, saddr, offset, size);
> +	if (r)
> +		return r;
>   
>   	saddr /= AMDGPU_GPU_PAGE_SIZE;
> -	eaddr /= AMDGPU_GPU_PAGE_SIZE;
> +	eaddr = saddr + (size - 1) / AMDGPU_GPU_PAGE_SIZE;
>   
>   	tmp = amdgpu_vm_it_iter_first(&vm->va, saddr, eaddr);
>   	if (tmp) {
> @@ -1740,17 +1764,9 @@ int amdgpu_vm_bo_replace_map(struct amdgpu_device *adev,
>   	uint64_t eaddr;
>   	int r;
>   
> -	/* validate the parameters */
> -	if (saddr & ~PAGE_MASK || offset & ~PAGE_MASK || size & ~PAGE_MASK)
> -		return -EINVAL;
> -	if (saddr + size <= saddr || offset + size <= offset)
> -		return -EINVAL;
> -
> -	/* make sure object fit at this offset */
> -	eaddr = saddr + size - 1;
> -	if ((bo && offset + size > amdgpu_bo_size(bo)) ||
> -	    (eaddr >= adev->vm_manager.max_pfn << AMDGPU_GPU_PAGE_SHIFT))
> -		return -EINVAL;
> +	r = amdgpu_vm_verify_parameters(adev, bo, saddr, offset, size);
> +	if (r)
> +		return r;
>   
>   	/* Allocate all the needed memory */
>   	mapping = kmalloc(sizeof(*mapping), GFP_KERNEL);
> @@ -1764,7 +1780,7 @@ int amdgpu_vm_bo_replace_map(struct amdgpu_device *adev,
>   	}
>   
>   	saddr /= AMDGPU_GPU_PAGE_SIZE;
> -	eaddr /= AMDGPU_GPU_PAGE_SIZE;
> +	eaddr = saddr + (size - 1) / AMDGPU_GPU_PAGE_SIZE;
>   
>   	mapping->start = saddr;
>   	mapping->last = eaddr;
> @@ -1851,10 +1867,14 @@ int amdgpu_vm_bo_clear_mappings(struct amdgpu_device *adev,
>   	struct amdgpu_bo_va_mapping *before, *after, *tmp, *next;
>   	LIST_HEAD(removed);
>   	uint64_t eaddr;
> +	int r;
> +
> +	r = amdgpu_vm_verify_parameters(adev, NULL, saddr, 0, size);
> +	if (r)
> +		return r;
>   
> -	eaddr = saddr + size - 1;
>   	saddr /= AMDGPU_GPU_PAGE_SIZE;
> -	eaddr /= AMDGPU_GPU_PAGE_SIZE;
> +	eaddr = saddr + (size - 1) / AMDGPU_GPU_PAGE_SIZE;
>   
>   	/* Allocate all the needed memory */
>   	before = kzalloc(sizeof(*before), GFP_KERNEL);