Re: [PATCH] x86/boot/KASLR: exclude EFI_BOOT_SERVICES_{CODE|DATA} from KASLR's choice

[Naoya Horiguchi]

On Thu, Jul 06, 2017 at 05:18:09PM +0800, Baoquan He wrote:
> 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.

Thank you for the positive response, Baoquan.

> 
> 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@xxxxxxxxxxxxx>
> > 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@xxxxxxxxxxxxx>
> > ---
> >  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.

OK. efi_mirror_found need not be a global variable any longer.

> > +	}
> > +
> > +	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;

I intented that this is represented by the return value of process_efi_entry(),
but if you have more readable/clearer options, I'm fine with that.

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

No problem, feel free to move this snippet to your 2/2 patch.

Thanks,
Naoya Horiguchi