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.aka...@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] = §ions[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] = §ions[i]; > > >>>>>+ } else { > > >>>>>+ if (i == 1) { > > >>>>>+ sorted[1] = sorted[0]; > > >>>>>+ sorted[0] = §ions[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] = §ions[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, ®s, &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 = §ions[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; > > >>>>> } > > >>>>> > > >>>> > > >>> > > >> > > > > >