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

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@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(-)

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@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.

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

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.

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.

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

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@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.

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

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

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@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;
>>>   }
>>>
>>
>

On Fri, Jan 17, 2020 at 06:51:50AM +0100, Heinrich Schuchardt wrote:
> 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@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().

Because my current code *rejects* any unauthenticated images being loaded
and return no valid pointer to image handle.
If an error code in this case is changed and efi_load_image() still returns
a valid handle, we will also have to modify 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."

Regarding "Image Execution Information Table", Heavily disagree.
The current UEFI code, not only mine but yours, has bunch of unimplemented
features.

-Takahiro Akashi


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

On 1/21/20 7:12 AM, AKASHI Takahiro wrote:
> On Fri, Jan 17, 2020 at 06:51:50AM +0100, Heinrich Schuchardt wrote:
>> 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@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().
>
> Because my current code *rejects* any unauthenticated images being loaded
> and return no valid pointer to image handle.
> If an error code in this case is changed and efi_load_image() still returns
> a valid handle, we will also have to modify 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."
>
> Regarding "Image Execution Information Table", Heavily disagree.
> The current UEFI code, not only mine but yours, has bunch of unimplemented
> features.

What is the point of this patch if efi_start_image() does not provide
any check?

Best regards

Heinrich

>
> -Takahiro Akashi
>
>
>> 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;
>>>>>   }
>>>>>
>>>>
>>>
>>
>

On Tue, Jan 21, 2020 at 08:15:06AM +0100, Heinrich Schuchardt wrote:
> On 1/21/20 7:12 AM, AKASHI Takahiro wrote:
> >On Fri, Jan 17, 2020 at 06:51:50AM +0100, Heinrich Schuchardt wrote:
> >>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@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().
> >
> >Because my current code *rejects* any unauthenticated images being loaded
> >and return no valid pointer to image handle.
> >If an error code in this case is changed and efi_load_image() still returns
> >a valid handle, we will also have to modify 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."
> >
> >Regarding "Image Execution Information Table", Heavily disagree.
> >The current UEFI code, not only mine but yours, has bunch of unimplemented
> >features.
> 
> What is the point of this patch if efi_start_image() does not provide
> any check?

As I said in the previous reply,
efi_load_image() doesn't return any valid handle for unauthenticated binaries
in my *current* implementation, efi_start_image() has no chance to execute them.
No check is necessary. That's it.

-Takahiro Akashi

> Best regards
> 
> Heinrich
> 
> >
> >-Takahiro Akashi
> >
> >
> >>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;
> >>>>>  }
> >>>>>
> >>>>
> >>>
> >>
> >
>

Heinrich,

On Wed, Jan 22, 2020 at 10:13:48AM +0900, AKASHI Takahiro wrote:
> On Tue, Jan 21, 2020 at 08:15:06AM +0100, Heinrich Schuchardt wrote:
> > On 1/21/20 7:12 AM, AKASHI Takahiro wrote:
> > >On Fri, Jan 17, 2020 at 06:51:50AM +0100, Heinrich Schuchardt wrote:
> > >>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@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().
> > >
> > >Because my current code *rejects* any unauthenticated images being loaded
> > >and return no valid pointer to image handle.
> > >If an error code in this case is changed and efi_load_image() still returns
> > >a valid handle, we will also have to modify 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."
> > >
> > >Regarding "Image Execution Information Table", Heavily disagree.
> > >The current UEFI code, not only mine but yours, has bunch of unimplemented
> > >features.
> > 
> > What is the point of this patch if efi_start_image() does not provide
> > any check?
> 
> As I said in the previous reply,
> efi_load_image() doesn't return any valid handle for unauthenticated binaries
> in my *current* implementation, efi_start_image() has no chance to execute them.
> No check is necessary. That's it.

I double-checked edk2 code as well as UEFI specification, and found
a couple of insights:

a. EDK2 code has several internal help functions for verifications.
   If they fail to find any valid signature in db's, the status (or
   internal error code) is set to ACCESS_DENIED.
   Then, at the end of verification, if the status is not EFI_SUCCESS,
   the return code is anyhow rewritten to SECURITY_VIOLATION.
   (That is why my code returns ACCESS_DENIED right now.)
b. While UEFI specification requires that efiLoadImage() return
   SECURITY_VIOLATION if "the image signature is not valid,"
   it doesn't mention if a handle to the image be returned or not.
