Re: [PATCH 1/2] drm/nouveau: bound sync and op counts in EXEC and VM_BIND

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Medium] Un-rate-limited error logging creates a potential log-spam DoS vector.
- [Medium] Validation of flag-gated syscall parameters outside their flag scope.
- [Low] Format string specifier mismatch in error logging for __u32 counts.
--

--- Patch [1]: [PATCH 1/2] drm/nouveau: bound sync and op counts in EXEC and VM_BIND ---
commit 6d0adda3ff0dd4a0e4440dc92362fcb0ad993d9a
Author: Junrui Luo <moonafterrain@outlook.com>
Subject: drm/nouveau: bound sync and op counts in EXEC and VM_BIND

This commit introduces upper bounds for sync and op counts in the EXEC and
VM_BIND ioctls. By restricting the size of these counts, it prevents potential
excessive memory allocations that could trigger warnings or exhaust memory.

> diff --git a/drivers/gpu/drm/nouveau/nouveau_exec.c b/drivers/gpu/drm/nouveau/nouveau_exec.c
> index a08ab1cfea9be..7bdccbae53b1d 100644
> --- a/drivers/gpu/drm/nouveau/nouveau_exec.c
> +++ b/drivers/gpu/drm/nouveau/nouveau_exec.c
> @@ -389,6 +389,18 @@ nouveau_exec_ioctl_exec(struct drm_device *dev,
>  		return nouveau_abi16_put(abi16, -EINVAL);
>  	}
>  
> +	if (unlikely(req->wait_count > NOUVEAU_MAX_SYNCS)) {
> +		NV_PRINTK(err, cli, "exec wait count exceeds limit: %d max %d\n",
> +			  req->wait_count, NOUVEAU_MAX_SYNCS);

[Severity: Low]
Are the format string specifiers correct here? The req->wait_count field is
a __u32 type, so using %d will print large values as negative numbers.

[Severity: Medium]
Could this un-rate-limited NV_PRINTK log be intentionally triggered by
userspace to spam the kernel log buffer and hide other messages?

> +		return nouveau_abi16_put(abi16, -EINVAL);
> +	}
> +
> +	if (unlikely(req->sig_count > NOUVEAU_MAX_SYNCS)) {
> +		NV_PRINTK(err, cli, "exec sig count exceeds limit: %d max %d\n",
> +			  req->sig_count, NOUVEAU_MAX_SYNCS);
> +		return nouveau_abi16_put(abi16, -EINVAL);
> +	}
> +
>  	ret = nouveau_exec_ucopy(&args, req);

[ ... ]

> diff --git a/drivers/gpu/drm/nouveau/nouveau_uvmm.c b/drivers/gpu/drm/nouveau/nouveau_uvmm.c
> index f5e4756b4de4a..bced1481674e7 100644
> --- a/drivers/gpu/drm/nouveau/nouveau_uvmm.c
> +++ b/drivers/gpu/drm/nouveau/nouveau_uvmm.c
> @@ -1807,6 +1807,24 @@ nouveau_uvmm_ioctl_vm_bind(struct drm_device *dev,
>  	if (unlikely(!nouveau_cli_uvmm_locked(cli)))
>  		return -ENOSYS;
>  
> +	if (unlikely(req->op_count > NOUVEAU_VM_BIND_MAX_OPS)) {
> +		NV_PRINTK(err, cli, "vm_bind op count exceeds limit: %d max %d\n",
> +			  req->op_count, NOUVEAU_VM_BIND_MAX_OPS);
> +		return -EINVAL;
> +	}
> +
> +	if (unlikely(req->wait_count > NOUVEAU_MAX_SYNCS)) {

[Severity: Medium]
According to the userspace API documentation, sync objects are only accepted
when DRM_NOUVEAU_VM_BIND_RUN_ASYNC is set in req->flags. If a user submits a
VM_BIND ioctl without this flag, might wait_count and sig_count contain
uninitialized garbage from userspace stack slots?

If so, is it safe to validate these flag-gated parameters outside the scope
of their flag check, rather than explicitly ignoring or rejecting them when
the flag is absent?

> +		NV_PRINTK(err, cli, "vm_bind wait count exceeds limit: %d max %d\n",
> +			  req->wait_count, NOUVEAU_MAX_SYNCS);
> +		return -EINVAL;
> +	}
> +
> +	if (unlikely(req->sig_count > NOUVEAU_MAX_SYNCS)) {
> +		NV_PRINTK(err, cli, "vm_bind sig count exceeds limit: %d max %d\n",
> +			  req->sig_count, NOUVEAU_MAX_SYNCS);
> +		return -EINVAL;
> +	}
> +
>  	ret = nouveau_uvmm_vm_bind_ucopy(&args, req);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260808-nouveau-fixes-v1-0-c3ebdc17a89c@outlook.com?part=1