| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <202007281235.B9743EA@keescook>
Date: Tue, 28 Jul 2020 12:37:12 -0700
From: Kees Cook <keescook@...omium.org>
To: Arvind Sankar <nivedita@...m.mit.edu>
Cc: x86@...nel.org, linux-kernel@...r.kernel.org
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@...m.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
Powered by blists - more mailing lists