[prev in list] [next in list] [prev in thread] [next in thread] 

List:       dri-devel
Subject:    Re: [PATCH] drm/vmwgfx: Fix possible invalid drm gem put calls
From:       "Maaz Mombasawala (VMWare)" <maazm () fastmail ! com>
Date:       2023-08-23 17:03:08
Message-ID: 0f721521-2dd3-f8b7-84f8-ee77bb7509a4 () fastmail ! com
[Download RAW message or body]

LGTM!

Reviewed-by: Maaz Mombasawala<mombasawalam@vmware.com>

Maaz Mombasawala (VMware)<maazm@fastmail.com>

On 8/17/2023 9:13 PM, Zack Rusin wrote:
> From: Zack Rusin <zackr@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@vmware.com>
> Reported-by: Ian Forbes <iforbes@vmware.com>
> Fixes: 9ef8d83e8e25 ("drm/vmwgfx: Do not drop the reference to the handle too soon")
> Cc: <stable@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;
>   }
>   
[prev in list] [next in list] [prev in thread] [next in thread]