[PATCH v4 08/16] efi_loader: image_loader: support image authentication

Heinrich Schuchardt xypron.glpk at gmx.de
Fri Jan 17 06:51:50 CET 2020


On 1/17/20 6:11 AM, AKASHI Takahiro wrote:
> On Thu, Jan 09, 2020 at 12:55:17AM +0100, Heinrich Schuchardt wrote:
>> On 12/18/19 1:45 AM, AKASHI Takahiro wrote:
>>> With this commit, image validation can be enforced, as UEFI specification
>>> section 32.5 describes, if CONFIG_EFI_SECURE_BOOT is enabled.
>>>
>>> Currently we support
>>> * authentication based on db and dbx,
>>>    so dbx-validated image will always be rejected.
>>> * following signature types:
>>>      EFI_CERT_SHA256_GUID (SHA256 digest for unsigned images)
>>>      EFI_CERT_X509_GUID (x509 certificate for signed images)
>>> Timestamp-based certificate revocation is not supported here.
>>>
>>> Internally, authentication data is stored in one of certificates tables
>>> of PE image (See efi_image_parse()) and will be verified by
>>> efi_image_authenticate() before loading a given image.
>>>
>>> It seems that UEFI specification defines the verification process
>>> in a bit ambiguous way. I tried to implement it as closely to as
>>> EDK2 does.
>>>
>>> Signed-off-by: AKASHI Takahiro <takahiro.akashi at linaro.org>
>>> ---
>>>   include/efi_loader.h              |   7 +-
>>>   lib/efi_loader/efi_boottime.c     |   2 +-
>>>   lib/efi_loader/efi_image_loader.c | 454 +++++++++++++++++++++++++++++-
>>>   3 files changed, 449 insertions(+), 14 deletions(-)
>>>
>>> diff --git a/include/efi_loader.h b/include/efi_loader.h
>>> index 1f88caf86709..e12b49098fb0 100644
>>> --- a/include/efi_loader.h
>>> +++ b/include/efi_loader.h
>>> @@ -11,6 +11,7 @@
>>>   #include <common.h>
>>>   #include <part_efi.h>
>>>   #include <efi_api.h>
>>> +#include <pe.h>
>>>
>>>   static inline int guidcmp(const void *g1, const void *g2)
>>>   {
>>> @@ -398,7 +399,8 @@ efi_status_t efi_set_watchdog(unsigned long timeout);
>>>   /* Called from places to check whether a timer expired */
>>>   void efi_timer_check(void);
>>>   /* PE loader implementation */
>>> -efi_status_t efi_load_pe(struct efi_loaded_image_obj *handle, void *efi,
>>> +efi_status_t efi_load_pe(struct efi_loaded_image_obj *handle,
>>> +			 void *efi, size_t efi_size,
>>>   			 struct efi_loaded_image *loaded_image_info);
>>>   /* Called once to store the pristine gd pointer */
>>>   void efi_save_gd(void);
>>> @@ -726,6 +728,9 @@ void efi_sigstore_free(struct efi_signature_store *sigstore);
>>>   struct efi_signature_store *efi_sigstore_parse_sigdb(u16 *name);
>>>
>>>   bool efi_secure_boot_enabled(void);
>>> +
>>> +bool efi_image_parse(void *efi, size_t len, struct efi_image_regions **regp,
>>> +		     WIN_CERTIFICATE **auth, size_t *auth_len);
>>>   #endif /* CONFIG_EFI_SECURE_BOOT */
>>>
>>>   #else /* CONFIG_IS_ENABLED(EFI_LOADER) */
>>> diff --git a/lib/efi_loader/efi_boottime.c b/lib/efi_loader/efi_boottime.c
>>> index 493d906c641d..311681764034 100644
>>> --- a/lib/efi_loader/efi_boottime.c
>>> +++ b/lib/efi_loader/efi_boottime.c
>>> @@ -1879,7 +1879,7 @@ efi_status_t EFIAPI efi_load_image(bool boot_policy,
>>>   	efi_dp_split_file_path(file_path, &dp, &fp);
>>>   	ret = efi_setup_loaded_image(dp, fp, image_obj, &info);
>>>   	if (ret == EFI_SUCCESS)
>>> -		ret = efi_load_pe(*image_obj, dest_buffer, info);
>>> +		ret = efi_load_pe(*image_obj, dest_buffer, source_size, info);
>>>   	if (!source_buffer)
>>>   		/* Release buffer to which file was loaded */
>>>   		efi_free_pages((uintptr_t)dest_buffer,
>>> diff --git a/lib/efi_loader/efi_image_loader.c b/lib/efi_loader/efi_image_loader.c
>>> index 13541cfa7a28..939758e61e3c 100644
>>> --- a/lib/efi_loader/efi_image_loader.c
>>> +++ b/lib/efi_loader/efi_image_loader.c
>>> @@ -9,7 +9,9 @@
>>>
>>>   #include <common.h>
>>>   #include <efi_loader.h>
>>> +#include <malloc.h>
>>>   #include <pe.h>
>>> +#include "../lib/crypto/pkcs7_parser.h"
>>>
>>>   const efi_guid_t efi_global_variable_guid = EFI_GLOBAL_VARIABLE_GUID;
>>>   const efi_guid_t efi_guid_device_path = EFI_DEVICE_PATH_PROTOCOL_GUID;
>>> @@ -205,6 +207,367 @@ static void efi_set_code_and_data_type(
>>>   	}
>>>   }
>>>
>>> +#ifdef CONFIG_EFI_SECURE_BOOT
>>> +/**
>>> + * efi_image_parse - parse a PE image
>>> + * @efi:	Pointer to image
>>> + * @len:	Size of @efi
>>> + * @regs:	Pointer to a list of regions
>>> + * @auth:	Pointer to a pointer to authentication data in PE
>>> + * @auth_len:	Size of @auth
>>> + *
>>> + * Parse image binary in PE32(+) format, assuming that sanity of PE image
>>> + * has been checked by a caller.
>>> + * On success, an address of authentication data in @efi and its size will
>>> + * be returned in @auth and @auth_len, respectively.
>>> + *
>>> + * Return:	true on success, false on error
>>> + */
>>> +bool efi_image_parse(void *efi, size_t len, struct efi_image_regions **regp,
>>> +		     WIN_CERTIFICATE **auth, size_t *auth_len)
>>
>>
>> This function is way too long (> 100 lines). Please, cut it into logical
>> units.
>>
>>
>>
>>> +{
>>> +	struct efi_image_regions *regs;
>>> +	IMAGE_DOS_HEADER *dos;
>>> +	IMAGE_NT_HEADERS32 *nt;
>>> +	IMAGE_SECTION_HEADER *sections, **sorted;
>>> +	int num_regions, num_sections, i, j;
>>> +	int ctidx = IMAGE_DIRECTORY_ENTRY_SECURITY;
>>> +	u32 align, size, authsz, authoff;
>>> +	size_t bytes_hashed;
>>> +
>>> +	dos = (void *)efi;
>>> +	nt = (void *)(efi + dos->e_lfanew);
>>> +
>>> +	/*
>>> +	 * Count maximum number of regions to be digested.
>>> +	 * We don't have to have an exact number here.
>>> +	 * See efi_image_region_add()'s in parsing below.
>>> +	 */
>>> +	num_regions = 3; /* for header */
>>> +	num_regions += nt->FileHeader.NumberOfSections;
>>> +	num_regions++; /* for extra */
>>> +
>>> +	regs = calloc(sizeof(*regs) + sizeof(struct image_region) * num_regions,
>>> +		      1);
>>> +	if (!regs)
>>> +		goto err;
>>> +	regs->max = num_regions;
>>> +
>>> +	/*
>>> +	 * Collect data regions for hash calculation
>>> +	 * 1. File headers
>>> +	 */
>>> +	if (nt->OptionalHeader.Magic == IMAGE_NT_OPTIONAL_HDR64_MAGIC) {
>>> +		IMAGE_NT_HEADERS64 *nt64 = (void *)nt;
>>> +		IMAGE_OPTIONAL_HEADER64 *opt = &nt64->OptionalHeader;
>>> +
>>> +		/* Skip CheckSum */
>>> +		efi_image_region_add(regs, efi, &opt->CheckSum, 0);
>>> +		if (nt64->OptionalHeader.NumberOfRvaAndSizes <= ctidx) {
>>> +			efi_image_region_add(regs,
>>> +					     &opt->CheckSum + 1,
>>> +					     efi + opt->SizeOfHeaders, 0);
>>> +		} else {
>>> +			/* Skip Certificates Table */
>>> +			efi_image_region_add(regs,
>>> +					     &opt->CheckSum + 1,
>>> +					     &opt->DataDirectory[ctidx], 0);
>>> +			efi_image_region_add(regs,
>>> +					     &opt->DataDirectory[ctidx] + 1,
>>> +					     efi + opt->SizeOfHeaders, 0);
>>> +		}
>>> +
>>> +		bytes_hashed = opt->SizeOfHeaders;
>>> +		align = opt->FileAlignment;
>>> +		authoff = opt->DataDirectory[ctidx].VirtualAddress;
>>> +		authsz = opt->DataDirectory[ctidx].Size;
>>> +	} else if (nt->OptionalHeader.Magic == IMAGE_NT_OPTIONAL_HDR32_MAGIC) {
>>> +		IMAGE_OPTIONAL_HEADER32 *opt = &nt->OptionalHeader;
>>> +
>>> +		efi_image_region_add(regs, efi, &opt->CheckSum, 0);
>>> +		efi_image_region_add(regs, &opt->CheckSum + 1,
>>> +				     &opt->DataDirectory[ctidx], 0);
>>> +		efi_image_region_add(regs, &opt->DataDirectory[ctidx] + 1,
>>> +				     efi + opt->SizeOfHeaders, 0);
>>> +
>>> +		bytes_hashed = opt->SizeOfHeaders;
>>> +		align = opt->FileAlignment;
>>> +		authoff = opt->DataDirectory[ctidx].VirtualAddress;
>>> +		authsz = opt->DataDirectory[ctidx].Size;
>>> +	} else {
>>> +		debug("%s: Invalid optional header magic %x\n", __func__,
>>> +		      nt->OptionalHeader.Magic);
>>> +		goto err;
>>> +	}
>>> +
>>> +	/* 2. Sections */
>>> +	num_sections = nt->FileHeader.NumberOfSections;
>>> +	sections = (void *)((uint8_t *)&nt->OptionalHeader +
>>> +			    nt->FileHeader.SizeOfOptionalHeader);
>>> +	sorted = calloc(sizeof(IMAGE_SECTION_HEADER *), num_sections);
>>> +	if (!sorted) {
>>> +		debug("%s: Out of memory\n", __func__);
>>> +		goto err;
>>> +	}
>>> +
>>> +	/*
>>> +	 * Make sure the section list is in ascending order.
>>> +	 * As we can assume that it is already ordered in most cases,
>>> +	 * the following code is optimized for this.
>>> +	 */
>>> +	for (sorted[0] = &sections[0], i = 1; i < num_sections; i++) {
>>
>> If sections[0] is not the lowest entry this function fails.
>>
>> Please, use qsort() supplied in lib/qsort.c.
>
> I'd rather fix my code as it is much simpler than qsort.

Using qsort will result in a smaller code size. With qsort your code
will also be much easier to read.

>
>>> +		if (sorted[i - 1]->VirtualAddress
>>> +				<= sections[i].VirtualAddress) {
>>> +			sorted[i] = &sections[i];
>>> +		} else {
>>> +			if (i == 1) {
>>> +				sorted[1] = sorted[0];
>>> +				sorted[0] = &sections[1];
>>> +				continue;
>>> +			}
>>> +
>>> +			sorted[i] = sorted[i - 1];
>>> +			for (j = i - 2; j >= 0; j--) {
>>> +				if (!j || sorted[j]->VirtualAddress
>>> +						<= sections[i].VirtualAddress) {
>>> +					sorted[j + 1] = &sections[i];
>>> +					continue;
>>> +				} else {
>>> +					sorted[j + 1] = sorted[j];
>>> +				}
>>> +			}
>>> +		}
>>> +	}
>>> +
>>> +	for (i = 0; i < num_sections; i++) {
>>> +		if (!sorted[i]->SizeOfRawData)
>>> +			continue;
>>> +
>>> +		size = (sorted[i]->SizeOfRawData + align - 1) & ~(align - 1);
>>> +		efi_image_region_add(regs, efi + sorted[i]->PointerToRawData,
>>> +				     efi + sorted[i]->PointerToRawData + size,
>>> +				     0);
>>> +		debug("section[%d](%s): raw: 0x%x-0x%x, virt: %x-%x\n",
>>> +		      i, sorted[i]->Name,
>>> +		      sorted[i]->PointerToRawData,
>>> +		      sorted[i]->PointerToRawData + size,
>>> +		      sorted[i]->VirtualAddress,
>>> +		      sorted[i]->VirtualAddress
>>> +			+ sorted[i]->Misc.VirtualSize);
>>> +
>>> +		bytes_hashed += size;
>>> +	}
>>> +	free(sorted);
>>> +
>>> +	/* 3. Extra data excluding Certificates Table */
>>> +	if (bytes_hashed + authsz < len) {
>>> +		debug("extra data for hash: %lu\n",
>>> +		      len - (bytes_hashed + authsz));
>>> +		efi_image_region_add(regs, efi + bytes_hashed,
>>> +				     efi + len - authsz, 0);
>>> +	}
>>> +
>>> +	/* Return Certificates Table */
>>> +	if (authsz) {
>>> +		if (len < authoff + authsz) {
>>> +			debug("%s: Size for auth too large: %u >= %zu\n",
>>> +			      __func__, authsz, len - authoff);
>>> +			goto err;
>>> +		}
>>> +		if (authsz < sizeof(*auth)) {
>>> +			debug("%s: Size for auth too small: %u < %zu\n",
>>> +			      __func__, authsz, sizeof(*auth));
>>> +			goto err;
>>> +		}
>>> +		*auth = efi + authoff;
>>> +		*auth_len = authsz;
>>> +		debug("WIN_CERTIFICATE: 0x%x, size: 0x%x\n", authoff, authsz);
>>> +	} else {
>>> +		*auth = NULL;
>>> +		*auth_len = 0;
>>> +	}
>>> +
>>> +	*regp = regs;
>>> +
>>> +	return true;
>>> +
>>> +err:
>>> +	free(regs);
>>> +
>>> +	return false;
>>> +}
>>> +
>>> +/**
>>> + * efi_image_unsigned_authenticate - authenticate unsigned image with
>>> + * SHA256 hash
>>> + * @regs:	List of regions to be verified
>>> + *
>>> + * If an image is not signed, it doesn't have a signature. In this case,
>>> + * its message digest is calculated and it will be compared with one of
>>> + * hash values stored in signature databases.
>>> + *
>>> + * Return:	true if authenticated, false if not
>>> + */
>>> +static bool efi_image_unsigned_authenticate(struct efi_image_regions *regs)
>>> +{
>>> +	struct efi_signature_store *db = NULL, *dbx = NULL;
>>> +	bool ret = false;
>>> +
>>> +	dbx = efi_sigstore_parse_sigdb(L"dbx");
>>> +	if (!dbx) {
>>> +		debug("Getting signature database(dbx) failed\n");
>>> +		goto out;
>>> +	}
>>> +
>>> +	db = efi_sigstore_parse_sigdb(L"db");
>>> +	if (!db) {
>>> +		debug("Getting signature database(db) failed\n");
>>> +		goto out;
>>> +	}
>>> +
>>> +	/* try black-list first */
>>> +	if (efi_signature_verify_with_sigdb(regs, NULL, dbx, NULL)) {
>>> +		debug("Image is not signed and rejected by \"dbx\"\n");
>>> +		goto out;
>>> +	}
>>> +
>>> +	/* try white-list */
>>> +	if (efi_signature_verify_with_sigdb(regs, NULL, db, NULL))
>>> +		ret = true;
>>> +	else
>>> +		debug("Image is not signed and not found in \"db\" or \"dbx\"\n");
>>> +
>>> +out:
>>> +	efi_sigstore_free(db);
>>> +	efi_sigstore_free(dbx);
>>> +
>>> +	return ret;
>>> +}
>>> +
>>> +/**
>>> + * efi_image_authenticate - verify a signature of signed image
>>> + * @efi:	Pointer to image
>>> + * @len:	Size of @efi
>>> + *
>>> + * A signed image should have its signature stored in a table of its PE header.
>>> + * So if an image is signed and only if if its signature is verified using
>>> + * signature databases, an image is authenticated.
>>> + * If an image is not signed, its validity is checked by using
>>> + * efi_image_unsigned_authenticated().
>>> + * TODO:
>>> + * When AuditMode==0, if the image's signature is not found in
>>> + * the authorized database, or is found in the forbidden database,
>>> + * the image will not be started and instead, information about it
>>> + * will be placed in this table.
>>> + * When AuditMode==1, an EFI_IMAGE_EXECUTION_INFO element is created
>>> + * in the EFI_IMAGE_EXECUTION_INFO_TABLE for every certificate found
>>> + * in the certificate table of every image that is validated.
>>> + *
>>> + * Return:	true if authenticated, false if not
>>> + */
>>> +static bool efi_image_authenticate(void *efi, size_t len)
>>> +{
>>> +	struct efi_image_regions *regs = NULL;
>>> +	WIN_CERTIFICATE *wincerts = NULL, *wincert;
>>> +	size_t wincerts_len;
>>> +	struct pkcs7_message *msg = NULL;
>>> +	struct efi_signature_store *db = NULL, *dbx = NULL;
>>> +	struct x509_certificate *cert = NULL;
>>> +	bool ret = false;
>>> +
>>> +	if (!efi_secure_boot_enabled())
>>> +		return true;
>>> +
>>> +	if (!efi_image_parse(efi, len, &regs, &wincerts,
>>> +			     &wincerts_len)) {
>>> +		debug("Parsing PE executable image failed\n");
>>> +		return false;
>>> +	}
>>> +
>>> +	if (!wincerts) {
>>> +		/* The image is not signed */
>>> +		ret = efi_image_unsigned_authenticate(regs);
>>> +		free(regs);
>>> +
>>> +		return ret;
>>> +	}
>>> +
>>> +	/*
>>> +	 * verify signature using db and dbx
>>> +	 */
>>> +	db = efi_sigstore_parse_sigdb(L"db");
>>> +	if (!db) {
>>> +		debug("Getting signature database(db) failed\n");
>>> +		goto err;
>>> +	}
>>> +
>>> +	dbx = efi_sigstore_parse_sigdb(L"dbx");
>>> +	if (!dbx) {
>>> +		debug("Getting signature database(dbx) failed\n");
>>> +		goto err;
>>> +	}
>>> +
>>> +	/* go through WIN_CERTIFICATE list */
>>> +	for (wincert = wincerts;
>>> +	     (void *)wincert < (void *)wincerts + wincerts_len;
>>> +	     wincert = (void *)wincert + ALIGN(wincert->dwLength, 8)) {
>>> +		if (wincert->dwLength < sizeof(*wincert)) {
>>> +			debug("%s: dwLength too small: %u < %zu\n",
>>> +			      __func__, wincert->dwLength, sizeof(*wincert));
>>> +			goto err;
>>> +		}
>>> +		msg = pkcs7_parse_message((void *)wincert + sizeof(*wincert),
>>> +					  wincert->dwLength - sizeof(*wincert));
>>> +		if (!msg) {
>>> +			debug("Parsing image's signature failed\n");
>>> +			goto err;
>>> +		}
>>> +
>>> +		/* try black-list first */
>>> +		if (efi_signature_verify_with_sigdb(regs, msg, dbx, NULL)) {
>>> +			debug("Signature was rejected by \"dbx\"\n");
>>> +			goto err;
>>> +		}
>>> +
>>> +		if (!efi_signature_verify_signers(msg, dbx)) {
>>> +			debug("Signer was rejected by \"dbx\"\n");
>>> +			goto err;
>>> +		} else {
>>> +			ret = true;
>>> +		}
>>> +
>>> +		/* try white-list */
>>> +		if (!efi_signature_verify_with_sigdb(regs, msg, db, &cert)) {
>>> +			debug("Verifying signature with \"db\" failed\n");
>>> +			goto err;
>>> +		} else {
>>> +			ret = true;
>>> +		}
>>> +
>>> +		if (!efi_signature_verify_cert(cert, dbx)) {
>>> +			debug("Certificate was rejected by \"dbx\"\n");
>>> +			goto err;
>>> +		} else {
>>> +			ret = true;
>>> +		}
>>> +	}
>>> +
>>> +err:
>>> +	x509_free_certificate(cert);
>>> +	efi_sigstore_free(db);
>>> +	efi_sigstore_free(dbx);
>>> +	pkcs7_free_message(msg);
>>> +	free(regs);
>>> +
>>> +	return ret;
>>> +}
>>> +#else
>>> +static bool efi_image_authenticate(void *efi, size_t len)
>>> +{
>>> +	return true;
>>> +}
>>> +#endif /* CONFIG_EFI_SECURE_BOOT */
>>> +
>>>   /**
>>>    * efi_load_pe() - relocate EFI binary
>>>    *
>>> @@ -216,7 +579,8 @@ static void efi_set_code_and_data_type(
>>>    * @loaded_image_info:	loaded image protocol
>>>    * Return:		status code
>>>    */
>>> -efi_status_t efi_load_pe(struct efi_loaded_image_obj *handle, void *efi,
>>> +efi_status_t efi_load_pe(struct efi_loaded_image_obj *handle,
>>> +			 void *efi, size_t efi_size,
>>>   			 struct efi_loaded_image *loaded_image_info)
>>>   {
>>>   	IMAGE_NT_HEADERS32 *nt;
>>> @@ -231,17 +595,57 @@ efi_status_t efi_load_pe(struct efi_loaded_image_obj *handle, void *efi,
>>>   	uint64_t image_base;
>>>   	unsigned long virt_size = 0;
>>>   	int supported = 0;
>>> +	void *new_efi = NULL;
>>> +	size_t new_efi_size;
>>> +	efi_status_t ret;
>>> +
>>> +	/*
>>> +	 * Size must be 8-byte aligned and the trailing bytes must be
>>> +	 * zero'ed. Otherwise hash value may be incorrect.
>>> +	 */
>>> +	if (efi_size & 0x7) {
>>> +		new_efi_size = (efi_size + 0x7) & ~0x7ULL;
>>> +		new_efi = calloc(new_efi_size, 1);
>>> +		if (!new_efi)
>>> +			return EFI_OUT_OF_RESOURCES;
>>> +		memcpy(new_efi, efi, efi_size);
>>> +		efi = new_efi;
>>> +		efi_size = new_efi_size;
>>> +	}
>>> +
>>> +	/* Sanity check for a file header */
>>> +	if (efi_size < sizeof(*dos)) {
>>> +		printf("%s: Truncated DOS Header\n", __func__);
>>> +		ret = EFI_LOAD_ERROR;
>>> +		goto err;
>>> +	}
>>>
>>>   	dos = efi;
>>>   	if (dos->e_magic != IMAGE_DOS_SIGNATURE) {
>>>   		printf("%s: Invalid DOS Signature\n", __func__);
>>> -		return EFI_LOAD_ERROR;
>>> +		ret = EFI_LOAD_ERROR;
>>> +		goto err;
>>> +	}
>>> +
>>> +	/* assume sizeof(IMAGE_NT_HEADERS32) <= sizeof(IMAGE_NT_HEADERS64) */
>>> +	if (efi_size < dos->e_lfanew + sizeof(IMAGE_NT_HEADERS32)) {
>>> +		printf("%s: Invalid offset for Extended Header\n", __func__);
>>> +		ret = EFI_LOAD_ERROR;
>>> +		goto err;
>>>   	}
>>>
>>>   	nt = (void *) ((char *)efi + dos->e_lfanew);
>>> +	if ((nt->OptionalHeader.Magic == IMAGE_NT_OPTIONAL_HDR64_MAGIC) &&
>>> +	    (efi_size < dos->e_lfanew + sizeof(IMAGE_NT_HEADERS64))) {
>>> +		printf("%s: Invalid offset for Extended Header\n", __func__);
>>> +		ret = EFI_LOAD_ERROR;
>>> +		goto err;
>>> +	}
>>> +
>>>   	if (nt->Signature != IMAGE_NT_SIGNATURE) {
>>>   		printf("%s: Invalid NT Signature\n", __func__);
>>> -		return EFI_LOAD_ERROR;
>>> +		ret = EFI_LOAD_ERROR;
>>> +		goto err;
>>>   	}
>>>
>>>   	for (i = 0; machines[i]; i++)
>>> @@ -253,14 +657,29 @@ efi_status_t efi_load_pe(struct efi_loaded_image_obj *handle, void *efi,
>>>   	if (!supported) {
>>>   		printf("%s: Machine type 0x%04x is not supported\n",
>>>   		       __func__, nt->FileHeader.Machine);
>>> -		return EFI_LOAD_ERROR;
>>> +		ret = EFI_LOAD_ERROR;
>>> +		goto err;
>>>   	}
>>>
>>> -	/* Calculate upper virtual address boundary */
>>>   	num_sections = nt->FileHeader.NumberOfSections;
>>>   	sections = (void *)&nt->OptionalHeader +
>>>   			    nt->FileHeader.SizeOfOptionalHeader;
>>>
>>> +	if (efi_size < ((void *)sections + sizeof(sections[0]) * num_sections
>>> +			- efi)) {
>>> +		printf("%s: Invalid number of sections: %d\n",
>>> +		       __func__, num_sections);
>>> +		ret = EFI_LOAD_ERROR;
>>> +		goto err;
>>> +	}
>>> +
>>> +	/* Authenticate an image */
>>> +	if (!efi_image_authenticate(efi, efi_size)) {
>>> +		ret = EFI_ACCESS_DENIED;
>>
>> According to the UEFI specification LoadImage() should return
>> EFI_SECURITY_VIOLATION in this case.
>
> Will check.
>
>> Further behaviour should depend on variables AuditMode and DeployedMode.
>>
>> If authentication fails, you must update the configuration table
>> identified by EFI_IMAGE_SECURITY_DATABASE_GUID containing the Image
>> Execution Information. If the authentication succeeds you may enter a
>> record.
>>
>> It seems to me that in your patch series you are not creating the
>> configuration table at all.
>
>>From the very beginning of my submissions, I have clearly said
> that this feature was *beyond the scope* in my current series.
> ===8<===
> Unsupported features: (marked as TODO in most cases in the source code,
>                          and won't be included in this series)
> (snip)
> * recording rejected images in EFI_IMAGE_EXECUTION_INFO_TABLE
> ===>8===
>
>> The content of the Image Execution Information Table should be used to
>> decide if StartImage() may start an image. This is still missing in the
>> patch series.
>
> No.
> Whether such information be in configuration table or not,
> non-authenticated image won't be started if secure boot is in force.

I cannot find any such check in efi_start_image().

Please, provide an implementation that complies with the UEFI specification:

"The information is used to create the Image Execution Information
Table, which is added to the EFI System Configuration Table and assigned
the GUID EFI_IMAGE_SECURITY_DATABASE_GUID."

Best regards

Heinrich

>
> Thanks,
> -Takahiro Akashi
>
>>
>> Best regards
>>
>> Heinrich
>>
>>> +		goto err;
>>> +	}
>>> +
>>> +	/* Calculate upper virtual address boundary */
>>>   	for (i = num_sections - 1; i >= 0; i--) {
>>>   		IMAGE_SECTION_HEADER *sec = &sections[i];
>>>   		virt_size = max_t(unsigned long, virt_size,
>>> @@ -279,7 +698,8 @@ efi_status_t efi_load_pe(struct efi_loaded_image_obj *handle, void *efi,
>>>   		if (!efi_reloc) {
>>>   			printf("%s: Could not allocate %lu bytes\n",
>>>   			       __func__, virt_size);
>>> -			return EFI_OUT_OF_RESOURCES;
>>> +			ret = EFI_OUT_OF_RESOURCES;
>>> +			goto err;
>>>   		}
>>>   		handle->entry = efi_reloc + opt->AddressOfEntryPoint;
>>>   		rel_size = opt->DataDirectory[rel_idx].Size;
>>> @@ -295,7 +715,8 @@ efi_status_t efi_load_pe(struct efi_loaded_image_obj *handle, void *efi,
>>>   		if (!efi_reloc) {
>>>   			printf("%s: Could not allocate %lu bytes\n",
>>>   			       __func__, virt_size);
>>> -			return EFI_OUT_OF_RESOURCES;
>>> +			ret = EFI_OUT_OF_RESOURCES;
>>> +			goto err;
>>>   		}
>>>   		handle->entry = efi_reloc + opt->AddressOfEntryPoint;
>>>   		rel_size = opt->DataDirectory[rel_idx].Size;
>>> @@ -304,13 +725,16 @@ efi_status_t efi_load_pe(struct efi_loaded_image_obj *handle, void *efi,
>>>   	} else {
>>>   		printf("%s: Invalid optional header magic %x\n", __func__,
>>>   		       nt->OptionalHeader.Magic);
>>> -		return EFI_LOAD_ERROR;
>>> +		ret = EFI_LOAD_ERROR;
>>> +		goto err;
>>>   	}
>>>
>>>   	/* Copy PE headers */
>>> -	memcpy(efi_reloc, efi, sizeof(*dos) + sizeof(*nt)
>>> -	       + nt->FileHeader.SizeOfOptionalHeader
>>> -	       + num_sections * sizeof(IMAGE_SECTION_HEADER));
>>> +	memcpy(efi_reloc, efi,
>>> +	       sizeof(*dos)
>>> +		 + sizeof(*nt)
>>> +		 + nt->FileHeader.SizeOfOptionalHeader
>>> +		 + num_sections * sizeof(IMAGE_SECTION_HEADER));
>>>
>>>   	/* Load sections into RAM */
>>>   	for (i = num_sections - 1; i >= 0; i--) {
>>> @@ -327,7 +751,8 @@ efi_status_t efi_load_pe(struct efi_loaded_image_obj *handle, void *efi,
>>>   				(unsigned long)image_base) != EFI_SUCCESS) {
>>>   		efi_free_pages((uintptr_t) efi_reloc,
>>>   			       (virt_size + EFI_PAGE_MASK) >> EFI_PAGE_SHIFT);
>>> -		return EFI_LOAD_ERROR;
>>> +		ret = EFI_LOAD_ERROR;
>>> +		goto err;
>>>   	}
>>>
>>>   	/* Flush cache */
>>> @@ -340,4 +765,9 @@ efi_status_t efi_load_pe(struct efi_loaded_image_obj *handle, void *efi,
>>>   	loaded_image_info->image_size = virt_size;
>>>
>>>   	return EFI_SUCCESS;
>>> +
>>> +err:
>>> +	free(new_efi);
>>> +
>>> +	return ret;
>>>   }
>>>
>>
>



More information about the U-Boot mailing list