[PATCH v14 0/5] x86/boot/KASLR: Parse ACPI table and limit KASLR to choosing immovable memory

[PATCH v14 0/5] x86/boot/KASLR: Parse ACPI table and limit KASLR to choosing immovable memory

[Chao Fan]

***Background:
People reported that KASLR may randomly choose some positions
which are located in movable memory regions. This will break memory
hotplug feature and make the movable memory chosen by KASLR can't be
removed.

***Solutions:
Get the information of memory hot-remove, then KASLR knows the
right regions. Information about memory hot-remove is in ACPI
SRAT, which will be parsed after start_kernel(), so KASLR
can't get the information.

Somebody suggest to add a kernel parameter to specify the
immovable memory so that limit KASLR in these regions. Then I make
a PATCHSET. After several versions, Ingo gave a suggestion:
https://www.mail-archive.com/linux-kernel@vger.kernel.org/msg1634024.html
Follow Ingo's suggestion, imitate the ACPI code to parse the ACPI
tables, so that the KASLR can get necessary memory information in
ACPI tables.
I think ACPI code is an independent part, so imitate the codes
and functions to 'compressed/' directory, so that KASLR won't
influence the initialization of ACPI.

PATCH 1/5 Add get_acpi_rsdp() to parse RSDP in cmdline from KEXEC
PATCH 2/5 Add efi_get_rsdp_addr() to find RSDP from EFI table when
          booting from EFI.
PATCH 3/5 Add bios_get_rsdp_addr() to search RSDP in memory when EFI
          table not found.
PATCH 4/5 Compute SRAT from RSDP and walk SRAT to store the immovable
          memory regions.
PATCH 5/5 Calculate the intersection between memory regions from e820/efi
          memory table and immovable memory regions. Limit KASLR to
          choosing these regions for randomization.

v13->v14:
Follow Baoquan's suggestion:
 - Use simple_strtoull() for now.
Follow Boris' suggestion:
 - Put 'return 0' out of 'ifdef' to make sure function return.
Follow Masa's suggestion:
 - Change the end of string as '\0'

v12->v13:
Follow Boris' suggestion:
 - Copy kstrtoull() to boot/string.c
Follow Masa's suggestion:
 - Change some code logical
Follow Baoquan's suggestion:
 - Add tag to disable export symbol

v11->v12:
Follow Boris' suggestion:
 - Change patch log and code comments.
 - Add 'CONFIG_EARLY_PARSE_RSDP' to make code easy to read
 - Put strtoull() to misc.c
Follow Masa's suggestion:
 - Remove the detection for 'movable_node'
 - Change the code logical about cmdline_find_option()

v10->v11:
Follow Boris' suggestion:
 - Link kstrtoull() instead of copying it.
 - Drop the useless wrapped function.

v9->v10:
Follow Baoquan's suggestion:
 - Change some log
 - Merge last two patch together.

v8-v9:
Follow Boris' suggestion:
 - Change code style.
 - Splite PATCH 1/3 to more path.
 - Introduce some new function
 - Use existing function to rework some code
Follow Masayoshi's suggetion:
 - Make code more readable

v7-v8:
Follow Kees Cook's suggestion:
 - Use mem_overlaps() to check memory region.
 - Use #ifdef in the definition of function.

v6->v7:
Follow Rafael's suggestion:
 - Add more comments and patch log.
Follow test robot's suggestion:
 - Add "static" tag for function

v5->v6:
Follow Baoquan He's suggestion:
 - Change some log.
 - Add the check for acpi_rsdp
 - Change some code logical to make code clear

v4->v5:
Follow Dou Liyang's suggestion:
 - Add more comments about some functions based on kernel code.
 - Change some typo in comments.
 - Clean useless variable.
 - Add check for the boundary of array.
 - Add check for 'movable_node' parameter

v3->v4:
Follow Thomas Gleixner's suggestion:
 - Put the whole efi related function into #define CONFIG_EFI and return
   false in the other stub.

v2->v3:
 - Test in more conditions, so remove the 'RFC' tag.
 - Change some comments.

v1->v2:
 -  Simplify some code.
Follow Baoquan He's suggestion:
 - Reuse the head file of acpi code.

Any comments will be welcome.


Chao Fan (5):
  x86/boot: Introduce get_acpi_rsdp() to parse RSDP in cmdline from
    KEXEC
  x86/boot: Introduce efi_get_rsdp_addr() to find RSDP from EFI table
  x86/boot: Introduce bios_get_rsdp_addr() to search RSDP in memory
  x86/boot: Parse SRAT address from RSDP and store immovable memory
  x86/boot/KASLR: Limit KASLR to extracting kernel in immovable memory

 arch/x86/Kconfig                  |  12 ++
 arch/x86/boot/compressed/Makefile |   2 +
 arch/x86/boot/compressed/acpi.c   | 323 ++++++++++++++++++++++++++++++
 arch/x86/boot/compressed/kaslr.c  |  79 ++++++--
 arch/x86/boot/compressed/misc.h   |  19 ++
 5 files changed, 420 insertions(+), 15 deletions(-)
 create mode 100644 arch/x86/boot/compressed/acpi.c

-- 
2.19.2

[PATCH v14 1/5] x86/boot: Introduce get_acpi_rsdp() to parse RSDP in cmdline from KEXEC

[Chao Fan]

Memory information in SRAT is necessary to fix the conflict between
KASLR and memory-hotremove.

ACPI SRAT (System/Static Resource Affinity Table) shows the details
about memory ranges, including ranges of memory provided by hot-added
memory devices. SRAT is introduced by Root System Description
Pointer(RSDP), so RSDP should be found firstly.

When booting form KEXEC/EFI/BIOS, the methods to find RSDP
are different. When booting from KEXEC, 'acpi_rsdp' may have been
added to cmdline, so parse cmdline to find RSDP.

Signed-off-by: Chao Fan <fanc.fnst@cn.fujitsu.com>
---
 arch/x86/boot/compressed/acpi.c | 30 ++++++++++++++++++++++++++++++
 1 file changed, 30 insertions(+)
 create mode 100644 arch/x86/boot/compressed/acpi.c

diff --git a/arch/x86/boot/compressed/acpi.c b/arch/x86/boot/compressed/acpi.c
new file mode 100644
index 000000000000..44f19546c169
--- /dev/null
+++ b/arch/x86/boot/compressed/acpi.c
@@ -0,0 +1,30 @@
+// SPDX-License-Identifier: GPL-2.0
+#define BOOT_CTYPE_H
+#include "misc.h"
+#include "error.h"
+
+#include <linux/efi.h>
+#include <linux/numa.h>
+#include <linux/acpi.h>
+#include <asm/efi.h>
+
+/* Max length of 64-bit hex address string is 18, prefix "0x" + 16 hex digits */
+#define MAX_ADDRESS_LENGTH 18
+
+static acpi_physical_address get_acpi_rsdp(void)
+{
+#ifdef CONFIG_KEXEC
+	char val[MAX_ADDRESS_LENGTH+1];
+	unsigned long long res;
+	int len;
+
+	len = cmdline_find_option("acpi_rsdp", val, MAX_ADDRESS_LENGTH+1);
+	if (len > 0) {
+		char *e;
+
+		val[len] = '\0';
+		return (acpi_physical_address)simple_strtoull(val, &e, 16);
+	}
+#endif
+	return 0;
+}
-- 
2.19.2

[PATCH v14 2/5] x86/boot: Introduce efi_get_rsdp_addr() to find RSDP from EFI table

[Chao Fan]

Memory information in SRAT is necessary to fix the conflict between
KASLR and memory-hotremove. So RSDP and SRAT should be parsed.

When booting form KEXEC/EFI/BIOS, the methods to compute RSDP
are different. When booting from EFI, EFI table points to RSDP.
So parse the EFI table and find the RSDP.

Signed-off-by: Chao Fan <fanc.fnst@cn.fujitsu.com>
---
 arch/x86/boot/compressed/acpi.c | 79 +++++++++++++++++++++++++++++++++
 1 file changed, 79 insertions(+)

diff --git a/arch/x86/boot/compressed/acpi.c b/arch/x86/boot/compressed/acpi.c
index 44f19546c169..4151881d8713 100644
--- a/arch/x86/boot/compressed/acpi.c
+++ b/arch/x86/boot/compressed/acpi.c
@@ -28,3 +28,82 @@ static acpi_physical_address get_acpi_rsdp(void)
 #endif
 	return 0;
 }
