Commit 4217c6ac authored by Steven Price's avatar Steven Price
Browse files

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: default avatarRob Clark <robdclark@chromium.org>
Fixes: f3ba9122 ("drm/panfrost: Add initial panfrost driver")
Signed-off-by: default avatarSteven Price <steven.price@arm.com>
Reviewed-by: default avatarRob Clark <robdclark@gmail.com>
Link: https://patchwork.freedesktop.org/patch/msgid/20221219140130.410578-1-steven.price@arm.com
parent 4e699e34

drivers/gpu/drm/panfrost/panfrost_drv.c

+18 −9
Original line number Diff line number Diff line
@@ -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,

					     &args->handle);

	bo = panfrost_gem_create(dev, args->size, args->flags);

	if (IS_ERR(bo))

		return PTR_ERR(bo);



	mapping = panfrost_gem_mapping_get(bo, priv);

	if (!mapping) {

		drm_gem_object_put(&bo->base.base);

		return -EINVAL;

	}

	ret = drm_gem_handle_create(file, &bo->base.base, &args->handle);

	if (ret)

		goto out;



	mapping = panfrost_gem_mapping_get(bo, priv);

	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;

	}



	return 0;

out:

	drm_gem_object_put(&bo->base.base);

	return ret;

}



/**

drivers/gpu/drm/panfrost/panfrost_gem.c

+1 −15
Original line number Diff line number Diff line
@@ -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, &shmem->base, handle);

	/* drop reference from allocate - handle holds it now. */

	drm_gem_object_put(&shmem->base);

	if (ret)

		return ERR_PTR(ret);



	return bo;

}

drivers/gpu/drm/panfrost/panfrost_gem.h

+1 −4
Original line number Diff line number Diff line
@@ -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 panfrost_gem_close(struct drm_gem_object *obj,