Re: [Intel-gfx] [PATCH] drm/i915/gt: Avoid out-of-bounds access when loading HuC

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

On 4/13/2023 1:03 PM, Lucas De Marchi wrote:
> When HuC is loaded by GSC, there is no header definition for the kernel
> to look at and firmware is just handed to GSC. However when reading the
> version, it should still check the size of the blob to guarantee it's not
> incurring into out-of-bounds array access.
>
> If firmware is smaller than expected, the following message is now
> printed:
>
> 	# echo boom > /lib/firmware/i915/dg2_huc_gsc.bin
> 	# dmesg | grep -i huc
> 	[drm] GT0: HuC firmware i915/dg2_huc_gsc.bin: invalid size: 5 < 184
> 	[drm] *ERROR* GT0: HuC firmware i915/dg2_huc_gsc.bin: fetch failed -ENODATA
> 	...
>
> Even without this change the size, header and signature are still
> checked by GSC when loading, so this only avoids the out-of-bounds array
> access.
>
> Fixes: a7b516bd981f ("drm/i915/huc: Add fetch support for gsc-loaded HuC binary")
> Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
> Cc: Alan Previn <alan.previn.teres.alexis@intel.com>
> Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com>

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

Daniele

> ---
>   drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c | 21 +++++++++++++++++----
>   1 file changed, 17 insertions(+), 4 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 1ac6f9f340e3..a82a53dbbc86 100644
> --- a/drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c
> +++ b/drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c
> @@ -489,12 +489,25 @@ static void __force_fw_fetch_failures(struct intel_uc_fw *uc_fw, int e)
>   	}
>   }
>   
> -static int check_gsc_manifest(const struct firmware *fw,
> +static int check_gsc_manifest(struct intel_gt *gt,
> +			      const struct firmware *fw,
>   			      struct intel_uc_fw *uc_fw)
>   {
>   	u32 *dw = (u32 *)fw->data;
> -	u32 version_hi = dw[HUC_GSC_VERSION_HI_DW];
> -	u32 version_lo = dw[HUC_GSC_VERSION_LO_DW];
> +	u32 version_hi, version_lo;
> +	size_t min_size;
> +
> +	/* Check the size of the blob before examining buffer contents */
> +	min_size = sizeof(u32) * (HUC_GSC_VERSION_LO_DW + 1);
> +	if (unlikely(fw->size < min_size)) {
> +		gt_warn(gt, "%s firmware %s: invalid size: %zu < %zu\n",
> +			intel_uc_fw_type_repr(uc_fw->type), uc_fw->file_selected.path,
> +			fw->size, min_size);
> +		return -ENODATA;
> +	}
> +
> +	version_hi = dw[HUC_GSC_VERSION_HI_DW];
> +	version_lo = dw[HUC_GSC_VERSION_LO_DW];
>   
>   	uc_fw->file_selected.ver.major = FIELD_GET(HUC_GSC_MAJOR_VER_HI_MASK, version_hi);
>   	uc_fw->file_selected.ver.minor = FIELD_GET(HUC_GSC_MINOR_VER_HI_MASK, version_hi);
> @@ -665,7 +678,7 @@ static int check_fw_header(struct intel_gt *gt,
>   		return 0;
>   
>   	if (uc_fw->loaded_via_gsc)
> -		err = check_gsc_manifest(fw, uc_fw);
> +		err = check_gsc_manifest(gt, fw, uc_fw);
>   	else
>   		err = check_ccs_header(gt, fw, uc_fw);
>   	if (err)