+
+/* Search EFI table for RSDP. */
+static acpi_physical_address efi_get_rsdp_addr(void)
+{
+	acpi_physical_address rsdp_addr = 0;
+#ifdef CONFIG_EFI
+	efi_system_table_t *systab;
+	struct efi_info *e;
+	bool efi_64;
+	char *sig;
+	int size;
+	int i;
+
+	e = &boot_params->efi_info;
+	sig = (char *)&e->efi_loader_signature;
+
+	if (!strncmp(sig, EFI64_LOADER_SIGNATURE, 4)) {
+		efi_64 = true;
+	} else if (!strncmp(sig, EFI32_LOADER_SIGNATURE, 4)) {
+		efi_64 = false;
+	} else {
+		debug_putstr("Wrong EFI loader signature.\n");
+		return 0;
+	}
+
+	/* Get systab from boot params. Based on efi_init(). */
+#ifdef CONFIG_X86_64
+	systab = (efi_system_table_t *)(e->efi_systab | ((__u64)e->efi_systab_hi<<32));
+#else
+	if (e->efi_systab_hi || e->efi_memmap_hi) {
+		debug_putstr("Error getting RSDP address: EFI system table located above 4GB.\n");
+		return 0;
+	}
+	systab = (efi_system_table_t *)e->efi_systab;
+#endif
+
+	if (!systab)
+		return 0;
+
+	/*
+	 * Get EFI tables from systab. Based on efi_config_init() and
+	 * efi_config_parse_tables().
+	 */
+	size = efi_64 ? sizeof(efi_config_table_64_t) :
+			sizeof(efi_config_table_32_t);
+
+	for (i = 0; i < systab->nr_tables; i++) {
+		void *config_tables;
+		unsigned long table;
+		efi_guid_t guid;
+
+		config_tables = (void *)(systab->tables + size * i);
+		if (efi_64) {
+			efi_config_table_64_t *tmp_table;
+
+			tmp_table = (efi_config_table_64_t *)config_tables;
+			guid = tmp_table->guid;
+			table = tmp_table->table;
+
+			if (!IS_ENABLED(CONFIG_X86_64) && table >> 32) {
+				debug_putstr("Error getting RSDP address: EFI config table located above 4GB.\n");
+				return 0;
+			}
+		} else {
+			efi_config_table_32_t *tmp_table;
+
+			tmp_table = (efi_config_table_32_t *)config_tables;
+			guid = tmp_table->guid;
+			table = tmp_table->table;
+		}
+
+		if (!(efi_guidcmp(guid, ACPI_TABLE_GUID)))
+			rsdp_addr = (acpi_physical_address)table;
+		else if (!(efi_guidcmp(guid, ACPI_20_TABLE_GUID)))
+			return (acpi_physical_address)table;
+	}
+#endif
+	return rsdp_addr;
+}
-- 
2.19.2

[PATCH v14 3/5] x86/boot: Introduce bios_get_rsdp_addr() to search RSDP in memory

[Chao Fan]

To fix the conflict between KASLR and memory-hotremove, memory
Memory information in SRAT is necessary to fix the conflict
between KASLR and memory-hotremove. So RSDP and SRAT should be parsed.

When booting form KEXEC/EFI/BIOS, the methods to compute RSDP
are different. When booting from BIOS, there is no variable who can
point to RSDP directly, so scan memory for the RSDP and verify RSDP
by signature and checksum.

Signed-off-by: Chao Fan <fanc.fnst@cn.fujitsu.com>
---
 arch/x86/boot/compressed/acpi.c | 86 +++++++++++++++++++++++++++++++++
 1 file changed, 86 insertions(+)

diff --git a/arch/x86/boot/compressed/acpi.c b/arch/x86/boot/compressed/acpi.c
index 4151881d8713..795a78559a58 100644
--- a/arch/x86/boot/compressed/acpi.c
+++ b/arch/x86/boot/compressed/acpi.c
@@ -107,3 +107,89 @@ static acpi_physical_address efi_get_rsdp_addr(void)
 #endif
 	return rsdp_addr;
 }
+
+static u8 compute_checksum(u8 *buffer, u32 length)
+{
+	u8 *end = buffer + length;
+	u8 sum = 0;
+
+	while (buffer < end)
+		sum += *(buffer++);
+
+	return sum;
+}
+
+/* Search a block of memory for the RSDP signature. */
+static u8 *scan_mem_for_rsdp(u8 *start, u32 length)
+{
+	struct acpi_table_rsdp *rsdp;
+	u8 *address;
+	u8 *end;
+
+	end = start + length;
+
+	/* Search from given start address for the requested length */
+	for (address = start; address < end; address += ACPI_RSDP_SCAN_STEP) {
+		/*
+		 * Both RSDP signature and checksum must be correct.
+		 * Note: Sometimes there exists more than one RSDP in memory;
+		 * the valid RSDP has a valid checksum, all others have an
+		 * invalid checksum.
+		 */
+		rsdp = (struct acpi_table_rsdp *)address;
+
+		/* BAD Signature */
+		if (!ACPI_VALIDATE_RSDP_SIG(rsdp->signature))
+			continue;
+
+		/* Check the standard checksum */
+		if (compute_checksum((u8 *) rsdp, ACPI_RSDP_CHECKSUM_LENGTH))
+			continue;
+
+		/* Check extended checksum if table version >= 2 */
+		if ((rsdp->revision >= 2) &&
+		    (compute_checksum((u8 *) rsdp, ACPI_RSDP_XCHECKSUM_LENGTH)))
+			continue;
+
+		/* Signature and checksum valid, we have found a real RSDP */
+		return address;
+	}
+	return NULL;
+}
+
+/* Search RSDP address, based on acpi_find_root_pointer(). */
+static acpi_physical_address bios_get_rsdp_addr(void)
+{
+	u8 *table_ptr;
+	u32 address;
+	u8 *rsdp;
+
+	/* Get the location of the Extended BIOS Data Area (EBDA) */
+	table_ptr = (u8 *)ACPI_EBDA_PTR_LOCATION;
+	*(u32 *)(void *)&address = *(u16 *)(void *)table_ptr;
+	address <<= 4;
+	table_ptr = (u8 *)(long)address;
+
+	/*
+	 * Search EBDA paragraphs (EBDA is required to be a minimum of
+	 * 1K length)
+	 */
+	if (address > 0x400) {
+		rsdp = scan_mem_for_rsdp(table_ptr, ACPI_EBDA_WINDOW_SIZE);
+		if (rsdp) {
+			address += (u32)ACPI_PTR_DIFF(rsdp, table_ptr);
+			return (acpi_physical_address)address;
+		}
+	}
+
+	table_ptr = (u8 *)ACPI_HI_RSDP_WINDOW_BASE;
+	rsdp = scan_mem_for_rsdp(table_ptr, ACPI_HI_RSDP_WINDOW_SIZE);
+
+	/* Search upper memory: 16-byte boundaries in E0000h-FFFFFh */
+	if (rsdp) {
+		address = (u32)(ACPI_HI_RSDP_WINDOW_BASE +
+				ACPI_PTR_DIFF(rsdp, table_ptr));
+		return (acpi_physical_address)address;
+	}
+	return 0;
+}
-- 
2.19.2

[PATCH v14 4/5] x86/boot: Parse SRAT address from RSDP and store immovable memory

[Chao Fan]

SRAT should be parsed by RSDP to fix the conflict between KASLR
and memory-hotremove, then find the immovable memory regions and store
them in an array called immovable_mem[]. With immovable_mem[], KASLR
can avoid to extract kernel to specific regions.

Since 'RANDOMIZE_BASE' && 'MEMORY_HOTREMOVE' is needed, introduce
'CONFIG_EARLY_PARSE_RSDP' to make ifdeffery clear.

Signed-off-by: Chao Fan <fanc.fnst@cn.fujitsu.com>
---
 arch/x86/Kconfig                  |  12 +++
 arch/x86/boot/compressed/Makefile |   2 +
 arch/x86/boot/compressed/acpi.c   | 128 ++++++++++++++++++++++++++++++
 arch/x86/boot/compressed/kaslr.c  |   4 -
 arch/x86/boot/compressed/misc.h   |  19 +++++
 5 files changed, 161 insertions(+), 4 deletions(-)

diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
index ba7e3464ee92..333c383478b7 100644
--- a/arch/x86/Kconfig
+++ b/arch/x86/Kconfig
@@ -2149,6 +2149,18 @@ config X86_NEED_RELOCS
 	def_bool y
 	depends on RANDOMIZE_BASE || (X86_32 && RELOCATABLE)
 
