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

[David Airlie]

> From: Junrui Luo <moonafterrain@xxxxxxxxxxx>
>
> nouveau_exec_ucopy() and nouveau_uvmm_vm_bind_ucopy() pass user-supplied
> u32 counts to u_memcpya(). DRM_IOCTL_NOUVEAU_EXEC bounds only
> req->push_count against push_max, leaving req->wait_count and
> req->sig_count unchecked; DRM_IOCTL_NOUVEAU_VM_BIND bounds none of
> op_count, wait_count or sig_count. u_memcpya() itself only rejects
> multiplication overflow, which on 64-bit never triggers for a u32 count
> times a small element size.
>
> A wait_count of 0xffffffff therefore becomes a 64 GB vmemdup_user()
> request. Since vmemdup_user() allocates with GFP_USER and hence without
> __GFP_NOWARN, a size above INT_MAX trips the WARN_ON_ONCE() in
> __kvmalloc_node_noprof(); below that the kernel attempts an up to 2 GB
> vmalloc that GFP_USER also leaves uncharged to the caller's memcg. Both
> ioctls are DRM_RENDER_ALLOW, so any client holding a render node can
> issue this.
>
> Reject the oversized counts at the ioctl entry points, the way
> nouveau_gem_ioctl_pushbuf() and the existing push_count check already
> do, so that the client is told which limit it exceeded. Sync objects get
> NOUVEAU_MAX_SYNCS, matching both NOUVEAU_GEM_MAX_BUFFERS and the value
> xe settled on for the same field in DRM_XE_MAX_SYNCS. VM_BIND operations
> have no comparable semantic limit, so NOUVEAU_VM_BIND_MAX_OPS is set
> well above any batch size a client is expected to submit; it exists only
> to keep the copy-in allocation finite.
>
> Fixes: b88baab82871 ("drm/nouveau: implement new VM_BIND uAPI")
> Reported-by: Yuhao Jiang <danisjiang@xxxxxxxxx>
> Assisted-by: Claude:claude-opus-5
> Cc: stable@xxxxxxxxxxxxxxx
> Signed-off-by: Junrui Luo <moonafterrain@xxxxxxxxxxx>

Have you run a complete Vulkan CTS with NVK across this with no regressions?

I'm weary of those limits being too low, but I think the only app that
might push them beyond CTS would be Forza Horizon 5 or 6.

Dave.


> ---
>  drivers/gpu/drm/nouveau/nouveau_exec.c | 12 ++++++++++++
>  drivers/gpu/drm/nouveau/nouveau_uvmm.c | 18 ++++++++++++++++++
>  include/uapi/drm/nouveau_drm.h         | 18 ++++++++++++++++++
>  3 files changed, 48 insertions(+)
>
> diff --git a/drivers/gpu/drm/nouveau/nouveau_exec.c b/drivers/gpu/drm/nouveau/nouveau_exec.c
> index a08ab1cfea9b..7bdccbae53b1 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);
> +               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);
>         if (ret)
>                 goto out;
> diff --git a/drivers/gpu/drm/nouveau/nouveau_uvmm.c b/drivers/gpu/drm/nouveau/nouveau_uvmm.c
> index f5e4756b4de4..bced1481674e 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)) {
> +               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);
>         if (ret)
>                 return ret;
> diff --git a/include/uapi/drm/nouveau_drm.h b/include/uapi/drm/nouveau_drm.h
> index 1fa82fa6af38..35ddf97ca873 100644
> --- a/include/uapi/drm/nouveau_drm.h
> +++ b/include/uapi/drm/nouveau_drm.h
> @@ -220,6 +220,14 @@ struct drm_nouveau_gem_cpu_fini {
>         __u32 handle;
>  };
>
> +/*
> + * NOUVEAU_MAX_SYNCS - maximum number of sync objects per ioctl
> + *
> + * The maximum value EXEC and VM_BIND accept in their wait_count and
> + * sig_count fields.
> + */
> +#define NOUVEAU_MAX_SYNCS 1024
> +
>  /**
>   * struct drm_nouveau_sync - sync object
>   *
> @@ -332,6 +340,16 @@ struct drm_nouveau_vm_bind_op {
>         __u64 range;
>  };
>
> +/*
> + * NOUVEAU_VM_BIND_MAX_OPS - maximum number of &drm_nouveau_vm_bind_ops
> + *
> + * The maximum value VM_BIND accepts in its op_count field. There is no
> + * semantic limit on the number of operations a bind may carry; this bound
> + * exists only to keep the copy-in allocation finite and is far above any
> + * batch size a client is expected to submit.
> + */
> +#define NOUVEAU_VM_BIND_MAX_OPS 65536
> +
>  /**
>   * struct drm_nouveau_vm_bind - structure for DRM_IOCTL_NOUVEAU_VM_BIND
>   */
>
> --
> 2.51.2
>
>