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