+config EARLY_SRAT_PARSE
+	bool "Early SRAT table parsing"
+	def_bool y
+	depends on RANDOMIZE_BASE && MEMORY_HOTREMOVE
+	help
+	  This option enables early SRAT parsing in compressed boot stage
+	  so that memory hot-remove ranges do not overlap with KASLR
+	  chosen ranges. Kernel won't be extracted in hot-removable
+	  memory, so that make sure memory-hotremove works well with
+	  KASLR enabled.
+	  Say Y if you want to use both KASLR and memory-hotremove.
+
 config PHYSICAL_ALIGN
 	hex "Alignment value to which kernel should be aligned"
 	default "0x200000"
diff --git a/arch/x86/boot/compressed/Makefile b/arch/x86/boot/compressed/Makefile
index 466f66c8a7f8..23491d74939d 100644
--- a/arch/x86/boot/compressed/Makefile
+++ b/arch/x86/boot/compressed/Makefile
@@ -84,6 +84,8 @@ ifdef CONFIG_X86_64
 	vmlinux-objs-y += $(obj)/pgtable_64.o
 endif
 
+vmlinux-objs-$(CONFIG_EARLY_SRAT_PARSE) += $(obj)/acpi.o
+
 $(obj)/eboot.o: KBUILD_CFLAGS += -fshort-wchar -mno-red-zone
 
 vmlinux-objs-$(CONFIG_EFI_STUB) += $(obj)/eboot.o $(obj)/efi_stub_$(BITS).o \
diff --git a/arch/x86/boot/compressed/acpi.c b/arch/x86/boot/compressed/acpi.c
index 795a78559a58..89ed72fb664f 100644
--- a/arch/x86/boot/compressed/acpi.c
+++ b/arch/x86/boot/compressed/acpi.c
@@ -11,6 +11,9 @@
 /* Max length of 64-bit hex address string is 18, prefix "0x" + 16 hex digit. */
 #define MAX_ADDRESS_LENGTH 18
 
+/* Immovable memory regions. */
+struct mem_vector immovable_mem[MAX_NUMNODES*2];
+
 static acpi_physical_address get_acpi_rsdp(void)
 {
 #ifdef CONFIG_KEXEC
@@ -193,3 +196,128 @@ static acpi_physical_address bios_get_rsdp_addr(void)
 	}
 	return 0;
 }
+
+/* Determine RSDP, based on acpi_os_get_root_pointer(). */
+static acpi_physical_address get_rsdp_addr(void)
+{
+	acpi_physical_address pa;
+
+	pa = get_acpi_rsdp();
+
+	if (!pa)
+		pa = efi_get_rsdp_addr();
+
+	if (!pa)
+		pa = bios_get_rsdp_addr();
+
+	return pa;
+}
+
+/* Compute SRAT table from RSDP. */
+static struct acpi_table_header *get_acpi_srat_table(void)
+{
+	acpi_physical_address acpi_table;
+	acpi_physical_address root_table;
+	struct acpi_table_header *header;
+	struct acpi_table_rsdp *rsdp;
+	u32 num_entries;
+	char arg[10];
+	u8 *entry;
+	u32 size;
+	u32 len;
+
+	rsdp = (struct acpi_table_rsdp *)get_rsdp_addr();
+	if (!rsdp)
+		return NULL;
+
+	/* Get RSDT or XSDT from RSDP. */
+	if (!(cmdline_find_option("acpi", arg, sizeof(arg)) == 4 &&
+	    !strncmp(arg, "rsdt", 4)) &&
+	    rsdp->xsdt_physical_address &&
+	    rsdp->revision > 1) {
+		root_table = rsdp->xsdt_physical_address;
+		size = ACPI_XSDT_ENTRY_SIZE;
+	} else {
+		root_table = rsdp->rsdt_physical_address;
+		size = ACPI_RSDT_ENTRY_SIZE;
+	}
+
+	/* Get ACPI root table from RSDT or XSDT.*/
+	header = (struct acpi_table_header *)root_table;
+	if (!header)
+		return NULL;
+
+	len = header->length;
+	if (len < sizeof(struct acpi_table_header) + size)
+		return NULL;
+
+	num_entries = (u32)((len - sizeof(struct acpi_table_header)) / size);
+	entry = ACPI_ADD_PTR(u8, header, sizeof(struct acpi_table_header));
+
+	while (num_entries--) {
+		u64 address64;
+
+		if (size == ACPI_RSDT_ENTRY_SIZE)
+			acpi_table = ((acpi_physical_address)
+				      (*ACPI_CAST_PTR(u32, entry)));
+		else {
+			*(u64 *)(void *)&address64 = *(u64 *)(void *)entry;
+			acpi_table = (acpi_physical_address) address64;
+		}
+
+		if (acpi_table) {
+			header = (struct acpi_table_header *)acpi_table;
+
+			if (ACPI_COMPARE_NAME(header->signature, ACPI_SIG_SRAT))
+				return header;
+		}
+		entry += size;
+	}
+	return NULL;
+}
+
+/*
+ * According to ACPI table, filter the immovable memory regions
+ * and store them in immovable_mem[].
+ */
+void get_immovable_mem(void)
+{
+	struct acpi_table_header *table_header;
+	struct acpi_subtable_header *table;
+	struct acpi_srat_mem_affinity *ma;
+	unsigned long table_end;
+	char arg[10];
+	int i = 0;
+
+	if (cmdline_find_option("acpi", arg, sizeof(arg)) == 3 &&
+	    !strncmp(arg, "off", 3))
+		return;
+
+	table_header = get_acpi_srat_table();
+	if (!table_header)
+		return;
+
+	table_end = (unsigned long)table_header + table_header->length;
+	table = (struct acpi_subtable_header *)
+		((unsigned long)table_header + sizeof(struct acpi_table_srat));
+
+	while (((unsigned long)table) +
+		       sizeof(struct acpi_subtable_header) < table_end) {
+		if (table->type == ACPI_SRAT_TYPE_MEMORY_AFFINITY) {
+			ma = (struct acpi_srat_mem_affinity *)table;
+			if (!(ma->flags & ACPI_SRAT_MEM_HOT_PLUGGABLE)) {
+				immovable_mem[i].start = ma->base_address;
+				immovable_mem[i].size = ma->length;
+				i++;
+			}
+
+			if (i >= MAX_NUMNODES*2) {
+				debug_putstr("Too many immovable memory regions, aborting.\n");
+				return;
+			}
+		}
+		table = (struct acpi_subtable_header *)
+			((unsigned long)table + table->length);
+	}
+	num_immovable_mem = i;
+}
diff --git a/arch/x86/boot/compressed/kaslr.c b/arch/x86/boot/compressed/kaslr.c
index 9ed9709d9947..b251572e77af 100644
--- a/arch/x86/boot/compressed/kaslr.c
+++ b/arch/x86/boot/compressed/kaslr.c
@@ -87,10 +87,6 @@ static unsigned long get_boot_seed(void)
 #define KASLR_COMPRESSED_BOOT
 #include "../../lib/kaslr.c"
 
-struct mem_vector {
-	unsigned long long start;
-	unsigned long long size;
-};
 
 /* Only supporting at most 4 unusable memmap regions with kaslr */
 #define MAX_MEMMAP_REGIONS	4
diff --git a/arch/x86/boot/compressed/misc.h b/arch/x86/boot/compressed/misc.h
index a1d5918765f3..b49748366a5b 100644
--- a/arch/x86/boot/compressed/misc.h
+++ b/arch/x86/boot/compressed/misc.h
@@ -77,6 +77,11 @@ void choose_random_location(unsigned long input,
 			    unsigned long *output,
 			    unsigned long output_size,
 			    unsigned long *virt_addr);
+struct mem_vector {
+	unsigned long long start;
+	unsigned long long size;
+};
+
 /* cpuflags.c */
 bool has_cpuflag(int flag);
 #else
@@ -116,3 +121,17 @@ static inline void console_init(void)
 void set_sev_encryption_mask(void);
 
 #endif
+
+/* acpi.c */
+#ifdef CONFIG_RANDOMIZE_BASE
+/* Amount of immovable memory regions */
+int num_immovable_mem;
+#endif
+
+#ifdef CONFIG_EARLY_SRAT_PARSE
+void get_immovable_mem(void);
+#else
+static void get_immovable_mem(void)
+{
+}
+#endif
-- 
2.19.2

[PATCH v14 5/5] x86/boot/KASLR: Limit KASLR to extracting kernel in immovable memory

[Chao Fan]

KASLR randomly chooses some positions which may locate in movable
memory regions. It will break memory hotplug feature and make the
movable memory chosen by KASLR practically immovable.

The solution is to limit KASLR to choose memory regions in immovable
node according to SRAT tables.
When CONFIG_EARLY_PARSE_RSDP is enabled, walk through SRAT to get the
information of immovable memory so that KASLR knows where should be
chosen for randomization.

