Re: [PATCH] drm/amdkfd: reject foreign device pages in svm_range_dma_map_dev()

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Critical] drm/amdkfd: race and UAF on prange->svm_bo in svm_range_dma_map_dev()
- [Critical] drm/amdkfd: foreign svm_bo pages on same GPU mapped in svm_range_dma_map_dev()
--

commit c0faccc56cc8a73d1adc2bd585a5c86379e8e7e7
Author: Aveline Noir <jm5905938@gmail.com>

drm/amdkfd: reject foreign device pages in svm_range_dma_map_dev()

This commit updates svm_range_dma_map_dev() to reject zone device pages whose
pgmap does not match the device of prange->svm_bo with -EFAULT, clearing the
DMA address entry before returning.

> diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_svm.c b/drivers/gpu/drm/amd/amdkfd/kfd_svm.c
> index ed5e3318877be..6e9b9dcaa1361 100644
> --- a/drivers/gpu/drm/amd/amdkfd/kfd_svm.c
> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_svm.c
> @@ -181,7 +181,21 @@ svm_range_dma_map_dev(struct amdgpu_device *adev, struct svm_range *prange,
>  
>  		page = hmm_pfn_to_page(hmm_pfns[i]);
>  		if (is_zone_device_page(page)) {
> -			struct amdgpu_device *bo_adev = prange->svm_bo->node->adev;
> +			struct amdgpu_device *bo_adev = prange->svm_bo ?
> +				prange->svm_bo->node->adev : NULL;

[Severity: Critical]
Can reading prange->svm_bo here without checking prange->ttm_res and without
holding prange->lock race with svm_range_bo_release() and cause a
use-after-free or NULL pointer dereference?

A struct svm_range only holds a reference on prange->svm_bo while
prange->ttm_res is non-NULL. When svm_range_vram_node_free() releases a
range's VRAM allocation, it clears prange->ttm_res and drops the range's
reference on prange->svm_bo while leaving prange->svm_bo non-NULL until the
final reference is released:

drivers/gpu/drm/amd/amdkfd/kfd_svm.c:svm_range_vram_node_free() {
	if (prange->ttm_res) {
		prange->ttm_res = NULL;
		mutex_unlock(&prange->lock);
		svm_range_bo_unref(prange->svm_bo);
	} else
		mutex_unlock(&prange->lock);
}

When the last reference to svm_bo is dropped (for example via asynchronous
folio release work in svm_migrate_folio_free() -> svm_range_bo_unref_async()
-> svm_range_bo_wq_release()), svm_range_bo_release() clears prange->svm_bo
under prange->lock without holding prange->migrate_mutex and then frees
svm_bo:

drivers/gpu/drm/amd/amdkfd/kfd_svm.c:svm_range_bo_release() {
		mutex_lock(&prange->lock);
		prange->svm_bo = NULL;
		/* prange should not hold vram page now */
		WARN_ONCE(prange->actual_loc, "prange should not hold vram page");
		mutex_unlock(&prange->lock);
}

In svm_range_validate_and_map(), svm_range_dma_map() ->
svm_range_dma_map_dev() runs before svm_range_lock(prange) is acquired:

drivers/gpu/drm/amd/amdkfd/kfd_svm.c:svm_range_validate_and_map() {
		if (!r) {
			offset = (addr >> PAGE_SHIFT) - prange->start;
			r = svm_range_dma_map(prange, ctx->bitmap, offset, npages,
					      range->hmm_range.hmm_pfns);
			if (r)
				pr_debug("failed %d to dma map range\n", r);
		}

		svm_range_lock(prange);
}

If prange->ttm_res is NULL while a stale prange->svm_bo pointer is still set,
can svm_range_bo_release() clear prange->svm_bo and free svm_bo concurrently
with svm_range_dma_map_dev()? In addition, if a stale prange->svm_bo on the
same device allows addr[i] to be marked with SVM_RANGE_VRAM_DOMAIN while
prange->ttm_res is NULL, wouldn't svm_range_map_to_gpus() see
(prange->svm_bo && prange->ttm_res) as false and pass vram_base_offset = 0, or
crash in svm_range_get_pte_flags() when dereferencing prange->svm_bo->node?

> +
> +			/*
> +			 * The pgmap owner is per device or XGMI hive, so HMM
> +			 * may return a page of another svm_bo, e.g. one shared
> +			 * with another process after fork(). The VRAM address
> +			 * below and svm_range_get_pte_flags() assume the
> +			 * svm_bo device.
> +			 */
> +			if (!bo_adev ||
> +			    page_pgmap(page) != &bo_adev->kfd.pgmap) {
> +				addr[i] = 0;
> +				return -EFAULT;
> +			}

[Severity: Critical]
Does checking page_pgmap(page) != &bo_adev->kfd.pgmap still allow foreign
svm_bo pages that reside on the same amdgpu_device to be mapped into the
range's GPU page tables?

In svm_migrate_get_vram_page(), each KFD VRAM zone device page records its
owning struct svm_range_bo in page->zone_device_data, whereas
bo_adev->kfd.pgmap is shared across the entire amdgpu_device:

drivers/gpu/drm/amd/amdkfd/kfd_migrate.c:svm_migrate_get_vram_page() {
	svm_range_bo_ref(prange->svm_bo);
	page->zone_device_data = prange->svm_bo;
	zone_device_page_init(page, page_pgmap(page), 0);
}

After fork(), a child range can hold both system RAM pages and COW-shared
device-private pages belonging to the parent's svm_bo on a given GPU. If the
child migrates that range to the same GPU via svm_migrate_ram_to_vram(),
svm_range_vram_node_new() allocates a new child svm_bo on that GPU, and
svm_migrate_vma_to_vram() only selects system RAM pages via
MIGRATE_VMA_SELECT_SYSTEM, leaving the parent's device-private PTEs in place:

drivers/gpu/drm/amd/amdkfd/kfd_migrate.c:svm_migrate_vma_to_vram() {
	migrate.vma = vma;
	migrate.start = start;
	migrate.end = end;
	migrate.flags = MIGRATE_VMA_SELECT_SYSTEM;
	migrate.pgmap_owner = SVM_ADEV_PGMAP_OWNER(adev);
}

When svm_range_validate_and_map() -> svm_range_dma_map_dev() subsequently
validates the child range, hmm_range_fault() returns the parent's zone device
pages, and page_pgmap(page) == &bo_adev->kfd.pgmap succeeds because both
svm_bo objects are on the same amdgpu_device.

Wouldn't accepting a page with page->zone_device_data != prange->svm_bo:
1. Map COW-shared parent VRAM pages as writable in the child's GPU VM when
   vma->vm_flags has VM_WRITE set, allowing the child GPU to overwrite parent
   memory?
2. Leave dangling GPU PTEs when the parent's svm_bo is evicted or freed,
   because the child prange is only on its own prange->svm_bo->range_list?
3. Use the wrong XCP partition in svm_range_get_pte_flags()
   (bo_node = prange->svm_bo->node) on multi-partition GPUs that share a
   single adev->kfd.pgmap?
4. Underflow old->vram_pages during svm_range_split_pages(), which counts all
   dma_addr entries with SVM_RANGE_VRAM_DOMAIN set even though those foreign
   pages were never added to prange->vram_pages?

Should this check verify that prange->ttm_res is non-NULL and
page->zone_device_data == prange->svm_bo instead of comparing page_pgmap(page)
against &bo_adev->kfd.pgmap?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261103-kfd-svm-foreign-vram-20261103-v1-1-d6282053cad3@gmail.com?part=1