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.
[0] https://lore.kernel.org/linux-nvdimm/alpine.DEB.2.20.1701111246400.3579@nanos/
Signed-off-by: Arvind Sankar <[email protected]>
---
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 ffbcc0444f70..8bf4ceceb8ce 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
* Arvind Sankar <[email protected]> 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.
>
> [0] https://lore.kernel.org/linux-nvdimm/alpine.DEB.2.20.1701111246400.3579@nanos/
So why not just standardize all the calculations around u64, and for
32-bit kernels add in a debug check that double checks that it all
remained within the first 32 bits?
It's much easier to argue about this if we don't have to worry about
32-bit overflows and if the types are simple and bitness invariant.
Yes, technically it's slightly bloated on 32-bit kernels, but
maintainability of 32-bit kernels is the primary concern now ...
Thanks,
Ingo