Re: [PATCH v2] drm/imagination: Fix parameter validation in pvr_fw_object_destroy()

[Alessio Belle <Alessio.Belle@imgtec.com>]

Hi Alexandru,

On Thu, 2026-09-03 at 15:26 +0300, Alexandru Dadu wrote:
> Fix parameter validation to avoid possible NULL pointer dereference from
> pvr_fw_object_destroy() when handling allocation failures.
> 
> Sashiko report:
> If pvr_gem_object_create() fails in
> pvr_fw_object_create_and_map_common(), fw_obj->gem is explicitly set to
> NULL before jumping to the error cleanup path.
> The cleanup path then calls pvr_fw_object_destroy().
> 
> Potential kernel panic when calling pvr_fw_object_destroy() with a NULL
> fw_obj->gem.

nit: this could be dropped now since it's more or less rehashing the rest.

> 
> Reported-by: Sashiko <sashiko-bot@kernel.org>
> Link: https://lore.kernel.org/dri-devel/20260810132202.3C16D1F000E9@smtp.kernel.org/
> Fixes: cc1aeedb98ad ("drm/imagination: Implement firmware infrastructure and META FW support")
> Signed-off-by: Alexandru Dadu <alexandru.dadu@imgtec.com>
> ---
> Changes in v2:
> - Commit message and cover letter updates.
> - Link to v1: https://patch.msgid.link/20260812-fix-null-pointer-dereference-v1-1-f69b2ffe9520@imgtec.com
> ---
>  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) {

Not sure you saw the comment from v1, but could you return early on invalid
pointers instead and leave the declarations (minus initialisation) at the top,
to keep the indentation to a minimum?

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: bd4f284df04d76fd65e57141cb1e6e7a49e4c3cb
> change-id: 20260812-fix-null-pointer-dereference-2c891142d988
> 
> Best regards,
> --  
> Alexandru Dadu <alexandru.dadu@imgtec.com>
>