c. "Status Codes Returned" can also read that it depends on
   "platform policy" if we return ACCESS_DENIED or SECURITY_VIOLATION.
   But the policy may be vendor/platform, or even U-Boot specific as
   UEFI specification doesn't mention anything about that.

Thinking of the fact that we don't have any consensus nor implementation
of "policy" yet, I believe that the best solution for now is:
   efi_load_image() return EFI_SECURITY_VIOLATION if the image signature
   is not verified and does *not* return a handle to image.

This behavior is safe and yet won't prevent us from additionally implementing
"policy" framework as well as image information table when adding
Audit/DeployedMode support in the future.
Can you agree?

-Takahiro Akashi


> -Takahiro Akashi
> 
> > Best regards
> > 
> > Heinrich
> > 
> > >
> > >-Takahiro Akashi
> > >
> > >
> > >>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;
> > >>>>>  }
> > >>>>>
> > >>>>
> > >>>
> > >>
> > >
> >

On 1/22/20 8:42 AM, AKASHI Takahiro wrote:
> Heinrich,
>
> On Wed, Jan 22, 2020 at 10:13:48AM +0900, AKASHI Takahiro wrote:
>> On Tue, Jan 21, 2020 at 08:15:06AM +0100, Heinrich Schuchardt wrote:
>>> On 1/21/20 7:12 AM, AKASHI Takahiro wrote:
>>>> On Fri, Jan 17, 2020 at 06:51:50AM +0100, Heinrich Schuchardt wrote:
>>>>> 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@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().
>>>>
>>>> Because my current code *rejects* any unauthenticated images being loaded
>>>> and return no valid pointer to image handle.
>>>> If an error code in this case is changed and efi_load_image() still returns
>>>> a valid handle, we will also have to modify 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."
>>>>
>>>> Regarding "Image Execution Information Table", Heavily disagree.
>>>> The current UEFI code, not only mine but yours, has bunch of unimplemented
>>>> features.
>>>
>>> What is the point of this patch if efi_start_image() does not provide
>>> any check?
>>
>> As I said in the previous reply,
>> efi_load_image() doesn't return any valid handle for unauthenticated binaries
>> in my *current* implementation, efi_start_image() has no chance to execute them.
>> No check is necessary. That's it.
>
> I double-checked edk2 code as well as UEFI specification, and found
> a couple of insights:
>
> a. EDK2 code has several internal help functions for verifications.
>     If they fail to find any valid signature in db's, the status (or
>     internal error code) is set to ACCESS_DENIED.
>     Then, at the end of verification, if the status is not EFI_SUCCESS,
>     the return code is anyhow rewritten to SECURITY_VIOLATION.
>     (That is why my code returns ACCESS_DENIED right now.)
> b. While UEFI specification requires that efiLoadImage() return
>     SECURITY_VIOLATION if "the image signature is not valid,"
>     it doesn't mention if a handle to the image be returned or not.

Please, have look at the return values for LoadImage() in the UEFI 2.8 spec.

If you return ACCESS_DENIED, <cite>Image was not loaded because the
platform policy prohibits the image from being loaded. NULL is returned
in *ImageHandle</cite>

If you return SECURITY_VIOLATION, <cite>Image was loaded and an
ImageHandle was created with a valid EFI_LOADED_IMAGE_PROTOCOL.</cite>

In the case of SECURITY_VIOLATION we have to ensure that
efi_start_image() does not start the image. This is why we need the
EFI_IMAGE_SECURITY_DATABASE.

> c. "Status Codes Returned" can also read that it depends on
>     "platform policy" if we return ACCESS_DENIED or SECURITY_VIOLATION.
>     But the policy may be vendor/platform, or even U-Boot specific as
>     UEFI specification doesn't mention anything about that.
>
> Thinking of the fact that we don't have any consensus nor implementation
> of "policy" yet, I believe that the best solution for now is:
>     efi_load_image() return EFI_SECURITY_VIOLATION if the image signature
>     is not verified and does *not* return a handle to image.

This would violate the UEFI spec.

Best regards

Heinrich

>
> This behavior is safe and yet won't prevent us from additionally implementing
> "policy" framework as well as image information table when adding
> Audit/DeployedMode support in the future.
> Can you agree?
>
> -Takahiro Akashi
>
>
>> -Takahiro Akashi
>>
>>> Best regards
>>>
>>> Heinrich
>>>
>>>>
>>>> -Takahiro Akashi
>>>>
>>>>
>>>>> 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;
>>>>>>>>   }
>>>>>>>>
>>>>>>>
>>>>>>
>>>>>
>>>>
>>>
>

