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

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

[Alexandru Dadu]

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.

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) {
+		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>

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

[Alessio Belle]

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>
>