Re: [PATCH] drm/vgem: Close use-after-free race in vgem_gem_create

[Sam Ravnborg <sam@ravnborg.org>]

Hi Daniel.
On Sun, Feb 02, 2020 at 02:21:33PM +0100, Daniel Vetter wrote:
> 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
  ^^  Small typo

> 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);
>  
>  	return 0;
>  }
> -- 
> 2.24.1
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel