[PATCH] drm/amdgpu: unmap GART dma pages before free

[PATCH] drm/amdgpu: unmap GART dma pages before free

[Michail Tatas]

amdgpu_gart_table_ram_alloc() maps the GART dma pages
and neither the error path nor amdgpu_gart_table_ram_free()
unmaps those pages before freeing them, which could leave
the device writing to freed memory.

Fix by unmapping the GART dma pages before freeing them.

Signed-off-by: Michail Tatas <michail.tatas@gmail.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_gart.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_gart.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_gart.c
index c4c21dbbbdbf..780fb01530e6 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_gart.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_gart.c
@@ -209,6 +209,8 @@ int amdgpu_gart_table_ram_alloc(struct amdgpu_device *adev)
 error_resv:
 	amdgpu_bo_unreserve(bo);
 error:
+	dma_unmap_page(&adev->pdev->dev, sg_dma_address(sg->sgl), adev->gart.table_size,
+		       DMA_BIDIRECTIONAL);
 	amdgpu_bo_unref(&bo);
 	if (sg) {
 		sg_free_table(sg);
@@ -240,6 +242,8 @@ void amdgpu_gart_table_ram_free(struct amdgpu_device *adev)
 		amdgpu_bo_unreserve(adev->gart.bo);
 	}
 	amdgpu_bo_unref(&adev->gart.bo);
+	dma_unmap_page(&adev->pdev->dev, sg_dma_address(sg->sgl), adev->gart.table_size,
+		       DMA_BIDIRECTIONAL);
 	sg_free_table(sg);
 	kfree(sg);
 	p = virt_to_page(adev->gart.ptr);
-- 
2.43.0

Re: [PATCH] drm/amdgpu: unmap GART dma pages before free

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] drm/amdgpu: NULL pointer dereference in amdgpu_gart_table_ram_alloc error path
--

commit cffcb1919743d55f9dc644f758abf99d4187be7c
Author: Michail Tatas <michail.tatas@gmail.com>

drm/amdgpu: unmap GART dma pages before free

This commit unmaps the GART dma pages before freeing them to avoid leaving
the device writing to freed memory.

> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_gart.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_gart.c
> index c4c21dbbbdbf8..780fb01530e69 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_gart.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_gart.c
[ ... ]
> @@ -209,6 +209,8 @@ int amdgpu_gart_table_ram_alloc(struct amdgpu_device *adev)
>  error_resv:
>  	amdgpu_bo_unreserve(bo);
>  error:
> +	dma_unmap_page(&adev->pdev->dev, sg_dma_address(sg->sgl), adev->gart.table_size,
> +		       DMA_BIDIRECTIONAL);

[Severity: High]
Can this cause a NULL pointer dereference during memory pressure?

If memory allocation fails earlier in amdgpu_gart_table_ram_alloc(),
sg is set to NULL and execution jumps to the error label:

	sg = kmalloc_obj(*sg);
	if (!sg) {
		ret = -ENOMEM;
		goto error;
	}

The new dma_unmap_page() call unconditionally accesses sg->sgl at the error
label. If the execution arrived here due to the allocation failure above,
dereferencing sg->sgl will crash.

Additionally, if sg_alloc_table() fails, sg is allocated but sg->sgl may be
uninitialized when accessed here.

>  	amdgpu_bo_unref(&bo);
>  	if (sg) {
>  		sg_free_table(sg);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/arKhDFCSoPDNYaxX@michalis-linux?part=1