From: Rob Clark <[email protected]>
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 <[email protected]>
---
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);
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;
}
--
2.38.1
From: Rob Clark <[email protected]>
Userspace can guess the handle value and try to race GEM object creation
with handle close, resulting in a use-after-free if we dereference the
object after dropping the handle's reference. For that reason, dropping
the handle's reference must be done *after* we are done dereferencing
the object.
Signed-off-by: Rob Clark <[email protected]>
---
drivers/gpu/drm/virtio/virtgpu_ioctl.c | 19 +++++++++++++++++--
1 file changed, 17 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/virtio/virtgpu_ioctl.c b/drivers/gpu/drm/virtio/virtgpu_ioctl.c
index a5cccccb4998..f1c55c1630ca 100644
--- a/drivers/gpu/drm/virtio/virtgpu_ioctl.c
+++ b/drivers/gpu/drm/virtio/virtgpu_ioctl.c
@@ -366,10 +366,18 @@ static int virtio_gpu_resource_create_ioctl(struct drm_device *dev, void *data,
drm_gem_object_release(obj);
return ret;
}
- drm_gem_object_put(obj);
rc->res_handle = qobj->hw_res_handle; /* similiar to a VM address */
rc->bo_handle = handle;
+
+ /*
+ * The handle owns the reference now. But we must drop our
+ * remaining reference *after* we no longer need to dereference
+ * the obj. Otherwise userspace could guess the handle and
+ * race closing it from another thread.
+ */
+ drm_gem_object_put(obj);
+
return 0;
}
@@ -731,11 +739,18 @@ static int virtio_gpu_resource_create_blob_ioctl(struct drm_device *dev,
drm_gem_object_release(obj);
return ret;
}
- drm_gem_object_put(obj);
rc_blob->res_handle = bo->hw_res_handle;
rc_blob->bo_handle = handle;
+ /*
+ * The handle owns the reference now. But we must drop our
+ * remaining reference *after* we no longer need to dereference
+ * the obj. Otherwise userspace could guess the handle and
+ * race closing it from another thread.
+ */
+ drm_gem_object_put(obj);
+
return 0;
}
--
2.38.1
On Fri, Dec 16, 2022 at 3:34 PM Rob Clark <[email protected]> wrote:
>
> From: Rob Clark <[email protected]>
>
> 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 <[email protected]>
> ---
> 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:33 PM Rob Clark <[email protected]> wrote:
>
> From: Rob Clark <[email protected]>
>
> Userspace can guess the handle value and try to race GEM object creation
> with handle close, resulting in a use-after-free if we dereference the
> object after dropping the handle's reference. For that reason, dropping
> the handle's reference must be done *after* we are done dereferencing
> the object.
>
> Signed-off-by: Rob Clark <[email protected]>
Reviewed-by: Chia-I Wu <[email protected]>
On Fri, Dec 16, 2022 at 3:59 PM Chia-I Wu <[email protected]> wrote:
>
> On Fri, Dec 16, 2022 at 3:34 PM Rob Clark <[email protected]> wrote:
> >
> > From: Rob Clark <[email protected]>
> >
> > 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 <[email protected]>
> > ---
> > 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 <[email protected]> wrote:
>
> On Fri, Dec 16, 2022 at 3:59 PM Chia-I Wu <[email protected]> wrote:
> >
> > On Fri, Dec 16, 2022 at 3:34 PM Rob Clark <[email protected]> wrote:
> > >
> > > From: Rob Clark <[email protected]>
> > >
> > > 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 <[email protected]>
> > > ---
> > > 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 <[email protected]>
>
> 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 <[email protected]>
> ---
> 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;
> }
On 12/17/22 02:33, Rob Clark wrote:
> From: Rob Clark <[email protected]>
>
> Userspace can guess the handle value and try to race GEM object creation
> with handle close, resulting in a use-after-free if we dereference the
> object after dropping the handle's reference. For that reason, dropping
> the handle's reference must be done *after* we are done dereferencing
> the object.
>
> Signed-off-by: Rob Clark <[email protected]>
> ---
> drivers/gpu/drm/virtio/virtgpu_ioctl.c | 19 +++++++++++++++++--
> 1 file changed, 17 insertions(+), 2 deletions(-)
Added fixes/stable tags and applied this virtio-gpu patch to misc-fixes.
The Panfrost patch is untouched.
--
Best regards,
Dmitry
On Mon, Jan 9, 2023 at 3:28 PM Dmitry Osipenko
<[email protected]> wrote:
>
> On 12/17/22 02:33, Rob Clark wrote:
> > From: Rob Clark <[email protected]>
> >
> > Userspace can guess the handle value and try to race GEM object creation
> > with handle close, resulting in a use-after-free if we dereference the
> > object after dropping the handle's reference. For that reason, dropping
> > the handle's reference must be done *after* we are done dereferencing
> > the object.
> >
> > Signed-off-by: Rob Clark <[email protected]>
> > ---
> > drivers/gpu/drm/virtio/virtgpu_ioctl.c | 19 +++++++++++++++++--
> > 1 file changed, 17 insertions(+), 2 deletions(-)
>
> Added fixes/stable tags and applied this virtio-gpu patch to misc-fixes.
> The Panfrost patch is untouched.
Thanks.. the panfrost patch was not intended to be part of the same
series (but apparently that is what happens when I send them at the
same time), and was superceded by a patch from Steven Price (commit
4217c6ac8174 ("drm/panfrost: Fix GEM handle creation ref-counting")
already applied to misc-fixes
BR,
-R
On 1/10/23 04:47, Rob Clark wrote:
> On Mon, Jan 9, 2023 at 3:28 PM Dmitry Osipenko
> <[email protected]> wrote:
>>
>> On 12/17/22 02:33, Rob Clark wrote:
>>> From: Rob Clark <[email protected]>
>>>
>>> Userspace can guess the handle value and try to race GEM object creation
>>> with handle close, resulting in a use-after-free if we dereference the
>>> object after dropping the handle's reference. For that reason, dropping
>>> the handle's reference must be done *after* we are done dereferencing
>>> the object.
>>>
>>> Signed-off-by: Rob Clark <[email protected]>
>>> ---
>>> drivers/gpu/drm/virtio/virtgpu_ioctl.c | 19 +++++++++++++++++--
>>> 1 file changed, 17 insertions(+), 2 deletions(-)
>>
>> Added fixes/stable tags and applied this virtio-gpu patch to misc-fixes.
>> The Panfrost patch is untouched.
>
> Thanks.. the panfrost patch was not intended to be part of the same
> series (but apparently that is what happens when I send them at the
> same time), and was superceded by a patch from Steven Price (commit
> 4217c6ac8174 ("drm/panfrost: Fix GEM handle creation ref-counting")
> already applied to misc-fixes
Okay, I wanted to make clear what has been applied.
--
Best regards,
Dmitry