[edk2] [RFC PATCH] MdeModulePkg: account for disjoint code and data regions in runtime relocations

Discussion:

Ard Biesheuvel

2015-07-02 12:31:24 UTC

Hello all,

This is a proof of concept patch that fixes the problems that occur when
enabling the EFI_PROPERTIES_RUNTIME_MEMORY_PROTECTION_NON_EXECUTABLE_PE_DATA
MemoryProtectionAttribute, which may split PE/COFF memory images into disjoint
regions in virtual memory.

It is not a complete solution, but it works both on AARCH64 (ArmVirtQemu) and
X64 (Ovmf). With the 4 KB alignment patch and Laszlo's end-of-DXE series applied,
I can boot into the Linux kernel (Ubuntu 3.13.0-55-generic) without problems,
while the memory map reveals that the splitting has in fact occurred.

Note that this may require 64 KB section alignment for non-ELF based toolchains,
if they may potentially emit EFI_IMAGE_REL_BASED_LOW or EFI_IMAGE_REL_BASED_HIGH
relocations. For toolchains that rely on GenFw for the ELF to PE/COFF
conversion, this is not required.

------------------8<----------------------

When reapplying PE/COFF relocations for the switch to virtual mode, we
should not assume that the virtual memory image is identical to the
PE/COFF file image, since PE/COFF may no longer be adjacent in virtual
memory.

So instead of using a fixed adjustment derived from the virtual address of
ImageBase, apply ConvertPointer () to each relocation target to obtain the
new value.

Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Ard Biesheuvel <***@linaro.org>
---
MdeModulePkg/Core/RuntimeDxe/Runtime.c | 229 +++++++++++++++++++-
1 file changed, 227 insertions(+), 2 deletions(-)

diff --git a/MdeModulePkg/Core/RuntimeDxe/Runtime.c b/MdeModulePkg/Core/RuntimeDxe/Runtime.c
index c61301cf80d8..c8dbfdd5a3ef 100644
--- a/MdeModulePkg/Core/RuntimeDxe/Runtime.c
+++ b/MdeModulePkg/Core/RuntimeDxe/Runtime.c
@@ -213,6 +213,232 @@ RuntimeDriverConvertInternalPointer (
}

