Quoting Daniel Vetter (2020-02-02 13:21:33)
> There's two references floating around here (for the object reference,
> not the handle_count reference, that's a different thing):
>
> - The temporary reference held by vgem_gem_create, acquired by
> creating the object and released by calling
> drm_gem_object_put_unlocked.
>
> - The reference held by the object handle, created by
> drm_gem_handle_create. This one generally outlives the function,
> except if a 2nd thread races with a GEM_CLOSE ioctl call.
>
> So usually everything is correct, except in that race case, where the
> access to gem_object->size could be looking at freed data already.
> Which again isn't a real problem (userspace shot its feet off already
> with the race, we could return garbage), but maybe someone can exploit
> this as an information leak.
>
> Cc: Dan Carpenter <dan.carpenter@xxxxxxxxxx>
> Cc: Hillf Danton <hdanton@xxxxxxxx>
> Cc: Reported-by: syzbot+0dc4444774d419e916c8@xxxxxxxxxxxxxxxxxxxxxxxxx
> Cc: stable@xxxxxxxxxxxxxxx
> Cc: Emil Velikov <emil.velikov@xxxxxxxxxxxxx>
> Cc: Daniel Vetter <daniel.vetter@xxxxxxxx>
> Cc: Sean Paul <seanpaul@xxxxxxxxxxxx>
> Cc: Chris Wilson <chris@xxxxxxxxxxxxxxxxxx>
> Cc: Eric Anholt <eric@xxxxxxxxxx>
> Cc: Sam Ravnborg <sam@xxxxxxxxxxxx>
> Cc: Rob Clark <robdclark@xxxxxxxxxxxx>
> Signed-off-by: Daniel Vetter <daniel.vetter@xxxxxxxxx>
> ---
> drivers/gpu/drm/vgem/vgem_drv.c | 9 ++++++---
> 1 file changed, 6 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/vgem/vgem_drv.c b/drivers/gpu/drm/vgem/vgem_drv.c
> index 5bd60ded3d81..909eba43664a 100644
> --- a/drivers/gpu/drm/vgem/vgem_drv.c
> +++ b/drivers/gpu/drm/vgem/vgem_drv.c
> @@ -196,9 +196,10 @@ static struct drm_gem_object *vgem_gem_create(struct drm_device *dev,
> return ERR_CAST(obj);
>
> ret = drm_gem_handle_create(file, &obj->base, handle);
> - drm_gem_object_put_unlocked(&obj->base);
> - if (ret)
> + if (ret) {
> + drm_gem_object_put_unlocked(&obj->base);
> return ERR_PTR(ret);
> + }
>
> return &obj->base;
> }
> @@ -221,7 +222,9 @@ static int vgem_gem_dumb_create(struct drm_file *file, struct drm_device *dev,
> args->size = gem_object->size;
> args->pitch = pitch;
>
> - DRM_DEBUG("Created object of size %lld\n", size);
> + drm_gem_object_put_unlocked(gem_object);
> +
> + DRM_DEBUG("Created object of size %llu\n", args->size);
I was thinking we either should return size from vgem_gem_create (the
strategy we took in i915) or simply remove the vgem_gem_create() as that
doesn't improve readability.
-static struct drm_gem_object *vgem_gem_create(struct drm_device *dev,
- struct drm_file *file,
- unsigned int *handle,
- unsigned long size)
+static int vgem_gem_dumb_create(struct drm_file *file, struct drm_device *dev,
+ struct drm_mode_create_dumb *args)
{
struct drm_vgem_gem_object *obj;
- int ret;
+ u64 pitch, size;
+ u32 handle;
+
+ pitch = args->width * DIV_ROUND_UP(args->bpp, 8);
+ size = mul_u32_u32(args->height, pitch);
+ if (size == 0 || pitch < args->width)
+ return -EINVAL;
obj = __vgem_gem_create(dev, size);
if (IS_ERR(obj))
- return ERR_CAST(obj);
+ return PTR_ERR(obj);
+
+ size = obj->base.size;
- ret = drm_gem_handle_create(file, &obj->base, handle);
+ ret = drm_gem_handle_create(file, &obj->base, &handle);
drm_gem_object_put_unlocked(&obj->base);
if (ret)
return ERR_PTR(ret);
- return &obj->base;
-}
-
-static int vgem_gem_dumb_create(struct drm_file *file, struct drm_device *dev,
- struct drm_mode_create_dumb *args)
-{
- struct drm_gem_object *gem_object;
- u64 pitch, size;
-
- pitch = args->width * DIV_ROUND_UP(args->bpp, 8);
- size = args->height * pitch;
- if (size == 0)
- return -EINVAL;
-
- gem_object = vgem_gem_create(dev, file, &args->handle, size);
- if (IS_ERR(gem_object))
- return PTR_ERR(gem_object);
-
- args->size = gem_object->size;
+ args->size = size;
args->pitch = pitch;
+ args->handle = handle;
At the end of the day, it makes no difference,
Reviewed-by: Chris Wilson <chris@xxxxxxxxxxxxxxxxxx>
-Chris
