On Sun, Feb 02, 2020 at 05:37:31PM +0000, Chris Wilson wrote:
> 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,
Yeah there's room for more polish, but didn't want to do that in the cc:
stable patch.
> Reviewed-by: Chris Wilson <chris@xxxxxxxxxxxxxxxxxx>
Thanks for your review, finally applied to drm-misc-next-fixes now that CI
has blessed me with its attention for a bit!
-Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
