DMARC-Filter: OpenDMARC Filter v1.3.2 mx1.redhat.com A3BBB5F7B6
Date: Thu, 6 Jul 2017 17:18:09 +0800
From: Baoquan He <bhe@redhat.com>
To: Naoya Horiguchi <n-horiguchi@ah.jp.nec.com>
Cc: Kees Cook <keescook@chromium.org>, LKML <linux-kernel@vger.kernel.org>,
        "x86@kernel.org" <x86@kernel.org>,
        Thomas Gleixner <tglx@linutronix.de>, "H. Peter Anvin" <hpa@zytor.com>,
        Ingo Molnar <mingo@kernel.org>,
        "izumi.taku@jp.fujitsu.com" <izumi.taku@jp.fujitsu.com>,
        Thomas Garnier <thgarnie@google.com>,
        "fanc.fnst@cn.fujitsu.com" <fanc.fnst@cn.fujitsu.com>,
        Matt Fleming <matt@codeblueprint.co.uk>,
        Junichi Nomura <j-nomura@ce.jp.nec.com>
Subject: Re: [PATCH] x86/boot/KASLR: exclude EFI_BOOT_SERVICES_{CODE|DATA}
 from KASLR's choice
Message-ID: <20170706091809.GI19994@x1>
References: <20170706083106.GA21796@hori1.linux.bs1.fc.nec.co.jp>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20170706083106.GA21796@hori1.linux.bs1.fc.nec.co.jp>
User-Agent: Mutt/1.7.0 (2016-08-17)
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 6086
Lines: 167

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
>