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

["Narvaez, Jesus" <jesus.narvaez@intel.com>]

On 8/24/2026 7:28 PM, Daniele Ceraolo Spurio wrote:
>
>
> 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.
I'm not sure I fully understand what you're saying here. Are you saying 
to move this cpd_header->header_length check to right after the 
cpd_header pointer is assigned, and then update min_size after that to 
also use cpd_header->header_length instead of sizeof(*cpd_header)? If 
so, my understanding is that we haven't validated cpd_header at that 
point, and we'd be reading cpd_header->header_length as part of the 
check before we know it's safe, so I think it would be best to leave it 
where it is.
>
>> +
>> +    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.
Makes sense, will remove.
>
>> +
>> +            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

OK understood, I can add those changes.

Jesus Narvaez

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