On Thu, Jan 23, 2020 at 06:41:53PM +0100, Heinrich Schuchardt wrote:
> On 1/22/20 8:42 AM, AKASHI Takahiro wrote:
> >Heinrich,
> >
> >On Wed, Jan 22, 2020 at 10:13:48AM +0900, AKASHI Takahiro wrote:
> >>On Tue, Jan 21, 2020 at 08:15:06AM +0100, Heinrich Schuchardt wrote:
> >>>On 1/21/20 7:12 AM, AKASHI Takahiro wrote:
> >>>>On Fri, Jan 17, 2020 at 06:51:50AM +0100, Heinrich Schuchardt wrote:
> >>>>>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@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().
> >>>>
> >>>>Because my current code *rejects* any unauthenticated images being loaded
> >>>>and return no valid pointer to image handle.
> >>>>If an error code in this case is changed and efi_load_image() still returns
> >>>>a valid handle, we will also have to modify 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."
> >>>>
> >>>>Regarding "Image Execution Information Table", Heavily disagree.
> >>>>The current UEFI code, not only mine but yours, has bunch of unimplemented
> >>>>features.
> >>>
> >>>What is the point of this patch if efi_start_image() does not provide
> >>>any check?
> >>
> >>As I said in the previous reply,
> >>efi_load_image() doesn't return any valid handle for unauthenticated binaries
> >>in my *current* implementation, efi_start_image() has no chance to execute them.
> >>No check is necessary. That's it.
> >
> >I double-checked edk2 code as well as UEFI specification, and found
> >a couple of insights:
> >
> >a. EDK2 code has several internal help functions for verifications.
> >    If they fail to find any valid signature in db's, the status (or
> >    internal error code) is set to ACCESS_DENIED.
> >    Then, at the end of verification, if the status is not EFI_SUCCESS,
> >    the return code is anyhow rewritten to SECURITY_VIOLATION.
> >    (That is why my code returns ACCESS_DENIED right now.)
> >b. While UEFI specification requires that efiLoadImage() return
> >    SECURITY_VIOLATION if "the image signature is not valid,"
> >    it doesn't mention if a handle to the image be returned or not.
> 
> Please, have look at the return values for LoadImage() in the UEFI 2.8 spec.
> 
> If you return ACCESS_DENIED, <cite>Image was not loaded because the
> platform policy prohibits the image from being loaded. NULL is returned
> in *ImageHandle</cite>
> 
> If you return SECURITY_VIOLATION, <cite>Image was loaded and an
> ImageHandle was created with a valid EFI_LOADED_IMAGE_PROTOCOL.</cite>
> 
> In the case of SECURITY_VIOLATION we have to ensure that
> efi_start_image() does not start the image.

Okay.

> This is why we need the
> EFI_IMAGE_SECURITY_DATABASE.

No. Disagree.
Whether we support Image Execution Information Table or not,
we should and can prevent the image from being executed.
It's just a matter of implementation of efi_start_image().

The information is nothing but for UEFI applications/drivers
like other tables in configuration table.

-Takahiro Akashi


> >c. "Status Codes Returned" can also read that it depends on
> >    "platform policy" if we return ACCESS_DENIED or SECURITY_VIOLATION.
> >    But the policy may be vendor/platform, or even U-Boot specific as
> >    UEFI specification doesn't mention anything about that.
> >
> >Thinking of the fact that we don't have any consensus nor implementation
> >of "policy" yet, I believe that the best solution for now is:
> >    efi_load_image() return EFI_SECURITY_VIOLATION if the image signature
> >    is not verified and does *not* return a handle to image.
> 
> This would violate the UEFI spec.
> 
> Best regards
> 
> Heinrich
> 
> >
> >This behavior is safe and yet won't prevent us from additionally implementing
> >"policy" framework as well as image information table when adding
> >Audit/DeployedMode support in the future.
> >Can you agree?
> >
> >-Takahiro Akashi
> >
> >
> >>-Takahiro Akashi
> >>
> >>>Best regards
> >>>
> >>>Heinrich
> >>>
> >>>>
> >>>>-Takahiro Akashi
> >>>>
> >>>>
> >>>>>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;
> >>>>>>>>  }
> >>>>>>>>
> >>>>>>>
> >>>>>>
> >>>>>
> >>>>
> >>>
> >
>