[PATCH] drm/amdgpu: fix NULL pointer dereference in amdgpu_vm_init error path

[PATCH] drm/amdgpu: fix NULL pointer dereference in amdgpu_vm_init error path

[Yang Zi]

In amdgpu_vm_init(), the error_free_root cleanup label runs
amdgpu_vm_pt_free_root() before amdgpu_bo_unreserve(). However,
amdgpu_vm_pt_free_root() walks the page table tree and frees each
entry via amdgpu_vm_pt_free(), which calls amdgpu_bo_unref() on the
root BO and clears vm->root.bo to NULL. The subsequent
amdgpu_bo_unreserve(vm->root.bo) then dereferences a NULL pointer.

Swap the two statements so that the root BO is unreserved before it is
freed. This mirrors the success path at the end of the function, which
unreserves before releasing its reference.

Found by static analysis of the error handling path; the failure cases
(dma_resv_reserve_fences() or amdgpu_vm_pt_clear() returning an error)
trigger this path.

Signed-off-by: Yang Zi <2959243019@qq.com>
---
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
index d2ad5b0e8759..dc72181ea0fa 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
@@ -2672,8 +2672,8 @@ int amdgpu_vm_init(struct amdgpu_device *adev, struct amdgpu_vm *vm,
     return 0;
 
 error_free_root:
-    amdgpu_vm_pt_free_root(adev, vm);
     amdgpu_bo_unreserve(vm->root.bo);
+    amdgpu_vm_pt_free_root(adev, vm);
     amdgpu_bo_unref(&root_bo);
 
 error_free_delayed:

Re: [PATCH] drm/amdgpu: fix NULL pointer dereference in amdgpu_vm_init error path

[Christian König]

On 8/25/26 11:29, Yang Zi wrote:
> In amdgpu_vm_init(), the error_free_root cleanup label runs
> amdgpu_vm_pt_free_root() before amdgpu_bo_unreserve(). However,
> amdgpu_vm_pt_free_root() walks the page table tree and frees each
> entry via amdgpu_vm_pt_free(), which calls amdgpu_bo_unref() on the
> root BO and clears vm->root.bo to NULL. The subsequent
> amdgpu_bo_unreserve(vm->root.bo) then dereferences a NULL pointer.
> 
> Swap the two statements so that the root BO is unreserved before it is
> freed. This mirrors the success path at the end of the function, which
> unreserves before releasing its reference.
> 
> Found by static analysis of the error handling path; the failure cases
> (dma_resv_reserve_fences() or amdgpu_vm_pt_clear() returning an error)
> trigger this path.
> 
> Signed-off-by: Yang Zi <2959243019@qq.com>
> ---
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> index d2ad5b0e8759..dc72181ea0fa 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> @@ -2672,8 +2672,8 @@ int amdgpu_vm_init(struct amdgpu_device *adev, struct amdgpu_vm *vm,
>      return 0;
> 
>  error_free_root:
> -    amdgpu_vm_pt_free_root(adev, vm);
>      amdgpu_bo_unreserve(vm->root.bo);
> +    amdgpu_vm_pt_free_root(adev, vm);

Clear NAK, that is just nonsense and will potentially trigger tons of warnings.

The amdgpu_bo_unreserve(() call just needs to use the local variable root_bo instead.

Regards,
Christian.

>      amdgpu_bo_unref(&root_bo);
> 
>  error_free_delayed:
> 

[PATCH] drm/amdgpu: fix NULL pointer dereference in amdgpu_vm_init error path

[Yang Zi]

In amdgpu_vm_init(), the error_free_root cleanup label runs
amdgpu_vm_pt_free_root() before amdgpu_bo_unreserve(). However,
amdgpu_vm_pt_free_root() walks the page table tree and frees each
entry via amdgpu_vm_pt_free(), which calls amdgpu_bo_unref() on the
root BO and clears vm->root.bo to NULL. The subsequent
amdgpu_bo_unreserve(vm->root.bo) then dereferences a NULL pointer.

Swap the two statements so that the root BO is unreserved before it is
freed. This mirrors the success path at the end of the function, which
unreserves before releasing its reference.

Found by static analysis of the error handling path; the failure cases
(dma_resv_reserve_fences() or amdgpu_vm_pt_clear() returning an error)
trigger this path.

Signed-off-by: Yang Zi <2959243019@qq.com>
---
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
index d2ad5b0e8759..dc72181ea0fa 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
@@ -2672,8 +2672,8 @@ int amdgpu_vm_init(struct amdgpu_device *adev, struct amdgpu_vm *vm,
     return 0;
 
 error_free_root:
-    amdgpu_vm_pt_free_root(adev, vm);
     amdgpu_bo_unreserve(vm->root.bo);
+    amdgpu_vm_pt_free_root(adev, vm);
     amdgpu_bo_unref(&root_bo);
 
 error_free_delayed: