Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
Pre-existing issues: - [Critical] Pre-existing NULL pointer dereference in `nouveau_svmm_part()` during device teardown. - [Critical] Use-After-Free in `nouveau_dmem_migrate_to_ram()` when accessing `svmm` after the DRM file descriptor is closed. -- --- Patch [1]: [PATCH v2] drm/nouveau/svm: drain fault handler before freeing svmm --- commit 6f30e7be0647c66cd039d8a3e352b2a4a7f9858c Author: Zhenhao Wan <[email protected]> drm/nouveau/svm: drain fault handler before freeing svmm The SVM fault handler nouveau_svm_fault() looks up each fault's nouveau_svmm under svm->mutex, caches it in the fault array, then drops svm->mutex and dereferences the svmm across blocking faults. nouveau_svmm is not refcounted, so draining the handler before freeing prevents a concurrently handled fault from dereferencing a freed svmm. > diff --git a/drivers/gpu/drm/nouveau/nouveau_svm.c > b/drivers/gpu/drm/nouveau/nouveau_svm.c > index dcc92131488ea..a87406d3da300 100644 > --- a/drivers/gpu/drm/nouveau/nouveau_svm.c > +++ b/drivers/gpu/drm/nouveau/nouveau_svm.c > @@ -304,6 +304,19 @@ nouveau_svmm_fini(struct nouveau_svmm **psvmm) > { > struct nouveau_svmm *svmm = *psvmm; > if (svmm) { > + struct nouveau_svm *svm = svmm->vmm->cli->drm->svm; > + > + /* The fault handler caches svmm pointers under svm->mutex and > + * dereferences them after dropping it, across blocking faults. > + * The instance is already unlinked (nouveau_svmm_part), so > drain > + * the handler before the free to release any in-flight > reference. > + * > + * On device teardown nouveau_svm_fini() ran first and freed > + * drm->svm (NULL) after draining the buffer; nothing to flush. > + */ [Severity: Critical] This is a pre-existing issue, but since the comment mentions that during device teardown nouveau_svm_fini() explicitly clears drm->svm to NULL, does this result in a NULL pointer dereference in nouveau_svmm_part()? During device teardown, nouveau_drm_device_fini() calls nouveau_svm_fini() which clears drm->svm. Later in the same function, it forcefully cleans up remaining clients, which calls nouveau_svmm_part(). nouveau_svmm_part() unconditionally attempts to lock the mutex: drivers/gpu/drm/nouveau/nouveau_svm.c:nouveau_svmm_part() { ... mutex_lock(&svmm->vmm->cli->drm->svm->mutex); ... } Could this cause a panic if a user space process holds an SVM-enabled channel open while the driver is being unbound or unloaded? > + if (svm) > + flush_work(&svm->buffer[0].work); > + > mutex_lock(&svmm->mutex); > svmm->vmm = NULL; > mutex_unlock(&svmm->mutex); [Severity: Critical] This is a pre-existing issue, but does freeing the svmm structure here create a use-after-free risk during a CPU page fault if the DRM file descriptor was closed? If userspace migrates anonymous memory to device memory via the NOUVEAU_SVM_BIND ioctl, the device-private pages are mapped into the process's VMA. Their folio_zone_device_data() is set to the client's per-file svmm structure. If userspace then closes the DRM file descriptor but keeps the process alive, the svmm structure is freed during cleanup. However, the device-private pages are not migrated back to RAM or unmapped from the process's page tables. When the process later accesses this memory, a CPU page fault occurs, triggering the device pagemap handler which attempts to lock the freed svmm mutex: drivers/gpu/drm/nouveau/nouveau_dmem.c:nouveau_dmem_migrate_to_ram() { ... svmm = folio_zone_device_data(sfolio); mutex_lock(&svmm->mutex); ... } Will this lock attempt trigger a use-after-free memory corruption? > mmu_notifier_put(&svmm->notifier); > *psvmm = NULL; > } > } -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=1
