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@oracle.com Cc: Hillf Danton hdanton@sina.com Cc: Reported-by: syzbot+0dc4444774d419e916c8@syzkaller.appspotmail.com Cc: stable@vger.kernel.org Cc: Emil Velikov emil.velikov@collabora.com Cc: Daniel Vetter daniel.vetter@ffwll.ch Cc: Sean Paul seanpaul@chromium.org Cc: Chris Wilson chris@chris-wilson.co.uk Cc: Eric Anholt eric@anholt.net Cc: Sam Ravnborg sam@ravnborg.org Cc: Rob Clark robdclark@chromium.org Signed-off-by: Daniel Vetter daniel.vetter@intel.com

---

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@chris-wilson.co.uk

Thanks for your review, finally applied to drm-misc-next-fixes now that CI has blessed me with its attention for a bit! -Daniel