Messages in this thread | | | From | Kees Cook <> | Date | Mon, 17 Jul 2017 16:13:21 -0700 | Subject | Re: [PATCH v5 4/4] x86/boot/KASLR: Restrict kernel to be randomized in mirror regions |
| |
On Thu, Jul 13, 2017 at 7:19 AM, Baoquan He <bhe@redhat.com> wrote: > Kernel text may be located in non-mirror regions (movable zone) when both > address range mirroring feature and KASLR are enabled. > > The address range mirroring feature arranges such mirror region into > normal zone and other region into movable zone in order to locate > kernel code and data in mirror region. The physical memory region > whose descriptors in EFI memory map has EFI_MEMORY_MORE_RELIABLE > attribute (bit: 16) are mirrored. > > If efi is detected, iterate efi memory map and pick the mirror region to > process for adding candidate of randomization slot. If efi is disabled > or no mirror region found, still process e820 memory map. > > Signed-off-by: Baoquan He <bhe@redhat.com> > --- > arch/x86/boot/compressed/kaslr.c | 68 ++++++++++++++++++++++++++++++++++++++-- > 1 file changed, 66 insertions(+), 2 deletions(-) > > diff --git a/arch/x86/boot/compressed/kaslr.c b/arch/x86/boot/compressed/kaslr.c > index 99c7194f7ea6..9059b571eca1 100644 > --- a/arch/x86/boot/compressed/kaslr.c > +++ b/arch/x86/boot/compressed/kaslr.c > @@ -37,7 +37,9 @@ > #include <linux/uts.h> > #include <linux/utsname.h> > #include <linux/ctype.h> > +#include <linux/efi.h> > #include <generated/utsrelease.h> > +#include <asm/efi.h> > > /* Macros used by the included decompressor code below. */ > #define STATIC > @@ -558,6 +560,65 @@ static void process_mem_region(struct mem_vector *entry, > } > } > > +#ifdef CONFIG_EFI > +/* > + * Returns true if mirror region found (and must have been processed > + * for slots adding) > + */ > +static bool process_efi_entries(unsigned long minimum, > + unsigned long image_size) > +{ > + struct efi_info *e = &boot_params->efi_info; > + bool efi_mirror_found = false; > + struct mem_vector region; > + efi_memory_desc_t *md; > + unsigned long pmap; > + char *signature; > + u32 nr_desc; > + int i; > + > + signature = (char *)&boot_params->efi_info.efi_loader_signature; > + if (strncmp(signature, EFI32_LOADER_SIGNATURE, 4) && > + strncmp(signature, EFI64_LOADER_SIGNATURE, 4)) > + 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 false; > + } > + pmap = e->efi_memmap; > +#else > + pmap = (e->efi_memmap | ((__u64)e->efi_memmap_hi << 32)); > +#endif > + > + 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); > + efi_mirror_found = true; > + > + if (slot_area_index == MAX_SLOT_AREA) { > + debug_putstr("Aborted efi scan (slot_areas full)!\n");
Should this text be capitalized to "EFI" or expanded to "EFI mirror"?
> + break; > + } > + } > + } > + > + return efi_mirror_found; > +} > +#else > +static inline bool process_efi_entries(unsigned long minimum, > + unsigned long image_size) > +{ > + return false; > +} > +#endif > + > static void process_e820_entries(unsigned long minimum, > unsigned long image_size) > { > @@ -586,13 +647,16 @@ static unsigned long find_random_phys_addr(unsigned long minimum, > { > /* Check if we had too many memmaps. */ > if (memmap_too_large) { > - debug_putstr("Aborted e820 scan (more than 4 memmap= args)!\n"); > + debug_putstr("Aborted memory entries scan (more than 4 memmap= args)!\n"); > return 0; > } > > /* Make sure minimum is aligned. */ > minimum = ALIGN(minimum, CONFIG_PHYSICAL_ALIGN); > > + if(process_efi_entries(minimum, image_size))
Style nit: space between "if" and "(".
> + return slots_fetch_random(); > + > process_e820_entries(minimum, image_size); > return slots_fetch_random(); > } > @@ -652,7 +716,7 @@ void choose_random_location(unsigned long input, > */ > min_addr = min(*output, 512UL << 20); > > - /* Walk e820 and find a random address. */ > + /* Walk available memory entries to find a random address. */ > random_addr = find_random_phys_addr(min_addr, output_size); > if (!random_addr) { > warn("Physical KASLR disabled: no suitable memory region!"); > -- > 2.5.5 >
Otherwise, looks fine to me for the KASLR piece; I can't speak specifically to the EFI logic. :)
Reviewed-by: Kees Cook <keescook@chromium.org>
-Kees
-- Kees Cook Pixel Security
| |