drm/panfrost: Fix GEM handle creation UAF

  From: Rob Clark <robdclark@chromium.org>

Relying on an unreturned handle to hold a reference to an object we
dereference is not safe.  Userspace can guess the handle and race us
by closing the handle from another thread.  The _create_with_handle()
that returns an object ptr is pretty much a pattern to avoid.  And
ideally creating the handle would be done after any needed dererencing.
But in this case creation of the mapping is tied to the handle creation.
Fortunately the mapping is refcnt'd and holds a reference to the object,
so we can drop the handle's reference once we hold a mapping reference.

Signed-off-by: Rob Clark <robdclark@chromium.org>
---
 drivers/gpu/drm/panfrost/panfrost_drv.c |  7 +++++++
 drivers/gpu/drm/panfrost/panfrost_gem.c | 10 +++++++---
 2 files changed, 14 insertions(+), 3 deletions(-)
  

On Fri, Dec 16, 2022 at 3:34 PM Rob Clark <robdclark@gmail.com> wrote:
>
> From: Rob Clark <robdclark@chromium.org>
>
> Relying on an unreturned handle to hold a reference to an object we
> dereference is not safe.  Userspace can guess the handle and race us
> by closing the handle from another thread.  The _create_with_handle()
> that returns an object ptr is pretty much a pattern to avoid.  And
> ideally creating the handle would be done after any needed dererencing.
> But in this case creation of the mapping is tied to the handle creation.
> Fortunately the mapping is refcnt'd and holds a reference to the object,
> so we can drop the handle's reference once we hold a mapping reference.
>
> Signed-off-by: Rob Clark <robdclark@chromium.org>
> ---
>  drivers/gpu/drm/panfrost/panfrost_drv.c |  7 +++++++
>  drivers/gpu/drm/panfrost/panfrost_gem.c | 10 +++++++---
>  2 files changed, 14 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/panfrost/panfrost_drv.c b/drivers/gpu/drm/panfrost/panfrost_drv.c
> index 2fa5afe21288..aa5848de647c 100644
> --- a/drivers/gpu/drm/panfrost/panfrost_drv.c
> +++ b/drivers/gpu/drm/panfrost/panfrost_drv.c
> @@ -98,6 +98,13 @@ static int panfrost_ioctl_create_bo(struct drm_device *dev, void *data,
>                 return PTR_ERR(bo);
>
>         mapping = panfrost_gem_mapping_get(bo, priv);
> +
> +       /*
> +        * Now that the mapping holds a reference to the bo until we no longer
> +        * need it, we can safely drop the handle's reference.
> +        */
Not too familiar with panfrost, but I don't see
panfrost_gem_mapping_get hold a reference to the bo?

> +       drm_gem_object_put(&bo->base.base);
> +
>         if (!mapping) {
>                 drm_gem_object_put(&bo->base.base);
>                 return -EINVAL;
> diff --git a/drivers/gpu/drm/panfrost/panfrost_gem.c b/drivers/gpu/drm/panfrost/panfrost_gem.c
> index 293e799e2fe8..e3e21c500d24 100644
> --- a/drivers/gpu/drm/panfrost/panfrost_gem.c
> +++ b/drivers/gpu/drm/panfrost/panfrost_gem.c
> @@ -234,6 +234,10 @@ struct drm_gem_object *panfrost_gem_create_object(struct drm_device *dev, size_t
>         return &obj->base.base;
>  }
>
> +/*
> + * NOTE: if this succeeds, both the handle and the returned object have
> + * an outstanding reference.
> + */
I might suggest dropping the "_with_handle" suffix.

The naming convention is used in several drivers.  I think we should
make it the case that the _with_handle variants always return the
handle without the pointer.  (And with the change, it immediately
becomes clear that qxl and vmwgfx also have similar issues).

>  struct panfrost_gem_object *
>  panfrost_gem_create_with_handle(struct drm_file *file_priv,
>                                 struct drm_device *dev, size_t size,
> @@ -261,10 +265,10 @@ panfrost_gem_create_with_handle(struct drm_file *file_priv,
>          * and handle has the id what user can see.
>          */
>         ret = drm_gem_handle_create(file_priv, &shmem->base, handle);
> -       /* drop reference from allocate - handle holds it now. */
> -       drm_gem_object_put(&shmem->base);
> -       if (ret)
> +       if (ret) {
> +               drm_gem_object_put(&shmem->base);
>                 return ERR_PTR(ret);
> +       }
>
>         return bo;
>  }
> --
> 2.38.1
>
  

On Fri, Dec 16, 2022 at 3:59 PM Chia-I Wu <olvaffe@gmail.com> wrote:
>
> On Fri, Dec 16, 2022 at 3:34 PM Rob Clark <robdclark@gmail.com> wrote:
> >
> > From: Rob Clark <robdclark@chromium.org>
> >
> > Relying on an unreturned handle to hold a reference to an object we
> > dereference is not safe.  Userspace can guess the handle and race us
> > by closing the handle from another thread.  The _create_with_handle()
> > that returns an object ptr is pretty much a pattern to avoid.  And
> > ideally creating the handle would be done after any needed dererencing.
> > But in this case creation of the mapping is tied to the handle creation.
> > Fortunately the mapping is refcnt'd and holds a reference to the object,
> > so we can drop the handle's reference once we hold a mapping reference.
> >
> > Signed-off-by: Rob Clark <robdclark@chromium.org>
> > ---
> >  drivers/gpu/drm/panfrost/panfrost_drv.c |  7 +++++++
> >  drivers/gpu/drm/panfrost/panfrost_gem.c | 10 +++++++---
> >  2 files changed, 14 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/panfrost/panfrost_drv.c b/drivers/gpu/drm/panfrost/panfrost_drv.c
> > index 2fa5afe21288..aa5848de647c 100644
> > --- a/drivers/gpu/drm/panfrost/panfrost_drv.c
> > +++ b/drivers/gpu/drm/panfrost/panfrost_drv.c
> > @@ -98,6 +98,13 @@ static int panfrost_ioctl_create_bo(struct drm_device *dev, void *data,
> >                 return PTR_ERR(bo);
> >
> >         mapping = panfrost_gem_mapping_get(bo, priv);
> > +
> > +       /*
> > +        * Now that the mapping holds a reference to the bo until we no longer
> > +        * need it, we can safely drop the handle's reference.
> > +        */
> Not too familiar with panfrost, but I don't see
> panfrost_gem_mapping_get hold a reference to the bo?

It doesn't directly, but the mapping already holds a reference, taken
before the handle reference is dropped

It is all a bit too subtle for my taste.

> > +       drm_gem_object_put(&bo->base.base);
> > +
> >         if (!mapping) {
> >                 drm_gem_object_put(&bo->base.base);
> >                 return -EINVAL;
> > diff --git a/drivers/gpu/drm/panfrost/panfrost_gem.c b/drivers/gpu/drm/panfrost/panfrost_gem.c
> > index 293e799e2fe8..e3e21c500d24 100644
> > --- a/drivers/gpu/drm/panfrost/panfrost_gem.c
> > +++ b/drivers/gpu/drm/panfrost/panfrost_gem.c
> > @@ -234,6 +234,10 @@ struct drm_gem_object *panfrost_gem_create_object(struct drm_device *dev, size_t
> >         return &obj->base.base;
> >  }
> >
> > +/*
> > + * NOTE: if this succeeds, both the handle and the returned object have
> > + * an outstanding reference.
> > + */
> I might suggest dropping the "_with_handle" suffix.

Maybe _and_handle would be a better name (for this and other cases
that return both a handle and obj)?

> The naming convention is used in several drivers.  I think we should
> make it the case that the _with_handle variants always return the
> handle without the pointer.  (And with the change, it immediately
> becomes clear that qxl and vmwgfx also have similar issues).

ugg, yeah, qxl does have the issue in the qxl_mode_dumb_create path.
I overlooked that it returns an obj pointer by reference.

on the surface vmwgfx looked ok, but I could have overlooked something.

BR,
-R

> >  struct panfrost_gem_object *
> >  panfrost_gem_create_with_handle(struct drm_file *file_priv,
> >                                 struct drm_device *dev, size_t size,
> > @@ -261,10 +265,10 @@ panfrost_gem_create_with_handle(struct drm_file *file_priv,
> >          * and handle has the id what user can see.
> >          */
> >         ret = drm_gem_handle_create(file_priv, &shmem->base, handle);
> > -       /* drop reference from allocate - handle holds it now. */
> > -       drm_gem_object_put(&shmem->base);
> > -       if (ret)
> > +       if (ret) {
> > +               drm_gem_object_put(&shmem->base);
> >                 return ERR_PTR(ret);
> > +       }
> >
> >         return bo;
> >  }
> > --
> > 2.38.1
> >
  

On Fri, Dec 16, 2022 at 4:20 PM Rob Clark <robdclark@gmail.com> wrote:
>
> On Fri, Dec 16, 2022 at 3:59 PM Chia-I Wu <olvaffe@gmail.com> wrote:
> >
> > On Fri, Dec 16, 2022 at 3:34 PM Rob Clark <robdclark@gmail.com> wrote:
> > >
> > > From: Rob Clark <robdclark@chromium.org>
> > >
> > > Relying on an unreturned handle to hold a reference to an object we
> > > dereference is not safe.  Userspace can guess the handle and race us
> > > by closing the handle from another thread.  The _create_with_handle()
> > > that returns an object ptr is pretty much a pattern to avoid.  And
> > > ideally creating the handle would be done after any needed dererencing.
> > > But in this case creation of the mapping is tied to the handle creation.
> > > Fortunately the mapping is refcnt'd and holds a reference to the object,
> > > so we can drop the handle's reference once we hold a mapping reference.
> > >
> > > Signed-off-by: Rob Clark <robdclark@chromium.org>
> > > ---
> > >  drivers/gpu/drm/panfrost/panfrost_drv.c |  7 +++++++
> > >  drivers/gpu/drm/panfrost/panfrost_gem.c | 10 +++++++---
> > >  2 files changed, 14 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/panfrost/panfrost_drv.c b/drivers/gpu/drm/panfrost/panfrost_drv.c
> > > index 2fa5afe21288..aa5848de647c 100644
> > > --- a/drivers/gpu/drm/panfrost/panfrost_drv.c
> > > +++ b/drivers/gpu/drm/panfrost/panfrost_drv.c
> > > @@ -98,6 +98,13 @@ static int panfrost_ioctl_create_bo(struct drm_device *dev, void *data,
> > >                 return PTR_ERR(bo);
> > >
> > >         mapping = panfrost_gem_mapping_get(bo, priv);
> > > +
> > > +       /*
> > > +        * Now that the mapping holds a reference to the bo until we no longer
> > > +        * need it, we can safely drop the handle's reference.
> > > +        */
> > Not too familiar with panfrost, but I don't see
> > panfrost_gem_mapping_get hold a reference to the bo?
>
> It doesn't directly, but the mapping already holds a reference, taken
> before the handle reference is dropped
>
> It is all a bit too subtle for my taste.
Ack.
> > > +       drm_gem_object_put(&bo->base.base);
> > > +
> > >         if (!mapping) {
> > >                 drm_gem_object_put(&bo->base.base);
> > >                 return -EINVAL;
> > > diff --git a/drivers/gpu/drm/panfrost/panfrost_gem.c b/drivers/gpu/drm/panfrost/panfrost_gem.c
> > > index 293e799e2fe8..e3e21c500d24 100644
> > > --- a/drivers/gpu/drm/panfrost/panfrost_gem.c
> > > +++ b/drivers/gpu/drm/panfrost/panfrost_gem.c
> > > @@ -234,6 +234,10 @@ struct drm_gem_object *panfrost_gem_create_object(struct drm_device *dev, size_t
> > >         return &obj->base.base;
> > >  }
> > >
> > > +/*
> > > + * NOTE: if this succeeds, both the handle and the returned object have
> > > + * an outstanding reference.
> > > + */
> > I might suggest dropping the "_with_handle" suffix.
>
> Maybe _and_handle would be a better name (for this and other cases
> that return both a handle and obj)?
Sounds good.  We will want that for panfrost, qxl, and vmwgfx.

The other drivers and helpers only need the handle.  Their
_with_handle should be fixed to not return the pointer.

>
> > The naming convention is used in several drivers.  I think we should
> > make it the case that the _with_handle variants always return the
> > handle without the pointer.  (And with the change, it immediately
> > becomes clear that qxl and vmwgfx also have similar issues).
>
> ugg, yeah, qxl does have the issue in the qxl_mode_dumb_create path.
> I overlooked that it returns an obj pointer by reference.
>
> on the surface vmwgfx looked ok, but I could have overlooked something.
>
> BR,
> -R
>
> > >  struct panfrost_gem_object *
> > >  panfrost_gem_create_with_handle(struct drm_file *file_priv,
> > >                                 struct drm_device *dev, size_t size,
> > > @@ -261,10 +265,10 @@ panfrost_gem_create_with_handle(struct drm_file *file_priv,
> > >          * and handle has the id what user can see.
> > >          */
> > >         ret = drm_gem_handle_create(file_priv, &shmem->base, handle);
> > > -       /* drop reference from allocate - handle holds it now. */
> > > -       drm_gem_object_put(&shmem->base);
> > > -       if (ret)
> > > +       if (ret) {
> > > +               drm_gem_object_put(&shmem->base);
> > >                 return ERR_PTR(ret);
> > > +       }
> > >
> > >         return bo;
> > >  }
> > > --
> > > 2.38.1
> > >
  

On 16/12/2022 23:33, Rob Clark wrote:
> From: Rob Clark <robdclark@chromium.org>
> 
> Relying on an unreturned handle to hold a reference to an object we
> dereference is not safe.  Userspace can guess the handle and race us
> by closing the handle from another thread.  The _create_with_handle()
> that returns an object ptr is pretty much a pattern to avoid.  And
> ideally creating the handle would be done after any needed dererencing.
> But in this case creation of the mapping is tied to the handle creation.
> Fortunately the mapping is refcnt'd and holds a reference to the object,
> so we can drop the handle's reference once we hold a mapping reference.

Thanks for spotting this, it's a small window but definitely a bug.

> Signed-off-by: Rob Clark <robdclark@chromium.org>
> ---
>  drivers/gpu/drm/panfrost/panfrost_drv.c |  7 +++++++
>  drivers/gpu/drm/panfrost/panfrost_gem.c | 10 +++++++---
>  2 files changed, 14 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/panfrost/panfrost_drv.c b/drivers/gpu/drm/panfrost/panfrost_drv.c
> index 2fa5afe21288..aa5848de647c 100644
> --- a/drivers/gpu/drm/panfrost/panfrost_drv.c
> +++ b/drivers/gpu/drm/panfrost/panfrost_drv.c
> @@ -98,6 +98,13 @@ static int panfrost_ioctl_create_bo(struct drm_device *dev, void *data,
>  		return PTR_ERR(bo);
>  
>  	mapping = panfrost_gem_mapping_get(bo, priv);
> +
> +	/*
> +	 * Now that the mapping holds a reference to the bo until we no longer
> +	 * need it, we can safely drop the handle's reference.
> +	 */
> +	drm_gem_object_put(&bo->base.base);
> +
>  	if (!mapping) {
>  		drm_gem_object_put(&bo->base.base);

This !mapping call to drm_gem_object_put() is suspicious. It doesn't
make any sense and if it can be reached is going to drive the reference
count negative. So I don't think the bug is completely gone.

If user space does the trick of freeing the handle between the call to
panfrost_gem_create_with_handle() and panfrost_gem_mapping_get() then
even with the extra reference we now have the call to
panfrost_gem_mapping_get() will fail and two references are dropped.

I think the whole _create_with_handle() approach was a bad idea and it's
best to simply drop the _with_handle part. I'll post a patch tidying
this up along with removing the drm_gem_object_put() in the !mapping case.

Thanks,

Steve

>  		return -EINVAL;
> diff --git a/drivers/gpu/drm/panfrost/panfrost_gem.c b/drivers/gpu/drm/panfrost/panfrost_gem.c
> index 293e799e2fe8..e3e21c500d24 100644
> --- a/drivers/gpu/drm/panfrost/panfrost_gem.c
> +++ b/drivers/gpu/drm/panfrost/panfrost_gem.c
> @@ -234,6 +234,10 @@ struct drm_gem_object *panfrost_gem_create_object(struct drm_device *dev, size_t
>  	return &obj->base.base;
>  }
>  
> +/*
> + * NOTE: if this succeeds, both the handle and the returned object have
> + * an outstanding reference.
> + */
>  struct panfrost_gem_object *
>  panfrost_gem_create_with_handle(struct drm_file *file_priv,
>  				struct drm_device *dev, size_t size,
> @@ -261,10 +265,10 @@ panfrost_gem_create_with_handle(struct drm_file *file_priv,
>  	 * and handle has the id what user can see.
>  	 */
>  	ret = drm_gem_handle_create(file_priv, &shmem->base, handle);
> -	/* drop reference from allocate - handle holds it now. */
> -	drm_gem_object_put(&shmem->base);
> -	if (ret)
> +	if (ret) {
> +		drm_gem_object_put(&shmem->base);
>  		return ERR_PTR(ret);
> +	}
>  
>  	return bo;
>  }