Re: [PATCH] drm/imagination: Fix NULL pointer dereference

[Alessio Belle]

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 <sashiko-bot@xxxxxxxxxx>

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/20260810132202.3C16D1F000E9@xxxxxxxxxxxxxxx/

> Signed-off-by: Alexandru Dadu <alexandru.dadu@xxxxxxxxxx>
> ---
>  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 <alexandru.dadu@xxxxxxxxxx>
>