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@xxxxxxxxxx>
Signed-off-by: xinhui pan <xinhui.pan@xxxxxxx>
---
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);
