diff mbox series

[3/3] drm/i915/uc: Fix two issues with over-size firmware files

Message ID 20221220024147.4118685-4-John.C.Harrison@Intel.com (mailing list archive)
State New, archived
Headers show
Series Fixes for various UC related issues | expand

Commit Message

John Harrison Dec. 20, 2022, 2:41 a.m. UTC 
From: John Harrison <John.C.Harrison@Intel.com>

In the case where a firmware file is too large (e.g. someone
downloaded a web page ASCII dump from github...), the firmware object
is released but the pointer is not zerod. If no other firmware file
was found then release would be called again leading to a double kfree.

Also, the size check was only being applied to the initial firmware
load not any of the subsequent attempts. So move the check into a
wrapper that is used for all loads.

Fixes: 016241168dc5 ("drm/i915/uc: use different ggtt pin offsets for uc loads")
Signed-off-by: John Harrison <John.C.Harrison@Intel.com>
Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
Cc: Alan Previn <alan.previn.teres.alexis@intel.com>
Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
Cc: Matt Roper <matthew.d.roper@intel.com>
Cc: Jani Nikula <jani.nikula@intel.com>
Cc: Matthew Auld <matthew.auld@intel.com>
Cc: "Thomas Hellström" <thomas.hellstrom@linux.intel.com>
---
 drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c | 42 ++++++++++++++++--------
 1 file changed, 28 insertions(+), 14 deletions(-)

Comments

Daniele Ceraolo Spurio Dec. 20, 2022, 11:15 a.m. UTC | #1 
On 12/20/2022 3:41 AM, John.C.Harrison@Intel.com wrote:
> From: John Harrison <John.C.Harrison@Intel.com>
>
> In the case where a firmware file is too large (e.g. someone
> downloaded a web page ASCII dump from github...), the firmware object
> is released but the pointer is not zerod. If no other firmware file
> was found then release would be called again leading to a double kfree.
>
> Also, the size check was only being applied to the initial firmware
> load not any of the subsequent attempts. So move the check into a
> wrapper that is used for all loads.
>
> Fixes: 016241168dc5 ("drm/i915/uc: use different ggtt pin offsets for uc loads")
> Signed-off-by: John Harrison <John.C.Harrison@Intel.com>
> Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
> Cc: Alan Previn <alan.previn.teres.alexis@intel.com>
> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> Cc: Matt Roper <matthew.d.roper@intel.com>
> Cc: Jani Nikula <jani.nikula@intel.com>
> Cc: Matthew Auld <matthew.auld@intel.com>
> Cc: "Thomas Hellström" <thomas.hellstrom@linux.intel.com>

There was another patch that was sent to fix the double free 
(https://patchwork.freedesktop.org/series/111545/), but this one is 
better because it also fixes the size check.

Reviewed-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>

Daniele

> ---
>   drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c | 42 ++++++++++++++++--------
>   1 file changed, 28 insertions(+), 14 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c b/drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c
> index d6ff6c584c1e1..06b5f92ba3a55 100644
> --- a/drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c
> +++ b/drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c
> @@ -675,6 +675,32 @@ static int check_fw_header(struct intel_gt *gt,
>   	return 0;
>   }
>   
> +int try_firmware_load(struct intel_uc_fw *uc_fw, const struct firmware **fw)
> +{
> +	struct intel_gt *gt = __uc_fw_to_gt(uc_fw);
> +	struct device *dev = gt->i915->drm.dev;
> +	int err;
> +
> +	err = firmware_request_nowarn(fw, uc_fw->file_selected.path, dev);
> +
> +	if (err)
> +		return err;
> +
> +	if ((*fw)->size > INTEL_UC_RSVD_GGTT_PER_FW) {
> +		drm_err(&gt->i915->drm,
> +			"%s firmware %s: size (%zuKB) exceeds max supported size (%uKB)\n",
> +			intel_uc_fw_type_repr(uc_fw->type), uc_fw->file_selected.path,
> +			(*fw)->size / SZ_1K, INTEL_UC_RSVD_GGTT_PER_FW / SZ_1K);
> +
> +		/* try to find another blob to load */
> +		release_firmware(*fw);
> +		*fw = NULL;
> +		return -ENOENT;
> +	}
> +
> +	return 0;
> +}
> +
>   /**
>    * intel_uc_fw_fetch - fetch uC firmware
>    * @uc_fw: uC firmware
> @@ -688,7 +714,6 @@ int intel_uc_fw_fetch(struct intel_uc_fw *uc_fw)
>   	struct intel_gt *gt = __uc_fw_to_gt(uc_fw);
>   	struct drm_i915_private *i915 = gt->i915;
>   	struct intel_uc_fw_file file_ideal;
> -	struct device *dev = i915->drm.dev;
>   	struct drm_i915_gem_object *obj;
>   	const struct firmware *fw = NULL;
>   	bool old_ver = false;
> @@ -704,20 +729,9 @@ int intel_uc_fw_fetch(struct intel_uc_fw *uc_fw)
>   	__force_fw_fetch_failures(uc_fw, -EINVAL);
>   	__force_fw_fetch_failures(uc_fw, -ESTALE);
>   
> -	err = firmware_request_nowarn(&fw, uc_fw->file_selected.path, dev);
> +	err = try_firmware_load(uc_fw, &fw);
>   	memcpy(&file_ideal, &uc_fw->file_wanted, sizeof(file_ideal));
>   
> -	if (!err && fw->size > INTEL_UC_RSVD_GGTT_PER_FW) {
> -		drm_err(&i915->drm,
> -			"%s firmware %s: size (%zuKB) exceeds max supported size (%uKB)\n",
> -			intel_uc_fw_type_repr(uc_fw->type), uc_fw->file_selected.path,
> -			fw->size / SZ_1K, INTEL_UC_RSVD_GGTT_PER_FW / SZ_1K);
> -
> -		/* try to find another blob to load */
> -		release_firmware(fw);
> -		err = -ENOENT;
> -	}
> -
>   	/* Any error is terminal if overriding. Don't bother searching for older versions */
>   	if (err && intel_uc_fw_is_overridden(uc_fw))
>   		goto fail;
> @@ -738,7 +752,7 @@ int intel_uc_fw_fetch(struct intel_uc_fw *uc_fw)
>   			break;
>   		}
>   
> -		err = firmware_request_nowarn(&fw, uc_fw->file_selected.path, dev);
> +		err = try_firmware_load(uc_fw, &fw);
>   	}
>   
>   	if (err)
diff mbox series

