Re: [PATCH] drm/panfrost: Fix GEM handle creation ref-counting
On 19/12/2022 17:10, Rob Clark wrote:
> On Mon, Dec 19, 2022 at 6:02 AM Steven Price wrote:
>>
>> panfrost_gem_create_with_handle() previously returned a BO but with the
>> only reference being from the handle, which user space could in theory
>> guess and release, causing a use-after-free. Additionally if the call to
>> panfrost_gem_mapping_get() in panfrost_ioctl_create_bo() failed then
>> a(nother) reference on the BO was dropped.
>>
>> The _create_with_handle() is a problematic pattern, so ditch it and
>> instead create the handle in panfrost_ioctl_create_bo(). If the call to
>> panfrost_gem_mapping_get() fails then this means that user space has
>> indeed gone behind our back and freed the handle. In which case just
>> return an error code.
>>
>> Reported-by: Rob Clark
>
> Yeah, I like getting rid of the _create_with_handle() pattern, the
> only place where that pattern works is if you immediately return the
> handle to userspace (and don't otherwise touch the obj)
>
> Reviewed-by: Rob Clark
Thanks, I've pushed this to drm-misc-fixes:
4217c6ac8174 ("drm/panfrost: Fix GEM handle creation ref-counting")
Steve
>> Fixes: f3ba91228e8e ("drm/panfrost: Add initial panfrost driver")
>> Signed-off-by: Steven Price
>> ---
>> drivers/gpu/drm/panfrost/panfrost_drv.c | 27 -
>> drivers/gpu/drm/panfrost/panfrost_gem.c | 16 +--
>> drivers/gpu/drm/panfrost/panfrost_gem.h | 5 +
>> 3 files changed, 20 insertions(+), 28 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/panfrost/panfrost_drv.c
>> b/drivers/gpu/drm/panfrost/panfrost_drv.c
>> index fa619fe72086..abb0dadd8f63 100644
>> --- a/drivers/gpu/drm/panfrost/panfrost_drv.c
>> +++ b/drivers/gpu/drm/panfrost/panfrost_drv.c
>> @@ -82,6 +82,7 @@ static int panfrost_ioctl_create_bo(struct drm_device
>> *dev, void *data,
>> struct panfrost_gem_object *bo;
>> struct drm_panfrost_create_bo *args = data;
>> struct panfrost_gem_mapping *mapping;
>> + int ret;
>>
>> if (!args->size || args->pad ||
>> (args->flags & ~(PANFROST_BO_NOEXEC | PANFROST_BO_HEAP)))
>> @@ -92,21 +93,29 @@ static int panfrost_ioctl_create_bo(struct drm_device
>> *dev, void *data,
>> !(args->flags & PANFROST_BO_NOEXEC))
>> return -EINVAL;
>>
>> - bo = panfrost_gem_create_with_handle(file, dev, args->size,
>> args->flags,
>> ->handle);
>> + bo = panfrost_gem_create(dev, args->size, args->flags);
>> if (IS_ERR(bo))
>> return PTR_ERR(bo);
>>
>> + ret = drm_gem_handle_create(file, >base.base, >handle);
>> + if (ret)
>> + goto out;
>> +
>> mapping = panfrost_gem_mapping_get(bo, priv);
>> - if (!mapping) {
>> - drm_gem_object_put(>base.base);
>> - return -EINVAL;
>> + if (mapping) {
>> + args->offset = mapping->mmnode.start << PAGE_SHIFT;
>> + panfrost_gem_mapping_put(mapping);
>> + } else {
>> + /* This can only happen if the handle from
>> +* drm_gem_handle_create() has already been guessed and freed
>> +* by user space
>> +*/
>> + ret = -EINVAL;
>> }
>>
>> - args->offset = mapping->mmnode.start << PAGE_SHIFT;
>> - panfrost_gem_mapping_put(mapping);
>> -
>> - return 0;
>> +out:
>> + drm_gem_object_put(>base.base);
>> + return ret;
>> }
>>
>> /**
>> diff --git a/drivers/gpu/drm/panfrost/panfrost_gem.c
>> b/drivers/gpu/drm/panfrost/panfrost_gem.c
>> index 293e799e2fe8..3c812fbd126f 100644
>> --- a/drivers/gpu/drm/panfrost/panfrost_gem.c
>> +++ b/drivers/gpu/drm/panfrost/panfrost_gem.c
>> @@ -235,12 +235,8 @@ struct drm_gem_object
>> *panfrost_gem_create_object(struct drm_device *dev, size_t
>> }
>>
>> struct panfrost_gem_object *
>> -panfrost_gem_create_with_handle(struct drm_file *file_priv,
>> - struct drm_device *dev, size_t size,
>> - u32 flags,
>> - uint32_t *handle)
>> +panfrost_gem_create(struct drm_device *dev, size_t size, u32 flags)
>> {
>> - int ret;
>> struct drm_gem_shmem_object *shmem;
>> struct panfrost_gem_object *bo;
>>
>> @@ -256,16 +252,6 @@ panfrost_gem_create_with_handle(struct drm_file
>> *file_priv,
>> bo->noexec = !!(flags & PANFROST_BO_NOEXEC);
>> bo->is_heap = !!(flags & PANFROST_BO_HEAP);
>>
>> - /*
>> -* Allocate an id of idr table where the obj is registered
>> -* and handle has the id what user can see.
>> -*/
>> - ret = drm_gem_handle_create(file_priv, >base, handle);
>> - /* drop reference from allocate - handle holds it now. */
>> - drm_gem_object_put(>base);
>> - if (ret)
>> - return ERR_PTR(ret);
>> -
>> return bo;
>> }
>>
>> diff --git
Re: [PATCH] drm/panfrost: Fix GEM handle creation ref-counting
On Mon, Dec 19, 2022 at 6:02 AM Steven Price wrote:
>
> panfrost_gem_create_with_handle() previously returned a BO but with the
> only reference being from the handle, which user space could in theory
> guess and release, causing a use-after-free. Additionally if the call to
> panfrost_gem_mapping_get() in panfrost_ioctl_create_bo() failed then
> a(nother) reference on the BO was dropped.
>
> The _create_with_handle() is a problematic pattern, so ditch it and
> instead create the handle in panfrost_ioctl_create_bo(). If the call to
> panfrost_gem_mapping_get() fails then this means that user space has
> indeed gone behind our back and freed the handle. In which case just
> return an error code.
>
> Reported-by: Rob Clark
Yeah, I like getting rid of the _create_with_handle() pattern, the
only place where that pattern works is if you immediately return the
handle to userspace (and don't otherwise touch the obj)
Reviewed-by: Rob Clark
> Fixes: f3ba91228e8e ("drm/panfrost: Add initial panfrost driver")
> Signed-off-by: Steven Price
> ---
> drivers/gpu/drm/panfrost/panfrost_drv.c | 27 -
> drivers/gpu/drm/panfrost/panfrost_gem.c | 16 +--
> drivers/gpu/drm/panfrost/panfrost_gem.h | 5 +
> 3 files changed, 20 insertions(+), 28 deletions(-)
>
> diff --git a/drivers/gpu/drm/panfrost/panfrost_drv.c
> b/drivers/gpu/drm/panfrost/panfrost_drv.c
> index fa619fe72086..abb0dadd8f63 100644
> --- a/drivers/gpu/drm/panfrost/panfrost_drv.c
> +++ b/drivers/gpu/drm/panfrost/panfrost_drv.c
> @@ -82,6 +82,7 @@ static int panfrost_ioctl_create_bo(struct drm_device *dev,
> void *data,
> struct panfrost_gem_object *bo;
> struct drm_panfrost_create_bo *args = data;
> struct panfrost_gem_mapping *mapping;
> + int ret;
>
> if (!args->size || args->pad ||
> (args->flags & ~(PANFROST_BO_NOEXEC | PANFROST_BO_HEAP)))
> @@ -92,21 +93,29 @@ static int panfrost_ioctl_create_bo(struct drm_device
> *dev, void *data,
> !(args->flags & PANFROST_BO_NOEXEC))
> return -EINVAL;
>
> - bo = panfrost_gem_create_with_handle(file, dev, args->size,
> args->flags,
> ->handle);
> + bo = panfrost_gem_create(dev, args->size, args->flags);
> if (IS_ERR(bo))
> return PTR_ERR(bo);
>
> + ret = drm_gem_handle_create(file, >base.base, >handle);
> + if (ret)
> + goto out;
> +
> mapping = panfrost_gem_mapping_get(bo, priv);
> - if (!mapping) {
> - drm_gem_object_put(>base.base);
> - return -EINVAL;
> + if (mapping) {
> + args->offset = mapping->mmnode.start << PAGE_SHIFT;
> + panfrost_gem_mapping_put(mapping);
> + } else {
> + /* This can only happen if the handle from
> +* drm_gem_handle_create() has already been guessed and freed
> +* by user space
> +*/
> + ret = -EINVAL;
> }
>
> - args->offset = mapping->mmnode.start << PAGE_SHIFT;
> - panfrost_gem_mapping_put(mapping);
> -
> - return 0;
> +out:
> + drm_gem_object_put(>base.base);
> + return ret;
> }
>
> /**
> diff --git a/drivers/gpu/drm/panfrost/panfrost_gem.c
> b/drivers/gpu/drm/panfrost/panfrost_gem.c
> index 293e799e2fe8..3c812fbd126f 100644
> --- a/drivers/gpu/drm/panfrost/panfrost_gem.c
> +++ b/drivers/gpu/drm/panfrost/panfrost_gem.c
> @@ -235,12 +235,8 @@ struct drm_gem_object *panfrost_gem_create_object(struct
> drm_device *dev, size_t
> }
>
> struct panfrost_gem_object *
> -panfrost_gem_create_with_handle(struct drm_file *file_priv,
> - struct drm_device *dev, size_t size,
> - u32 flags,
> - uint32_t *handle)
> +panfrost_gem_create(struct drm_device *dev, size_t size, u32 flags)
> {
> - int ret;
> struct drm_gem_shmem_object *shmem;
> struct panfrost_gem_object *bo;
>
> @@ -256,16 +252,6 @@ panfrost_gem_create_with_handle(struct drm_file
> *file_priv,
> bo->noexec = !!(flags & PANFROST_BO_NOEXEC);
> bo->is_heap = !!(flags & PANFROST_BO_HEAP);
>
> - /*
> -* Allocate an id of idr table where the obj is registered
> -* and handle has the id what user can see.
> -*/
> - ret = drm_gem_handle_create(file_priv, >base, handle);
> - /* drop reference from allocate - handle holds it now. */
> - drm_gem_object_put(>base);
> - if (ret)
> - return ERR_PTR(ret);
> -
> return bo;
> }
>
> diff --git a/drivers/gpu/drm/panfrost/panfrost_gem.h
> b/drivers/gpu/drm/panfrost/panfrost_gem.h
> index 8088d5fd8480..ad2877eeeccd 100644
> --- a/drivers/gpu/drm/panfrost/panfrost_gem.h
> +++ b/drivers/gpu/drm/panfrost/panfrost_gem.h
> @@ -69,10 +69,7 @@ panfrost_gem_prime_import_sg_table(struct
[PATCH] drm/panfrost: Fix GEM handle creation ref-counting
panfrost_gem_create_with_handle() previously returned a BO but with the
only reference being from the handle, which user space could in theory
guess and release, causing a use-after-free. Additionally if the call to
panfrost_gem_mapping_get() in panfrost_ioctl_create_bo() failed then
a(nother) reference on the BO was dropped.
The _create_with_handle() is a problematic pattern, so ditch it and
instead create the handle in panfrost_ioctl_create_bo(). If the call to
panfrost_gem_mapping_get() fails then this means that user space has
indeed gone behind our back and freed the handle. In which case just
return an error code.
Reported-by: Rob Clark
Fixes: f3ba91228e8e ("drm/panfrost: Add initial panfrost driver")
Signed-off-by: Steven Price
---
drivers/gpu/drm/panfrost/panfrost_drv.c | 27 -
drivers/gpu/drm/panfrost/panfrost_gem.c | 16 +--
drivers/gpu/drm/panfrost/panfrost_gem.h | 5 +
3 files changed, 20 insertions(+), 28 deletions(-)
diff --git a/drivers/gpu/drm/panfrost/panfrost_drv.c
b/drivers/gpu/drm/panfrost/panfrost_drv.c
index fa619fe72086..abb0dadd8f63 100644
--- a/drivers/gpu/drm/panfrost/panfrost_drv.c
+++ b/drivers/gpu/drm/panfrost/panfrost_drv.c
@@ -82,6 +82,7 @@ static int panfrost_ioctl_create_bo(struct drm_device *dev,
void *data,
struct panfrost_gem_object *bo;
struct drm_panfrost_create_bo *args = data;
struct panfrost_gem_mapping *mapping;
+ int ret;
if (!args->size || args->pad ||
(args->flags & ~(PANFROST_BO_NOEXEC | PANFROST_BO_HEAP)))
@@ -92,21 +93,29 @@ static int panfrost_ioctl_create_bo(struct drm_device *dev,
void *data,
!(args->flags & PANFROST_BO_NOEXEC))
return -EINVAL;
- bo = panfrost_gem_create_with_handle(file, dev, args->size, args->flags,
->handle);
+ bo = panfrost_gem_create(dev, args->size, args->flags);
if (IS_ERR(bo))
return PTR_ERR(bo);
+ ret = drm_gem_handle_create(file, >base.base, >handle);
+ if (ret)
+ goto out;
+
mapping = panfrost_gem_mapping_get(bo, priv);
- if (!mapping) {
- drm_gem_object_put(>base.base);
- return -EINVAL;
+ if (mapping) {
+ args->offset = mapping->mmnode.start << PAGE_SHIFT;
+ panfrost_gem_mapping_put(mapping);
+ } else {
+ /* This can only happen if the handle from
+* drm_gem_handle_create() has already been guessed and freed
+* by user space
+*/
+ ret = -EINVAL;
}
- args->offset = mapping->mmnode.start << PAGE_SHIFT;
- panfrost_gem_mapping_put(mapping);
-
- return 0;
+out:
+ drm_gem_object_put(>base.base);
+ return ret;
}
/**
diff --git a/drivers/gpu/drm/panfrost/panfrost_gem.c
b/drivers/gpu/drm/panfrost/panfrost_gem.c
index 293e799e2fe8..3c812fbd126f 100644
--- a/drivers/gpu/drm/panfrost/panfrost_gem.c
+++ b/drivers/gpu/drm/panfrost/panfrost_gem.c
@@ -235,12 +235,8 @@ struct drm_gem_object *panfrost_gem_create_object(struct
drm_device *dev, size_t
}
struct panfrost_gem_object *
-panfrost_gem_create_with_handle(struct drm_file *file_priv,
- struct drm_device *dev, size_t size,
- u32 flags,
- uint32_t *handle)
+panfrost_gem_create(struct drm_device *dev, size_t size, u32 flags)
{
- int ret;
struct drm_gem_shmem_object *shmem;
struct panfrost_gem_object *bo;
@@ -256,16 +252,6 @@ panfrost_gem_create_with_handle(struct drm_file *file_priv,
bo->noexec = !!(flags & PANFROST_BO_NOEXEC);
bo->is_heap = !!(flags & PANFROST_BO_HEAP);
- /*
-* Allocate an id of idr table where the obj is registered
-* and handle has the id what user can see.
-*/
- ret = drm_gem_handle_create(file_priv, >base, handle);
- /* drop reference from allocate - handle holds it now. */
- drm_gem_object_put(>base);
- if (ret)
- return ERR_PTR(ret);
-
return bo;
}
diff --git a/drivers/gpu/drm/panfrost/panfrost_gem.h
b/drivers/gpu/drm/panfrost/panfrost_gem.h
index 8088d5fd8480..ad2877eeeccd 100644
--- a/drivers/gpu/drm/panfrost/panfrost_gem.h
+++ b/drivers/gpu/drm/panfrost/panfrost_gem.h
@@ -69,10 +69,7 @@ panfrost_gem_prime_import_sg_table(struct drm_device *dev,
struct sg_table *sgt);
struct panfrost_gem_object *
-panfrost_gem_create_with_handle(struct drm_file *file_priv,
- struct drm_device *dev, size_t size,
- u32 flags,
- uint32_t *handle);
+panfrost_gem_create(struct drm_device *dev, size_t size, u32 flags);
int panfrost_gem_open(struct drm_gem_object *obj, struct drm_file *file_priv);
void