/**
+ Reapply fixups on a fixed up PE32/PE32+ image to allow virutal calling at EFI
+ runtime.
+
+ This function reapplies relocation fixups to the PE/COFF image specified by ImageBase
+ and ImageSize so the image will execute correctly when the PE/COFF image is mapped
+ to the address specified by VirtualImageBase. RelocationData must be identical
+ to the FixupData buffer from the PE_COFF_LOADER_IMAGE_CONTEXT structure
+ after this PE/COFF image was relocated with BasePecoffLib::PeCoffLoaderRelocateImage().
+
+ Note that if the platform does not maintain coherency between the instruction cache(s) and the data
+ cache(s) in hardware, then the caller is responsible for performing cache maintenance operations
+ prior to transferring control to a PE/COFF image that is loaded using this funation.
+
+ @param ImageBase The base address of a PE/COFF image that has been loaded
+ and relocated into system memory.
+ @param ImageSize The size, in bytes, of the PE/COFF image.
+ @param RelocationData A pointer to the relocation data that was collected when the PE/COFF
+ image was relocated using PeCoffLoaderRelocateImage().
+
+ @return EFI_SUCCESS The fixups have been reapplied successfully.
+
+ @return EFI_UNSUPPORTED ImageBase does not point to a valid PE image
+ @return EFI_UNSUPPORTED No reloc directory found or contents invalid
+ @return EFI_UNSUPPORTED Image uses high/low relocations and section alignment < 64 KB
+
+**/
+STATIC
+EFI_STATUS
+EFIAPI
+RuntimeDriverRelocateImageForRuntime (
+ IN PHYSICAL_ADDRESS ImageBase,
+ IN UINTN ImageSize,
+ IN VOID *RelocationData
+ )
+{
+ CHAR8 *OldBase;
+ EFI_IMAGE_DOS_HEADER *DosHdr;
+ EFI_IMAGE_OPTIONAL_HEADER_PTR_UNION Hdr;
+ UINT32 NumberOfRvaAndSizes;
+ EFI_IMAGE_DATA_DIRECTORY *DataDirectory;
+ EFI_IMAGE_DATA_DIRECTORY *RelocDir;
+ EFI_IMAGE_BASE_RELOCATION *RelocBase;
+ EFI_IMAGE_BASE_RELOCATION *RelocBaseEnd;
+ UINT16 *Reloc;
+ UINT16 *RelocEnd;
+ CHAR8 *Fixup;
+ CHAR8 *FixupBase;
+ UINT16 *Fixup16;
+ UINT32 *Fixup32;
+ UINT64 *Fixup64;
+ CHAR8 *FixupData;
+ UINT16 Magic;
+ UINTN ConvertAddress;
+
+ OldBase = (CHAR8 *)((UINTN)ImageBase);
+
+ //
+ // Find the image's relocate dir info
+ //
+ DosHdr = (EFI_IMAGE_DOS_HEADER *)OldBase;
+ if (DosHdr->e_magic == EFI_IMAGE_DOS_SIGNATURE) {
+ //
+ // Valid DOS header so get address of PE header
+ //
+ Hdr.Pe32 = (EFI_IMAGE_NT_HEADERS32 *)(((CHAR8 *)DosHdr) + DosHdr->e_lfanew);
+ } else {
+ //
+ // No Dos header so assume image starts with PE header.
+ //
+ Hdr.Pe32 = (EFI_IMAGE_NT_HEADERS32 *)OldBase;
+ }
+
+ if (Hdr.Pe32->Signature != EFI_IMAGE_NT_SIGNATURE) {
+ //
+ // Not a valid PE image so Exit
+ //
+ return EFI_UNSUPPORTED;
+ }
+
+ Magic = Hdr.Pe32->OptionalHeader.Magic;
+
+ if (Magic == EFI_IMAGE_NT_OPTIONAL_HDR32_MAGIC) {
+ //
+ // Use PE32 offset
+ //
+ NumberOfRvaAndSizes = Hdr.Pe32->OptionalHeader.NumberOfRvaAndSizes;
+ DataDirectory = (EFI_IMAGE_DATA_DIRECTORY *)&(Hdr.Pe32->OptionalHeader.DataDirectory[0]);
+ } else {
+ //
+ // Use PE32+ offset
+ //
+ NumberOfRvaAndSizes = Hdr.Pe32Plus->OptionalHeader.NumberOfRvaAndSizes;
+ DataDirectory = (EFI_IMAGE_DATA_DIRECTORY *)&(Hdr.Pe32Plus->OptionalHeader.DataDirectory[0]);
+ }
+
+ //
+ // Find the relocation block
+ //
+ // Per the PE/COFF spec, you can't assume that a given data directory
+ // is present in the image. You have to check the NumberOfRvaAndSizes in
+ // the optional header to verify a desired directory entry is there.
+ //
+ if (NumberOfRvaAndSizes > EFI_IMAGE_DIRECTORY_ENTRY_BASERELOC) {
+ RelocDir = DataDirectory + EFI_IMAGE_DIRECTORY_ENTRY_BASERELOC;
+ RelocBase = (EFI_IMAGE_BASE_RELOCATION *)(UINTN)(ImageBase + RelocDir->VirtualAddress);
+ RelocBaseEnd = (EFI_IMAGE_BASE_RELOCATION *)(UINTN)(ImageBase + RelocDir->VirtualAddress + RelocDir->Size);
+ } else {
+ //
+ // Cannot find relocations, cannot continue to relocate the image, ASSERT for this invalid image.
+ //
+ ASSERT (FALSE);
+ return EFI_UNSUPPORTED;
+ }
+
+ //
+ // ASSERT for the invalid image when RelocBase and RelocBaseEnd are both NULL.
+ //
+ ASSERT (RelocBase != NULL && RelocBaseEnd != NULL);
+
+ //
+ // Run the whole relocation block. And re-fixup data that has not been
+ // modified. The FixupData is used to see if the image has been modified
+ // since it was relocated. This is so data sections that have been updated
+ // by code will not be fixed up, since that would set them back to
+ // defaults.
+ //
+ FixupData = RelocationData;
+ while (RelocBase < RelocBaseEnd) {
+ //
+ // Add check for RelocBase->SizeOfBlock field.
+ //
+ if ((RelocBase->SizeOfBlock == 0) || (RelocBase->SizeOfBlock > RelocDir->Size)) {
+ //
+ // Data invalid, cannot continue to relocate the image, just return.
+ //
+ return EFI_UNSUPPORTED;
+ }
+
+ Reloc = (UINT16 *) ((UINT8 *) RelocBase + sizeof (EFI_IMAGE_BASE_RELOCATION));
+ RelocEnd = (UINT16 *) ((UINT8 *) RelocBase + RelocBase->SizeOfBlock);
+ FixupBase = (CHAR8 *) ((UINTN)ImageBase) + RelocBase->VirtualAddress;
+
+ //
+ // Run this relocation record
+ //
+ while (Reloc < RelocEnd) {
+
+ Fixup = FixupBase + (*Reloc & 0xFFF);
+ switch ((*Reloc) >> 12) {
+
+ case EFI_IMAGE_REL_BASED_ABSOLUTE:
+ break;
+
+ case EFI_IMAGE_REL_BASED_HIGH:
+ //
+ // In order to be able to apply relocations to code and data regions
+ // that may be disjoint in the virtual mapping, we disallow relocations
+ // of type EFI_IMAGE_REL_BASED_HIGH or EFI_IMAGE_REL_BASED_LOW, unless
+ // the section alignment is at least 64 KB.
+ //
+ if (Hdr.Pe32->OptionalHeader.SectionAlignment < SIZE_64KB) {
+ return EFI_UNSUPPORTED;
+ }
+ Fixup16 = (UINT16 *) Fixup;
+ if (*(UINT16 *) FixupData == *Fixup16) {
+ ConvertAddress = (UINTN) *Fixup16 << 16;
+ RuntimeDriverConvertInternalPointer ((VOID **) &ConvertAddress);
+ *Fixup16 = (UINT16) (ConvertAddress >> 16);
+ }
+
+ FixupData = FixupData + sizeof (UINT16);
+ break;
+
+ case EFI_IMAGE_REL_BASED_LOW:
+ if (Hdr.Pe32->OptionalHeader.SectionAlignment < SIZE_64KB) {
+ return EFI_UNSUPPORTED;
+ }
+ //
+ // Since the adjustment is guaranteed to be a multiple of 64 KB,
+ // there is nothing to do here.
+ //
+ break;
+
+ case EFI_IMAGE_REL_BASED_HIGHLOW:
+ Fixup32 = (UINT32 *) Fixup;
+ FixupData = ALIGN_POINTER (FixupData, sizeof (UINT32));
+ if (*(UINT32 *) FixupData == *Fixup32) {
+ ConvertAddress = (UINTN) *Fixup32;
+ RuntimeDriverConvertInternalPointer ((VOID **) &ConvertAddress);
+ *Fixup32 = (UINT32) ConvertAddress;
+ }
+
+ FixupData = FixupData + sizeof (UINT32);
+ break;
+
+ case EFI_IMAGE_REL_BASED_DIR64:
+ Fixup64 = (UINT64 *)Fixup;
+ FixupData = ALIGN_POINTER (FixupData, sizeof (UINT64));
+ if (*(UINT64 *) FixupData == *Fixup64) {
+ ConvertAddress = (UINTN) *Fixup64;
+ RuntimeDriverConvertInternalPointer ((VOID **) &ConvertAddress);
+ *Fixup64 = (UINT64) ConvertAddress;
+ }
+
+ FixupData = FixupData + sizeof (UINT64);
+ break;
+
+ default:
+ ASSERT (FALSE);
+ }
+ //
+ // Next relocation record
+ //
+ Reloc += 1;
+ }
+ //
+ // next reloc block
+ //
+ RelocBase = (EFI_IMAGE_BASE_RELOCATION *) RelocEnd;
+ }
+
+ return EFI_SUCCESS;
+}
+
+
+/**

Changes the runtime addressing mode of EFI firmware from physical to virtual.

@@ -312,9 +538,8 @@ RuntimeDriverSetVirtualAddressMap (
Status = RuntimeDriverConvertPointer (0, (VOID **) &VirtImageBase);
ASSERT_EFI_ERROR (Status);

- PeCoffLoaderRelocateImageForRuntime (
+ RuntimeDriverRelocateImageForRuntime (
(EFI_PHYSICAL_ADDRESS) (UINTN) RuntimeImage->ImageBase,
- VirtImageBase,
(UINTN) RuntimeImage->ImageSize,
RuntimeImage->RelocationData
);

--
1.9.1

Matt Fleming

2015-07-09 14:57:35 UTC

Permalink

Post by Ard Biesheuvel
Hello all,
This is a proof of concept patch that fixes the problems that occur when
enabling the EFI_PROPERTIES_RUNTIME_MEMORY_PROTECTION_NON_EXECUTABLE_PE_DATA
MemoryProtectionAttribute, which may split PE/COFF memory images into disjoint
regions in virtual memory.
It is not a complete solution, but it works both on AARCH64 (ArmVirtQemu) and
X64 (Ovmf). With the 4 KB alignment patch and Laszlo's end-of-DXE series applied,
I can boot into the Linux kernel (Ubuntu 3.13.0-55-generic) without problems,
while the memory map reveals that the splitting has in fact occurred.
Note that this may require 64 KB section alignment for non-ELF based toolchains,
if they may potentially emit EFI_IMAGE_REL_BASED_LOW or EFI_IMAGE_REL_BASED_HIGH
relocations. For toolchains that rely on GenFw for the ELF to PE/COFF
conversion, this is not required.

Cool! I'd quite like to test this patch out but I'm not sure I've got
all of the required patch dependencies in my tree.

Could you shove everything into some public git repo so I can run it
through the LUV autobuilder?

--
Matt Fleming, Intel Open Source Technology Center

Ard Biesheuvel

2015-07-09 15:58:36 UTC

Permalink

Post by Matt Fleming

Cool! I'd quite like to test this patch out but I'm not sure I've got
all of the required patch dependencies in my tree.
Could you shove everything into some public git repo so I can run it
through the LUV autobuilder?

Sure. In the meantime, I have cooked up a slightly more elaborate
series that also fixes the
EFI_IMAGE_REL_BASED_LOW/EFI_IMAGE_REL_BASED_HIGH issues (even though
they don't seem to be used widely) and does some cleanups. This is
unlikely to be adopted as-is, since there is still an unresolved issue
with inter-region relative relocations.

Tree is here
https://git.linaro.org/people/ard.biesheuvel/uefi-next.git/shortlog/refs/heads/pecoff-runtime-relocation

When running this, you should notice regions in the memory map with
either the RO (0x20000) or XP (0x4000) bit set in the kernel log.
For instance,

"""
[ 0.000000] efi: mem38: type=6, attr=0x800000000000400f,
range=[0x0000000007e9a000-0x0000000007e9f000) (0MB)
[ 0.000000] efi: mem39: type=5, attr=0x800000000002000f,
range=[0x0000000007e9f000-0x0000000007ea4000) (0MB)
[ 0.000000] efi: mem40: type=6, attr=0x800000000000400f,
range=[0x0000000007ea4000-0x0000000007eaa000) (0MB)
[ 0.000000] efi: mem41: type=5, attr=0x800000000002000f,
range=[0x0000000007eaa000-0x0000000007eae000) (0MB)
"""

And no crash .... (hopefully)

--
Ard.

Matt Fleming

2015-07-10 13:53:11 UTC

Permalink

Post by Ard Biesheuvel
Sure. In the meantime, I have cooked up a slightly more elaborate
series that also fixes the
EFI_IMAGE_REL_BASED_LOW/EFI_IMAGE_REL_BASED_HIGH issues (even though
they don't seem to be used widely) and does some cleanups. This is
unlikely to be adopted as-is, since there is still an unresolved issue
with inter-region relative relocations.
Tree is here
https://git.linaro.org/people/ard.biesheuvel/uefi-next.git/shortlog/refs/heads/pecoff-runtime-relocation

Thanks Ard! This is really, super helpful.

Post by Ard Biesheuvel
When running this, you should notice regions in the memory map with
either the RO (0x20000) or XP (0x4000) bit set in the kernel log.
For instance,
"""
[ 0.000000] efi: mem38: type=6, attr=0x800000000000400f, range=[0x0000000007e9a000-0x0000000007e9f000) (0MB)
[ 0.000000] efi: mem39: type=5, attr=0x800000000002000f, range=[0x0000000007e9f000-0x0000000007ea4000) (0MB)
[ 0.000000] efi: mem40: type=6, attr=0x800000000000400f, range=[0x0000000007ea4000-0x0000000007eaa000) (0MB)
[ 0.000000] efi: mem41: type=5, attr=0x800000000002000f, range=[0x0000000007eaa000-0x0000000007eae000) (0MB)
"""

Yep, I see this and it looks good.

Post by Ard Biesheuvel
And no crash .... (hopefully)

With a standard kernel, I don't see a crash. However, with the minimal
top 2 patches in,

https://git.kernel.org/cgit/linux/kernel/git/mfleming/efi.git/log/?h=memmap

I see a page fault while executing SetVirtualAddressMap(). It appears to
be caused by the firmware performing relocation fixups on an address
that's mapped as EfiRuntimeServicesCode, which is in the EFI memory map
as EFI_MEMORY_RO.

I suspect you don't see this issue on aarch64 because you call
SetVirtualAddressMap() so early in boot.

I'll try and find some time to dig into this issue this coming week, but
if anyone wants to beat me to it, feel free ;-)

The first question is: Why is the image's reloc section in a
EfiRuntimeServiceCode region?

--
Matt Fleming, Intel Open Source Technology Center

Ard Biesheuvel

2015-07-10 13:57:52 UTC

Permalink

Post by Matt Fleming

Thanks Ard! This is really, super helpful.

Sure, no problem.

Post by Matt Fleming

Yep, I see this and it looks good.

Post by Ard Biesheuvel
And no crash .... (hopefully)

Indeed.

Post by Matt Fleming
I'll try and find some time to dig into this issue this coming week, but
if anyone wants to beat me to it, feel free ;-)
The first question is: Why is the image's reloc section in a
EfiRuntimeServiceCode region?

Are you sure it is the .reloc section itself? It could well be the
target of a relocation fixup that is inside a code region, which
cannot be applied due to the page permissions.

--
Ard.

Matt Fleming

2015-07-10 14:14:05 UTC

Permalink

Post by Ard Biesheuvel
Are you sure it is the .reloc section itself? It could well be the
target of a relocation fixup that is inside a code region, which
cannot be applied due to the page permissions.

Oh yes, that would make more sense. I was under the impression that the
runtime drivers were built as position independent executables (I don't
know why I thought that). Whoops!

OK, I've got more kernel work to do then. Looks like we have to first
call SetVirtualAddressMap() and *then* apply the more restrictive page
table permissions.

I wonder whether this will have a noticeable effect on boot time.

--
Matt Fleming, Intel Open Source Technology Center

Ard Biesheuvel

2015-07-10 14:23:58 UTC

Permalink

Post by Matt Fleming

Oh yes, that would make more sense. I was under the impression that the
runtime drivers were built as position independent executables (I don't
know why I thought that). Whoops!
OK, I've got more kernel work to do then. Looks like we have to first
call SetVirtualAddressMap() and *then* apply the more restrictive page
table permissions.

Is there a reason to set strict permissions at all on the 1:1 mapping?
Does it stick around after having called SVAM () ?

My (naive) suggestion would be to apply the strict permissions only to
the virtual remapping of the runtime services, but perhaps it doesn't
work like that on x86.

--
Ard.

Post by Matt Fleming
I wonder whether this will have a noticeable effect on boot time.
--
Matt Fleming, Intel Open Source Technology Center

Matt Fleming

2015-07-16 13:57:23 UTC

Permalink

Post by Ard Biesheuvel
Is there a reason to set strict permissions at all on the 1:1 mapping?
Does it stick around after having called SVAM () ?
My (naive) suggestion would be to apply the strict permissions only to
the virtual remapping of the runtime services, but perhaps it doesn't
work like that on x86.

Yeah, that was my intention. The problem is that we need to inform
SetVirtualAddressMap() where those virtual mappings live before we
enforce the strict permissions (the layout is not static).

So for x86 we effectively need to do,

virt_map_efi_regions(map);

set_virtual_address_map(map);

virt_map_efi_regions_harder(map);

With all the resultant TLB flushing that's involved with rewriting the
page table bits.

Anyway, I've got something working now that correctly detects if the
firmware writes to a read-only runtime region or executes an NX region.
I'd just like to do a bit of performance analysis on it first.

--
Matt Fleming, Intel Open Source Technology Center