[PATCH] drm/nouveau/svm: drain fault handler before freeing svmm

[PATCH] drm/nouveau/svm: drain fault handler before freeing svmm

[Zhenhao Wan]

The SVM fault handler nouveau_svm_fault() looks up each fault's
nouveau_svmm from the per-device instance list under svm->mutex, caches
it in the fault array, then drops svm->mutex and dereferences the svmm
across blocking faults (mmu_interval_notifier_insert(), hmm_range_fault()),
taking svmm->mutex and touching svmm->vmm.

nouveau_svmm has no reference of its own. It is freed on DRM file close,
via nouveau_svmm_fini() -> mmu_notifier_put(), which frees asynchronously
through call_srcu() without waiting for the handler; svm->mutex is not
held during the dereference, and mmget_not_zero() pins the mm, not the
svmm. A fault handled concurrently with a close can therefore dereference
a freed svmm. flush_work() on the fault buffer otherwise runs only at
device removal and suspend, never on the per-client close path.

The instance is already unlinked earlier in the same close, by
nouveau_svmm_part(), so no new fault can resolve to this svmm. Drain the
fault handler in nouveau_svmm_fini() before the free, so any handler that
cached the pointer has completed. nouveau_cli_fini() already flushes
cli->work here, and device-scope teardown already uses the same idiom.

Fixes: eeaf06ac1a55 ("drm/nouveau/svm: initial support for shared virtual memory")
Reported-by: Yuhao Jiang <danisjiang@gmail.com>
Assisted-by: Claude:claude-opus-5
Cc: stable@vger.kernel.org
Signed-off-by: Zhenhao Wan <whi4ed0g@gmail.com>
---
 drivers/gpu/drm/nouveau/nouveau_svm.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/drivers/gpu/drm/nouveau/nouveau_svm.c b/drivers/gpu/drm/nouveau/nouveau_svm.c
index dcc92131488e..01191b126be5 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);
+
 		mutex_lock(&svmm->mutex);
 		svmm->vmm = NULL;
 		mutex_unlock(&svmm->mutex);

---
base-commit: db2ddb87143519e20a95aa36c60b36107b736a58
change-id: 20260813-nouveau-svm-svmm-uaf-bbcf36265aaa

Best regards,
--  
Zhenhao Wan <whi4ed0g@gmail.com>

Re: [PATCH] drm/nouveau/svm: drain fault handler before freeing svmm

[sashiko-bot]

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 <whi4ed0g@gmail.com>

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/20260813-nouveau-svm-svmm-uaf-v1-1-2d3112dea734@gmail.com?part=1