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 <[email protected]> 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/[email protected]?part=1
