Re: [PATCH 2/2] drm/i915/gsc: Validate the CPD entry offset before manifest read

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

On 8/7/2026 12:23 PM, Jesus Narvaez wrote:
> The CPD manifest offset was dereferenced without checking the bounds
> first. Therefore, ensure the offset doesn't point into the CPD header
> or past the bounds of the FW blob before reading the GSC manifest.
>
> Also, validate CPD header length before using it to size the CPD
> entry table, and cast the boot1 partition's offset and size to size_t
> to avoid an integer overflow.
>
> Discovered using AI-assisted static analysis confirmed by Intel Product
> Security.
>
> Reported-by: Martin Hodo <martin.hodo@intel.com>
> Fixes: 56fafa569764 ("drm/i915/mtl/gsc: extract release and security versions from the gsc binary")
> Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
> Cc: Alan Previn <alan.previn.teres.alexis@intel.com>
> Cc: <stable@vger.kernel.org> # v6.6+
> Signed-off-by: Jesus Narvaez <jesus.narvaez@intel.com>
> ---
>   drivers/gpu/drm/i915/gt/uc/intel_gsc_fw.c | 27 ++++++++++++++++++++---
>   1 file changed, 24 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/gt/uc/intel_gsc_fw.c b/drivers/gpu/drm/i915/gt/uc/intel_gsc_fw.c
> index d550eb6edfb8..1b9976869c46 100644
> --- a/drivers/gpu/drm/i915/gt/uc/intel_gsc_fw.c
> +++ b/drivers/gpu/drm/i915/gt/uc/intel_gsc_fw.c
> @@ -139,7 +139,7 @@ int intel_gsc_fw_get_binary_info(struct intel_uc_fw *gsc_fw, const void *data, s
>   	 * --------------------------------------------------
>   	 */
>   
> -	min_size = layout->boot1.offset + layout->boot1.size;
> +	min_size = (size_t)layout->boot1.offset + layout->boot1.size;
>   	if (size < min_size) {
>   		gt_err(gt, "GSC FW too small for boot section! %zu < %zu\n",
>   		       size, min_size);
> @@ -195,7 +195,14 @@ int intel_gsc_fw_get_binary_info(struct intel_uc_fw *gsc_fw, const void *data, s
>   		return -EINVAL;
>   	}
>   
> -	min_size += sizeof(*cpd_entry) * cpd_header->num_of_entries;
> +	if (cpd_header->header_length < sizeof(struct intel_gsc_cpd_header_v2)) {
> +		gt_err(gt, "invalid CPD header length in GSC bin: %u < %zu!\n",
> +		       cpd_header->header_length, sizeof(*cpd_header));
> +		return -EINVAL;
> +	}

I think this check should be a few lines higher, where we assign the 
cpd_header pointer. That way you can also switch the min_size assignment 
there to use cpd_header->header_length and leave the one here untouched.

> +
> +	min_size = bpdt_entry->sub_partition_offset + cpd_header->header_length +
> +		   sizeof(*cpd_entry) * cpd_header->num_of_entries;
>   	if (layout->boot1.size < min_size) {
>   		gt_err(gt, "GSC FW boot section too small for CPD entries: %u < %zu\n",
>   		       layout->boot1.size, min_size);
> @@ -205,7 +212,21 @@ int intel_gsc_fw_get_binary_info(struct intel_uc_fw *gsc_fw, const void *data, s
>   	cpd_entry = (void *)cpd_header + cpd_header->header_length;
>   	for (i = 0; i < cpd_header->num_of_entries; i++, cpd_entry++) {
>   		if (strcmp(cpd_entry->name, "RBEP.man") == 0) {
> -			manifest = (void *)cpd_header + cpd_entry_offset(cpd_entry);
> +			u32 man_off = cpd_entry_offset(cpd_entry);
> +
> +			if (man_off < cpd_header->header_length) {
> +				gt_err(gt, "GSC FW manifest offset points into CPD header: %u < %u\n",
> +				       man_off, cpd_header->header_length);
> +				return -EINVAL;
> +			}

I don't think this check is valuable. The offset could point to a lot of 
different invalid places and we can't check them all, we just need to 
make sure it points inside the binary. If the binary is malformed it 
will fail to load so we'll catch the error then.

> +
> +			if (man_off + sizeof(struct intel_gsc_manifest_header) >
> +			    layout->boot1.size - bpdt_entry->sub_partition_offset) {

using bpdt_entry->sub_partition_offset is kind of dodgy here because 
we're outside the loop where we go through all entries. If we ever need 
to look for a second entry then bpdt_entry might no longer point to the 
CDP at this point. IMO better to save it to a variable.
Also IMO some parenthesis would make things cleaner here.

Daniele

> +				gt_err(gt, "GSC FW boot section too small for manifest: %u < %zu\n",
> +				       layout->boot1.size, man_off + sizeof(*manifest));
> +				return -ENODATA;
> +			}
> +			manifest = (void *)cpd_header + man_off;
>   			intel_uc_fw_version_from_gsc_manifest(&gsc->release,
>   							      manifest);
>   			gsc->security_version = manifest->security_version;