Hi Alexandru, I think the title is a bit generic - could you try adding that this is more like fixing parameter validation / the bug is potential (I think?) and/or where it can happen?
On Wed, 2026-08-12 at 14:46 +0300, Alexandru Dadu wrote: > Fix the NULL pointer dereference from pvr_fw_object_destroy() when > handling allocation failures. > Might be worth pointing out here what Sashiko said ([1]) about pvr_fw_object_create_and_map_common() setting the GEM object pointer to NULL before jumping to this function. Speaking of, assuming this is a response to [1], you could potentially add: Reported-by: Sashiko <[email protected]> In cany case, this fixes some logic so: Fixes: cc1aeedb98ad ("drm/imagination: Implement firmware infrastructure and META FW support") // p.s. minus the wrapping [1] https://lore.kernel.org/dri-devel/[email protected]/ > Signed-off-by: Alexandru Dadu <[email protected]> > --- > drivers/gpu/drm/imagination/pvr_fw.c | 28 ++++++++++++++++------------ > 1 file changed, 16 insertions(+), 12 deletions(-) > > diff --git a/drivers/gpu/drm/imagination/pvr_fw.c > b/drivers/gpu/drm/imagination/pvr_fw.c > index 850a3ec8e775..88f11fb01304 100644 > --- a/drivers/gpu/drm/imagination/pvr_fw.c > +++ b/drivers/gpu/drm/imagination/pvr_fw.c > @@ -1425,22 +1425,26 @@ pvr_fw_object_create_and_map_offset(struct pvr_device > *pvr_dev, > */ > void pvr_fw_object_destroy(struct pvr_fw_object *fw_obj) > { > - struct pvr_gem_object *pvr_obj = fw_obj->gem; > - struct drm_gem_object *gem_obj = gem_from_pvr_gem(pvr_obj); > - struct pvr_device *pvr_dev = to_pvr_device(gem_obj->dev); > + if (!fw_obj) > + return; > > - mutex_lock(&pvr_dev->fw_dev.fw_objs.lock); > - list_del(&fw_obj->node); > - mutex_unlock(&pvr_dev->fw_dev.fw_objs.lock); > + if (fw_obj->gem) { nit: could you return early on invalid pointers instead and shift everything back left/up? The check at the end shouldn't be required anymore after this. Thanks, Alessio > + struct pvr_gem_object *pvr_obj = fw_obj->gem; > + struct drm_gem_object *gem_obj = gem_from_pvr_gem(pvr_obj); > + struct pvr_device *pvr_dev = to_pvr_device(gem_obj->dev); > > - if (drm_mm_node_allocated(&fw_obj->fw_mm_node)) { > - /* If we can't unmap, leak the memory. */ > - if (WARN_ON(pvr_fw_object_fw_unmap(fw_obj))) > - return; > - } > + mutex_lock(&pvr_dev->fw_dev.fw_objs.lock); > + list_del(&fw_obj->node); > + mutex_unlock(&pvr_dev->fw_dev.fw_objs.lock); > + > + if (drm_mm_node_allocated(&fw_obj->fw_mm_node)) { > + /* If we can't unmap, leak the memory. */ > + if (WARN_ON(pvr_fw_object_fw_unmap(fw_obj))) > + return; > + } > > - if (fw_obj->gem) > pvr_gem_object_put(fw_obj->gem); > + } > > kfree(fw_obj); > } > > --- > base-commit: e55fead22ff9ee047ab9f1903860c4b43043514e > change-id: 20260812-fix-null-pointer-dereference-2c891142d988 > > Best regards, > -- > Alexandru Dadu <[email protected]> >
