[PATCH 1/2] drm/amdgpu: fix use after free during BO move

[PATCH 1/2] drm/amdgpu: fix use after free during BO move

[Christian König]

The memory backing old_mem is already freed at that point, move the
check a bit more up.

Signed-off-by: Christian König <christian.koenig@amd.com>
Fixes: bfa3357ef9ab ("drm/ttm: allocate resource object instead of embedding it v2")
Bug: https://gitlab.freedesktop.org/drm/amd/-/issues/1699
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c | 18 +++++++++---------
 1 file changed, 9 insertions(+), 9 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
index 446943e32e3e..e2896ac2c9ce 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
@@ -513,6 +513,15 @@ static int amdgpu_bo_move(struct ttm_buffer_object *bo, bool evict,
 		goto out;
 	}
 
+	if (bo->type == ttm_bo_type_device &&
+	    new_mem->mem_type == TTM_PL_VRAM &&
+	    old_mem->mem_type != TTM_PL_VRAM) {
+		/* amdgpu_bo_fault_reserve_notify will re-set this if the CPU
+		 * accesses the BO after it's moved.
+		 */
+		abo->flags &= ~AMDGPU_GEM_CREATE_CPU_ACCESS_REQUIRED;
+	}
+
 	if (adev->mman.buffer_funcs_enabled) {
 		if (((old_mem->mem_type == TTM_PL_SYSTEM &&
 		      new_mem->mem_type == TTM_PL_VRAM) ||
@@ -543,15 +552,6 @@ static int amdgpu_bo_move(struct ttm_buffer_object *bo, bool evict,
 			return r;
 	}
 
-	if (bo->type == ttm_bo_type_device &&
-	    new_mem->mem_type == TTM_PL_VRAM &&
-	    old_mem->mem_type != TTM_PL_VRAM) {
-		/* amdgpu_bo_fault_reserve_notify will re-set this if the CPU
-		 * accesses the BO after it's moved.
-		 */
-		abo->flags &= ~AMDGPU_GEM_CREATE_CPU_ACCESS_REQUIRED;
-	}
-
 out:
 	/* update statistics */
 	atomic64_add(bo->base.size, &adev->num_bytes_moved);
-- 
2.25.1

[PATCH 2/2] drm/amdgpu: remove unused amdgpu_bo_validate

[Christian König]

Just drop some dead code.

Signed-off-by: Christian König <christian.koenig@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_object.c | 34 ----------------------
 drivers/gpu/drm/amd/amdgpu/amdgpu_object.h |  1 -
 2 files changed, 35 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
index 22a1344082d3..e2e8f0e1eda9 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
@@ -694,40 +694,6 @@ int amdgpu_bo_create_vm(struct amdgpu_device *adev,
 	return r;
 }
 
-/**
- * amdgpu_bo_validate - validate an &amdgpu_bo buffer object
- * @bo: pointer to the buffer object
- *
- * Sets placement according to domain; and changes placement and caching
- * policy of the buffer object according to the placement.
- * This is used for validating shadow bos.  It calls ttm_bo_validate() to
- * make sure the buffer is resident where it needs to be.
- *
- * Returns:
- * 0 for success or a negative error code on failure.
- */
-int amdgpu_bo_validate(struct amdgpu_bo *bo)
-{
-	struct ttm_operation_ctx ctx = { false, false };
-	uint32_t domain;
-	int r;
-
-	if (bo->tbo.pin_count)
-		return 0;
-
-	domain = bo->preferred_domains;
-
-retry:
-	amdgpu_bo_placement_from_domain(bo, domain);
-	r = ttm_bo_validate(&bo->tbo, &bo->placement, &ctx);
-	if (unlikely(r == -ENOMEM) && domain != bo->allowed_domains) {
-		domain = bo->allowed_domains;
-		goto retry;
-	}
-
-	return r;
-}
-
 /**
  * amdgpu_bo_add_to_shadow_list - add a BO to the shadow list
  *
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.h
index 38c834d0f930..0d7771e274ff 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.h
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.h
@@ -328,7 +328,6 @@ int amdgpu_bo_sync_wait_resv(struct amdgpu_device *adev, struct dma_resv *resv,
 int amdgpu_bo_sync_wait(struct amdgpu_bo *bo, void *owner, bool intr);
 u64 amdgpu_bo_gpu_offset(struct amdgpu_bo *bo);
 u64 amdgpu_bo_gpu_offset_no_check(struct amdgpu_bo *bo);
-int amdgpu_bo_validate(struct amdgpu_bo *bo);
 void amdgpu_bo_get_memory(struct amdgpu_bo *bo, uint64_t *vram_mem,
 				uint64_t *gtt_mem, uint64_t *cpu_mem);
 void amdgpu_bo_add_to_shadow_list(struct amdgpu_bo_vm *vmbo);
-- 
2.25.1

Re: [PATCH 1/2] drm/amdgpu: fix use after free during BO move

[Das, Nirmoy]

  Acked-by: Nirmoy Das <nirmoy.das@amd.com> for the 1st patch and second 
patch is

Reviewed-by: Nirmoy Das <nirmoy.das@amd.com>


On 9/7/2021 10:14 AM, Christian König wrote:
> The memory backing old_mem is already freed at that point, move the
> check a bit more up.
>
> Signed-off-by: Christian König <christian.koenig@amd.com>
> Fixes: bfa3357ef9ab ("drm/ttm: allocate resource object instead of embedding it v2")
> Bug: https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgitlab.freedesktop.org%2Fdrm%2Famd%2F-%2Fissues%2F1699&amp;data=04%7C01%7Cnirmoy.das%40amd.com%7Ce76c4a0ac29e480fcf7108d971d79344%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637665992971099794%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&amp;sdata=JttDenpA2ZII0Ttktn3HMVodWWU0kJoPVPvQ3%2BnN4sw%3D&amp;reserved=0
> ---
>   drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c | 18 +++++++++---------
>   1 file changed, 9 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
> index 446943e32e3e..e2896ac2c9ce 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
> @@ -513,6 +513,15 @@ static int amdgpu_bo_move(struct ttm_buffer_object *bo, bool evict,
>   		goto out;
>   	}
>   
> +	if (bo->type == ttm_bo_type_device &&
> +	    new_mem->mem_type == TTM_PL_VRAM &&
> +	    old_mem->mem_type != TTM_PL_VRAM) {
> +		/* amdgpu_bo_fault_reserve_notify will re-set this if the CPU
> +		 * accesses the BO after it's moved.
> +		 */
> +		abo->flags &= ~AMDGPU_GEM_CREATE_CPU_ACCESS_REQUIRED;
> +	}
> +
>   	if (adev->mman.buffer_funcs_enabled) {
>   		if (((old_mem->mem_type == TTM_PL_SYSTEM &&
>   		      new_mem->mem_type == TTM_PL_VRAM) ||
> @@ -543,15 +552,6 @@ static int amdgpu_bo_move(struct ttm_buffer_object *bo, bool evict,
>   			return r;
>   	}
>   
> -	if (bo->type == ttm_bo_type_device &&
> -	    new_mem->mem_type == TTM_PL_VRAM &&
> -	    old_mem->mem_type != TTM_PL_VRAM) {
> -		/* amdgpu_bo_fault_reserve_notify will re-set this if the CPU
> -		 * accesses the BO after it's moved.
> -		 */
> -		abo->flags &= ~AMDGPU_GEM_CREATE_CPU_ACCESS_REQUIRED;
> -	}
> -
>   out:
>   	/* update statistics */
>   	atomic64_add(bo->base.size, &adev->num_bytes_moved);

Re: [PATCH 1/2] drm/amdgpu: fix use after free during BO move

[Michel Dänzer]

On 2021-09-07 10:14, Christian König wrote:
> The memory backing old_mem is already freed at that point, move the
> check a bit more up.
> 
> Signed-off-by: Christian König <christian.koenig@amd.com>
> Fixes: bfa3357ef9ab ("drm/ttm: allocate resource object instead of embedding it v2")
> Bug: https://gitlab.freedesktop.org/drm/amd/-/issues/1699
> ---
>  drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c | 18 +++++++++---------
>  1 file changed, 9 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
> index 446943e32e3e..e2896ac2c9ce 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
> @@ -513,6 +513,15 @@ static int amdgpu_bo_move(struct ttm_buffer_object *bo, bool evict,
>  		goto out;
>  	}
>  
> +	if (bo->type == ttm_bo_type_device &&
> +	    new_mem->mem_type == TTM_PL_VRAM &&
> +	    old_mem->mem_type != TTM_PL_VRAM) {
> +		/* amdgpu_bo_fault_reserve_notify will re-set this if the CPU
> +		 * accesses the BO after it's moved.
> +		 */
> +		abo->flags &= ~AMDGPU_GEM_CREATE_CPU_ACCESS_REQUIRED;
> +	}
> +
>  	if (adev->mman.buffer_funcs_enabled) {
>  		if (((old_mem->mem_type == TTM_PL_SYSTEM &&
>  		      new_mem->mem_type == TTM_PL_VRAM) ||
> @@ -543,15 +552,6 @@ static int amdgpu_bo_move(struct ttm_buffer_object *bo, bool evict,
>  			return r;
>  	}
>  
> -	if (bo->type == ttm_bo_type_device &&
> -	    new_mem->mem_type == TTM_PL_VRAM &&
> -	    old_mem->mem_type != TTM_PL_VRAM) {
> -		/* amdgpu_bo_fault_reserve_notify will re-set this if the CPU
> -		 * accesses the BO after it's moved.
> -		 */
> -		abo->flags &= ~AMDGPU_GEM_CREATE_CPU_ACCESS_REQUIRED;
> -	}
> -
>  out:
>  	/* update statistics */
>  	atomic64_add(bo->base.size, &adev->num_bytes_moved);
> 

Reviewed-by: Michel Dänzer <mdaenzer@redhat.com>

Thanks!


-- 
Earthling Michel Dänzer               |               https://redhat.com
Libre software enthusiast             |             Mesa and X developer