]> git.proxmox.com Git - efi-boot-shim.git/blobdiff - pe.c
dbx: generate our own UUID
[efi-boot-shim.git] / pe.c
diff --git a/pe.c b/pe.c
index 9a3679e16a164cea7b09276bda833545afa2e7b4..33056017156d5e7ef2e71038298edd6e202db967 100644 (file)
--- a/pe.c
+++ b/pe.c
 
 #include <Library/BaseCryptLib.h>
 
-/*
- * Perform basic bounds checking of the intra-image pointers
- */
-void *
-ImageAddress (void *image, uint64_t size, uint64_t address)
-{
-       /* ensure our local pointer isn't bigger than our size */
-       if (address > size)
-               return NULL;
-
-       /* Insure our math won't overflow */
-       if (UINT64_MAX - address < (uint64_t)(intptr_t)image)
-               return NULL;
-
-       /* return the absolute pointer */
-       return image + address;
-}
-
-/*
- * Perform the actual relocation
- */
-EFI_STATUS
-relocate_coff (PE_COFF_LOADER_IMAGE_CONTEXT *context,
-              EFI_IMAGE_SECTION_HEADER *Section,
-              void *orig, void *data)
-{
-       EFI_IMAGE_BASE_RELOCATION *RelocBase, *RelocBaseEnd;
-       UINT64 Adjust;
-       UINT16 *Reloc, *RelocEnd;
-       char *Fixup, *FixupBase;
-       UINT16 *Fixup16;
-       UINT32 *Fixup32;
-       UINT64 *Fixup64;
-       int size = context->ImageSize;
-       void *ImageEnd = (char *)orig + size;
-       int n = 0;
-
-       /* Alright, so here's how this works:
-        *
-        * context->RelocDir gives us two things:
-        * - the VA the table of base relocation blocks are (maybe) to be
-        *   mapped at (RelocDir->VirtualAddress)
-        * - the virtual size (RelocDir->Size)
-        *
-        * The .reloc section (Section here) gives us some other things:
-        * - the name! kind of. (Section->Name)
-        * - the virtual size (Section->VirtualSize), which should be the same
-        *   as RelocDir->Size
-        * - the virtual address (Section->VirtualAddress)
-        * - the file section size (Section->SizeOfRawData), which is
-        *   a multiple of OptHdr->FileAlignment.  Only useful for image
-        *   validation, not really useful for iteration bounds.
-        * - the file address (Section->PointerToRawData)
-        * - a bunch of stuff we don't use that's 0 in our binaries usually
-        * - Flags (Section->Characteristics)
-        *
-        * and then the thing that's actually at the file address is an array
-        * of EFI_IMAGE_BASE_RELOCATION structs with some values packed behind
-        * them.  The SizeOfBlock field of this structure includes the
-        * structure itself, and adding it to that structure's address will
-        * yield the next entry in the array.
-        */
-       RelocBase = ImageAddress(orig, size, Section->PointerToRawData);
-       /* RelocBaseEnd here is the address of the first entry /past/ the
-        * table.  */
-       RelocBaseEnd = ImageAddress(orig, size, Section->PointerToRawData +
-                                               Section->Misc.VirtualSize);
-
-       if (!RelocBase && !RelocBaseEnd)
-               return EFI_SUCCESS;
-
-       if (!RelocBase || !RelocBaseEnd) {
-               perror(L"Reloc table overflows binary\n");
-               return EFI_UNSUPPORTED;
-       }
-
-       Adjust = (UINTN)data - context->ImageAddress;
-
-       if (Adjust == 0)
-               return EFI_SUCCESS;
-
-       while (RelocBase < RelocBaseEnd) {
-               Reloc = (UINT16 *) ((char *) RelocBase + sizeof (EFI_IMAGE_BASE_RELOCATION));
-
-               if (RelocBase->SizeOfBlock == 0) {
-                       perror(L"Reloc %d block size 0 is invalid\n", n);
-                       return EFI_UNSUPPORTED;
-               } else if (RelocBase->SizeOfBlock > context->RelocDir->Size) {
-                       perror(L"Reloc %d block size %d greater than reloc dir"
-                                       "size %d, which is invalid\n", n,
-                                       RelocBase->SizeOfBlock,
-                                       context->RelocDir->Size);
-                       return EFI_UNSUPPORTED;
-               }
-
-               RelocEnd = (UINT16 *) ((char *) RelocBase + RelocBase->SizeOfBlock);
-               if ((void *)RelocEnd < orig || (void *)RelocEnd > ImageEnd) {
-                       perror(L"Reloc %d entry overflows binary\n", n);
-                       return EFI_UNSUPPORTED;
-               }
-
-               FixupBase = ImageAddress(data, size, RelocBase->VirtualAddress);
-               if (!FixupBase) {
-                       perror(L"Reloc %d Invalid fixupbase\n", n);
-                       return EFI_UNSUPPORTED;
-               }
-
-               while (Reloc < RelocEnd) {
-                       Fixup = FixupBase + (*Reloc & 0xFFF);
-                       switch ((*Reloc) >> 12) {
-                       case EFI_IMAGE_REL_BASED_ABSOLUTE:
-                               break;
-
-                       case EFI_IMAGE_REL_BASED_HIGH:
-                               Fixup16   = (UINT16 *) Fixup;
-                               *Fixup16 = (UINT16) (*Fixup16 + ((UINT16) ((UINT32) Adjust >> 16)));
-                               break;
-
-                       case EFI_IMAGE_REL_BASED_LOW:
-                               Fixup16   = (UINT16 *) Fixup;
-                               *Fixup16  = (UINT16) (*Fixup16 + (UINT16) Adjust);
-                               break;
-
-                       case EFI_IMAGE_REL_BASED_HIGHLOW:
-                               Fixup32   = (UINT32 *) Fixup;
-                               *Fixup32  = *Fixup32 + (UINT32) Adjust;
-                               break;
-
-                       case EFI_IMAGE_REL_BASED_DIR64:
-                               Fixup64 = (UINT64 *) Fixup;
-                               *Fixup64 = *Fixup64 + (UINT64) Adjust;
-                               break;
-
-                       default:
-                               perror(L"Reloc %d Unknown relocation\n", n);
-                               return EFI_UNSUPPORTED;
-                       }
-                       Reloc += 1;
-               }
-               RelocBase = (EFI_IMAGE_BASE_RELOCATION *) RelocEnd;
-               n++;
-       }
-
-       return EFI_SUCCESS;
-}
-
 #define check_size_line(data, datasize_in, hashbase, hashsize, l) ({   \
        if ((unsigned long)hashbase >                                   \
                        (unsigned long)data + datasize_in) {            \
@@ -185,113 +39,6 @@ relocate_coff (PE_COFF_LOADER_IMAGE_CONTEXT *context,
 })
 #define check_size(d, ds, h, hs) check_size_line(d, ds, h, hs, __LINE__)
 
