Re: [PATCH] drm/nouveau/uvmm: reject zero-length range in validate_range

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

Pre-existing issues:
- [Critical] Use-After-Free and Double-Free due to missing `op->reg` clearing in `nouveau_uvmm_bind_job_submit` error paths.
- [High] The MSM and Panthor drivers, as well as the core GPUVM API, fail to reject zero-length VM_BIND mapping requests, causing interval tree corruption.
--

--- Patch [1]: [PATCH] drm/nouveau/uvmm: reject zero-length range in validate_range ---
commit e165efbc8c57a263c051c3911827b177dadfa7cf
Author: Zhenhao Wan <[email protected]>

drm/nouveau/uvmm: reject zero-length range in validate_range

This commit adds a missing check for range == 0 in
nouveau_uvmm_validate_range(). Since drm_gpuvm_range_valid() leaves the
zero-range rejection to its callers, nouveau previously allowed a zero-length
VM_BIND request, which caused an underflow when inserting into the interval
tree and corrupted overlap checks.

> diff --git a/drivers/gpu/drm/nouveau/nouveau_uvmm.c b/drivers/gpu/drm/nouveau/nouveau_uvmm.c
> index f5e4756b4de4a..0efedd9ecc750 100644
> --- a/drivers/gpu/drm/nouveau/nouveau_uvmm.c
> +++ b/drivers/gpu/drm/nouveau/nouveau_uvmm.c
> @@ -1008,7 +1008,7 @@ nouveau_uvmm_validate_range(struct nouveau_uvmm *uvmm, u64 addr, u64 range)
>  	if (addr & ~PAGE_MASK)
>  		return -EINVAL;
>  
> -	if (range & ~PAGE_MASK)
> +	if (!range || range & ~PAGE_MASK)

[Severity: High]
This isn't a bug introduced by this patch, but while fixing this for nouveau,
could the same missing zero-length rejection affect the MSM and Panthor
drivers?

In drivers/gpu/drm/msm/msm_gem_vma.c:lookup_op(), the driver validates VM_BIND
operations but doesn't explicitly reject op->range == 0.

In drivers/gpu/drm/panthor/panthor_mmu.c, panthor_vm_bind_prepare_op_ctx() and
panthor_vm_bind_job_create() seem to allow op->size == 0 for asynchronous MAP
and UNMAP operations.

Since drm_gpuvm_range_valid() intentionally ignores zero-ranges, wouldn't
these eventually call __drm_gpuva_insert() in drivers/gpu/drm/drm_gpuvm.c?

In that function, GPUVA_LAST(va) computes addr + range - 1, which underflows
to addr - 1 when range is 0. Does this corrupt the red-black interval tree?


[Severity: Critical]
This is a pre-existing issue, but looking further down in nouveau_uvmm.c, is
there a use-after-free risk in the error paths for
nouveau_uvmm_bind_job_submit()?

If an OP_UNMAP_SPARSE operation fails (e.g. because op->reg->dirty is true):

drivers/gpu/drm/nouveau/nouveau_uvmm.c:nouveau_uvmm_bind_job_submit() {
    ...
			if (!op->reg || op->reg->dirty) {
				ret = -ENOENT;
				goto unwind_continue;
			}
    ...
}

It takes the goto unwind_continue path without setting op->reg = NULL.

Then in the cleanup function:

drivers/gpu/drm/nouveau/nouveau_uvmm.c:nouveau_uvmm_bind_job_cleanup() {
    ...
			if (op->reg) {
				nouveau_uvma_region_sparse_unref(op->reg);
				nouveau_uvmm_lock(uvmm);
				nouveau_uvma_region_remove(op->reg);
				nouveau_uvmm_unlock(uvmm);
				nouveau_uvma_region_complete(op->reg);
				nouveau_uvma_region_put(op->reg);
			}
    ...
}

The cleanup function unconditionally tears down the region if op->reg is
non-NULL.

Since the operation failed and was skipped, the region was never legitimately
removed.

Could this cause the region to be incorrectly destroyed while potentially
still in use?

>  		return -EINVAL;
>  
>  	if (!drm_gpuvm_range_valid(&uvmm->base, addr, range))

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