[PATCH 3/6] drm/i915/uc/gsc: extract release and security versions from the gsc binary
Ceraolo Spurio, Daniele
daniele.ceraolospurio at intel.com
Thu May 25 16:56:20 UTC 2023
On 5/24/2023 10:14 PM, Teres Alexis, Alan Previn wrote:
> On Fri, 2023-05-05 at 09:04 -0700, Ceraolo Spurio, Daniele wrote:
>
>
> alan: snip
>
>
>> +int intel_gsc_fw_get_binary_info(struct intel_uc_fw *gsc_fw, const void *data, size_t size)
>> +{
> alan:snip
>> + /*
>> + * The GSC binary starts with the pointer layout, which contains the
>> + * locations of the various partitions of the binary. The one we're
>> + * interested in to get the version is the boot1 partition, where we can
>> + * find a BPDT header followed by entries, one of which points to the
>> + * RBE sub-section of the partition. From here, we can parse the CPD
> alan: nit: could we add the meaning of 'RBE', probably not here but in the header file where GSC_RBE is defined?
I have no idea what RBE means, the specs just says to look for the
section named that way. I didn't dig because we don't really care about
its name, just that the CPD header is in there.
>> + * header and the following entries to find the manifest location
>> + * (entry identified by the "RBEP.man" name), from which we can finally
>> + * extract the version.
>> + *
>> + * --------------------------------------------------
>> + * [ intel_gsc_layout_pointers ]
>> + * [ ... ]
>> + * [ boot1_offset >---------------------------]------o
>> + * [ ... ] |
>> + * -------------------------------------------------- |
>> + * |
>> + * -------------------------------------------------- |
>> + * [ intel_gsc_bpdt_header ]<-----o
>> + * --------------------------------------------------
> alan: special thanks for the diagram - love these! :)
> alan: snip
>
>> + min_size = layout->boot1_offset + layout->boot1_offset > size;
> alan: latter is a binary so + 1? or is this a typo and supposed to be:
> min_size = layout->boot1_offset;
it's a cut & paste typo, it should be
min_size = layout->boot1_offset + layout->boot1_size;
thanks for spotting it.
> actually since we are accessing a bpdt_header hanging off that offset, it should rather be:
> min_size = layout->boot1_offset + sizeof(*bpdt_header);
I can add a check to make sure that boot1_size >= sizeof(*bpdt_header)
>> + if (size < min_size) {
>> + gt_err(gt, "GSC FW too small for boot section! %zu < %zu\n",
>> + size, min_size);
>> + return -ENODATA;
>> + }
>> +
>> + bpdt_header = data + layout->boot1_offset;
>> + if (bpdt_header->signature != INTEL_GSC_BPDT_HEADER_SIGNATURE) {
>> + gt_err(gt, "invalid signature for meu BPDT header: 0x%08x!\n",
>> + bpdt_header->signature);
>> + return -EINVAL;
>> + }
>> +
> alan: IIUC, to be strict about the size-crawl-checking, we should check minsize
> again - this time with the additional "bpdt_header->descriptor_count * sizeof(*bpdt_entry)".
> (hope i got that right?) - adding that check before parsing through the (bpdt_entry++)'s ->type
will do (same for the comments below)
>> + bpdt_entry = (void *)bpdt_header + sizeof(*bpdt_header);
>> + for (i = 0; i < bpdt_header->descriptor_count; i++, bpdt_entry++) {
>> + if ((bpdt_entry->type & INTEL_GSC_BPDT_ENTRY_TYPE_MASK) !=
>> + INTEL_GSC_BPDT_ENTRY_TYPE_GSC_RBE)
>> + continue;
>> +
>> + cpd_header = (void *)bpdt_header + bpdt_entry->sub_partition_offset;
>> + break;
>> + }
>> +
>> + if (!cpd_header) {
>> + gt_err(gt, "couldn't find CPD header in GSC binary!\n");
>> + return -ENODATA;
>> + }
> alan: same as above, so for size-crawl-checking, we should check minsize again with the addition of cpd_header, no?
>> +
>> + if (cpd_header->header_marker != INTEL_GSC_CPD_HEADER_MARKER) {
>> + gt_err(gt, "invalid marker for meu CPD header in GSC bin: 0x%08x!\n",
>> + cpd_header->header_marker);
>> + return -EINVAL;
>> + }
> alan: and again here, the size crawl checking with cpd_header->num_of_entries * *cpd_entry
>> + 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);
>> + intel_uc_fw_version_from_meu_manifest(&gsc->release,
>> + manifest);
>> + gsc->security_version = manifest->security_version;
>> + break;
>> + }
>> + }
>> +
>> + return 0;
> alan:snip
>
>>
>> diff --git a/drivers/gpu/drm/i915/gt/uc/intel_gsc_fw.h b/drivers/gpu/drm/i915/gt/uc/intel_gsc_fw.h
>> index fff8928218df..8d7b9e4f1ffc 100644
>> --- a/drivers/gpu/drm/i915/gt/uc/intel_gsc_fw.h
>> +++ b/drivers/gpu/drm/i915/gt/uc/intel_gsc_fw.h
> alan:snip
>
>
>> diff --git a/drivers/gpu/drm/i915/gt/uc/intel_gsc_meu_headers.h b/drivers/gpu/drm/i915/gt/uc/intel_gsc_meu_headers.h
>> index d55a66202576..8bce2b8aed84 100644
>> --- a/drivers/gpu/drm/i915/gt/uc/intel_gsc_meu_headers.h
>> +++ b/drivers/gpu/drm/i915/gt/uc/intel_gsc_meu_headers.h
> alan:snip
>
>
>
>> +struct intel_gsc_layout_pointers {
>> + u8 rom_bypass_vector[16];
> alan:snip...
>> + u32 temp_pages_offset;
>> + u32 temp_pages_size;
>> +} __packed;
> alan: structure layout seems unnecessarily repetitive... why not ->
> struct partition_info {
> u32 offset;
> u32 size;
> };
> struct intel_gsc_layout_pointers {
> u8 rom_bypass_vector[16];
> ...
> struct partition_info datap;
> struct partition_info bootregion[5];
> struct partition_info trace;
> }__packed;
>
>
>> +
> alan: we should put the 'bpdt' acronym meaning and if its an intel specific
> name, then a bit of additional comment explaining what it means.
BPD stands for Boot Partition Directory, not sure what the T is for
(Type maybe?). I'll add a comment that these are the structures for the
GSC boot partition, it should make things clear enough.
>> +struct intel_gsc_bpdt_header {
>> + u32 signature;
>> +#define INTEL_GSC_BPDT_HEADER_SIGNATURE 0x000055AA
>> +
>> + u16 descriptor_count; /* bum of entries after the header */
> alan:s/bum/num
>> +
>> + u8 version;
>> + u8 configuration;
>> +
>> + u32 crc32;
>> +
>> + u32 build_version;
>> + struct intel_gsc_meu_version tool_version;
> alan: nit: "struct intel_gsc_meu_version meu_version" is better no?
no. struct intel_gsc_meu_version is the basic struct used for versions
encoded in the meu headers and it is used for multiple entries in the
various headers. This particular one is the tool_version, hence the name.
>> +} __packed;
>> +
>> +
>> +struct intel_gsc_bpdt_entry {
>> + /*
>> + * Bits 0-15: BPDT entry type
>> + * Bits 16-17: reserved
>> + * Bit 18: code sub-partition
>> + * Bits 19-31: reserved
>> + */
> alan: nit: i think its neater to just put above comments next to the #defines on the lines following 'type' and
> create a genmask for code-sub-partition (if we use it, else skip it?) - the benefit being a little less clutter
I think the lines would get too long if I put the comments on the same
lines as the defines.
We only use the lower 16 bits, hence why I only defined those, but I
wanted to have the whole thing documented for completeness.
>
>> + u32 type;
>> +#define INTEL_GSC_BPDT_ENTRY_TYPE_MASK GENMASK(15,0)
>> +#define INTEL_GSC_BPDT_ENTRY_TYPE_GSC_RBE 0x1
>> +
>> + u32 sub_partition_offset; /* from the base of the BPDT header */
>> + u32 sub_partition_size;
>> +} __packed;
>> +
> alan:snip
>
>
>> --- a/drivers/gpu/drm/i915/gt/uc/intel_gsc_uc.h
>> +++ b/drivers/gpu/drm/i915/gt/uc/intel_gsc_uc.h
>> @@ -17,6 +17,9 @@ struct intel_gsc_uc {
>> struct intel_uc_fw fw;
>>
>> /* GSC-specific additions */
>> + struct intel_uc_fw_ver release;
>> + u32 security_version;
> alan: for consistency and less redundancy, can't we add "security_version"
> into 'struct intel_uc_fw_ver' (which is zero for firmware that doesn't
> have it). That way, intel_gsc_uc can re-use intel_uc_fw.file_selected
> just like huc?
I'm not sure what you mean by re-using intel_uc_fw.file_selected. Is
that for the call from intel_uc_fw_version_from_meu_manifest? I'm
purposely not doing that. Note that the GSC has 3 versions:
Release version (incremented with each build and encoded in the header)
Security version (also encoded in the header)
Compatibility version (queried via message to the GSC)
The one we care about for communicating with the GSC is the last one, so
that's the one I stored in intel_uc_fw.file_selected (in the next
patch). The other 2 versions are not strictly required to use the GSC
and we only fetch them for debug purposes, so if something goes wrong we
know exactly what we've loaded.
Daniele
>
> alan:snip
>
>
>
>> diff --git a/drivers/gpu/drm/i915/gt/uc/intel_huc_fw.c b/drivers/gpu/drm/i915/gt/uc/intel_huc_fw.c
>> index 0a0bd5504057..0c01d48b1dd9 100644
>> --- a/drivers/gpu/drm/i915/gt/uc/intel_huc_fw.c
>> +++ b/drivers/gpu/drm/i915/gt/uc/intel_huc_fw.c
> alan:snip
>
>
>> 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 796f54a62eef..cd8fc194f7fa 100644
>> --- a/drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c
>> +++ b/drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c
> alan:snip
>
>> diff --git a/drivers/gpu/drm/i915/gt/uc/intel_uc_fw.h b/drivers/gpu/drm/i915/gt/uc/intel_uc_fw.h
>> index 8f2306627332..279244744d43 100644
>> --- a/drivers/gpu/drm/i915/gt/uc/intel_uc_fw.h
>> +++ b/drivers/gpu/drm/i915/gt/uc/intel_uc_fw.h
> alan:snip
More information about the dri-devel
mailing list