| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Thu, 6 Jul 2017 17:36:58 +0800
From: Chao Fan <fanc.fnst@...fujitsu.com>
To: Naoya Horiguchi <n-horiguchi@...jp.nec.com>
CC: Baoquan He <bhe@...hat.com>, Kees Cook <keescook@...omium.org>,
LKML <linux-kernel@...r.kernel.org>,
"x86@...nel.org" <x86@...nel.org>,
Thomas Gleixner <tglx@...utronix.de>,
"H. Peter Anvin" <hpa@...or.com>, Ingo Molnar <mingo@...nel.org>,
"izumi.taku@...fujitsu.com" <izumi.taku@...fujitsu.com>,
Thomas Garnier <thgarnie@...gle.com>,
Matt Fleming <matt@...eblueprint.co.uk>,
Junichi Nomura <j-nomura@...jp.nec.com>
Subject: Re: [PATCH] x86/boot/KASLR: exclude EFI_BOOT_SERVICES_{CODE|DATA}
from KASLR's choice
On Thu, Jul 06, 2017 at 09:22:38AM +0000, Naoya Horiguchi wrote:
>On Thu, Jul 06, 2017 at 05:13:32PM +0800, Chao Fan wrote:
>> On Thu, Jul 06, 2017 at 08:31:07AM +0000, Naoya Horiguchi wrote:
>> >Hi Baoquan, everyone,
>> >
>> >I'm also interested in KASLR/EFI related issue (but not the same issue
>> >with yours, so I separated the thread.)
>> >
>> >This patch is based on Baoquan's recent patches[1], adding more code
>> >on the new function process_efi_entry().
>> >If it's OK, could you queue this onto your tree/series?
>> >
>> >[1] "[PATCH v3 0/2] x86/boot/KASLR: Restrict kernel to be randomized"
>> > https://lkml.org/lkml/2017/7/5/98
>> >
>> >Thanks,
>> >Naoya Horiguchi
>> >---
>> >From: Naoya Horiguchi <n-horiguchi@...jp.nec.com>
>> >Date: Thu, 6 Jul 2017 16:40:52 +0900
>> >Subject: [PATCH] x86/boot/KASLR: exclude EFI_BOOT_SERVICES_{CODE|DATA} from
>> > KASLR's choice
>> >
>> >KASLR chooses kernel location from E820_TYPE_RAM regions by walking over
>> >e820 entries now. E820_TYPE_RAM includes EFI_BOOT_SERVICES_CODE and
>> >EFI_BOOT_SERVICES_DATA, so those regions can be the target. According to
>> >UEFI spec, all memory regions marked as EfiBootServicesCode and
>> >EfiBootServicesData are available for free memory after the first call
>> >of ExitBootServices(). So such regions should be usable for kernel on
>> >spec basis.
>> >
>> >In x86, however, we have some workaround for broken firmware, where we
>> >keep such regions reserved until SetVirtualAddressMap() is done.
>> >See the following code in should_map_region():
>> >
>> > static bool should_map_region(efi_memory_desc_t *md)
>> > {
>> > ...
>> > /*
>> > * Map boot services regions as a workaround for buggy
>> > * firmware that accesses them even when they shouldn't.
>> > *
>> > * See efi_{reserve,free}_boot_services().
>> > */
>> > if (md->type == EFI_BOOT_SERVICES_CODE ||
>> > md->type == EFI_BOOT_SERVICES_DATA)
>> > return false;
>> >
>> >This workaround suppressed a boot crash, but potential issues still
>> >remain because no one prevents the regions from overlapping with kernel
>> >image by KASLR.
>> >
>> >So let's make sure that EFI_BOOT_SERVICES_{CODE|DATA} regions are never
>> >chosen as kernel memory for the workaround to work fine.
>> >
>> >Signed-off-by: Naoya Horiguchi <n-horiguchi@...jp.nec.com>
>> >---
>> > arch/x86/boot/compressed/kaslr.c | 41 +++++++++++++++++++++++++++++++---------
>> > 1 file changed, 32 insertions(+), 9 deletions(-)
>> >
>> >diff --git a/arch/x86/boot/compressed/kaslr.c b/arch/x86/boot/compressed/kaslr.c
>> >index 94f08fd375ae..f43fed0441a6 100644
>> >--- a/arch/x86/boot/compressed/kaslr.c
>> >+++ b/arch/x86/boot/compressed/kaslr.c
>> >@@ -563,7 +563,8 @@ static void process_mem_region(struct mem_vector *entry,
>> > /* Marks if efi mirror regions have been found and handled. */
>> > static bool efi_mirror_found;
>> >
>> >-static void process_efi_entry(unsigned long minimum, unsigned long image_size)
>> >+/* Returns true if we really enter efi memmap walk, otherwise returns false. */
>> >+static bool process_efi_entry(unsigned long minimum, unsigned long image_size)
>> > {
>> > struct efi_info *e = &boot_params->efi_info;
>> > struct mem_vector region;
>> >@@ -577,13 +578,13 @@ static void process_efi_entry(unsigned long minimum, unsigned long image_size)
>> > signature = (char *)&boot_params->efi_info.efi_loader_signature;
>> > if (strncmp(signature, EFI32_LOADER_SIGNATURE, 4) &&
>> > strncmp(signature, EFI64_LOADER_SIGNATURE, 4))
>> >- return;
>> >+ return false;
>> >
>> > #ifdef CONFIG_X86_32
>> > /* Can't handle data above 4GB at this time */
>> > if (e->efi_memmap_hi) {
>> > warn("Memory map is above 4GB, EFI should be disabled.\n");
>> >- return;
>> >+ return false;
>> > }
>> > pmap = e->efi_memmap;
>> > #else
>> >@@ -593,13 +594,36 @@ static void process_efi_entry(unsigned long minimum, unsigned long image_size)
>> > nr_desc = e->efi_memmap_size / e->efi_memdesc_size;
>> > for (i = 0; i < nr_desc; i++) {
>> > md = (efi_memory_desc_t *)(pmap + (i * e->efi_memdesc_size));
>> >- if (md->attribute & EFI_MEMORY_MORE_RELIABLE) {
>> >- region.start = md->phys_addr;
>> >- region.size = md->num_pages << EFI_PAGE_SHIFT;
>> >- process_mem_region(®ion, minimum, image_size);
>> >+ if (md->attribute & EFI_MEMORY_MORE_RELIABLE)
>> > efi_mirror_found = true;
>>
>> Hi Horiguchi-san,
>>
>> If efi_mirror_found is changed to be true, we won't need to walk other
>> entries, so I think:
>> if (md->attribute & EFI_MEMORY_MORE_RELIABLE) {
>> efi_mirror_found = true;
>> break;
>> }
>> will be enough to show that mirror regions exist. And will walk
>> less entries. How do you think about this?
>
>Thank you for the review, Chao.
>And you're right, I'll add break here.
>
># I'll post revised one tomorrow waiting for more comments.
>
>> Another question: what's the benifit of putting this part of
>> "efi_mirror_found = true" to a independent cycle.
>
>We can't easily cancel process_mem_region(), so if we process a few normal
>regions like EFI_CONVENTIONAL_MEMORY and then find a EFI_MEMORY_MORE_RELIABLE
>region, that's a bit troublesome.
>So I decided to first check whether EFI_MEMORY_MORE_RELIABLE region exists or not.
OK, I got it. Thanks for your explanation.
Thanks,
Chao Fan
>
>Thanks,
>Naoya Horiguchi
>
>
Powered by blists - more mailing lists