Re: [PATCH 3/6] drm/i915/uc/gsc: extract release and security versions from the gsc binary

From: "Teres Alexis, Alan Previn" <alan.previn.teres.alexis@intel.com>
To: "Ceraolo Spurio, Daniele" <daniele.ceraolospurio@intel.com>,
	"intel-gfx@lists.freedesktop.org"
	<intel-gfx@lists.freedesktop.org>
Cc: "dri-devel@lists.freedesktop.org" <dri-devel@lists.freedesktop.org>
Subject: Re: [PATCH 3/6] drm/i915/uc/gsc: extract release and security versions from the gsc binary
Date: Thu, 25 May 2023 05:14:08 +0000	[thread overview]
Message-ID: <674e85c47cd29b39ad902e5445234b93b2f99883.camel@intel.com> (raw)
In-Reply-To: <20230505160415.889525-4-daniele.ceraolospurio@intel.com>

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?
> +	 * 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;
actually since we are accessing a bpdt_header hanging off that offset, it should rather be:
	min_size = layout->boot1_offset + 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
> +	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.
> +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?
> +} __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

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

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