Patch

diff --git a/drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c b/drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c
index d6ff6c584c1e1..06b5f92ba3a55 100644
--- a/drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c
+++ b/drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c
@@ -675,6 +675,32 @@  static int check_fw_header(struct intel_gt *gt,
 	return 0;
 }
 
+int try_firmware_load(struct intel_uc_fw *uc_fw, const struct firmware **fw)
+{
+	struct intel_gt *gt = __uc_fw_to_gt(uc_fw);
+	struct device *dev = gt->i915->drm.dev;
+	int err;
+
+	err = firmware_request_nowarn(fw, uc_fw->file_selected.path, dev);
+
+	if (err)
+		return err;
+
+	if ((*fw)->size > INTEL_UC_RSVD_GGTT_PER_FW) {
+		drm_err(&gt->i915->drm,
+			"%s firmware %s: size (%zuKB) exceeds max supported size (%uKB)\n",
+			intel_uc_fw_type_repr(uc_fw->type), uc_fw->file_selected.path,
+			(*fw)->size / SZ_1K, INTEL_UC_RSVD_GGTT_PER_FW / SZ_1K);
+
+		/* try to find another blob to load */
+		release_firmware(*fw);
+		*fw = NULL;
+		return -ENOENT;
+	}
+
+	return 0;
+}
+
 /**
  * intel_uc_fw_fetch - fetch uC firmware
  * @uc_fw: uC firmware
@@ -688,7 +714,6 @@  int intel_uc_fw_fetch(struct intel_uc_fw *uc_fw)
 	struct intel_gt *gt = __uc_fw_to_gt(uc_fw);
 	struct drm_i915_private *i915 = gt->i915;
 	struct intel_uc_fw_file file_ideal;
-	struct device *dev = i915->drm.dev;
 	struct drm_i915_gem_object *obj;
 	const struct firmware *fw = NULL;
 	bool old_ver = false;
@@ -704,20 +729,9 @@  int intel_uc_fw_fetch(struct intel_uc_fw *uc_fw)
 	__force_fw_fetch_failures(uc_fw, -EINVAL);
 	__force_fw_fetch_failures(uc_fw, -ESTALE);
 
-	err = firmware_request_nowarn(&fw, uc_fw->file_selected.path, dev);
+	err = try_firmware_load(uc_fw, &fw);
 	memcpy(&file_ideal, &uc_fw->file_wanted, sizeof(file_ideal));
 
-	if (!err && fw->size > INTEL_UC_RSVD_GGTT_PER_FW) {
-		drm_err(&i915->drm,
-			"%s firmware %s: size (%zuKB) exceeds max supported size (%uKB)\n",
-			intel_uc_fw_type_repr(uc_fw->type), uc_fw->file_selected.path,
-			fw->size / SZ_1K, INTEL_UC_RSVD_GGTT_PER_FW / SZ_1K);
-
-		/* try to find another blob to load */
-		release_firmware(fw);
-		err = -ENOENT;
-	}
-
 	/* Any error is terminal if overriding. Don't bother searching for older versions */
 	if (err && intel_uc_fw_is_overridden(uc_fw))
 		goto fail;
@@ -738,7 +752,7 @@  int intel_uc_fw_fetch(struct intel_uc_fw *uc_fw)
 			break;
 		}
 
-		err = firmware_request_nowarn(&fw, uc_fw->file_selected.path, dev);
+		err = try_firmware_load(uc_fw, &fw);
 	}
 
 	if (err)