Message ID | 20200202132133.1891846-1-daniel.vetter@ffwll.ch (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
Series | drm/vgem: Close use-after-free race in vgem_gem_create | expand |
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
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, Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk> -Chris
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
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; }
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(-)