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

[Christian König]

Am 12.04.24 um 08:47 schrieb xinhui pan:
> Verify the parameters of
> amdgpu_vm_bo_(map/replace_map/clearing_mappings) in one common place.
>
> Reported-by: Vlad Stolyarov <hexed at google.com>
> Signed-off-by: xinhui pan <xinhui.pan at amd.com>
> ---
>   drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 63 ++++++++++++++++----------
>   1 file changed, 39 insertions(+), 24 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> index 8af3f0fd3073..ea9721666756 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);
>   }

Please add a one line comment here describing why we have the function.

E.g. "validate operation parameters to prevent potential abuse" or 
something like that.
>   
> +static int amdgpu_vm_bo_verify_parameters(struct amdgpu_device *adev,
> +					  struct amdgpu_bo *bo,
> +					  uint64_t saddr,
> +					  uint64_t offset,
> +					  uint64_t size)

Probably better to drop the _bo_ from the name cause this doesn't work 
on bo_va structures.

> +{
> +	size_t tmp, lpfn;

Extremely bad idea, size_t might only be 32bit. Please use uint64_t here 
as well.

> +
> +	if (saddr & AMDGPU_GPU_PAGE_MASK
> +	    || offset & AMDGPU_GPU_PAGE_MASK
> +	    || size & AMDGPU_GPU_PAGE_MASK)
> +		return -EINVAL;
> +
> +	/* Check overflow */

That comment is a bit superfluous when check_add_overflow() is used. 
Maybe drop it.

> +	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
>    *
> @@ -1674,20 +1705,11 @@ int amdgpu_vm_bo_map(struct amdgpu_device *adev,
>   	struct amdgpu_vm *vm = bo_va->base.vm;
>   	uint64_t eaddr;
>   
> -	/* 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))
> +	if (amdgpu_vm_bo_verify_parameters(adev, bo, saddr, offset, size))
>   		return -EINVAL;

Probably better to return the return value of 
amdgpu_vm_bo_verify_parameters().

>   
> +	eaddr = (saddr + size - 1) / AMDGPU_GPU_PAGE_SIZE;
>   	saddr /= AMDGPU_GPU_PAGE_SIZE;
> -	eaddr /= AMDGPU_GPU_PAGE_SIZE;

Please keep the saddr, eaddr calculation order.

Apart from those nit picks looks really good to me.

Regards,
Christian.

>   
>   	tmp = amdgpu_vm_it_iter_first(&vm->va, saddr, eaddr);
>   	if (tmp) {
> @@ -1740,16 +1762,7 @@ 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))
> +	if (amdgpu_vm_bo_verify_parameters(adev, bo, saddr, offset, size))
>   		return -EINVAL;
>   
>   	/* Allocate all the needed memory */
> @@ -1763,8 +1776,8 @@ int amdgpu_vm_bo_replace_map(struct amdgpu_device *adev,
>   		return r;
>   	}
>   
> +	eaddr = (saddr + size - 1) / AMDGPU_GPU_PAGE_SIZE;
>   	saddr /= AMDGPU_GPU_PAGE_SIZE;
> -	eaddr /= AMDGPU_GPU_PAGE_SIZE;
>   
>   	mapping->start = saddr;
>   	mapping->last = eaddr;
> @@ -1852,9 +1865,11 @@ int amdgpu_vm_bo_clear_mappings(struct amdgpu_device *adev,
>   	LIST_HEAD(removed);
>   	uint64_t eaddr;
>   
> -	eaddr = saddr + size - 1;
> +	if (amdgpu_vm_bo_verify_parameters(adev, NULL, saddr, 0, size))
> +		return -EINVAL;
> +
> +	eaddr = (saddr + size - 1) / AMDGPU_GPU_PAGE_SIZE;
>   	saddr /= AMDGPU_GPU_PAGE_SIZE;
> -	eaddr /= AMDGPU_GPU_PAGE_SIZE;
>   
>   	/* Allocate all the needed memory */
>   	before = kzalloc(sizeof(*before), GFP_KERNEL);