[PATCH v2] arm64/efi: don't pad between EFI_MEMORY_RUNTIME regions
From: mark.rutland@arm.com (Mark Rutland)
Date: 2015-09-10 15:03:00
Also in:
linux-efi
quoted hunk ↗ jump to hunk
OK so what we could do is the following: ------------8<--------------diff --git a/arch/arm64/kernel/efi.c b/arch/arm64/kernel/efi.c index e8ca6eaedd02..39fa2a70a7f1 100644 --- a/arch/arm64/kernel/efi.c +++ b/arch/arm64/kernel/efi.c@@ -233,6 +233,7 @@ void __init efi_init(void) static bool __init efi_virtmap_init(void) { efi_memory_desc_t *md; + u64 prev_end = 0; for_each_efi_memory_desc(&memmap, md) { u64 paddr, npages, size;@@ -256,13 +257,26 @@ static bool __init efi_virtmap_init(void) * executable, everything else can be mapped with the XN bits * set. */ - if (!is_normal_ram(md)) + if (!is_normal_ram(md)) { prot = __pgprot(PROT_DEVICE_nGnRE); - else if (md->type == EFI_RUNTIME_SERVICES_CODE) + } else if (md->type == EFI_RUNTIME_SERVICES_CODE) { prot = PAGE_KERNEL_EXEC; - else + } else { + /* + * If we are running with >4 KB pages and the current + * region shares a page frame with the preceding one, + * we should not map the leading page again since doing + * so may take its executable permissions away. + */ + if (PAGE_SIZE > EFI_PAGE_SIZE && paddr < prev_end) { + paddr += PAGE_SIZE; + size -= PAGE_SIZE; + if (!size) + continue; + } prot = PAGE_KERNEL; - + } + prev_end = paddr + size; create_pgd_mapping(&efi_mm, paddr, md->virt_addr, size, prot); } return true; ------------8<--------------This will ensure that only the pages that are shared between 2 or more regions may have their permissions upgraded, but only if any of these regions requires it. I prefer the much simpler previous version, though, and I think it is more suitable for -stable. I can always follow up with an improvement like this for v4.3-late.
Ok. Let's go with your previous version for now. Could you put something in the commit message about this limitation, so that we don't forget?
quoted
quoted
quoted
quoted
else prot = PAGE_KERNEL;diff --git a/drivers/firmware/efi/libstub/arm-stub.c b/drivers/firmware/efi/libstub/arm-stub.c index e29560e6b40b..cb4e9c4de952 100644 --- a/drivers/firmware/efi/libstub/arm-stub.c +++ b/drivers/firmware/efi/libstub/arm-stub.c@@ -13,6 +13,7 @@ */ #include <linux/efi.h> +#include <linux/sort.h>Sort isn't an inline in this header. I thought it wasn't safe to call arbitary kernel functions from the stub?We call string functions, cache maintenance functions, libfdt functions etc etc so it seems not everyone got the memo :-) I agree that treating vmlinux both as a static library and as a payload from the stub's pov is a bit sloppy, and I do remember discussing this, but for the life of me, I can't remember the exact issue, other than the use of adrp/add and adrp/ldr pairs, which we fixed by setting the PE/COFF section alignment to 4 KB.I only had a vague recollection that there was a problem, which I thought was more to do with potential use of absolute kernel virtual addresses, which would be incorrect in the context of an EFI application.That was it, of course. Unlike the x86 stub, which is built with -fPIC (as is the ARM decompressor, btw), the arm64 kernel is position dependent. Fortunately, the small code model is mostly position independent by default, but it would be good if we could spot any problems at build time.quoted
Digging a bit, the stub code itself is safe due to commit f4f75ad5741fe033 ("efi: efistub: Convert into static library"), but thatlibstub is linked into vmlinux so that does not make a different at all
Ok. I assumed that inter-stub references would still be relative, but I have no idea what the linker would do.
quoted
We do seem to be ok so far, however. Maybe we just need to keep an eye out.I'd much rather restrict the code that goes into the stub somehow than deal with any absolute references. Perhaps we could reuse some of the section mismatch code in some way to tag certain code as stub-safe and do a verification pass on the binary.
It would be great if we could, though I'm not really sure how that would work. Imagine stub_foo calls fdt_blah, which calls kern_baz by absolute address. Normally the latter call is fine, but not when the original call comes from the stub. I don't think the section mismatch code has any way to contextualize calls like that, and I don't know what criteria we could use for annotating code. Let's not have that delay this patch, however. That's a bigger can of worms. Thanks, Mark.