Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
New issues: - [High] The patch introduces a NULL pointer dereference and a potential Use-After-Free (UAF) in `nouveau_svmm_fini()` during device teardown by unconditionally accessing `drm->svm`. Pre-existing issues: - [High] Preexisting NULL pointer dereference in `nouveau_svmm_part()` during device teardown due to premature freeing of `drm->svm`. -- --- Patch [1]: [PATCH] drm/nouveau/svm: drain fault handler before freeing svmm --- commit 31089477636a1a0ac80812429a50405db650eb3a Author: Zhenhao Wan <[email protected]> drm/nouveau/svm: drain fault handler before freeing svmm This patch drains the SVM fault handler in nouveau_svmm_fini() before freeing the svmm. It flushes the work on the fault buffer to ensure no in-flight references to the svmm remain, preventing a potential use-after-free during client file close. > diff --git a/drivers/gpu/drm/nouveau/nouveau_svm.c > b/drivers/gpu/drm/nouveau/nouveau_svm.c > index dcc92131488ea..01191b126be53 100644 > --- a/drivers/gpu/drm/nouveau/nouveau_svm.c > +++ b/drivers/gpu/drm/nouveau/nouveau_svm.c > @@ -304,6 +304,17 @@ 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 looked up under > + * svm->mutex and then dereferences them after dropping it, > + * across blocking faults. The instance has already been > + * unlinked (nouveau_svmm_part), so no new fault can resolve to > + * this svmm; drain the handler to release any in-flight > + * reference before the svmm is freed. > + */ > + flush_work(&svm->buffer[0].work); [Severity: High] Can `svm` be NULL here during device teardown? During device removal, `nouveau_drm_device_fini()` is called, which establishes this teardown order: nouveau_drm_device_fini() { ... nouveau_svm_fini(drm); // Frees drm->svm and sets it to NULL ... mutex_lock(&drm->clients_lock); list_for_each_entry_safe(cli, temp_cli, &drm->clients, head) { ... nouveau_cli_fini(cli); // Cascades to nouveau_svmm_fini() } } If a client is still open during device removal, will this unconditionally dereference a NULL `svm` pointer when calling `flush_work()`? Additionally, because there is no locking to synchronize `nouveau_svm_fini(drm)` with a concurrent user-initiated file close, could a file close operation racing with device removal encounter a use-after-free when accessing `drm->svm` here? [Severity: High] This is a pre-existing issue, but does `nouveau_svmm_part()` suffer from a similar teardown sequence problem? During `nouveau_drm_device_fini()`, after `drm->svm` is destroyed, `nouveau_abi16_fini()` deletes channels via `nouveau_channel_del()`. This eventually calls `nouveau_svmm_part()`: nouveau_svmm_part() { ... mutex_lock(&svmm->vmm->cli->drm->svm->mutex); ... } Will this cause a NULL pointer dereference on `drm->svm->mutex` if the device is unplugged while a client has an active SVM channel? > mutex_lock(&svmm->mutex); > svmm->vmm = NULL; -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=1