-EFI_STATUS
-get_section_vma (UINTN section_num,
-                char *buffer, size_t bufsz UNUSED,
-                PE_COFF_LOADER_IMAGE_CONTEXT *context,
-                char **basep, size_t *sizep,
-                EFI_IMAGE_SECTION_HEADER **sectionp)
-{
-       EFI_IMAGE_SECTION_HEADER *sections = context->FirstSection;
-       EFI_IMAGE_SECTION_HEADER *section;
-       char *base = NULL, *end = NULL;
-
-       if (section_num >= context->NumberOfSections)
-               return EFI_NOT_FOUND;
-
-       if (context->FirstSection == NULL) {
-               perror(L"Invalid section %d requested\n", section_num);
-               return EFI_UNSUPPORTED;
-       }
-
-       section = &sections[section_num];
-
-       base = ImageAddress (buffer, context->ImageSize, section->VirtualAddress);
-       end = ImageAddress (buffer, context->ImageSize,
-                           section->VirtualAddress + section->Misc.VirtualSize - 1);
-
-       if (!(section->Characteristics & EFI_IMAGE_SCN_MEM_DISCARDABLE)) {
-               if (!base) {
-                       perror(L"Section %d has invalid base address\n", section_num);
-                       return EFI_UNSUPPORTED;
-               }
-               if (!end) {
-                       perror(L"Section %d has zero size\n", section_num);
-                       return EFI_UNSUPPORTED;
-               }
-       }
-
-       if (!(section->Characteristics & EFI_IMAGE_SCN_CNT_UNINITIALIZED_DATA) &&
-           (section->VirtualAddress < context->SizeOfHeaders ||
-            section->PointerToRawData < context->SizeOfHeaders)) {
-               perror(L"Section %d is inside image headers\n", section_num);
-               return EFI_UNSUPPORTED;
-       }
-
-       if (end < base) {
-               perror(L"Section %d has negative size\n", section_num);
-               return EFI_UNSUPPORTED;
-       }
-
-       *basep = base;
-       *sizep = end - base;
-       *sectionp = section;
-       return EFI_SUCCESS;
-}
-
-EFI_STATUS
-get_section_vma_by_name (char *name, size_t namesz,
-                        char *buffer, size_t bufsz,
-                        PE_COFF_LOADER_IMAGE_CONTEXT *context,
-                        char **basep, size_t *sizep,
-                        EFI_IMAGE_SECTION_HEADER **sectionp)
-{
-       UINTN i;
-       char namebuf[9];
-
-       if (!name || namesz == 0 || !buffer || bufsz < namesz || !context
-           || !basep || !sizep || !sectionp)
-               return EFI_INVALID_PARAMETER;
-
-       /*
-        * This code currently is only used for ".reloc\0\0" and
-        * ".sbat\0\0\0", and it doesn't know how to look up longer section
-        * names.
-        */
-       if (namesz > 8)
-               return EFI_UNSUPPORTED;
-
-       SetMem(namebuf, sizeof(namebuf), 0);
-       CopyMem(namebuf, name, MIN(namesz, 8));
-
-       /*
-        * Copy the executable's sections to their desired offsets
-        */
-       for (i = 0; i < context->NumberOfSections; i++) {
-               EFI_STATUS status;
-               EFI_IMAGE_SECTION_HEADER *section = NULL;
-               char *base = NULL;
-               size_t size = 0;
-
-               status = get_section_vma(i, buffer, bufsz, context, &base, &size, &section);
-               if (!EFI_ERROR(status)) {
-                       if (CompareMem(section->Name, namebuf, 8) == 0) {
-                               *basep = base;
-                               *sizep = size;
-                               *sectionp = section;
-                               return EFI_SUCCESS;
-                       }
-                       continue;
-               }
-
-               switch(status) {
-               case EFI_NOT_FOUND:
-                       break;
-               }
-       }
-
-       return EFI_NOT_FOUND;
-}
 
 /*
  * Calculate the SHA1 and SHA256 hashes of a binary
@@ -585,249 +332,6 @@ done:
        return efi_status;
 }
 
-/* here's a chart:
- *             i686    x86_64  aarch64
- *  64-on-64:  nyet    yes     yes
- *  64-on-32:  nyet    yes     nyet
- *  32-on-32:  yes     yes     no
- */
-static int
-allow_64_bit(void)
-{
-#if defined(__x86_64__) || defined(__aarch64__)
-       return 1;
-#elif defined(__i386__) || defined(__i686__)
-       /* Right now blindly assuming the kernel will correctly detect this
-        * and /halt the system/ if you're not really on a 64-bit cpu */
-       if (in_protocol)
-               return 1;
-       return 0;
-#else /* assuming everything else is 32-bit... */
-       return 0;
-#endif
-}
-
-static int
-allow_32_bit(void)
-{
-#if defined(__x86_64__)
-#if defined(ALLOW_32BIT_KERNEL_ON_X64)
-       if (in_protocol)
-               return 1;
-       return 0;
-#else
-       return 0;
-#endif
-#elif defined(__i386__) || defined(__i686__)
-       return 1;
-#elif defined(__aarch64__)
-       return 0;
-#else /* assuming everything else is 32-bit... */
-       return 1;
-#endif
-}
-
-static int
-image_is_64_bit(EFI_IMAGE_OPTIONAL_HEADER_UNION *PEHdr)
-{
-       /* .Magic is the same offset in all cases */
-       if (PEHdr->Pe32Plus.OptionalHeader.Magic
-                       == EFI_IMAGE_NT_OPTIONAL_HDR64_MAGIC)
-               return 1;
-       return 0;
-}
-
-static const UINT16 machine_type =
-#if defined(__x86_64__)
-       IMAGE_FILE_MACHINE_X64;
-#elif defined(__aarch64__)
-       IMAGE_FILE_MACHINE_ARM64;
-#elif defined(__arm__)
-       IMAGE_FILE_MACHINE_ARMTHUMB_MIXED;
-#elif defined(__i386__) || defined(__i486__) || defined(__i686__)
-       IMAGE_FILE_MACHINE_I386;
-#elif defined(__ia64__)
-       IMAGE_FILE_MACHINE_IA64;
-#else
-#error this architecture is not supported by shim
-#endif
-
-static int
-image_is_loadable(EFI_IMAGE_OPTIONAL_HEADER_UNION *PEHdr)
-{
-       /* If the machine type doesn't match the binary, bail, unless
-        * we're in an allowed 64-on-32 scenario */
-       if (PEHdr->Pe32.FileHeader.Machine != machine_type) {
-               if (!(machine_type == IMAGE_FILE_MACHINE_I386 &&
-                     PEHdr->Pe32.FileHeader.Machine == IMAGE_FILE_MACHINE_X64 &&
-                     allow_64_bit())) {
-                       return 0;
-               }
-       }
-
-       /* If it's not a header type we recognize at all, bail */
-       switch (PEHdr->Pe32Plus.OptionalHeader.Magic) {
-       case EFI_IMAGE_NT_OPTIONAL_HDR64_MAGIC:
-       case EFI_IMAGE_NT_OPTIONAL_HDR32_MAGIC:
-               break;
-       default:
-               return 0;
-       }
-
-       /* and now just check for general 64-vs-32 compatibility */
-       if (image_is_64_bit(PEHdr)) {
-               if (allow_64_bit())
-                       return 1;
-       } else {
-               if (allow_32_bit())
-                       return 1;
-       }
-       return 0;
-}
-
-/*
- * Read the binary header and grab appropriate information from it
- */
-EFI_STATUS
-read_header(void *data, unsigned int datasize,
-           PE_COFF_LOADER_IMAGE_CONTEXT *context)
-{
-       EFI_IMAGE_DOS_HEADER *DosHdr = data;
-       EFI_IMAGE_OPTIONAL_HEADER_UNION *PEHdr = data;
-       unsigned long HeaderWithoutDataDir, SectionHeaderOffset, OptHeaderSize;
-       unsigned long FileAlignment = 0;
-       UINT16 DllFlags;
-
-       if (datasize < sizeof (PEHdr->Pe32)) {
-               perror(L"Invalid image\n");
-               return EFI_UNSUPPORTED;
-       }
-
-       if (DosHdr->e_magic == EFI_IMAGE_DOS_SIGNATURE)
-               PEHdr = (EFI_IMAGE_OPTIONAL_HEADER_UNION *)((char *)data + DosHdr->e_lfanew);
-
-       if (!image_is_loadable(PEHdr)) {
-               perror(L"Platform does not support this image\n");
-               return EFI_UNSUPPORTED;
-       }
-
-       if (image_is_64_bit(PEHdr)) {
-               context->NumberOfRvaAndSizes = PEHdr->Pe32Plus.OptionalHeader.NumberOfRvaAndSizes;
-               context->SizeOfHeaders = PEHdr->Pe32Plus.OptionalHeader.SizeOfHeaders;
-               context->ImageSize = PEHdr->Pe32Plus.OptionalHeader.SizeOfImage;
-               context->SectionAlignment = PEHdr->Pe32Plus.OptionalHeader.SectionAlignment;
-               FileAlignment = PEHdr->Pe32Plus.OptionalHeader.FileAlignment;
-               OptHeaderSize = sizeof(EFI_IMAGE_OPTIONAL_HEADER64);
-       } else {
-               context->NumberOfRvaAndSizes = PEHdr->Pe32.OptionalHeader.NumberOfRvaAndSizes;
-               context->SizeOfHeaders = PEHdr->Pe32.OptionalHeader.SizeOfHeaders;
-               context->ImageSize = (UINT64)PEHdr->Pe32.OptionalHeader.SizeOfImage;
-               context->SectionAlignment = PEHdr->Pe32.OptionalHeader.SectionAlignment;
-               FileAlignment = PEHdr->Pe32.OptionalHeader.FileAlignment;
-               OptHeaderSize = sizeof(EFI_IMAGE_OPTIONAL_HEADER32);
-       }
-
-       if (FileAlignment % 2 != 0) {
-               perror(L"File Alignment is invalid (%d)\n", FileAlignment);
-               return EFI_UNSUPPORTED;
-       }
-       if (FileAlignment == 0)
-               FileAlignment = 0x200;
-       if (context->SectionAlignment == 0)
-               context->SectionAlignment = PAGE_SIZE;
-       if (context->SectionAlignment < FileAlignment)
-               context->SectionAlignment = FileAlignment;
-
-       context->NumberOfSections = PEHdr->Pe32.FileHeader.NumberOfSections;
-
-       if (EFI_IMAGE_NUMBER_OF_DIRECTORY_ENTRIES < context->NumberOfRvaAndSizes) {
-               perror(L"Image header too small\n");
-               return EFI_UNSUPPORTED;
-       }
-
-       HeaderWithoutDataDir = OptHeaderSize
-                       - sizeof (EFI_IMAGE_DATA_DIRECTORY) * EFI_IMAGE_NUMBER_OF_DIRECTORY_ENTRIES;
-       if (((UINT32)PEHdr->Pe32.FileHeader.SizeOfOptionalHeader - HeaderWithoutDataDir) !=
-                       context->NumberOfRvaAndSizes * sizeof (EFI_IMAGE_DATA_DIRECTORY)) {
-               perror(L"Image header overflows data directory\n");
-               return EFI_UNSUPPORTED;
-       }
-
-       SectionHeaderOffset = DosHdr->e_lfanew
-                               + sizeof (UINT32)
-                               + sizeof (EFI_IMAGE_FILE_HEADER)
-                               + PEHdr->Pe32.FileHeader.SizeOfOptionalHeader;
-       if (((UINT32)context->ImageSize - SectionHeaderOffset) / EFI_IMAGE_SIZEOF_SECTION_HEADER
-                       <= context->NumberOfSections) {
-               perror(L"Image sections overflow image size\n");
-               return EFI_UNSUPPORTED;
-       }
-
-       if ((context->SizeOfHeaders - SectionHeaderOffset) / EFI_IMAGE_SIZEOF_SECTION_HEADER
-                       < (UINT32)context->NumberOfSections) {
-               perror(L"Image sections overflow section headers\n");
-               return EFI_UNSUPPORTED;
-       }
-
-       if ((((UINT8 *)PEHdr - (UINT8 *)data) + sizeof(EFI_IMAGE_OPTIONAL_HEADER_UNION)) > datasize) {
-               perror(L"Invalid image\n");
-               return EFI_UNSUPPORTED;
-       }
-
-       if (PEHdr->Te.Signature != EFI_IMAGE_NT_SIGNATURE) {
-               perror(L"Unsupported image type\n");
-               return EFI_UNSUPPORTED;
-       }
-
-       if (PEHdr->Pe32.FileHeader.Characteristics & EFI_IMAGE_FILE_RELOCS_STRIPPED) {
-               perror(L"Unsupported image - Relocations have been stripped\n");
-               return EFI_UNSUPPORTED;
-       }
-
-       context->PEHdr = PEHdr;
-
-       if (image_is_64_bit(PEHdr)) {
-               context->ImageAddress = PEHdr->Pe32Plus.OptionalHeader.ImageBase;
-               context->EntryPoint = PEHdr->Pe32Plus.OptionalHeader.AddressOfEntryPoint;
-               context->RelocDir = &PEHdr->Pe32Plus.OptionalHeader.DataDirectory[EFI_IMAGE_DIRECTORY_ENTRY_BASERELOC];
-               context->SecDir = &PEHdr->Pe32Plus.OptionalHeader.DataDirectory[EFI_IMAGE_DIRECTORY_ENTRY_SECURITY];
-               DllFlags = PEHdr->Pe32Plus.OptionalHeader.DllCharacteristics;
-       } else {
-               context->ImageAddress = PEHdr->Pe32.OptionalHeader.ImageBase;
-               context->EntryPoint = PEHdr->Pe32.OptionalHeader.AddressOfEntryPoint;
-               context->RelocDir = &PEHdr->Pe32.OptionalHeader.DataDirectory[EFI_IMAGE_DIRECTORY_ENTRY_BASERELOC];
-               context->SecDir = &PEHdr->Pe32.OptionalHeader.DataDirectory[EFI_IMAGE_DIRECTORY_ENTRY_SECURITY];
-               DllFlags = PEHdr->Pe32.OptionalHeader.DllCharacteristics;
-       }
-
-       if ((mok_policy & MOK_POLICY_REQUIRE_NX) &&
-           !(DllFlags & EFI_IMAGE_DLLCHARACTERISTICS_NX_COMPAT)) {
-               perror(L"Policy requires NX, but image does not support NX\n");
-               return EFI_UNSUPPORTED;
-        }
-
-       context->FirstSection = (EFI_IMAGE_SECTION_HEADER *)((char *)PEHdr + PEHdr->Pe32.FileHeader.SizeOfOptionalHeader + sizeof(UINT32) + sizeof(EFI_IMAGE_FILE_HEADER));
-
-       if (context->ImageSize < context->SizeOfHeaders) {
-               perror(L"Invalid image\n");
-               return EFI_UNSUPPORTED;
-       }
-
-       if ((unsigned long)((UINT8 *)context->SecDir - (UINT8 *)data) >
-           (datasize - sizeof(EFI_IMAGE_DATA_DIRECTORY))) {
-               perror(L"Invalid image\n");
-               return EFI_UNSUPPORTED;
-       }
-
-       if (context->SecDir->VirtualAddress > datasize ||
-           (context->SecDir->VirtualAddress == datasize &&
-            context->SecDir->Size > 0)) {
-               perror(L"Malformed security header\n");
-               return EFI_INVALID_PARAMETER;
-       }
-       return EFI_SUCCESS;
-}
-
 EFI_STATUS
 verify_sbat_section(char *SBATBase, size_t SBATSize)
 {
@@ -851,7 +355,11 @@ verify_sbat_section(char *SBATBase, size_t SBATSize)
                return in_protocol ? EFI_SUCCESS : EFI_SECURITY_VIOLATION;
        }
 