Rename process_mem_region() as __process_mem_region() and name new
function as process_mem_region().

Signed-off-by: Chao Fan <fanc.fnst@cn.fujitsu.com>
---
 arch/x86/boot/compressed/kaslr.c | 75 +++++++++++++++++++++++++++-----
 1 file changed, 64 insertions(+), 11 deletions(-)

diff --git a/arch/x86/boot/compressed/kaslr.c b/arch/x86/boot/compressed/kaslr.c
index b251572e77af..a301c49ad02c 100644
--- a/arch/x86/boot/compressed/kaslr.c
+++ b/arch/x86/boot/compressed/kaslr.c
@@ -97,6 +97,11 @@ static bool memmap_too_large;
 /* Store memory limit specified by "mem=nn[KMG]" or "memmap=nn[KMG]" */
 static unsigned long long mem_limit = ULLONG_MAX;
 
+#ifdef CONFIG_EARLY_SRAT_PARSE
+/* The immovable memory regions */
+extern struct mem_vector immovable_mem[MAX_NUMNODES*2];
+#endif
+
 
 enum mem_avoid_index {
 	MEM_AVOID_ZO_RANGE = 0,
@@ -413,6 +418,9 @@ static void mem_avoid_init(unsigned long input, unsigned long input_size,
 	/* Mark the memmap regions we need to avoid */
 	handle_mem_options();
 
+	/* Mark the immovable regions we need to choose */
+	get_immovable_mem();
+
 #ifdef CONFIG_X86_VERBOSE_BOOTUP
 	/* Make sure video RAM can be used. */
 	add_identity_map(0, PMD_SIZE);
@@ -568,9 +576,9 @@ static unsigned long slots_fetch_random(void)
 	return 0;
 }
 
-static void process_mem_region(struct mem_vector *entry,
-			       unsigned long minimum,
-			       unsigned long image_size)
+static void __process_mem_region(struct mem_vector *entry,
+				 unsigned long minimum,
+				 unsigned long image_size)
 {
 	struct mem_vector region, overlap;
 	unsigned long start_orig, end;
@@ -646,6 +654,57 @@ static void process_mem_region(struct mem_vector *entry,
 	}
 }
 
+static bool process_mem_region(struct mem_vector *region,
+			       unsigned long long minimum,
+			       unsigned long long image_size)
+{
+	int i;
+	/*
+	 * If no immovable memory found, or MEMORY_HOTREMOVE disabled,
+	 * walk all the regions, so use region directly.
+	 */
+	if (!num_immovable_mem) {
+		__process_mem_region(region, minimum, image_size);
+
+		if (slot_area_index == MAX_SLOT_AREA) {
+			debug_putstr("Aborted e820/efi memmap scan (slot_areas full)!\n");
+			return 1;
+		}
+		return 0;
+	}
+
+#ifdef CONFIG_EARLY_SRAT_PARSE
+	/*
+	 * If immovable memory found, filter the intersection between
+	 * immovable memory and region to __process_mem_region().
+	 * Otherwise, go on old code.
+	 */
+	for (i = 0; i < num_immovable_mem; i++) {
+		struct mem_vector entry;
+		unsigned long long start, end, entry_end, region_end;
+
+		if (!mem_overlaps(region, &immovable_mem[i]))
+			continue;
+
+		start = immovable_mem[i].start;
+		end = start + immovable_mem[i].size;
+		region_end = region->start + region->size;
+
+		entry.start = clamp(region->start, start, end);
+		entry_end = clamp(region_end, start, end);
+		entry.size = entry_end - entry.start;
+
+		__process_mem_region(&entry, minimum, image_size);
+
+		if (slot_area_index == MAX_SLOT_AREA) {
+			debug_putstr("Aborted e820/efi memmap scan (slot_areas full)!\n");
+			return 1;
+		}
+	}
+	return 0;
+#endif
+}
+
 #ifdef CONFIG_EFI
 /*
  * Returns true if mirror region found (and must have been processed
@@ -711,11 +770,8 @@ process_efi_entries(unsigned long minimum, unsigned long image_size)
 
 		region.start = md->phys_addr;
 		region.size = md->num_pages << EFI_PAGE_SHIFT;
-		process_mem_region(&region, minimum, image_size);
-		if (slot_area_index == MAX_SLOT_AREA) {
-			debug_putstr("Aborted EFI scan (slot_areas full)!\n");
+		if (process_mem_region(&region, minimum, image_size))
 			break;
-		}
 	}
 	return true;
 }
@@ -742,11 +798,8 @@ static void process_e820_entries(unsigned long minimum,
 			continue;
 		region.start = entry->addr;
 		region.size = entry->size;
-		process_mem_region(&region, minimum, image_size);
-		if (slot_area_index == MAX_SLOT_AREA) {
-			debug_putstr("Aborted e820 scan (slot_areas full)!\n");
+		if (process_mem_region(&region, minimum, image_size))
 			break;
-		}
 	}
 }
 
-- 
2.19.2

Re: [PATCH v14 1/5] x86/boot: Introduce get_acpi_rsdp() to parse RSDP in cmdline from KEXEC

[Ingo Molnar]

* Chao Fan <fanc.fnst@cn.fujitsu.com> wrote:

> Memory information in SRAT is necessary to fix the conflict between
> KASLR and memory-hotremove.
> 
> ACPI SRAT (System/Static Resource Affinity Table) shows the details
> about memory ranges, including ranges of memory provided by hot-added
> memory devices. SRAT is introduced by Root System Description
> Pointer(RSDP), so RSDP should be found firstly.
> 
> When booting form KEXEC/EFI/BIOS, the methods to find RSDP
> are different. When booting from KEXEC, 'acpi_rsdp' may have been
> added to cmdline, so parse cmdline to find RSDP.

What is the nature of the basic conflict? Should the kernel only allow 
hot-removing memory ranges that are explicitly marked as such in the 
SRAT? I presume that's already so, correct? Or should the kernel avoiding 
KASLR-randomizing into hot-removable memory areas?

Some minor nits:

> Signed-off-by: Chao Fan <fanc.fnst@cn.fujitsu.com>
> ---
>  arch/x86/boot/compressed/acpi.c | 30 ++++++++++++++++++++++++++++++
>  1 file changed, 30 insertions(+)
>  create mode 100644 arch/x86/boot/compressed/acpi.c
> 
> diff --git a/arch/x86/boot/compressed/acpi.c b/arch/x86/boot/compressed/acpi.c
> new file mode 100644
> index 000000000000..44f19546c169
> --- /dev/null
> +++ b/arch/x86/boot/compressed/acpi.c
> @@ -0,0 +1,30 @@
> +// SPDX-License-Identifier: GPL-2.0
> +#define BOOT_CTYPE_H
> +#include "misc.h"
> +#include "error.h"
> +
> +#include <linux/efi.h>
> +#include <linux/numa.h>
> +#include <linux/acpi.h>
> +#include <asm/efi.h>
> +
> +/* Max length of 64-bit hex address string is 18, prefix "0x" + 16 hex digits */
> +#define MAX_ADDRESS_LENGTH 18

I'd suggest adding the +1 for the string termination \0 as well, and use 
that below.

Also, 'max address length' is way too generic (and hence pretty 
meaningless) - how about MAX_HEX_ADDRESS_STRING_LEN or so?

> +
> +static acpi_physical_address get_acpi_rsdp(void)
> +{
> +#ifdef CONFIG_KEXEC
> +	char val[MAX_ADDRESS_LENGTH+1];
> +	unsigned long long res;
> +	int len;
> +
> +	len = cmdline_find_option("acpi_rsdp", val, MAX_ADDRESS_LENGTH+1);
> +	if (len > 0) {
> +		char *e;
> +
> +		val[len] = '\0';
> +		return (acpi_physical_address)simple_strtoull(val, &e, 16);

'return' is not a function - no need for the parenthesis.

Thanks,

	Ingo

Re: [PATCH v14 2/5] x86/boot: Introduce efi_get_rsdp_addr() to find RSDP from EFI table

[Ingo Molnar]

* Chao Fan <fanc.fnst@cn.fujitsu.com> wrote:

> Memory information in SRAT is necessary to fix the conflict between
> KASLR and memory-hotremove. So RSDP and SRAT should be parsed.
> 
> When booting form KEXEC/EFI/BIOS, the methods to compute RSDP
> are different. When booting from EFI, EFI table points to RSDP.
> So parse the EFI table and find the RSDP.
> 
> Signed-off-by: Chao Fan <fanc.fnst@cn.fujitsu.com>
> ---
>  arch/x86/boot/compressed/acpi.c | 79 +++++++++++++++++++++++++++++++++
>  1 file changed, 79 insertions(+)
> 
> diff --git a/arch/x86/boot/compressed/acpi.c b/arch/x86/boot/compressed/acpi.c
> index 44f19546c169..4151881d8713 100644
> --- a/arch/x86/boot/compressed/acpi.c
> +++ b/arch/x86/boot/compressed/acpi.c
> @@ -28,3 +28,82 @@ static acpi_physical_address get_acpi_rsdp(void)
>  #endif
>  	return 0;
>  }
> +
> +/* Search EFI table for RSDP. */
> +static acpi_physical_address efi_get_rsdp_addr(void)
> +{
> +	acpi_physical_address rsdp_addr = 0;
> +#ifdef CONFIG_EFI
> +	efi_system_table_t *systab;
> +	struct efi_info *e;

'e' is pretty meaningless, the canonical name for efi_info local 
variables is typically 'ei' (although this is not consistent across the 
code).

> +	bool efi_64;

Is this a flag that shows whether the EFI loader is 64-bit?

> +	char *sig;
> +	int size;
> +	int i;
> +
> +	e = &boot_params->efi_info;
> +	sig = (char *)&e->efi_loader_signature;
> +
> +	if (!strncmp(sig, EFI64_LOADER_SIGNATURE, 4)) {
> +		efi_64 = true;
> +	} else if (!strncmp(sig, EFI32_LOADER_SIGNATURE, 4)) {
> +		efi_64 = false;
> +	} else {
> +		debug_putstr("Wrong EFI loader signature.\n");
> +		return 0;
> +	}
> +
> +	/* Get systab from boot params. Based on efi_init(). */
> +#ifdef CONFIG_X86_64
> +	systab = (efi_system_table_t *)(e->efi_systab | ((__u64)e->efi_systab_hi<<32));
> +#else
> +	if (e->efi_systab_hi || e->efi_memmap_hi) {
> +		debug_putstr("Error getting RSDP address: EFI system table located above 4GB.\n");
> +		return 0;
> +	}
> +	systab = (efi_system_table_t *)e->efi_systab;
> +#endif
> +
> +	if (!systab)
> +		return 0;

Is it normal that EFI provides no 'systab'?

> +	/*
> +	 * Get EFI tables from systab. Based on efi_config_init() and
> +	 * efi_config_parse_tables().
> +	 */
> +	size = efi_64 ? sizeof(efi_config_table_64_t) :
> +			sizeof(efi_config_table_32_t);
> +
> +	for (i = 0; i < systab->nr_tables; i++) {
> +		void *config_tables;
> +		unsigned long table;
> +		efi_guid_t guid;
> +
> +		config_tables = (void *)(systab->tables + size * i);
> +		if (efi_64) {
> +			efi_config_table_64_t *tmp_table;
> +
> +			tmp_table = (efi_config_table_64_t *)config_tables;

Since 'config_tables' is a void * there's no need to cast the type.

> +			guid = tmp_table->guid;
> +			table = tmp_table->table;
> +
> +			if (!IS_ENABLED(CONFIG_X86_64) && table >> 32) {
> +				debug_putstr("Error getting RSDP address: EFI config table located above 4GB.\n");
> +				return 0;
> +			}
> +		} else {
> +			efi_config_table_32_t *tmp_table;
> +
> +			tmp_table = (efi_config_table_32_t *)config_tables;

Ditto.

> +			guid = tmp_table->guid;
> +			table = tmp_table->table;
> +		}

So it looks like 

> +
> +		if (!(efi_guidcmp(guid, ACPI_TABLE_GUID)))
> +			rsdp_addr = (acpi_physical_address)table;
> +		else if (!(efi_guidcmp(guid, ACPI_20_TABLE_GUID)))
> +			return (acpi_physical_address)table;

'return' is not a function.

Thanks,

	Ingo

Re: [PATCH v14 2/5] x86/boot: Introduce efi_get_rsdp_addr() to find RSDP from EFI table

[Ingo Molnar]

* Ingo Molnar <mingo@kernel.org> wrote:

> > +		if (!(efi_guidcmp(guid, ACPI_TABLE_GUID)))
> > +			rsdp_addr = (acpi_physical_address)table;
> > +		else if (!(efi_guidcmp(guid, ACPI_20_TABLE_GUID)))
> > +			return (acpi_physical_address)table;
> 
> 'return' is not a function.

Disregard this - I got confused by the type cast.

The type cast is ugly nevertheless. 'table' is an 'unsigned long'.

So 'acpi_physical_address' is basically an u64/u32, depending on 
ACPI_MACHINE_WIDTH, right?

Since this is x86, can ACPI_MACHINE_WIDTH ever get out of sync with the 
native 'unsigned long' size?

If not then why not make the return type 'unsigned long', instead of 
'acpi_physical_address' that you have to wade through a couple of headers 
to figure out its true size. Does that cause complications elsewhere?

I.e. the excessive type casts are ugly and make the code somewhat 
fragile.

Thanks,

	Ingo

Re: [PATCH v14 3/5] x86/boot: Introduce bios_get_rsdp_addr() to search RSDP in memory

[Ingo Molnar]

* Chao Fan <fanc.fnst@cn.fujitsu.com> wrote:

> +		/* Check the standard checksum */
> +		if (compute_checksum((u8 *) rsdp, ACPI_RSDP_CHECKSUM_LENGTH))
> +			continue;

Could you please run your patches through checkpatch, does it not 
complain about this line?

> +/* Search RSDP address, based on acpi_find_root_pointer(). */
> +static acpi_physical_address bios_get_rsdp_addr(void)
> +{
> +	u8 *table_ptr;
> +	u32 address;
> +	u8 *rsdp;
> +
> +	/* Get the location of the Extended BIOS Data Area (EBDA) */
> +	table_ptr = (u8 *)ACPI_EBDA_PTR_LOCATION;
> +	*(u32 *)(void *)&address = *(u16 *)(void *)table_ptr;

what? So you take the address of 'u32 address', which turns it into an 
u32 * - then you cast it to void *, then back to u32 * and then deference 
it???

Thanks,

	Ingo

Re: [PATCH v14 4/5] x86/boot: Parse SRAT address from RSDP and store immovable memory

[Ingo Molnar]

* Chao Fan <fanc.fnst@cn.fujitsu.com> wrote:

> SRAT should be parsed by RSDP to fix the conflict between KASLR
> and memory-hotremove, then find the immovable memory regions and store
> them in an array called immovable_mem[]. With immovable_mem[], KASLR
> can avoid to extract kernel to specific regions.
> 
> Since 'RANDOMIZE_BASE' && 'MEMORY_HOTREMOVE' is needed, introduce
> 'CONFIG_EARLY_PARSE_RSDP' to make ifdeffery clear.
> 
> Signed-off-by: Chao Fan <fanc.fnst@cn.fujitsu.com>
> ---
>  arch/x86/Kconfig                  |  12 +++
>  arch/x86/boot/compressed/Makefile |   2 +
>  arch/x86/boot/compressed/acpi.c   | 128 ++++++++++++++++++++++++++++++
>  arch/x86/boot/compressed/kaslr.c  |   4 -
>  arch/x86/boot/compressed/misc.h   |  19 +++++
>  5 files changed, 161 insertions(+), 4 deletions(-)
> 
> diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
> index ba7e3464ee92..333c383478b7 100644
> --- a/arch/x86/Kconfig
> +++ b/arch/x86/Kconfig
> @@ -2149,6 +2149,18 @@ config X86_NEED_RELOCS
>  	def_bool y
>  	depends on RANDOMIZE_BASE || (X86_32 && RELOCATABLE)
>  
> +config EARLY_SRAT_PARSE
> +	bool "Early SRAT table parsing"
> +	def_bool y
> +	depends on RANDOMIZE_BASE && MEMORY_HOTREMOVE
> +	help
> +	  This option enables early SRAT parsing in compressed boot stage
> +	  so that memory hot-remove ranges do not overlap with KASLR
> +	  chosen ranges. Kernel won't be extracted in hot-removable
> +	  memory, so that make sure memory-hotremove works well with
> +	  KASLR enabled.
> +	  Say Y if you want to use both KASLR and memory-hotremove.

So why would we want to make this a config option, instead of enabling it 
unconditionally?

How reliable are the hot-removable memory markings in various firmware 
versions?

> +/* Compute SRAT table from RSDP. */
> +static struct acpi_table_header *get_acpi_srat_table(void)
> +{
> +	acpi_physical_address acpi_table;
> +	acpi_physical_address root_table;
> +	struct acpi_table_header *header;
> +	struct acpi_table_rsdp *rsdp;
> +	u32 num_entries;
> +	char arg[10];

The '10' is just a magic number attached to a meaningless local variable 
name. Please explain the limit in the code, and the role of the variable 
if it's non-obvious from the name. Or better, try to find a more obvious 
name?

Thanks,

	Ingo

Re: [PATCH v14 5/5] x86/boot/KASLR: Limit KASLR to extracting kernel in immovable memory

[Ingo Molnar]

* Chao Fan <fanc.fnst@cn.fujitsu.com> wrote:

> KASLR randomly chooses some positions which may locate in movable
> memory regions. It will break memory hotplug feature and make the
> movable memory chosen by KASLR practically immovable.
> 
> The solution is to limit KASLR to choose memory regions in immovable
> node according to SRAT tables.
> When CONFIG_EARLY_PARSE_RSDP is enabled, walk through SRAT to get the
> information of immovable memory so that KASLR knows where should be
> chosen for randomization.
> 
> Rename process_mem_region() as __process_mem_region() and name new
> function as process_mem_region().
> 
> Signed-off-by: Chao Fan <fanc.fnst@cn.fujitsu.com>
> ---
>  arch/x86/boot/compressed/kaslr.c | 75 +++++++++++++++++++++++++++-----
>  1 file changed, 64 insertions(+), 11 deletions(-)

Ok, I like this basic approach of automatically detecing memory areas we 
should not KASLR into - it's far better than earlier iterations.

> +++ b/arch/x86/boot/compressed/kaslr.c
> @@ -97,6 +97,11 @@ static bool memmap_too_large;
>  /* Store memory limit specified by "mem=nn[KMG]" or "memmap=nn[KMG]" */
>  static unsigned long long mem_limit = ULLONG_MAX;
>  
> +#ifdef CONFIG_EARLY_SRAT_PARSE
> +/* The immovable memory regions */
> +extern struct mem_vector immovable_mem[MAX_NUMNODES*2];
> +#endif

What logic is the maximum size of this array based on?

Thanks,

	Ingo

Re: [PATCH v14 1/5] x86/boot: Introduce get_acpi_rsdp() to parse RSDP in cmdline from KEXEC

[Thomas Gleixner]

On Mon, 17 Dec 2018, Ingo Molnar wrote:
> * Chao Fan <fanc.fnst@cn.fujitsu.com> wrote:
> > +		val[len] = '\0';
> > +		return (acpi_physical_address)simple_strtoull(val, &e, 16);
> 
> 'return' is not a function - no need for the parenthesis.

It's a typecast which better has parenthesis....

Re: [PATCH v14 2/5] x86/boot: Introduce efi_get_rsdp_addr() to find RSDP from EFI table

[Thomas Gleixner]

On Mon, 17 Dec 2018, Ingo Molnar wrote:
> * Chao Fan <fanc.fnst@cn.fujitsu.com> wrote:
> > +		else if (!(efi_guidcmp(guid, ACPI_20_TABLE_GUID)))
> > +			return (acpi_physical_address)table;
> 
> 'return' is not a function.

It's still a typecast :)

Re: [PATCH v14 1/5] x86/boot: Introduce get_acpi_rsdp() to parse RSDP in cmdline from KEXEC

[Ingo Molnar]

* Thomas Gleixner <tglx@linutronix.de> wrote:

> On Mon, 17 Dec 2018, Ingo Molnar wrote:
> > * Chao Fan <fanc.fnst@cn.fujitsu.com> wrote:
> > > +		val[len] = '\0';
> > > +		return (acpi_physical_address)simple_strtoull(val, &e, 16);
> > 
> > 'return' is not a function - no need for the parenthesis.
> 
> It's a typecast which better has parenthesis....

Yeah, see my followup to the other reply where I withdraw the objection 
and point out the ugliness of the various typecasts.

Thanks,

	Ingo

Re: [PATCH v14 2/5] x86/boot: Introduce efi_get_rsdp_addr() to find RSDP from EFI table

[Ingo Molnar]

* Thomas Gleixner <tglx@linutronix.de> wrote:

> On Mon, 17 Dec 2018, Ingo Molnar wrote:
> > * Chao Fan <fanc.fnst@cn.fujitsu.com> wrote:
> > > +		else if (!(efi_guidcmp(guid, ACPI_20_TABLE_GUID)))
> > > +			return (acpi_physical_address)table;
> > 
> > 'return' is not a function.
> 
> It's still a typecast :)

Yeah, see my followup from an hour ago:

  Message-ID: <20181217173605.GA14613@gmail.com>

Thanks,

	Ingo

Re: [PATCH v14 1/5] x86/boot: Introduce get_acpi_rsdp() to parse RSDP in cmdline from KEXEC

[Chao Fan]

On Mon, Dec 17, 2018 at 06:25:10PM +0100, Ingo Molnar wrote:
>
>* Chao Fan <fanc.fnst@cn.fujitsu.com> wrote:
>
>> Memory information in SRAT is necessary to fix the conflict between
>> KASLR and memory-hotremove.
>> 
>> ACPI SRAT (System/Static Resource Affinity Table) shows the details
>> about memory ranges, including ranges of memory provided by hot-added
>> memory devices. SRAT is introduced by Root System Description
>> Pointer(RSDP), so RSDP should be found firstly.
>> 
>> When booting form KEXEC/EFI/BIOS, the methods to find RSDP
>> are different. When booting from KEXEC, 'acpi_rsdp' may have been
>> added to cmdline, so parse cmdline to find RSDP.
>
>What is the nature of the basic conflict? Should the kernel only allow 
>hot-removing memory ranges that are explicitly marked as such in the 
>SRAT? I presume that's already so, correct? Or should the kernel avoiding 
>KASLR-randomizing into hot-removable memory areas?

The basic conflict is the kernel avoiding KASLR-randomizing into
hot-removable memory areas.

>
>Some minor nits:
>
>> Signed-off-by: Chao Fan <fanc.fnst@cn.fujitsu.com>
>> ---
>>  arch/x86/boot/compressed/acpi.c | 30 ++++++++++++++++++++++++++++++
>>  1 file changed, 30 insertions(+)
>>  create mode 100644 arch/x86/boot/compressed/acpi.c
>> 
>> diff --git a/arch/x86/boot/compressed/acpi.c b/arch/x86/boot/compressed/acpi.c
>> new file mode 100644
>> index 000000000000..44f19546c169
>> --- /dev/null
>> +++ b/arch/x86/boot/compressed/acpi.c
>> @@ -0,0 +1,30 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +#define BOOT_CTYPE_H
>> +#include "misc.h"
>> +#include "error.h"
>> +
>> +#include <linux/efi.h>
>> +#include <linux/numa.h>
>> +#include <linux/acpi.h>
>> +#include <asm/efi.h>
>> +
>> +/* Max length of 64-bit hex address string is 18, prefix "0x" + 16 hex digits */
>> +#define MAX_ADDRESS_LENGTH 18
>
>I'd suggest adding the +1 for the string termination \0 as well, and use 
>that below.
>
>Also, 'max address length' is way too generic (and hence pretty 
>meaningless) - how about MAX_HEX_ADDRESS_STRING_LEN or so?

OK, I will change the name and add the \0.

Thanks,
Chao Fan

>
>> +
>> +static acpi_physical_address get_acpi_rsdp(void)
>> +{
>> +#ifdef CONFIG_KEXEC
>> +	char val[MAX_ADDRESS_LENGTH+1];
>> +	unsigned long long res;
>> +	int len;
>> +
>> +	len = cmdline_find_option("acpi_rsdp", val, MAX_ADDRESS_LENGTH+1);
>> +	if (len > 0) {
>> +		char *e;
>> +
>> +		val[len] = '\0';
>> +		return (acpi_physical_address)simple_strtoull(val, &e, 16);
>
>'return' is not a function - no need for the parenthesis.
>
>Thanks,
>
>	Ingo
>
>

Re: [PATCH v14 2/5] x86/boot: Introduce efi_get_rsdp_addr() to find RSDP from EFI table

[Chao Fan]

On Mon, Dec 17, 2018 at 06:30:32PM +0100, Ingo Molnar wrote:
>
>* Chao Fan <fanc.fnst@cn.fujitsu.com> wrote:
>
>> Memory information in SRAT is necessary to fix the conflict between
>> KASLR and memory-hotremove. So RSDP and SRAT should be parsed.
>> 
>> When booting form KEXEC/EFI/BIOS, the methods to compute RSDP
>> are different. When booting from EFI, EFI table points to RSDP.
>> So parse the EFI table and find the RSDP.
>> 
>> Signed-off-by: Chao Fan <fanc.fnst@cn.fujitsu.com>
>> ---
>>  arch/x86/boot/compressed/acpi.c | 79 +++++++++++++++++++++++++++++++++
>>  1 file changed, 79 insertions(+)
>> 
>> diff --git a/arch/x86/boot/compressed/acpi.c b/arch/x86/boot/compressed/acpi.c
>> index 44f19546c169..4151881d8713 100644
>> --- a/arch/x86/boot/compressed/acpi.c
>> +++ b/arch/x86/boot/compressed/acpi.c
>> @@ -28,3 +28,82 @@ static acpi_physical_address get_acpi_rsdp(void)
>>  #endif
>>  	return 0;
>>  }
>> +
>> +/* Search EFI table for RSDP. */
>> +static acpi_physical_address efi_get_rsdp_addr(void)
>> +{
>> +	acpi_physical_address rsdp_addr = 0;
>> +#ifdef CONFIG_EFI
>> +	efi_system_table_t *systab;
>> +	struct efi_info *e;
>
>'e' is pretty meaningless, the canonical name for efi_info local 
>variables is typically 'ei' (although this is not consistent across the 
>code).

OK, will change it.
>
>> +	bool efi_64;
>
>Is this a flag that shows whether the EFI loader is 64-bit?

Yes, I use the signature to decide the size of EFI table.
It's used here:
+	size = efi_64 ? sizeof(efi_config_table_64_t) :
+			sizeof(efi_config_table_32_t);

>
>> +	char *sig;
>> +	int size;
>> +	int i;
>> +
>> +	e = &boot_params->efi_info;
>> +	sig = (char *)&e->efi_loader_signature;
>> +
>> +	if (!strncmp(sig, EFI64_LOADER_SIGNATURE, 4)) {
>> +		efi_64 = true;
>> +	} else if (!strncmp(sig, EFI32_LOADER_SIGNATURE, 4)) {
>> +		efi_64 = false;
>> +	} else {
>> +		debug_putstr("Wrong EFI loader signature.\n");
>> +		return 0;
>> +	}
>> +
>> +	/* Get systab from boot params. Based on efi_init(). */
>> +#ifdef CONFIG_X86_64
>> +	systab = (efi_system_table_t *)(e->efi_systab | ((__u64)e->efi_systab_hi<<32));
>> +#else
>> +	if (e->efi_systab_hi || e->efi_memmap_hi) {
>> +		debug_putstr("Error getting RSDP address: EFI system table located above 4GB.\n");
>> +		return 0;
>> +	}
>> +	systab = (efi_system_table_t *)e->efi_systab;
>> +#endif
>> +
>> +	if (!systab)
>> +		return 0;
>
>Is it normal that EFI provides no 'systab'?

I think not normal, to make sure the code run normally.
We will use it below like: i < systab->nr_tables, if
systab is wrong, the code will fail to work.

>
>> +	/*
>> +	 * Get EFI tables from systab. Based on efi_config_init() and
>> +	 * efi_config_parse_tables().
>> +	 */
>> +	size = efi_64 ? sizeof(efi_config_table_64_t) :
>> +			sizeof(efi_config_table_32_t);
>> +
>> +	for (i = 0; i < systab->nr_tables; i++) {
>> +		void *config_tables;
>> +		unsigned long table;
>> +		efi_guid_t guid;
>> +
>> +		config_tables = (void *)(systab->tables + size * i);
>> +		if (efi_64) {
>> +			efi_config_table_64_t *tmp_table;
>> +
>> +			tmp_table = (efi_config_table_64_t *)config_tables;
>
>Since 'config_tables' is a void * there's no need to cast the type.
>
>> +			guid = tmp_table->guid;
>> +			table = tmp_table->table;
>> +
>> +			if (!IS_ENABLED(CONFIG_X86_64) && table >> 32) {
>> +				debug_putstr("Error getting RSDP address: EFI config table located above 4GB.\n");
>> +				return 0;
>> +			}
>> +		} else {
>> +			efi_config_table_32_t *tmp_table;
>> +
>> +			tmp_table = (efi_config_table_32_t *)config_tables;
>
>Ditto.
>
>> +			guid = tmp_table->guid;
>> +			table = tmp_table->table;
>> +		}
>
>So it looks like 
>
>> +
>> +		if (!(efi_guidcmp(guid, ACPI_TABLE_GUID)))
>> +			rsdp_addr = (acpi_physical_address)table;
>> +		else if (!(efi_guidcmp(guid, ACPI_20_TABLE_GUID)))
>> +			return (acpi_physical_address)table;
>
>'return' is not a function.

Got it, will clean the type cast.

Thanks,
Chao Fan

>
>Thanks,
>
>	Ingo
>
>

Re: [PATCH v14 3/5] x86/boot: Introduce bios_get_rsdp_addr() to search RSDP in memory

[Chao Fan]

On Mon, Dec 17, 2018 at 06:38:37PM +0100, Ingo Molnar wrote:
>
>* Chao Fan <fanc.fnst@cn.fujitsu.com> wrote:
>
>> +		/* Check the standard checksum */
>> +		if (compute_checksum((u8 *) rsdp, ACPI_RSDP_CHECKSUM_LENGTH))
>> +			continue;
>
>Could you please run your patches through checkpatch, does it not 
>complain about this line?

I did,  0 errors, 0 warnings.
If you mean the useless space, I can drop it.
But it's from acpi_tb_validate_rsdp() of drivers/acpi/acpica/tbxfroot.c.
>
>> +/* Search RSDP address, based on acpi_find_root_pointer(). */
>> +static acpi_physical_address bios_get_rsdp_addr(void)
>> +{
>> +	u8 *table_ptr;
>> +	u32 address;
>> +	u8 *rsdp;
>> +
>> +	/* Get the location of the Extended BIOS Data Area (EBDA) */
>> +	table_ptr = (u8 *)ACPI_EBDA_PTR_LOCATION;
>> +	*(u32 *)(void *)&address = *(u16 *)(void *)table_ptr;
>
>what? So you take the address of 'u32 address', which turns it into an 
>u32 * - then you cast it to void *, then back to u32 * and then deference 
>it???

I will clean it, it's from
#define ACPI_MOVE_16_TO_32(d, s)        *(u32 *)(void *)(d) = *(u16 *)(void *)(s)
in acpi_find_root_pointer().
I thought it is safe to use the code existing in kernel.

Thanks,
Chao Fan

>
>Thanks,
>
>	Ingo
>
>

Re: [PATCH v14 5/5] x86/boot/KASLR: Limit KASLR to extracting kernel in immovable memory

[Chao Fan]

On Mon, Dec 17, 2018 at 06:43:24PM +0100, Ingo Molnar wrote:
>
>* Chao Fan <fanc.fnst@cn.fujitsu.com> wrote:
>
>> KASLR randomly chooses some positions which may locate in movable
>> memory regions. It will break memory hotplug feature and make the
>> movable memory chosen by KASLR practically immovable.
>> 
>> The solution is to limit KASLR to choose memory regions in immovable
>> node according to SRAT tables.
>> When CONFIG_EARLY_PARSE_RSDP is enabled, walk through SRAT to get the
>> information of immovable memory so that KASLR knows where should be
>> chosen for randomization.
>> 
>> Rename process_mem_region() as __process_mem_region() and name new
>> function as process_mem_region().
>> 
>> Signed-off-by: Chao Fan <fanc.fnst@cn.fujitsu.com>
>> ---
>>  arch/x86/boot/compressed/kaslr.c | 75 +++++++++++++++++++++++++++-----
>>  1 file changed, 64 insertions(+), 11 deletions(-)
>
>Ok, I like this basic approach of automatically detecing memory areas we 
>should not KASLR into - it's far better than earlier iterations.

Thanks,

>
>> +++ b/arch/x86/boot/compressed/kaslr.c
>> @@ -97,6 +97,11 @@ static bool memmap_too_large;
>>  /* Store memory limit specified by "mem=nn[KMG]" or "memmap=nn[KMG]" */
>>  static unsigned long long mem_limit = ULLONG_MAX;
>>  
>> +#ifdef CONFIG_EARLY_SRAT_PARSE
>> +/* The immovable memory regions */
>> +extern struct mem_vector immovable_mem[MAX_NUMNODES*2];
>> +#endif
>
>What logic is the maximum size of this array based on?
>

Oh, sorry for that, I ever explained for that, I would add
more comments in next PATCH.

See arch/x86/mm/numa_internal.h:
struct numa_meminfo {
        int                     nr_blks;
        struct numa_memblk      blk[NR_NODE_MEMBLKS];
};

In arch/x86/include/asm/numa.h:
#define NR_NODE_MEMBLKS         (MAX_NUMNODES*2)

That means the memory in one node may be devided into 1 or 2 memory
regions(Also I saw that in the dmesg).
So think about how many regions we need to store the immovable memory.
The worst condition is:
1. There are MAX_NUMANODES nodes on this machine.
2. In SRAT table, every node is devided into 2 memory regions.
3. All of them are immovable.
So MAX_NUMNODES*2 is the biggest amount.

Thanks,
Chao Fan

>Thanks,
>
>	Ingo
>
>

Re: [PATCH v14 4/5] x86/boot: Parse SRAT address from RSDP and store immovable memory

[Chao Fan]

On Mon, Dec 17, 2018 at 06:41:49PM +0100, Ingo Molnar wrote:
>
>* Chao Fan <fanc.fnst@cn.fujitsu.com> wrote:
>
>> SRAT should be parsed by RSDP to fix the conflict between KASLR
>> and memory-hotremove, then find the immovable memory regions and store
>> them in an array called immovable_mem[]. With immovable_mem[], KASLR
>> can avoid to extract kernel to specific regions.
>> 
>> Since 'RANDOMIZE_BASE' && 'MEMORY_HOTREMOVE' is needed, introduce
>> 'CONFIG_EARLY_PARSE_RSDP' to make ifdeffery clear.
>> 
>> Signed-off-by: Chao Fan <fanc.fnst@cn.fujitsu.com>
>> ---
>>  arch/x86/Kconfig                  |  12 +++
>>  arch/x86/boot/compressed/Makefile |   2 +
>>  arch/x86/boot/compressed/acpi.c   | 128 ++++++++++++++++++++++++++++++
>>  arch/x86/boot/compressed/kaslr.c  |   4 -
>>  arch/x86/boot/compressed/misc.h   |  19 +++++
>>  5 files changed, 161 insertions(+), 4 deletions(-)
>> 
>> diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
>> index ba7e3464ee92..333c383478b7 100644
>> --- a/arch/x86/Kconfig
>> +++ b/arch/x86/Kconfig
>> @@ -2149,6 +2149,18 @@ config X86_NEED_RELOCS
>>  	def_bool y
>>  	depends on RANDOMIZE_BASE || (X86_32 && RELOCATABLE)
>>  
>> +config EARLY_SRAT_PARSE
>> +	bool "Early SRAT table parsing"
>> +	def_bool y
>> +	depends on RANDOMIZE_BASE && MEMORY_HOTREMOVE
>> +	help
>> +	  This option enables early SRAT parsing in compressed boot stage
>> +	  so that memory hot-remove ranges do not overlap with KASLR
>> +	  chosen ranges. Kernel won't be extracted in hot-removable
>> +	  memory, so that make sure memory-hotremove works well with
>> +	  KASLR enabled.
>> +	  Say Y if you want to use both KASLR and memory-hotremove.
>
>So why would we want to make this a config option, instead of enabling it 
>unconditionally?

Well, it can make ifdeffery more clear, and Boris suggested to do that.
If there is no KASLR enabled or MEMORY-HOTREMOVE enabled, the code is
not needed, so it's not good to enable it unconditionally.

>
>How reliable are the hot-removable memory markings in various firmware 
>versions?

But we can only get the information from firmware, I can't figure out
other information. And ACPI also gets the table from here.

>
>> +/* Compute SRAT table from RSDP. */
>> +static struct acpi_table_header *get_acpi_srat_table(void)
>> +{
>> +	acpi_physical_address acpi_table;
>> +	acpi_physical_address root_table;
>> +	struct acpi_table_header *header;
>> +	struct acpi_table_rsdp *rsdp;
>> +	u32 num_entries;
>> +	char arg[10];
>
>The '10' is just a magic number attached to a meaningless local variable 
>name. Please explain the limit in the code, and the role of the variable 
>if it's non-obvious from the name. Or better, try to find a more obvious 
>name?
>

Yes, the '10' is magic number, the meaning is detail. Here, the '10'
is used to store the result of 'acpi=' in cmdline. Well, see
Documentation/admin-guide/kernel-parameters.txt:
        acpi=           [HW,ACPI,X86,ARM64]
                        Advanced Configuration and Power Interface
                        Format: { force | on | off | strict | noirq | rsdt |
                                  copy_dsdt }
'copy_dsdt' is the longest result, its size is '9', plus '\0' is '10'.
So I set it as '10'.

And I see the usage like this is:
        char arg[5];
in pti_check_boottime_disable() of arch/x86/mm/pti.c, since the longest
result of 'pti=' in cmdline is 'auto', so it's '5' here.

And:
        char arg[32];
in fpu__init_parse_early_param() of arch/x86/kernel/fpu/init.c.

And so on. All usage of cmdline_find_option() needs a string, they are
often named as arg[] and with a number which can cover the longest result.

Thanks,
Chao Fan

>Thanks,
>
>	Ingo
>
>

Re: [PATCH v14 1/5] x86/boot: Introduce get_acpi_rsdp() to parse RSDP in cmdline from KEXEC

[Borislav Petkov]

On Tue, Dec 18, 2018 at 09:27:04AM +0800, Chao Fan wrote:
> The basic conflict is the kernel avoiding KASLR-randomizing into
> hot-removable memory areas.

And this should be in a prominent place in your commit message to give
an idea *why* you're doing this in the first place.

-- 
Regards/Gruss,
    Boris.

Good mailing practices for 400: avoid top-posting and trim the reply.

Re: [PATCH v14 1/5] x86/boot: Introduce get_acpi_rsdp() to parse RSDP in cmdline from KEXEC

[Chao Fan]

On Tue, Dec 18, 2018 at 12:28:48PM +0100, Borislav Petkov wrote:
>On Tue, Dec 18, 2018 at 09:27:04AM +0800, Chao Fan wrote:
>> The basic conflict is the kernel avoiding KASLR-randomizing into
>> hot-removable memory areas.
>
>And this should be in a prominent place in your commit message to give
>an idea *why* you're doing this in the first place.

Got it, I would complain more in the first PATH of this series.

Thanks,
Chao Fan
>
>-- 
>Regards/Gruss,
>    Boris.
>
>Good mailing practices for 400: avoid top-posting and trim the reply.
>
>

Re: [PATCH v14 2/5] x86/boot: Introduce efi_get_rsdp_addr() to find RSDP from EFI table

[Chao Fan]

On Mon, Dec 17, 2018 at 06:36:05PM +0100, Ingo Molnar wrote:
>
>* Ingo Molnar <mingo@kernel.org> wrote:
>
>> > +		if (!(efi_guidcmp(guid, ACPI_TABLE_GUID)))
>> > +			rsdp_addr = (acpi_physical_address)table;
>> > +		else if (!(efi_guidcmp(guid, ACPI_20_TABLE_GUID)))
>> > +			return (acpi_physical_address)table;
>> 
>> 'return' is not a function.
>

Hi Ingo,

I will remove the typecast in next version.
But when doing more tests, I found a special condition.

>Disregard this - I got confused by the type cast.
>
>The type cast is ugly nevertheless. 'table' is an 'unsigned long'.
>
>So 'acpi_physical_address' is basically an u64/u32, depending on 
>ACPI_MACHINE_WIDTH, right?
>
>Since this is x86, can ACPI_MACHINE_WIDTH ever get out of sync with the 
>native 'unsigned long' size?

Not always.
Here is define of acpi_physical_address:

#if ACPI_MACHINE_WIDTH == 64
typedef u64 acpi_physical_address;
#elif ACPI_MACHINE_WIDTH == 32
#ifdef ACPI_32BIT_PHYSICAL_ADDRESS
typedef u32 acpi_physical_address;
#else                           /* ACPI_32BIT_PHYSICAL_ADDRESS */

/*
 * It is reported that, after some calculations, the physical addresses
can
 * wrap over the 32-bit boundary on 32-bit PAE environment.
 * https://bugzilla.kernel.org/show_bug.cgi?id=87971
 */
typedef u64 acpi_physical_address;
#endif

That means in 32-bit with PAE, acpi_physical_address is u64.
I set up a debian9-32bit with PAE in QEMU and found size of
acpi_physical_address is 8. So they are not always sync.
Of course, that doesn't influence the conclusion, since return table directly
can always work. It will return u32 to u32, u64 to u64, or u32 to u64.
So the typecast about acpi_physical_address can be removed.

Thanks,
Chao Fan

>
>If not then why not make the return type 'unsigned long', instead of 
>'acpi_physical_address' that you have to wade through a couple of headers 
>to figure out its true size. Does that cause complications elsewhere?
>
>I.e. the excessive type casts are ugly and make the code somewhat 
>fragile.
>
>Thanks,
>
>	Ingo
>
>