Skip to content

Commit

Permalink
drm/vmwgfx: Do not drop the reference to the handle too soon
Browse files Browse the repository at this point in the history
v3: Fix vmw_user_bo_lookup which was also dropping the gem reference
before the kernel was done with buffer depending on userspace doing
the right thing. Same bug, different spot.

It is possible for userspace to predict the next buffer handle and
to destroy the buffer while it's still used by the kernel. Delay
dropping the internal reference on the buffers until kernel is done
with them.

Instead of immediately dropping the gem reference in vmw_user_bo_lookup
and vmw_gem_object_create_with_handle let the callers decide when they're
ready give the control back to userspace.

Also fixes the second usage of vmw_gem_object_create_with_handle in
vmwgfx_surface.c which wasn't grabbing an explicit reference
to the gem object which could have been destroyed by the userspace
on the owning surface at any point.

Signed-off-by: Zack Rusin <zackr@vmware.com>
Fixes: 8afa13a ("drm/vmwgfx: Implement DRIVER_GEM")
Reviewed-by: Martin Krastev <krastevm@vmware.com>
Reviewed-by: Maaz Mombasawala <mombasawalam@vmware.com>
Link: https://patchwork.freedesktop.org/patch/msgid/20230211050514.2431155-1-zack@kde.org
(cherry picked from commit 9ef8d83)
Cc: <stable@vger.kernel.org> # v5.17+
  • Loading branch information
zackr committed Feb 15, 2023
1 parent 1a68979 commit a950b98
Show file tree
Hide file tree
Showing 7 changed files with 20 additions and 10 deletions.
8 changes: 5 additions & 3 deletions drivers/gpu/drm/vmwgfx/vmwgfx_bo.c
Original file line number Diff line number Diff line change
Expand Up @@ -598,6 +598,7 @@ static int vmw_user_bo_synccpu_release(struct drm_file *filp,
ttm_bo_put(&vmw_bo->base);
}

drm_gem_object_put(&vmw_bo->base.base);
return ret;
}

