Re: [PATCH v2 8/8] x86/kaslr: Don't use 64-bit mem_vector for 32-bit kernel

[Kees Cook <keescook@chromium.org>]

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(&region, 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(&region, 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