[PATCH] drm/vmwgfx: Fix possible invalid drm gem put calls

[Martin Krastev (VMware)]

From: Martin Krastev <krastevm at vmware.com>


LGTM!

Reviewed-by: Martin Krastev <krastevm at vmware.com>


Regards,

Martin


On 18 Aug 2023 04:13:14, Zack Rusin wrote:

 >From: Zack Rusin <zackr at vmware.com>
 >
 >vmw_bo_unreference sets the input buffer to null on exit, resulting in
 >null ptr deref's on the subsequent drm gem put calls.
 >
 >This went unnoticed because only very old userspace would be exercising
 >those paths but it wouldn't be hard to hit on old distros with brand
 >new kernels.
 >
 >Introduce a new function that abstracts unrefing of user bo's to make
 >the code cleaner and more explicit.
 >
 >Signed-off-by: Zack Rusin <zackr at vmware.com>
 >Reported-by: Ian Forbes <iforbes at vmware.com>
 >Fixes: 9ef8d83e8e25 ("drm/vmwgfx: Do not drop the reference to the 
handle too soon")
 >Cc: <stable at vger.kernel.org> # v6.4+
 >---
 > drivers/gpu/drm/vmwgfx/vmwgfx_bo.c      | 6 ++----
 > drivers/gpu/drm/vmwgfx/vmwgfx_bo.h      | 8 ++++++++
 > drivers/gpu/drm/vmwgfx/vmwgfx_execbuf.c | 6 ++----
 > drivers/gpu/drm/vmwgfx/vmwgfx_kms.c     | 6 ++----
 > drivers/gpu/drm/vmwgfx/vmwgfx_overlay.c | 3 +--
 > drivers/gpu/drm/vmwgfx/vmwgfx_shader.c  | 3 +--
 > 6 files changed, 16 insertions(+), 16 deletions(-)
 >
 >diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_bo.c 
b/drivers/gpu/drm/vmwgfx/vmwgfx_bo.c
 >index 82094c137855..c43853597776 100644
 >--- a/drivers/gpu/drm/vmwgfx/vmwgfx_bo.c
 >+++ b/drivers/gpu/drm/vmwgfx/vmwgfx_bo.c
 >@@ -497,10 +497,9 @@ static int vmw_user_bo_synccpu_release(struct 
drm_file *filp,
 >                 if (!(flags & drm_vmw_synccpu_allow_cs)) {
 > atomic_dec(&vmw_bo->cpu_writers);
 >                 }
 >-               ttm_bo_put(&vmw_bo->tbo);
 >+               vmw_user_bo_unref(vmw_bo);
 >         }
 >
 >-       drm_gem_object_put(&vmw_bo->tbo.base);
 >         return ret;
 > }
 >
 >@@ -540,8 +539,7 @@ int vmw_user_bo_synccpu_ioctl(struct drm_device 
*dev, void *data,
 >                         return ret;
 >
 >                 ret = vmw_user_bo_synccpu_grab(vbo, arg->flags);
 >-               vmw_bo_unreference(&vbo);
 >-               drm_gem_object_put(&vbo->tbo.base);
 >+               vmw_user_bo_unref(vbo);
 >                 if (unlikely(ret != 0)) {
 >                         if (ret == -ERESTARTSYS || ret == -EBUSY)
 >                                 return -EBUSY;
 >diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_bo.h 
b/drivers/gpu/drm/vmwgfx/vmwgfx_bo.h
 >index 50a836e70994..1d433fceed3d 100644
 >--- a/drivers/gpu/drm/vmwgfx/vmwgfx_bo.h
 >+++ b/drivers/gpu/drm/vmwgfx/vmwgfx_bo.h
 >@@ -195,6 +195,14 @@ static inline struct vmw_bo 
*vmw_bo_reference(struct vmw_bo *buf)
 >         return buf;
 > }
 >
 >+static inline void vmw_user_bo_unref(struct vmw_bo *vbo)
 >+{
 >+       if (vbo) {
 >+               ttm_bo_put(&vbo->tbo);
 >+               drm_gem_object_put(&vbo->tbo.base);
 >+       }
 >+}
 >+
 > static inline struct vmw_bo *to_vmw_bo(struct drm_gem_object *gobj)
 > {
 >         return container_of((gobj), struct vmw_bo, tbo.base);
 >diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_execbuf.c 
b/drivers/gpu/drm/vmwgfx/vmwgfx_execbuf.c
 >index 6b9aa2b4ef54..25b96821df0f 100644
 >--- a/drivers/gpu/drm/vmwgfx/vmwgfx_execbuf.c
 >+++ b/drivers/gpu/drm/vmwgfx/vmwgfx_execbuf.c
 >@@ -1164,8 +1164,7 @@ static int vmw_translate_mob_ptr(struct 
vmw_private *dev_priv,
 >         }
 >         vmw_bo_placement_set(vmw_bo, VMW_BO_DOMAIN_MOB, 
VMW_BO_DOMAIN_MOB);
 >         ret = vmw_validation_add_bo(sw_context->ctx, vmw_bo);
 >-       ttm_bo_put(&vmw_bo->tbo);
 >-       drm_gem_object_put(&vmw_bo->tbo.base);
 >+       vmw_user_bo_unref(vmw_bo);
 >         if (unlikely(ret != 0))
 >                 return ret;
 >
 >@@ -1221,8 +1220,7 @@ static int vmw_translate_guest_ptr(struct 
vmw_private *dev_priv,
 >         vmw_bo_placement_set(vmw_bo, VMW_BO_DOMAIN_GMR | 
VMW_BO_DOMAIN_VRAM,
 >                              VMW_BO_DOMAIN_GMR | VMW_BO_DOMAIN_VRAM);
 >         ret = vmw_validation_add_bo(sw_context->ctx, vmw_bo);
 >-       ttm_bo_put(&vmw_bo->tbo);
 >-       drm_gem_object_put(&vmw_bo->tbo.base);
 >+       vmw_user_bo_unref(vmw_bo);
 >         if (unlikely(ret != 0))
 >                 return ret;
 >
 >diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c 
b/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c
 >index b62207be3363..1489ad73c103 100644
 >--- a/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c
 >+++ b/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c
 >@@ -1665,10 +1665,8 @@ static struct drm_framebuffer 
*vmw_kms_fb_create(struct drm_device *dev,
 >
 > err_out:
 >         /* vmw_user_lookup_handle takes one ref so does new_fb */
 >-       if (bo) {
 >-               vmw_bo_unreference(&bo);
 >-               drm_gem_object_put(&bo->tbo.base);
 >-       }
 >+       if (bo)
 >+               vmw_user_bo_unref(bo);
 >         if (surface)
 >                 vmw_surface_unreference(&surface);
 >
 >diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_overlay.c 
b/drivers/gpu/drm/vmwgfx/vmwgfx_overlay.c
 >index 7e112319a23c..fb85f244c3d0 100644
 >--- a/drivers/gpu/drm/vmwgfx/vmwgfx_overlay.c
 >+++ b/drivers/gpu/drm/vmwgfx/vmwgfx_overlay.c
 >@@ -451,8 +451,7 @@ int vmw_overlay_ioctl(struct drm_device *dev, void 
*data,
 >
 >         ret = vmw_overlay_update_stream(dev_priv, buf, arg, true);
 >
 >-       vmw_bo_unreference(&buf);
 >-       drm_gem_object_put(&buf->tbo.base);
 >+       vmw_user_bo_unref(buf);
 >
 > out_unlock:
 >         mutex_unlock(&overlay->mutex);
 >diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_shader.c 
b/drivers/gpu/drm/vmwgfx/vmwgfx_shader.c
 >index e7226db8b242..1e81ff2422cf 100644
 >--- a/drivers/gpu/drm/vmwgfx/vmwgfx_shader.c
 >+++ b/drivers/gpu/drm/vmwgfx/vmwgfx_shader.c
 >@@ -809,8 +809,7 @@ static int vmw_shader_define(struct drm_device 
*dev, struct drm_file *file_priv,
 >                                     shader_type, num_input_sig,
 >                                     num_output_sig, tfile, 
shader_handle);
 > out_bad_arg:
 >-       vmw_bo_unreference(&buffer);
 >-       drm_gem_object_put(&buffer->tbo.base);
 >+       vmw_user_bo_unref(buffer);
 >         return ret;
 > }
 >
 >--
 >2.39.2