Re: [PATCH] drm/nouveau: disable VM_BIND once a legacy BO VMA is created

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

Pre-existing issues:
- [High] A time-of-check to time-of-use (TOCTOU) race condition in `nouveau_gem_object_open()` allows a VM_BIND client to bypass the `NOUVEAU_GEM_DOMAIN_NO_SHARE` restriction and open exclusive buffers.
- [High] Unprivileged userspace can trigger a massive memory and GPU resource leak by repeatedly calling the `NOUVEAU_VM_INIT` ioctl.
--

--- Patch [1]: [PATCH] drm/nouveau: disable VM_BIND once a legacy BO VMA is created ---
commit f241cc2db54abcee53ca3ff7b4e2aadd6db117dd
Author: Zhenhao Wan <[email protected]>

drm/nouveau: disable VM_BIND once a legacy BO VMA is created

This commit fixes a use-after-free issue where a legacy per-client VMA could
be leaked if a client initializes VM_BIND after opening an object. It calls
nouveau_cli_disable_uvmm_noinit() in nouveau_gem_object_open() to disable
VM_BIND for clients creating legacy VMAs.

> diff --git a/drivers/gpu/drm/nouveau/nouveau_gem.c b/drivers/gpu/drm/nouveau/nouveau_gem.c
> index 20dba02d6175b..e33f76b5a2398 100644
> --- a/drivers/gpu/drm/nouveau/nouveau_gem.c
> +++ b/drivers/gpu/drm/nouveau/nouveau_gem.c
> @@ -108,6 +108,14 @@ nouveau_gem_object_open(struct drm_gem_object *gem, struct drm_file *file_priv)
>  	if (vmm->vmm.object.oclass < NVIF_CLASS_VMM_NV50)
>  		return 0;
>  
> +	/*
> +	 * Opening the object binds it through the legacy per-VMA path below.
> +	 * Disable VM_BIND for this client, as nouveau_gem_ioctl_new() does, so
> +	 * a later NOUVEAU_VM_INIT cannot switch the client to uvmm and leave
> +	 * nouveau_gem_object_close() unable to tear this VMA down.
> +	 */
> +	nouveau_cli_disable_uvmm_noinit(cli);
> +
>  	if (nvbo->no_share && uvmm &&
>  	    drm_gpuvm_resv(&uvmm->base) != nvbo->bo.base.resv)
>  		return -EPERM;

[Severity: High]
This is a pre-existing issue, but does this function have a time-of-check to
time-of-use race condition involving the cached uvmm pointer?

The uvmm pointer is cached without locks at the start of
nouveau_gem_object_open():

	struct nouveau_uvmm *uvmm = nouveau_cli_uvmm(cli);

If a client guesses the flink name of a no_share buffer, calls GEM_OPEN, and
concurrently calls NOUVEAU_VM_INIT:

CPU1 calls NOUVEAU_VM_INIT and sets cli->uvmm.ptr

CPU2 in nouveau_gem_object_open():
	struct nouveau_uvmm *uvmm = nouveau_cli_uvmm(cli); /* caches NULL */
	...
	nouveau_cli_disable_uvmm_noinit(cli); /* locks and disables */

	if (nvbo->no_share && uvmm &&
	    drm_gpuvm_resv(&uvmm->base) != nvbo->bo.base.resv)
		return -EPERM;

Can NOUVEAU_VM_INIT complete after uvmm is cached as NULL but before
nouveau_cli_disable_uvmm_noinit() acquires the lock?

If so, the no_share condition would be evaluated using the stale NULL pointer,
bypassing the restriction and allowing a client to map an exclusive buffer
belonging to another VM into its own GPU VM. Should uvmm be reloaded after
taking the lock or before the check?


[Severity: High]
This isn't a bug introduced by this patch, but can unprivileged userspace
trigger a massive memory and GPU resource leak through the VM_BIND ioctl?

Looking at nouveau_uvmm_ioctl_vm_init() in
drivers/gpu/drm/nouveau/nouveau_uvmm.c:

	uvmm = kzalloc_obj(*uvmm);
	if (!uvmm) {
		ret = -ENOMEM;
		goto out_unlock;
	}
...
	cli->uvmm.ptr = uvmm;

If an attacker repeatedly calls DRM_IOCTL_NOUVEAU_VM_INIT on a single DRM file
descriptor, does this ioctl handler lack a check to see if cli->uvmm.ptr is
already initialized?

It appears to unconditionally allocate a new struct nouveau_uvmm and overwrite
cli->uvmm.ptr. Does this permanently leak the previously allocated structures
and GPU page tables, potentially leading to a local denial of service?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1