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
