Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider: - [Critical] Type Mismatch / NULL Pointer Dereference in ttm_base_object_unref. - [High] Double Free / Use-After-Free of vmw_resource on error path. - [High] IDR Double Remove and Corruption. --
commit 0e71ac7463131ddc891fdadbc6bca7ae37b5bcac Author: WenTao Liang <[email protected]> fix: drm/vmwgfx: vmw_simple_resource_create_ioctl: fix base object refcount leak on ttm_base_object_init failure This commit attempts to fix a refcount leak by adding a call to ttm_base_object_unref() in the error path of vmw_simple_resource_create_ioctl() when ttm_base_object_init() fails. > diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_simple_resource.c > b/drivers/gpu/drm/vmwgfx/vmwgfx_simple_resource.c > index 0d51b45422699..05f0f55441426 100644 > --- a/drivers/gpu/drm/vmwgfx/vmwgfx_simple_resource.c > +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_simple_resource.c > @@ -176,6 +176,7 @@ vmw_simple_resource_create_ioctl(struct drm_device *dev, > void *data, > > if (ret) { > vmw_resource_unreference(&tmp); > + ttm_base_object_unref(&usimple->base); [Severity: Critical] Does this pass the correct pointer type to ttm_base_object_unref()? ttm_base_object_unref() expects a struct ttm_base_object **, but &usimple->base is a struct ttm_base_object *. If this compiles, ttm_base_object_unref() will interpret the first 8 bytes of usimple->base as a pointer. Since usimple was allocated with kzalloc(), this reads as NULL: drivers/gpu/drm/vmwgfx/ttm_object.c:ttm_base_object_unref() { struct ttm_base_object *base = *p_base; *p_base = NULL; kref_put(&base->refcount, ttm_release_base); } Can this dereference NULL when passing &base->refcount to kref_put()? [Severity: High] Does ttm_base_object_init() already clean up its own state on failure, making this unref call unsafe? If ttm_base_object_init() fails (for example, in ttm_ref_object_add()), it cleans up the IDR entry in its error path: drivers/gpu/drm/vmwgfx/ttm_object.c:ttm_base_object_init() { ... out_err1: spin_lock(&tdev->object_lock); idr_remove(&tdev->idr, base->handle); spin_unlock(&tdev->object_lock); return ret; } If ttm_base_object_unref() is called afterward, it will trigger ttm_release_base() which also unconditionally calls idr_remove() for the same handle: drivers/gpu/drm/vmwgfx/ttm_object.c:ttm_release_base() { ... spin_lock(&tdev->object_lock); idr_remove(&tdev->idr, base->handle); spin_unlock(&tdev->object_lock); ... } If another thread successfully allocates an object and receives the same ID in the window between these two calls, will this corrupt the IDR by deleting the other thread's valid ID? > goto out_err; > } > > func->set_arg_handle(data, usimple->base.handle); > out_err: > vmw_resource_unreference(&res); [Severity: High] If the type mismatch above is fixed, will this cause a double free and use-after-free of the vmw_resource? When ttm_base_object_unref() drops the refcount to 0, it calls ttm_release_base(), which invokes the provided release callback vmw_simple_resource_base_release(). That callback drops a reference on the embedded vmw_resource, dropping its refcount to 0 and freeing usimple. However, the error path then proceeds here to the out_err label and unconditionally calls vmw_resource_unreference(&res) on the already freed memory. > out_ret: > return ret; > } -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=1
