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

["Ceraolo Spurio, Daniele" <daniele.ceraolospurio@intel.com>]

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)