lkml.org 
[lkml]   [2017]   [Jul]   [6]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    Patch in this message
    /
    Date
    From
    SubjectRe: [PATCH] x86/boot/KASLR: exclude EFI_BOOT_SERVICES_{CODE|DATA} from KASLR's choice
    Hi Naoya Horiguchi,

    Thanks for making this!

    On 07/06/17 at 08:31am, 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?

    This is interesting. So you are suggesting that we should try to avoid
    those EFI_BOOT_SERVICES_{CODE|DATA} efi regions as long as efi map
    regions are available, meanwhile try to locate kernel inside mirrored
    regions if existed. I do know the efi work around, so it seems reasonable
    to me, I can add it when repost. Or you can post after mine has been
    merged.

    A little adjustment, please see the inline comment.
    >
    > [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@ah.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@ah.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(&region, minimum, image_size);
    > + if (md->attribute & EFI_MEMORY_MORE_RELIABLE)
    > efi_mirror_found = true;

    Here, we should define a local variable of bool type to mark if mirrored
    region is found.

    > + }
    > +
    > + for (i = 0; i < nr_desc; i++) {
    > + md = (efi_memory_desc_t *)(pmap + (i * e->efi_memdesc_size));
    > +
    > + /*
    > + * EFI_BOOT_SERVICES_{CODE|DATA} are avoided because boot
    > + * services regions could be accessed after ExitBootServices()
    > + * due to the workaround for buggy firmware.
    > + */
    > + if (!(md->type == EFI_LOADER_CODE ||
    > + md->type == EFI_LOADER_DATA ||
    > + md->type == EFI_CONVENTIONAL_MEMORY))
    > + continue;
    > +
    > + if (efi_mirror_found &&
    > + !(md->attribute & EFI_MEMORY_MORE_RELIABLE))
    > + continue;
    > +
    > + region.start = md->phys_addr;
    > + region.size = md->num_pages << EFI_PAGE_SHIFT;
    > + process_mem_region(&region, minimum, image_size);

    And can define a new global variable like efi_processed here.
    efi_processed = true;


    And yes, I missed this snippet of code to break loop if slot_area has
    been full, this saves time.

    > + if (slot_area_index == MAX_SLOT_AREA) {
    > + debug_putstr("Aborted EFI scan (slot_areas full)!\n");
    > + break;
    > }
    > }
    > + return true;
    > }
    >
    > static void process_e820_entry(unsigned long minimum, unsigned long image_size)
    > @@ -637,8 +661,7 @@ static unsigned long find_random_phys_addr(unsigned long minimum,
    > minimum = ALIGN(minimum, CONFIG_PHYSICAL_ALIGN);
    >
    > #ifdef CONFIG_EFI
    > - process_efi_entry(minimum, image_size);
    > - if (efi_mirror_found)
    > + if (process_efi_entry(minimum, image_size))
    > return slots_fetch_random();
    > #endif
    >
    > --
    > 2.7.4
    >

    \
     
     \ /
      Last update: 2017-07-06 11:18    [W:4.081 / U:0.060 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site