Messages in this thread | | | Date | Tue, 28 Jul 2020 12:37:12 -0700 | From | Kees Cook <> | Subject | Re: [PATCH v2 8/8] x86/kaslr: Don't use 64-bit mem_vector for 32-bit kernel |
| |
On Mon, Jul 27, 2020 at 07:08:01PM -0400, Arvind Sankar wrote: > Commit > f28442497b5c ("x86/boot: Fix KASLR and memmap= collision") > converted mem_vector type to use 64-bit on the 32-bit kernel as well, > based on Thomas's review [0]. However: > - the code still doesn't consistently use 64-bit types. For instance, > mem_avoid_overlap uses 32-bit types when checking for overlaps. This > is actually okay, as the passed in memory range will have been clipped > to below 4G, but it is difficult to reason about the safety of the > code. > - KASLR on 32-bit can't use memory above 4G anyway, so it's pointless > to keep track of ranges above 4G. > > Switch the type back to unsigned long, and use a helper function to clip > regions to 4G on 32-bit, when storing mem_avoid, immovable_mem, EFI, > E820 and command-line memory regions.
The reason for long long is to avoid having to check for overflows in any of the offset calculations. Why not just leave this as-is?
> > [0] https://lore.kernel.org/linux-nvdimm/alpine.DEB.2.20.1701111246400.3579@nanos/ > > Signed-off-by: Arvind Sankar <nivedita@alum.mit.edu> > --- > arch/x86/boot/compressed/acpi.c | 7 +++---- > arch/x86/boot/compressed/kaslr.c | 25 ++++++++++--------------- > arch/x86/boot/compressed/misc.h | 19 +++++++++++++++++-- > 3 files changed, 30 insertions(+), 21 deletions(-) > > diff --git a/arch/x86/boot/compressed/acpi.c b/arch/x86/boot/compressed/acpi.c > index 8bcbcee54aa1..d09e0ec5637e 100644 > --- a/arch/x86/boot/compressed/acpi.c > +++ b/arch/x86/boot/compressed/acpi.c > @@ -106,7 +106,7 @@ static acpi_physical_address kexec_get_rsdp_addr(void) > } > > /* Get systab from boot params. */ > - systab = (efi_system_table_64_t *) (ei->efi_systab | ((__u64)ei->efi_systab_hi << 32)); > + systab = (efi_system_table_64_t *) (ei->efi_systab | ((u64)ei->efi_systab_hi << 32)); > if (!systab) > error("EFI system table not found in kexec boot_params."); > > @@ -139,7 +139,7 @@ static acpi_physical_address efi_get_rsdp_addr(void) > > /* Get systab from boot params. */ > #ifdef CONFIG_X86_64 > - systab = ei->efi_systab | ((__u64)ei->efi_systab_hi << 32); > + systab = ei->efi_systab | ((u64)ei->efi_systab_hi << 32); > #else > if (ei->efi_systab_hi || ei->efi_memmap_hi) { > debug_putstr("Error getting RSDP address: EFI system table located above 4GB.\n"); > @@ -404,8 +404,7 @@ int count_immovable_mem_regions(void) > > ma = (struct acpi_srat_mem_affinity *)sub_table; > if (!(ma->flags & ACPI_SRAT_MEM_HOT_PLUGGABLE) && ma->length) { > - immovable_mem[num].start = ma->base_address; > - immovable_mem[num].size = ma->length; > + immovable_mem[num] = mem_vector_ctor(ma->base_address, ma->length); > num++; > } > > diff --git a/arch/x86/boot/compressed/kaslr.c b/arch/x86/boot/compressed/kaslr.c > index eca2acc65e2a..8c44041ae9cb 100644 > --- a/arch/x86/boot/compressed/kaslr.c > +++ b/arch/x86/boot/compressed/kaslr.c > @@ -98,7 +98,7 @@ static bool memmap_too_large; > * Store memory limit: MAXMEM on 64-bit and KERNEL_IMAGE_SIZE on 32-bit. > * It may be reduced by "mem=nn[KMG]" or "memmap=nn[KMG]" command line options. > */ > -static unsigned long long mem_limit; > +static unsigned long mem_limit; > > /* Number of immovable memory regions */ > static int num_immovable_mem; > @@ -141,8 +141,7 @@ enum parse_mode { > }; > > static int > -parse_memmap(char *p, unsigned long long *start, unsigned long long *size, > - enum parse_mode mode) > +parse_memmap(char *p, u64 *start, u64 *size, enum parse_mode mode) > { > char *oldp; > > @@ -172,7 +171,7 @@ parse_memmap(char *p, unsigned long long *start, unsigned long long *size, > */ > *size = 0; > } else { > - unsigned long long flags; > + u64 flags; > > /* > * efi_fake_mem=nn@ss:attr the attr specifies > @@ -211,7 +210,7 @@ static void mem_avoid_memmap(enum parse_mode mode, char *str) > > while (str && (i < MAX_MEMMAP_REGIONS)) { > int rc; > - unsigned long long start, size; > + u64 start, size; > char *k = strchr(str, ','); > > if (k) > @@ -230,8 +229,7 @@ static void mem_avoid_memmap(enum parse_mode mode, char *str) > continue; > } > > - mem_avoid[MEM_AVOID_MEMMAP_BEGIN + i].start = start; > - mem_avoid[MEM_AVOID_MEMMAP_BEGIN + i].size = size; > + mem_avoid[MEM_AVOID_MEMMAP_BEGIN + i] = mem_vector_ctor(start, size); > i++; > } > > @@ -422,8 +420,7 @@ static void mem_avoid_init(unsigned long input, unsigned long input_size, > initrd_start |= boot_params->hdr.ramdisk_image; > initrd_size = (u64)boot_params->ext_ramdisk_size << 32; > initrd_size |= boot_params->hdr.ramdisk_size; > - mem_avoid[MEM_AVOID_INITRD].start = initrd_start; > - mem_avoid[MEM_AVOID_INITRD].size = initrd_size; > + mem_avoid[MEM_AVOID_INITRD] = mem_vector_ctor(initrd_start, initrd_size); > /* No need to set mapping for initrd, it will be handled in VO. */ > > /* Avoid kernel command line. */ > @@ -673,7 +670,7 @@ static bool process_mem_region(struct mem_vector *region, > * immovable memory and @region. > */ > for (i = 0; i < num_immovable_mem; i++) { > - unsigned long long start, end, entry_end, region_end; > + unsigned long start, end, entry_end, region_end; > struct mem_vector entry; > > if (!mem_overlaps(region, &immovable_mem[i])) > @@ -728,7 +725,7 @@ process_efi_entries(unsigned long minimum, unsigned long image_size) > } > pmap = e->efi_memmap; > #else > - pmap = (e->efi_memmap | ((__u64)e->efi_memmap_hi << 32)); > + pmap = (e->efi_memmap | ((u64)e->efi_memmap_hi << 32)); > #endif > > nr_desc = e->efi_memmap_size / e->efi_memdesc_size; > @@ -765,8 +762,7 @@ process_efi_entries(unsigned long minimum, unsigned long image_size) > !(md->attribute & EFI_MEMORY_MORE_RELIABLE)) > continue; > > - region.start = md->phys_addr; > - region.size = md->num_pages << EFI_PAGE_SHIFT; > + region = mem_vector_ctor(md->phys_addr, md->num_pages << EFI_PAGE_SHIFT); > if (process_mem_region(®ion, minimum, image_size)) > break; > } > @@ -793,8 +789,7 @@ static void process_e820_entries(unsigned long minimum, > /* Skip non-RAM entries. */ > if (entry->type != E820_TYPE_RAM) > continue; > - region.start = entry->addr; > - region.size = entry->size; > + region = mem_vector_ctor(entry->addr, entry->size); > if (process_mem_region(®ion, minimum, image_size)) > break; > } > diff --git a/arch/x86/boot/compressed/misc.h b/arch/x86/boot/compressed/misc.h > index 726e264410ff..fd7c49ab0f02 100644 > --- a/arch/x86/boot/compressed/misc.h > +++ b/arch/x86/boot/compressed/misc.h > @@ -70,10 +70,25 @@ int cmdline_find_option(const char *option, char *buffer, int bufsize); > int cmdline_find_option_bool(const char *option); > > struct mem_vector { > - unsigned long long start; > - unsigned long long size; > + unsigned long start; > + unsigned long size; > }; > > +static inline > +struct mem_vector mem_vector_ctor(u64 start, u64 size) > +{ > + u64 end = start + size; > + struct mem_vector ret; > + > + start = min_t(u64, start, ULONG_MAX); > + end = min_t(u64, end, ULONG_MAX); > + > + ret.start = (unsigned long)start; > + ret.size = (unsigned long)(end - start); > + > + return ret; > +} > + > #if CONFIG_RANDOMIZE_BASE > /* kaslr.c */ > void choose_random_location(unsigned long input, > -- > 2.26.2 >
-- Kees Cook
| |