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] = &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;
> >  }
> >
> 

Reply via email to