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 <[email protected]> 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-d6282053c...@gmail.com?part=1
