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

[PATCH v2] 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 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()).

nouveau_svmm is not refcounted. On DRM file close nouveau_svmm_fini() ->
mmu_notifier_put() frees it asynchronously via call_srcu() without
waiting for the handler, and mmget_not_zero() pins the mm, not the svmm.
flush_work() on the fault buffer otherwise runs only at device removal
and suspend, never on close, so a fault handled concurrently with a close
can dereference a freed svmm.

The instance is already unlinked by nouveau_svmm_part() earlier in the
same close, so no new fault can resolve to it. Drain the handler in
nouveau_svmm_fini() before the free, guarding the flush with a NULL check
on drm->svm: on device teardown nouveau_svm_fini() runs first and frees
drm->svm (NULL) after blocking the notify and flushing the buffer, so the
handler is already drained; on per-client close drm->svm is alive and the
flush runs.

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>
---
Changes in v2:
- Fix a NULL deref introduced in v1: guard flush_work() with a NULL
  check on drm->svm.  On device teardown nouveau_svm_fini() frees
  drm->svm before the per-client svmm teardown, so nouveau_svmm_fini()
  would deref NULL (reported by Sashiko AI review); the handler is
  already drained there, so there is nothing left to flush.
- Tighten the commit message and the code comment; no functional change.
- Link to v1: https://patch.msgid.link/20260813-nouveau-svm-svmm-uaf-v1-1-2d3112dea734@gmail.com

To: Lyude Paul <lyude@redhat.com>
To: Danilo Krummrich <dakr@kernel.org>
To: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
To: Maxime Ripard <mripard@kernel.org>
To: Thomas Zimmermann <tzimmermann@suse.de>
To: David Airlie <airlied@gmail.com>
To: Simona Vetter <simona@ffwll.ch>
To: Ben Skeggs <bskeggs@redhat.com>
Cc: dri-devel@lists.freedesktop.org
Cc: nouveau@lists.freedesktop.org
Cc: linux-kernel@vger.kernel.org
---
 drivers/gpu/drm/nouveau/nouveau_svm.c | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/drivers/gpu/drm/nouveau/nouveau_svm.c b/drivers/gpu/drm/nouveau/nouveau_svm.c
index dcc92131488e..a87406d3da30 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.
+		 */
+		if (svm)
+			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 v2] 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:

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

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/20260814-nouveau-svm-svmm-uaf-v2-1-8e6590519f07@gmail.com?part=1