Re: [PATCH AUTOSEL 4.14 6/7] drm/nouveau: block a bunch of classes from userspace

From: Lyude Paul
Date: Tue Aug 24 2021 - 13:08:02 EST


This isn't at all intended to be a fix to be backported, so I don't think this
should be included. I don't know about 5/7, but I'll let Benjamin comment on
that one

On Mon, 2021-08-23 at 20:55 -0400, Sasha Levin wrote:
> From: Ben Skeggs <bskeggs@xxxxxxxxxx>
>
> [ Upstream commit 148a8653789c01f159764ffcc3f370008966b42f ]
>
> Long ago, there had been plans for making use of a bunch of these APIs
> from userspace and there's various checks in place to stop misbehaving.
>
> Countless other projects have occurred in the meantime, and the pieces
> didn't finish falling into place for that to happen.
>
> They will (hopefully) in the not-too-distant future, but it won't look
> quite as insane.  The super checks are causing problems right now, and
> are going to be removed.
>
> Signed-off-by: Ben Skeggs <bskeggs@xxxxxxxxxx>
> Reviewed-by: Lyude Paul <lyude@xxxxxxxxxx>
> Signed-off-by: Sasha Levin <sashal@xxxxxxxxxx>
> ---
>  drivers/gpu/drm/nouveau/include/nvif/cl0080.h |  3 +-
>  drivers/gpu/drm/nouveau/nouveau_drm.c         |  1 +
>  drivers/gpu/drm/nouveau/nouveau_usif.c        | 57 ++++++++++++++-----
>  .../gpu/drm/nouveau/nvkm/engine/device/user.c |  2 +-
>  4 files changed, 48 insertions(+), 15 deletions(-)
>
> diff --git a/drivers/gpu/drm/nouveau/include/nvif/cl0080.h
> b/drivers/gpu/drm/nouveau/include/nvif/cl0080.h
> index 2740278d226b..61c17acd507c 100644
> --- a/drivers/gpu/drm/nouveau/include/nvif/cl0080.h
> +++ b/drivers/gpu/drm/nouveau/include/nvif/cl0080.h
> @@ -4,7 +4,8 @@
>  
>  struct nv_device_v0 {
>         __u8  version;
> -       __u8  pad01[7];
> +       __u8  priv;
> +       __u8  pad02[6];
>         __u64 device;   /* device identifier, ~0 for client default */
>  };
>  
> diff --git a/drivers/gpu/drm/nouveau/nouveau_drm.c
> b/drivers/gpu/drm/nouveau/nouveau_drm.c
> index fb6b1d0f7fef..fc54a26598cc 100644
> --- a/drivers/gpu/drm/nouveau/nouveau_drm.c
> +++ b/drivers/gpu/drm/nouveau/nouveau_drm.c
> @@ -151,6 +151,7 @@ nouveau_cli_init(struct nouveau_drm *drm, const char
> *sname,
>         ret = nvif_device_init(&cli->base.object, 0, NV_DEVICE,
>                                &(struct nv_device_v0) {
>                                         .device = ~0,
> +                                       .priv = true,
>                                }, sizeof(struct nv_device_v0),
>                                &cli->device);
>         if (ret) {
> diff --git a/drivers/gpu/drm/nouveau/nouveau_usif.c
> b/drivers/gpu/drm/nouveau/nouveau_usif.c
> index 9dc10b17ad34..5da1f4d223d7 100644
> --- a/drivers/gpu/drm/nouveau/nouveau_usif.c
> +++ b/drivers/gpu/drm/nouveau/nouveau_usif.c
> @@ -32,6 +32,9 @@
>  #include <nvif/event.h>
>  #include <nvif/ioctl.h>
>  
> +#include <nvif/class.h>
> +#include <nvif/cl0080.h>
> +
>  struct usif_notify_p {
>         struct drm_pending_event base;
>         struct {
> @@ -261,7 +264,7 @@ usif_object_dtor(struct usif_object *object)
>  }
>  
>  static int
> -usif_object_new(struct drm_file *f, void *data, u32 size, void *argv, u32
> argc)
> +usif_object_new(struct drm_file *f, void *data, u32 size, void *argv, u32
> argc, bool parent_abi16)
>  {
>         struct nouveau_cli *cli = nouveau_cli(f);
>         struct nvif_client *client = &cli->base;
> @@ -271,23 +274,48 @@ usif_object_new(struct drm_file *f, void *data, u32
> size, void *argv, u32 argc)
>         struct usif_object *object;
>         int ret = -ENOSYS;
>  
> +       if ((ret = nvif_unpack(ret, &data, &size, args->v0, 0, 0, true)))
> +               return ret;
> +
> +       switch (args->v0.oclass) {
> +       case NV_DMA_FROM_MEMORY:
> +       case NV_DMA_TO_MEMORY:
> +       case NV_DMA_IN_MEMORY:
> +               return -EINVAL;
> +       case NV_DEVICE: {
> +               union {
> +                       struct nv_device_v0 v0;
> +               } *args = data;
> +
> +               if ((ret = nvif_unpack(ret, &data, &size, args->v0, 0, 0,
> false)))
> +                       return ret;
> +
> +               args->v0.priv = false;
> +               break;
> +       }
> +       default:
> +               if (!parent_abi16)
> +                       return -EINVAL;
> +               break;
> +       }
> +
>         if (!(object = kmalloc(sizeof(*object), GFP_KERNEL)))
>                 return -ENOMEM;
>         list_add(&object->head, &cli->objects);
>  
> -       if (!(ret = nvif_unpack(ret, &data, &size, args->v0, 0, 0, true))) {
> -               object->route = args->v0.route;
> -               object->token = args->v0.token;
> -               args->v0.route = NVDRM_OBJECT_USIF;
> -               args->v0.token = (unsigned long)(void *)object;
> -               ret = nvif_client_ioctl(client, argv, argc);
> -               args->v0.token = object->token;
> -               args->v0.route = object->route;
> +       object->route = args->v0.route;
> +       object->token = args->v0.token;
> +       args->v0.route = NVDRM_OBJECT_USIF;
> +       args->v0.token = (unsigned long)(void *)object;
> +       ret = nvif_client_ioctl(client, argv, argc);
> +       if (ret) {
> +               usif_object_dtor(object);
> +               return ret;
>         }
>  
> -       if (ret)
> -               usif_object_dtor(object);
> -       return ret;
> +       args->v0.token = object->token;
> +       args->v0.route = object->route;
> +       return 0;
>  }
>  
>  int
> @@ -301,6 +329,7 @@ usif_ioctl(struct drm_file *filp, void __user *user, u32
> argc)
>                 struct nvif_ioctl_v0 v0;
>         } *argv = data;
>         struct usif_object *object;
> +       bool abi16 = false;
>         u8 owner;
>         int ret;
>  
> @@ -331,11 +360,13 @@ usif_ioctl(struct drm_file *filp, void __user *user,
> u32 argc)
>                         mutex_unlock(&cli->mutex);
>                         goto done;
>                 }
> +
> +               abi16 = true;
>         }
>  
>         switch (argv->v0.type) {
>         case NVIF_IOCTL_V0_NEW:
> -               ret = usif_object_new(filp, data, size, argv, argc);
> +               ret = usif_object_new(filp, data, size, argv, argc, abi16);
>                 break;
>         case NVIF_IOCTL_V0_NTFY_NEW:
>                 ret = usif_notify_new(filp, data, size, argv, argc);
> diff --git a/drivers/gpu/drm/nouveau/nvkm/engine/device/user.c
> b/drivers/gpu/drm/nouveau/nvkm/engine/device/user.c
> index 513ee6b79553..08100eed9584 100644
> --- a/drivers/gpu/drm/nouveau/nvkm/engine/device/user.c
> +++ b/drivers/gpu/drm/nouveau/nvkm/engine/device/user.c
> @@ -347,7 +347,7 @@ nvkm_udevice_new(const struct nvkm_oclass *oclass, void
> *data, u32 size,
>                 return ret;
>  
>         /* give priviledged clients register access */
> -       if (client->super)
> +       if (args->v0.priv)
>                 func = &nvkm_udevice_super;
>         else
>                 func = &nvkm_udevice;

--
Cheers,
Lyude Paul (she/her)
Software Engineer at Red Hat