Expand Down Expand Up @@ -638,6 +639,7 @@ int vmw_user_bo_synccpu_ioctl(struct drm_device *dev, void *data,

ret = vmw_user_bo_synccpu_grab(vbo, arg->flags);
vmw_bo_unreference(&vbo);
drm_gem_object_put(&vbo->base.base);
if (unlikely(ret != 0)) {
if (ret == -ERESTARTSYS || ret == -EBUSY)
return -EBUSY;
Expand Down Expand Up @@ -695,7 +697,7 @@ int vmw_bo_unref_ioctl(struct drm_device *dev, void *data,
* struct vmw_buffer_object should be placed.
* Return: Zero on success, Negative error code on error.
*
* The vmw buffer object pointer will be refcounted.
* The vmw buffer object pointer will be refcounted (both ttm and gem)
*/
int vmw_user_bo_lookup(struct drm_file *filp,
uint32_t handle,
Expand All @@ -712,7 +714,6 @@ int vmw_user_bo_lookup(struct drm_file *filp,

*out = gem_to_vmw_bo(gobj);
ttm_bo_get(&(*out)->base);
drm_gem_object_put(gobj);

return 0;
}
Expand Down Expand Up @@ -793,7 +794,8 @@ int vmw_dumb_create(struct drm_file *file_priv,
ret = vmw_gem_object_create_with_handle(dev_priv, file_priv,
args->size, &args->handle,
&vbo);

/* drop reference from allocate - handle holds it now */
drm_gem_object_put(&vbo->base.base);
return ret;
}

Expand Down
2 changes: 2 additions & 0 deletions drivers/gpu/drm/vmwgfx/vmwgfx_execbuf.c
Original file line number Diff line number Diff line change
Expand Up @@ -1160,6 +1160,7 @@ static int vmw_translate_mob_ptr(struct vmw_private *dev_priv,
}
ret = vmw_validation_add_bo(sw_context->ctx, vmw_bo, true, false);
ttm_bo_put(&vmw_bo->base);
drm_gem_object_put(&vmw_bo->base.base);
if (unlikely(ret != 0))
return ret;

Expand Down Expand Up @@ -1214,6 +1215,7 @@ static int vmw_translate_guest_ptr(struct vmw_private *dev_priv,
}
ret = vmw_validation_add_bo(sw_context->ctx, vmw_bo, false, false);
ttm_bo_put(&vmw_bo->base);
drm_gem_object_put(&vmw_bo->base.base);
if (unlikely(ret != 0))
return ret;

Expand Down
4 changes: 2 additions & 2 deletions drivers/gpu/drm/vmwgfx/vmwgfx_gem.c
Original file line number Diff line number Diff line change
Expand Up @@ -152,8 +152,6 @@ int vmw_gem_object_create_with_handle(struct vmw_private *dev_priv,
(*p_vbo)->base.base.funcs = &vmw_gem_object_funcs;

ret = drm_gem_handle_create(filp, &(*p_vbo)->base.base, handle);
/* drop reference from allocate - handle holds it now */
drm_gem_object_put(&(*p_vbo)->base.base);
out_no_bo:
return ret;
}
Expand All @@ -180,6 +178,8 @@ int vmw_gem_object_create_ioctl(struct drm_device *dev, void *data,
rep->map_handle = drm_vma_node_offset_addr(&vbo->base.base.vma_node);
rep->cur_gmr_id = handle;
rep->cur_gmr_offset = 0;
/* drop reference from allocate - handle holds it now */
drm_gem_object_put(&vbo->base.base);
out_no_bo:
return ret;
}
Expand Down
4 changes: 3 additions & 1 deletion drivers/gpu/drm/vmwgfx/vmwgfx_kms.c
Original file line number Diff line number Diff line change
Expand Up @@ -1815,8 +1815,10 @@ 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)
if (bo) {
vmw_bo_unreference(&bo);
drm_gem_object_put(&bo->base.base);
}
if (surface)
vmw_surface_unreference(&surface);

Expand Down
1 change: 1 addition & 0 deletions drivers/gpu/drm/vmwgfx/vmwgfx_overlay.c
Original file line number Diff line number Diff line change
Expand Up @@ -458,6 +458,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->base.base);

out_unlock:
mutex_unlock(&overlay->mutex);
Expand Down
1 change: 1 addition & 0 deletions drivers/gpu/drm/vmwgfx/vmwgfx_shader.c
Original file line number Diff line number Diff line change
Expand Up @@ -807,6 +807,7 @@ static int vmw_shader_define(struct drm_device *dev, struct drm_file *file_priv,
num_output_sig, tfile, shader_handle);
out_bad_arg:
vmw_bo_unreference(&buffer);
drm_gem_object_put(&buffer->base.base);
return ret;
}

Expand Down
10 changes: 6 additions & 4 deletions drivers/gpu/drm/vmwgfx/vmwgfx_surface.c
Original file line number Diff line number Diff line change
Expand Up @@ -683,7 +683,7 @@ static void vmw_user_surface_base_release(struct ttm_base_object **p_base)
container_of(base, struct vmw_user_surface, prime.base);
struct vmw_resource *res = &user_srf->srf.res;

if (base->shareable && res && res->backup)
if (res && res->backup)
drm_gem_object_put(&res->backup->base.base);

*p_base = NULL;
Expand Down Expand Up @@ -864,7 +864,11 @@ int vmw_surface_define_ioctl(struct drm_device *dev, void *data,
goto out_unlock;
}
vmw_bo_reference(res->backup);
drm_gem_object_get(&res->backup->base.base);
/*
* We don't expose the handle to the userspace and surface
* already holds a gem reference
*/
drm_gem_handle_delete(file_priv, backup_handle);
}

tmp = vmw_resource_reference(&srf->res);
Expand Down Expand Up @@ -1568,8 +1572,6 @@ vmw_gb_surface_define_internal(struct drm_device *dev,
drm_vma_node_offset_addr(&res->backup->base.base.vma_node);
rep->buffer_size = res->backup->base.base.size;
rep->buffer_handle = backup_handle;
if (user_srf->prime.base.shareable)
drm_gem_object_get(&res->backup->base.base);
} else {
rep->buffer_map_handle = 0;
rep->buffer_size = 0;
Expand Down

0 comments on commit a950b98

Please sign in to comment.