-       sbat_size = SBATSize + 1;
+       if (checked_add(SBATSize, 1, &sbat_size)) {
+               dprint(L"SBATSize + 1 would overflow\n");
+               return EFI_SECURITY_VIOLATION;
+       }
+
        sbat_data = AllocatePool(sbat_size);
        if (!sbat_data) {
                console_print(L"Failed to allocate .sbat section buffer\n");
@@ -937,7 +445,7 @@ get_mem_attrs (uintptr_t addr, size_t size, uint64_t *attrs)
        if (EFI_ERROR(efi_status) || !proto)
                return efi_status;
 
-       if (physaddr & 0xfff || size & 0xfff || size == 0 || attrs == NULL) {
+       if (!IS_PAGE_ALIGNED(physaddr) || !IS_PAGE_ALIGNED(size) || size == 0 || attrs == NULL) {
                dprint(L"%a called on 0x%llx-0x%llx and attrs 0x%llx\n",
                       __func__, (unsigned long long)physaddr,
                       (unsigned long long)(physaddr+size-1),
@@ -971,7 +479,7 @@ update_mem_attrs(uintptr_t addr, uint64_t size,
                       (unsigned long long)addr, (unsigned long long)size,
                       &before, efi_status);
 
-       if (physaddr & 0xfff || size & 0xfff || size == 0) {
+       if (!IS_PAGE_ALIGNED(physaddr) || !IS_PAGE_ALIGNED(size) || size == 0) {
                dprint(L"%a called on 0x%llx-0x%llx (size 0x%llx) +%a%a%a -%a%a%a\n",
                       __func__, (unsigned long long)physaddr,
                       (unsigned long long)(physaddr + size - 1),
@@ -990,10 +498,20 @@ update_mem_attrs(uintptr_t addr, uint64_t size,
        uefi_clear_attrs = shim_mem_attrs_to_uefi_mem_attrs (clear_attrs);
        dprint("translating clear_attrs from 0x%lx to 0x%lx\n", clear_attrs, uefi_clear_attrs);
        efi_status = EFI_SUCCESS;
-       if (uefi_set_attrs)
+       if (uefi_set_attrs) {
                efi_status = proto->SetMemoryAttributes(proto, physaddr, size, uefi_set_attrs);
-       if (!EFI_ERROR(efi_status) && uefi_clear_attrs)
+               if (EFI_ERROR(efi_status)) {
+                       dprint(L"Failed to set memory attrs:0x%0x physaddr:0x%llx size:0x%0lx status:%r\n",
+                               uefi_set_attrs, physaddr, size, efi_status);
+               }
+       }
+       if (!EFI_ERROR(efi_status) && uefi_clear_attrs) {
                efi_status = proto->ClearMemoryAttributes(proto, physaddr, size, uefi_clear_attrs);
+               if (EFI_ERROR(efi_status)) {
+                       dprint(L"Failed to clear memory attrs:0x%0x physaddr:0x%llx size:0x%0lx status:%r\n",
+                               uefi_clear_attrs, physaddr, size, efi_status);
+               }
+       }
        ret = efi_status;
 
        efi_status = get_mem_attrs (addr, size, &after);
@@ -1243,6 +761,7 @@ handle_image (void *data, unsigned int datasize,
                    (Section->Characteristics & EFI_IMAGE_SCN_MEM_EXECUTE) &&
                    (mok_policy & MOK_POLICY_REQUIRE_NX)) {
                        perror(L"Section %d is writable and executable\n", i);
+                       BS->FreePages(*alloc_address, *alloc_pages);
                        return EFI_UNSUPPORTED;
                }
 
@@ -1268,6 +787,7 @@ handle_image (void *data, unsigned int datasize,
                if (CompareMem(Section->Name, ".reloc\0\0", 8) == 0) {
                        if (RelocSection) {
                                perror(L"Image has multiple relocation sections\n");
+                               BS->FreePages(*alloc_address, *alloc_pages);
                                return EFI_UNSUPPORTED;
                        }
                        /* If it has nonzero sizes, and our bounds check
@@ -1277,8 +797,12 @@ handle_image (void *data, unsigned int datasize,
                                        Section->Misc.VirtualSize &&
                                        base && end &&
                                        RelocBase == base &&
-                                       RelocBaseEnd == end) {
+                                       RelocBaseEnd <= end) {
                                RelocSection = Section;
+                       } else {
+                               perror(L"Relocation section is invalid \n");
+                               BS->FreePages(*alloc_address, *alloc_pages);
+                               return EFI_UNSUPPORTED;
                        }
                }
 
@@ -1288,10 +812,12 @@ handle_image (void *data, unsigned int datasize,
 
                if (!base) {
                        perror(L"Section %d has invalid base address\n", i);
+                       BS->FreePages(*alloc_address, *alloc_pages);
                        return EFI_UNSUPPORTED;
                }
                if (!end) {
                        perror(L"Section %d has zero size\n", i);
+                       BS->FreePages(*alloc_address, *alloc_pages);
                        return EFI_UNSUPPORTED;
                }
 
@@ -1299,6 +825,7 @@ handle_image (void *data, unsigned int datasize,
                    (Section->VirtualAddress < context.SizeOfHeaders ||
                     Section->PointerToRawData < context.SizeOfHeaders)) {
                        perror(L"Section %d is inside image headers\n", i);
+                       BS->FreePages(*alloc_address, *alloc_pages);
                        return EFI_UNSUPPORTED;
                }
 
@@ -1307,6 +834,7 @@ handle_image (void *data, unsigned int datasize,
                } else {
                        if (Section->PointerToRawData < context.SizeOfHeaders) {
                                perror(L"Section %d is inside image headers\n", i);
+                               BS->FreePages(*alloc_address, *alloc_pages);
                                return EFI_UNSUPPORTED;
                        }
 
@@ -1324,7 +852,7 @@ handle_image (void *data, unsigned int datasize,
 
        if (context.NumberOfRvaAndSizes <= EFI_IMAGE_DIRECTORY_ENTRY_BASERELOC) {
                perror(L"Image has no relocation entry\n");
-               FreePool(buffer);
+               BS->FreePages(*alloc_address, *alloc_pages);
                return EFI_UNSUPPORTED;
        }
 
@@ -1337,7 +865,7 @@ handle_image (void *data, unsigned int datasize,
 
                if (EFI_ERROR(efi_status)) {
                        perror(L"Relocation failed: %r\n", efi_status);
-                       FreePool(buffer);
+                       BS->FreePages(*alloc_address, *alloc_pages);
                        return efi_status;
                }
        }
@@ -1372,7 +900,11 @@ handle_image (void *data, unsigned int datasize,
                                     + Section->Misc.VirtualSize - 1);
 
                addr = (uintptr_t)base;
-               length = (uintptr_t)end - (uintptr_t)base + 1;
+               // Align the length up to PAGE_SIZE. This is required because
+               // platforms generally set memory attributes at page
+               // granularity, but the section length (unlike the section
+               // address) is not required to be aligned.
+               length = ALIGN_VALUE((uintptr_t)end - (uintptr_t)base + 1, PAGE_SIZE);
 
                if (Section->Characteristics & EFI_IMAGE_SCN_MEM_WRITE) {
                        set_attrs |= MEM_ATTR_W;
@@ -1399,10 +931,12 @@ handle_image (void *data, unsigned int datasize,
 
        if (!found_entry_point) {
                perror(L"Entry point is not within sections\n");
+               BS->FreePages(*alloc_address, *alloc_pages);
                return EFI_UNSUPPORTED;
        }
        if (found_entry_point > 1) {
                perror(L"%d sections contain entry point\n", found_entry_point);
+               BS->FreePages(*alloc_address, *alloc_pages);
                return EFI_UNSUPPORTED;
        }