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

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