On Thu, Nov 12, 2015 at 03:40:22PM +0000, Matt Fleming wrote:
> With commit e1a58320a38d ("x86/mm: Warn on W^X mappings") all users
> booting on 64-bit UEFI machines see the following warning,
>
> ------------[ cut here ]------------
> WARNING: CPU: 7 PID: 1 at arch/x86/mm/dump_pagetables.c:225 note_page+0x5dc/0x780()
> x86/mm: Found insecure W+X mapping at address ffff88000005f000/0xffff88000005f000
> ...
> x86/mm: Checked W+X mappings: FAILED, 165660 W+X pages found.
> ...
>
> This is caused by mapping EFI regions with RWX permissions. There
> isn't much we can do to restrict the permissions for these regions due
> to the way the firmware toolchains mix code and data, but we can at
> least isolate these mappings so that they do not appear in the regular
> kernel page tables.
>
> In commit d2f7cbe7b26a ("x86/efi: Runtime services virtual mapping")
> we started using 'trampoline_pgd' to map the EFI regions because there
> was an existing identity mapping there which we use during the
> SetVirtualAddressMap() call and for broken firmware that accesses
> those addresses.
>
> But 'trampoline_pgd' shares some PGD entries with 'swapper_pg_dir' and
> does not provide the isolation we require. Notaby the virtual address
Notably
> for __START_KERNEL_map and MODULES_START are mapped by the same PGD
> entry so we need to be more careful when copying changes over in
> efi_sync_low_kernel_mappings().
>
> This patch doesn't go the full mile, we still want to share some PGD
> entries with 'swapper_pg_dir'. Having completely separate page tables
> brings its own issues such as sychronising new mappings after memory
> hotplug and module loading. Sharing also keeps memory usage down.
Cool idea!
...
> @@ -831,6 +831,12 @@ static void __init __efi_enter_virtual_mode(void)
> efi.systab = NULL;
>
> efi_merge_regions();
> + if (efi_alloc_page_tables()) {
> + pr_err("Failed to allocate EFI page tables\n");
> + clear_bit(EFI_RUNTIME_SERVICES, &efi.flags);
> + return;
> + }
This should happen before efi_merge_regions() - no need to merge if we
can't alloc PGT.
> +
> new_memmap = efi_map_regions(&count, &pg_shift);
> if (!new_memmap) {
> pr_err("Error reallocating memory, EFI runtime non-functional!\n");
> @@ -888,29 +894,12 @@ static void __init __efi_enter_virtual_mode(void)
>
> efi_runtime_mkexec();
>
> - /*
> - * We mapped the descriptor array into the EFI pagetable above but we're
> - * not unmapping it here. Here's why:
> - *
> - * We're copying select PGDs from the kernel page table to the EFI page
> - * table and when we do so and make changes to those PGDs like unmapping
> - * stuff from them, those changes appear in the kernel page table and we
> - * go boom.
> - *
> - * From setup_real_mode():
> - *
> - * ...
> - * trampoline_pgd[0] = init_level4_pgt[pgd_index(__PAGE_OFFSET)].pgd;
> - *
> - * In this particular case, our allocation is in PGD 0 of the EFI page
> - * table but we've copied that PGD from PGD[272] of the EFI page table:
> - *
> - * pgd_index(__PAGE_OFFSET = 0xffff880000000000) = 272
> - *
> - * where the direct memory mapping in kernel space is.
> - *
> - * new_memmap's VA comes from that direct mapping and thus clearing it,
> - * it would get cleared in the kernel page table too.
> + /*
ERROR: code indent should use tabs where possible
#149: FILE: arch/x86/platform/efi/efi.c:897:
+ /*$
...
> void efi_sync_low_kernel_mappings(void)
> {
> - unsigned num_pgds;
> - pgd_t *pgd = (pgd_t *)__va(real_mode_header->trampoline_pgd);
> + unsigned num_entries;
> + pgd_t *pgd_k, *pgd_efi;
> + pud_t *pud_k, *pud_efi;
>
> if (efi_enabled(EFI_OLD_MEMMAP))
> return;
>
> - num_pgds = pgd_index(MODULES_END - 1) - pgd_index(PAGE_OFFSET);
> + /*
> + * We can share all PGD entries apart from the one entry that
> + * covers the EFI runtime mapping space.
> + *
> + * Make sure the EFI runtime region mappings are guaranteed to
> + * only span a single PGD entry and that the entry also maps
> + * other important kernel regions.
> + */
> + BUILD_BUG_ON(pgd_index(EFI_VA_END) != pgd_index(MODULES_END));
> + BUILD_BUG_ON((EFI_VA_START & PGDIR_MASK) !=
> + (EFI_VA_END & PGDIR_MASK));
You can align them in a more readable way:
BUILD_BUG_ON((EFI_VA_START & PGDIR_MASK) !=
(EFI_VA_END & PGDIR_MASK));
> +
> + pgd_efi = efi_pgd + pgd_index(PAGE_OFFSET);
> + pgd_k = pgd_offset_k(PAGE_OFFSET);
> +
> + num_entries = pgd_index(EFI_VA_END) - pgd_index(PAGE_OFFSET);
> + memcpy(pgd_efi, pgd_k, sizeof(pgd_t) * num_entries);
>
> - memcpy(pgd + pgd_index(PAGE_OFFSET),
> - init_mm.pgd + pgd_index(PAGE_OFFSET),
> - sizeof(pgd_t) * num_pgds);
> + /*
> + * We share all the PUD entries apart from those that map the
> + * EFI regions. Copy around them.
> + */
> + BUILD_BUG_ON((EFI_VA_START & ~PUD_MASK) != 0);
> + BUILD_BUG_ON((EFI_VA_END & ~PUD_MASK) != 0);
> +
> + pgd_efi = efi_pgd + pgd_index(EFI_VA_END);
> + pud_efi = pud_offset(pgd_efi, 0);
> +
> + pgd_k = pgd_offset_k(EFI_VA_END);
> + pud_k = pud_offset(pgd_k, 0);
> +
> + num_entries = pud_index(EFI_VA_END);
> + memcpy(pud_efi, pud_k, sizeof(pud_t) * num_entries);
> +
> + pud_efi = pud_offset(pgd_efi, EFI_VA_START);
> + pud_k = pud_offset(pgd_k, EFI_VA_START);
> +
> + num_entries = PTRS_PER_PUD - pud_index(EFI_VA_START);
> + memcpy(pud_efi, pud_k, sizeof(pud_t) * num_entries);
> }
--
Regards/Gruss,
Boris.
ECO tip #101: Trim your mails when you reply.
--
To unsubscribe from this list: send the line "unsubscribe linux-efi" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
