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

[PATCH v15 0/6] 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 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-hotremove is in ACPI
SRAT, which will be parsed after start_kernel(), so KASLR
can't get the information.

Someone suggested to add a kernel parameter to specify the
immovable memory then 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.
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/6 Copy kstrtoull() to boot/string.c to instead of using
          old simple_strtoull()
PATCH 2/6 Introduce get_acpi_rsdp() to parse RSDP in cmdline from KEXEC
PATCH 3/6 Introduce efi_get_rsdp_addr() to find RSDP from EFI table when
          booting from EFI.
PATCH 4/6 Introduce bios_get_rsdp_addr() to search RSDP in memory when
          booting from BIOS
PATCH 5/6 Compute SRAT from RSDP and walk SRAT to store the immovable
          memory regions and store the immovable memory regions.
PATCH 6/6 Calculate the intersection between memory regions from e820/efi
          memory table and immovable memory regions. Limit KASLR to
          choosing these regions for randomization.

***Tests:
Test it in QEMU guest with 10 NUMA nodes, every node has 512M memory.
Use earlyprintk with debug_putaddr() to display the immovable
memory regions when KASLR walks all memory. Compare the result with
dmesg. If they are same, the parsing job works well.
Here are test cases:
 - x86_64 machine booting from EFI
 - x86_64 machine booting from BIOS
 - i386 machine booting from BIOS
 - According to '/sys/firmware/efi/systab', add 'acpi_rsdp=' to cmdline to
   imitate case for get_acpi_rsdp()
 - Add 'acpi=off' to cmdline to test the detecting for cmdline.

v14->v15:
Follow Boris' suggestion:
 - Fix the usage of kstrtoull()
 - Add more test case
Follow Ingo's suggestion:
 - Clear the typecast
 - Change some variable name
 - If no efi systab found, use error() to halt
Follow Baoquan's suggestion:
 - Rebase from tip/master to tip/x86/boot

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 (6):
  x86/boot: Copy kstrtoull() to boot/string.c instead of using
    simple_strtoull()
  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   | 337 ++++++++++++++++++++++++++++++
 arch/x86/boot/compressed/kaslr.c  |  83 ++++++--
 arch/x86/boot/compressed/misc.h   |  19 ++
 arch/x86/boot/string.c            | 137 ++++++++++++
 arch/x86/boot/string.h            |   2 +
 7 files changed, 577 insertions(+), 15 deletions(-)
 create mode 100644 arch/x86/boot/compressed/acpi.c

-- 
2.20.1

[PATCH v15 1/6] x86/boot: Copy kstrtoull() to boot/string.c instead of using simple_strtoull()

[Chao Fan]

Copy kstrtoull() and necessary functions from lib/kstrtox.c to
boot/string.c so that code in boot/ can use kstrtoull() and the old
simple_strtoull() can be replaced.

In boot/string.c, using div_u64() from math64.h directly will cause the
dividend handled as 64-bit value and bring ld error. The solution is to
separate the dividend to upper and lower in boot/string.o. So copy the
useful div_u64() and div_u64_rem() to boot/string.c also. To avoid
redefinition in i386, rename them as __div_u64() and __div_u64_rem().

Signed-off-by: Chao Fan <fanc.fnst@cn.fujitsu.com>
---
 arch/x86/boot/string.c | 137 +++++++++++++++++++++++++++++++++++++++++
 arch/x86/boot/string.h |   2 +
 2 files changed, 139 insertions(+)

diff --git a/arch/x86/boot/string.c b/arch/x86/boot/string.c
index c4428a176973..3de8de39a1ca 100644
--- a/arch/x86/boot/string.c
+++ b/arch/x86/boot/string.c
@@ -13,6 +13,8 @@
  */
 
 #include <linux/types.h>
+#include <linux/kernel.h>
+#include <linux/errno.h>
 #include <asm/asm.h>
 #include "ctype.h"
 #include "string.h"
@@ -187,3 +189,138 @@ char *strchr(const char *s, int c)
 			return NULL;
 	return (char *)s;
 }
+
+static inline u64 __div_u64_rem(u64 dividend, u32 divisor, u32 *remainder)
+{
+	union {
+		u64 v64;
+		u32 v32[2];
+	} d = { dividend };
+	u32 upper;
+
+	upper = d.v32[1];
+	d.v32[1] = 0;
+	if (upper >= divisor) {
+		d.v32[1] = upper / divisor;
+		upper %= divisor;
+	}
+	asm ("divl %2" : "=a" (d.v32[0]), "=d" (*remainder) :
+		"rm" (divisor), "0" (d.v32[0]), "1" (upper));
+	return d.v64;
+}
+
+static inline u64 __div_u64(u64 dividend, u32 divisor)
+{
+	u32 remainder;
+
+	return __div_u64_rem(dividend, divisor, &remainder);
+}
+
+static inline char _tolower(const char c)
+{
+	return c | 0x20;
+}
+
+const char *_parse_integer_fixup_radix(const char *s, unsigned int *base)
+{
+	if (*base == 0) {
+		if (s[0] == '0') {
+			if (_tolower(s[1]) == 'x' && isxdigit(s[2]))
+				*base = 16;
+			else
+				*base = 8;
+		} else
+			*base = 10;
+	}
+	if (*base == 16 && s[0] == '0' && _tolower(s[1]) == 'x')
+		s += 2;
+	return s;
+}
+
+/*
+ * Convert non-negative integer string representation in explicitly given radix
+ * to an integer.
+ * Return number of characters consumed maybe or-ed with overflow bit.
+ * If overflow occurs, result integer (incorrect) is still returned.
+ *
+ * Don't you dare use this function.
+ */
+unsigned int _parse_integer(const char *s, unsigned int base, unsigned long long *p)
+{
+	unsigned long long res;
+	unsigned int rv;
+
+	res = 0;
+	rv = 0;
+	while (1) {
+		unsigned int c = *s;
+		unsigned int lc = c | 0x20; /* don't tolower() this line */
+		unsigned int val;
+
+		if ('0' <= c && c <= '9')
+			val = c - '0';
+		else if ('a' <= lc && lc <= 'f')
+			val = lc - 'a' + 10;
+		else
+			break;
+
+		if (val >= base)
+			break;
+		/*
+		 * Check for overflow only if we are within range of
+		 * it in the max base we support (16)
+		 */
+		if (unlikely(res & (~0ull << 60))) {
+			if (res > __div_u64(ULLONG_MAX - val, base))
+				rv |= KSTRTOX_OVERFLOW;
+		}
+		res = res * base + val;
+		rv++;
+		s++;
+	}
+	*p = res;
+	return rv;
+}
+
+static int _kstrtoull(const char *s, unsigned int base, unsigned long long *res)
+{
+	unsigned long long _res;
+	unsigned int rv;
+
+	s = _parse_integer_fixup_radix(s, &base);
+	rv = _parse_integer(s, base, &_res);
+	if (rv & KSTRTOX_OVERFLOW)
+		return -ERANGE;
+	if (rv == 0)
+		return -EINVAL;
+	s += rv;
+	if (*s == '\n')
+		s++;
+	if (*s)
+		return -EINVAL;
+	*res = _res;
+	return 0;
+}
+
+/**
+ * kstrtoull - convert a string to an unsigned long long
+ * @s: The start of the string. The string must be null-terminated, and may also
+ *  include a single newline before its terminating null. The first character
+ *  may also be a plus sign, but not a minus sign.
+ * @base: The number base to use. The maximum supported base is 16. If base is
+ *  given as 0, then the base of the string is automatically detected with the
+ *  conventional semantics - If it begins with 0x the number will be parsed as a
+ *  hexadecimal (case insensitive), if it otherwise begins with 0, it will be
+ *  parsed as an octal number. Otherwise it will be parsed as a decimal.
+ * @res: Where to write the result of the conversion on success.
+ *
+ * Returns 0 on success, -ERANGE on overflow and -EINVAL on parsing error.
+ * Used as a replacement for the obsolete simple_strtoull. Return code must
+ * be checked.
+ */
+int kstrtoull(const char *s, unsigned int base, unsigned long long *res)
+{
+	if (s[0] == '+')
+		s++;
+	return _kstrtoull(s, base, res);
+}
diff --git a/arch/x86/boot/string.h b/arch/x86/boot/string.h
index 3d78e27077f4..171007c99acc 100644
--- a/arch/x86/boot/string.h
+++ b/arch/x86/boot/string.h
@@ -29,4 +29,6 @@ extern unsigned int atou(const char *s);
 extern unsigned long long simple_strtoull(const char *cp, char **endp,
 					  unsigned int base);
 
+int kstrtoull(const char *s, unsigned int base, unsigned long long *res);
+#define KSTRTOX_OVERFLOW       (1U << 31)
 #endif /* BOOT_STRING_H */
-- 
2.20.1

[PATCH v15 2/6] x86/boot: Introduce get_acpi_rsdp() to parse RSDP in cmdline from KEXEC

[Chao Fan]

KASLR may randomly choose some positions which are located in movable
memory regions. This will make the movable memory chosen by KASLR
can't be removed.

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..7ca5001d7639
--- /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 "../string.h"
+
+#include <linux/acpi.h>
+
+/*
+ * Max length of 64-bit hex address string is 19, prefix "0x" + 16 hex
+ * digits, and '\0' for termination.
+ */
+#define MAX_HEX_ADDRESS_STRING_LEN 19
+
+static acpi_physical_address get_acpi_rsdp(void)
+{
+#ifdef CONFIG_KEXEC
+	char val[MAX_HEX_ADDRESS_STRING_LEN];
+	unsigned long long res;
+	int len = 0;
+
+	len = cmdline_find_option("acpi_rsdp", val, MAX_HEX_ADDRESS_STRING_LEN);
+	if (len > 0) {
+		val[len] = '\0';
+		if (!kstrtoull(val, 16, &res))
+			return res;
+	}
+#endif
+	return 0;
+}
-- 
2.20.1

[PATCH v15 3/6] 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 | 83 +++++++++++++++++++++++++++++++++
 1 file changed, 83 insertions(+)

diff --git a/arch/x86/boot/compressed/acpi.c b/arch/x86/boot/compressed/acpi.c
index 7ca5001d7639..f74c5d033d79 100644
--- a/arch/x86/boot/compressed/acpi.c
+++ b/arch/x86/boot/compressed/acpi.c
@@ -5,6 +5,8 @@
 #include "../string.h"
 
 #include <linux/acpi.h>
+#include <linux/efi.h>
+#include <asm/efi.h>
 
 /*
  * Max length of 64-bit hex address string is 19, prefix "0x" + 16 hex
@@ -28,3 +30,84 @@ 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 *ei;
+	bool efi_64;
+	char *sig;
+	int size;
+	int i;
+
+	ei = &boot_params->efi_info;
+	sig = (char *)&ei->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 *)(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");
+		return 0;
+	}
+	systab = (efi_system_table_t *)ei->efi_systab;
+#endif
+
+	if (!systab)
+		error("EFI system table is not found.");
+
+	/*
+	 * 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;
+			u64 table64;
+
+			tmp_table = config_tables;
+			guid = tmp_table->guid;
+			table64 = tmp_table->table;
+			table = table64;
+
+			if (!IS_ENABLED(CONFIG_X86_64) && table64 >> 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 = config_tables;
+			guid = tmp_table->guid;
+			table = tmp_table->table;
+		}
+
+		if (!(efi_guidcmp(guid, ACPI_TABLE_GUID)))
+			rsdp_addr = table;
+		else if (!(efi_guidcmp(guid, ACPI_20_TABLE_GUID)))
+			return table;
+	}
+#endif
+	return rsdp_addr;
+}
-- 
2.20.1

[PATCH v15 4/6] x86/boot: Introduce bios_get_rsdp_addr() to search RSDP in memory

[Chao Fan]

Memory information in SRAT table 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 f74c5d033d79..e9dd84f459ed 100644
--- a/arch/x86/boot/compressed/acpi.c
+++ b/arch/x86/boot/compressed/acpi.c
@@ -111,3 +111,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;
+	address = *(u16 *)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 address;
+		}
+	}
+
+	/* Search upper memory: 16-byte boundaries in E0000h-FFFFFh */
+	table_ptr = (u8 *)ACPI_HI_RSDP_WINDOW_BASE;
+	rsdp = scan_mem_for_rsdp(table_ptr, ACPI_HI_RSDP_WINDOW_SIZE);
+
+	if (rsdp) {
+		address = (u32)(ACPI_HI_RSDP_WINDOW_BASE +
+				ACPI_PTR_DIFF(rsdp, table_ptr));
+		return address;
+	}
+	return 0;
+}
-- 
2.20.1

[PATCH v15 5/6] 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_SRAT_PARSE' 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   | 138 ++++++++++++++++++++++++++++++
 arch/x86/boot/compressed/kaslr.c  |   4 -
 arch/x86/boot/compressed/misc.h   |  19 ++++
 5 files changed, 171 insertions(+), 4 deletions(-)

diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
index ba7e3464ee92..c1457609f2e3 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 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 e9dd84f459ed..f7a9e9cbd254 100644
--- a/arch/x86/boot/compressed/acpi.c
+++ b/arch/x86/boot/compressed/acpi.c
@@ -5,6 +5,7 @@
 #include "../string.h"
 
 #include <linux/acpi.h>
+#include <linux/numa.h>
 #include <linux/efi.h>
 #include <asm/efi.h>
 
@@ -14,6 +15,19 @@
  */
 #define MAX_HEX_ADDRESS_STRING_LEN 19
 
+/*
+ * Longest parameter of 'acpi=' in cmldine is 'copy_dsdt', so max length
+ * is 10, which contains '\0' for termination.
+ */
+#define MAX_ACPI_ARG_LENGTH 10
+
+/*
+ * Information of immovable memory regions.
+ * Max amount of memory regions is MAX_NUMNODES*2, so need such array
+ * to place immovable memory regions if all of the memory is immovable.
+ */
+struct mem_vector immovable_mem[MAX_NUMNODES*2];
+
 static acpi_physical_address get_acpi_rsdp(void)
 {
 #ifdef CONFIG_KEXEC
@@ -197,3 +211,127 @@ 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 address 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 *)(long)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.*/
+	if (!root_table)
+		return NULL;
+	header = (struct acpi_table_header *)(long)root_table;
+
+	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_CAST_PTR(u32, entry);
+		else {
+			address64 = *(u64 *)entry;
+			acpi_table = address64;
+		}
+
+		if (acpi_table) {
+			header = (struct acpi_table_header *)(long)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;
+	char arg[MAX_ACPI_ARG_LENGTH];
+	unsigned long table_end;
+	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.20.1

[PATCH v15 6/6] x86/boot/KASLR: Limit KASLR to extracting kernel in immovable memory

[Chao Fan]

KASLR may randomly choose some positions which are located in movable
memory regions. It will break memory hotplug feature and make the
movable memory chosen by KASLR can't be removed.

The solution is to limit KASLR to choose memory regions in immovable
node according to SRAT tables.
When CONFIG_EARLY_SRAT_PARSE 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 | 79 +++++++++++++++++++++++++++-----
 1 file changed, 68 insertions(+), 11 deletions(-)

diff --git a/arch/x86/boot/compressed/kaslr.c b/arch/x86/boot/compressed/kaslr.c
index b251572e77af..7dbec6910203 100644
--- a/arch/x86/boot/compressed/kaslr.c
+++ b/arch/x86/boot/compressed/kaslr.c
@@ -97,6 +97,15 @@ 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
+/*
+ * Information of immovable memory regions.
+ * Max amount of memory regions is MAX_NUMNODES*2, so need such array
+ * to place immovable memory regions if all of the memory is immovable.
+ */
+extern struct mem_vector immovable_mem[MAX_NUMNODES*2];
+#endif
+
 
 enum mem_avoid_index {
 	MEM_AVOID_ZO_RANGE = 0,
@@ -413,6 +422,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 +580,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 +658,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++) {
+		unsigned long long start, end, entry_end, region_end;
+		struct mem_vector entry;
+
+		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 +774,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 +802,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.20.1

Re: [PATCH v15 1/6] x86/boot: Copy kstrtoull() to boot/string.c instead of using simple_strtoull()

[Borislav Petkov]

On Mon, Jan 07, 2019 at 11:22:38AM +0800, Chao Fan wrote:
> Copy kstrtoull() and necessary functions from lib/kstrtox.c to
> boot/string.c so that code in boot/ can use kstrtoull() and the old
> simple_strtoull() can be replaced.
> 
> In boot/string.c, using div_u64() from math64.h directly will cause the
> dividend handled as 64-bit value and bring ld error. The solution is to
> separate the dividend to upper and lower in boot/string.o. So copy the
> useful div_u64() and div_u64_rem() to boot/string.c also. To avoid
> redefinition in i386, rename them as __div_u64() and __div_u64_rem().
> 
> Signed-off-by: Chao Fan <fanc.fnst@cn.fujitsu.com>
> ---
>  arch/x86/boot/string.c | 137 +++++++++++++++++++++++++++++++++++++++++
>  arch/x86/boot/string.h |   2 +
>  2 files changed, 139 insertions(+)

...

> +static inline char _tolower(const char c)
> +{
> +	return c | 0x20;
> +}
> +
> +const char *_parse_integer_fixup_radix(const char *s, unsigned int *base)

static

> +{
> +	if (*base == 0) {
> +		if (s[0] == '0') {
> +			if (_tolower(s[1]) == 'x' && isxdigit(s[2]))
> +				*base = 16;
> +			else
> +				*base = 8;
> +		} else
> +			*base = 10;
> +	}
> +	if (*base == 16 && s[0] == '0' && _tolower(s[1]) == 'x')
> +		s += 2;
> +	return s;
> +}
> +
> +/*
> + * Convert non-negative integer string representation in explicitly given radix
> + * to an integer.
> + * Return number of characters consumed maybe or-ed with overflow bit.
> + * If overflow occurs, result integer (incorrect) is still returned.
> + *
> + * Don't you dare use this function.
> + */
> +unsigned int _parse_integer(const char *s, unsigned int base, unsigned long long *p)

static

> +{
> +	unsigned long long res;
> +	unsigned int rv;
> +
> +	res = 0;
> +	rv = 0;
> +	while (1) {
> +		unsigned int c = *s;
> +		unsigned int lc = c | 0x20; /* don't tolower() this line */
> +		unsigned int val;
> +
> +		if ('0' <= c && c <= '9')
> +			val = c - '0';
> +		else if ('a' <= lc && lc <= 'f')
> +			val = lc - 'a' + 10;
> +		else
> +			break;
> +
> +		if (val >= base)
> +			break;
> +		/*
> +		 * Check for overflow only if we are within range of
> +		 * it in the max base we support (16)
> +		 */
> +		if (unlikely(res & (~0ull << 60))) {
> +			if (res > __div_u64(ULLONG_MAX - val, base))
> +				rv |= KSTRTOX_OVERFLOW;
> +		}
> +		res = res * base + val;
> +		rv++;
> +		s++;
> +	}
> +	*p = res;
> +	return rv;
> +}
 

-- 
Regards/Gruss,
    Boris.

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

Re: [PATCH v15 1/6] x86/boot: Copy kstrtoull() to boot/string.c instead of using simple_strtoull()

[Chao Fan]

On Wed, Jan 09, 2019 at 01:48:53PM +0100, Borislav Petkov wrote:
>On Mon, Jan 07, 2019 at 11:22:38AM +0800, Chao Fan wrote:
>> Copy kstrtoull() and necessary functions from lib/kstrtox.c to
>> boot/string.c so that code in boot/ can use kstrtoull() and the old
>> simple_strtoull() can be replaced.
>> 
>> In boot/string.c, using div_u64() from math64.h directly will cause the
>> dividend handled as 64-bit value and bring ld error. The solution is to
>> separate the dividend to upper and lower in boot/string.o. So copy the
>> useful div_u64() and div_u64_rem() to boot/string.c also. To avoid
>> redefinition in i386, rename them as __div_u64() and __div_u64_rem().
>> 
>> Signed-off-by: Chao Fan <fanc.fnst@cn.fujitsu.com>
>> ---
>>  arch/x86/boot/string.c | 137 +++++++++++++++++++++++++++++++++++++++++
>>  arch/x86/boot/string.h |   2 +
>>  2 files changed, 139 insertions(+)
>
>...
>
>> +static inline char _tolower(const char c)
>> +{
>> +	return c | 0x20;
>> +}
>> +
>> +const char *_parse_integer_fixup_radix(const char *s, unsigned int *base)
>
>static

Will add it.

>
>> +{
>> +	if (*base == 0) {
>> +		if (s[0] == '0') {
>> +			if (_tolower(s[1]) == 'x' && isxdigit(s[2]))
>> +				*base = 16;
>> +			else
>> +				*base = 8;
>> +		} else
>> +			*base = 10;
>> +	}
>> +	if (*base == 16 && s[0] == '0' && _tolower(s[1]) == 'x')
>> +		s += 2;
>> +	return s;
>> +}
>> +
>> +/*
>> + * Convert non-negative integer string representation in explicitly given radix
>> + * to an integer.
>> + * Return number of characters consumed maybe or-ed with overflow bit.
>> + * If overflow occurs, result integer (incorrect) is still returned.
>> + *
>> + * Don't you dare use this function.
>> + */
>> +unsigned int _parse_integer(const char *s, unsigned int base, unsigned long long *p)
>
>static

Will add it.

Thanks,
Chao Fan

>
>> +{
>> +	unsigned long long res;
>> +	unsigned int rv;
>> +
>> +	res = 0;
>> +	rv = 0;
>> +	while (1) {
>> +		unsigned int c = *s;
>> +		unsigned int lc = c | 0x20; /* don't tolower() this line */
>> +		unsigned int val;
>> +
>> +		if ('0' <= c && c <= '9')
>> +			val = c - '0';
>> +		else if ('a' <= lc && lc <= 'f')
>> +			val = lc - 'a' + 10;
>> +		else
>> +			break;
>> +
>> +		if (val >= base)
>> +			break;
>> +		/*
>> +		 * Check for overflow only if we are within range of
>> +		 * it in the max base we support (16)
>> +		 */
>> +		if (unlikely(res & (~0ull << 60))) {
>> +			if (res > __div_u64(ULLONG_MAX - val, base))
>> +				rv |= KSTRTOX_OVERFLOW;
>> +		}
>> +		res = res * base + val;
>> +		rv++;
>> +		s++;
>> +	}
>> +	*p = res;
>> +	return rv;
>> +}
> 
>
>-- 
>Regards/Gruss,
>    Boris.
>
>Good mailing practices for 400: avoid top-posting and trim the reply.
>
>

Re: [PATCH v15 2/6] x86/boot: Introduce get_acpi_rsdp() to parse RSDP in cmdline from KEXEC

[Borislav Petkov]

On Mon, Jan 07, 2019 at 11:22:39AM +0800, Chao Fan wrote:
> KASLR may randomly choose some positions which are located in movable
> memory regions. This will make the movable memory chosen by KASLR
> can't be removed.
> 
> 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..7ca5001d7639
> --- /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 "../string.h"
> +
> +#include <linux/acpi.h>

Ok, I corrected it to this, please replace in your version:

/*
 * Max length of 64-bit hex address string is 19, prefix "0x" + 16 hex
 * digits, and '\0' for termination.
 */
#define MAX_ADDR_LEN 19

static acpi_physical_address get_acpi_rsdp(void)
{
	acpi_physical_address addr = 0;

#ifdef CONFIG_KEXEC
	char val[MAX_ADDR_LEN] = { };
	int ret;

	ret = cmdline_find_option("acpi_rsdp", val, MAX_ADDR_LEN);
	if (ret < 0)
		return 0;

	if (kstrtoull(val, 16, &addr))
		return 0;
#endif
	return addr;
}


-- 
Regards/Gruss,
    Boris.

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

Re: [PATCH v15 3/6] x86/boot: Introduce efi_get_rsdp_addr() to find RSDP from EFI table

[Borislav Petkov]

On Mon, Jan 07, 2019 at 11:22:40AM +0800, Chao Fan 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 | 83 +++++++++++++++++++++++++++++++++
>  1 file changed, 83 insertions(+)
> 
> diff --git a/arch/x86/boot/compressed/acpi.c b/arch/x86/boot/compressed/acpi.c
> index 7ca5001d7639..f74c5d033d79 100644
> --- a/arch/x86/boot/compressed/acpi.c
> +++ b/arch/x86/boot/compressed/acpi.c
> @@ -5,6 +5,8 @@
>  #include "../string.h"
>  
>  #include <linux/acpi.h>
> +#include <linux/efi.h>
> +#include <asm/efi.h>
>  
>  /*
>   * Max length of 64-bit hex address string is 19, prefix "0x" + 16 hex
> @@ -28,3 +30,84 @@ 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;


<---- newline here.

> +#ifdef CONFIG_EFI
> +	efi_system_table_t *systab;
> +	struct efi_info *ei;
> +	bool efi_64;
> +	char *sig;
> +	int size;
> +	int i;
> +
> +	ei = &boot_params->efi_info;
> +	sig = (char *)&ei->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 *)(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");
> +		return 0;
> +	}
> +	systab = (efi_system_table_t *)ei->efi_systab;
> +#endif
> +
> +	if (!systab)
> +		error("EFI system table is not found.");

s/is//

> +
> +	/*
> +	 * 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;
> +			u64 table64;
> +
> +			tmp_table = config_tables;
> +			guid = tmp_table->guid;
> +			table64 = tmp_table->table;
> +			table = table64;

That table64 looks superfluous.

-- 
Regards/Gruss,
    Boris.

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

Re: [PATCH v15 4/6] x86/boot: Introduce bios_get_rsdp_addr() to search RSDP in memory

[Borislav Petkov]

On Mon, Jan 07, 2019 at 11:22:41AM +0800, Chao Fan wrote:
> Memory information in SRAT table 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(+)

...

> +/* 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;

But those u8's together:

	u8 *table_ptr, *rsdp;
	u32 address;

> +
> +	/* Get the location of the Extended BIOS Data Area (EBDA) */
> +	table_ptr = (u8 *)ACPI_EBDA_PTR_LOCATION;
> +	address = *(u16 *)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 address;
> +		}
> +	}
> +
> +	/* Search upper memory: 16-byte boundaries in E0000h-FFFFFh */
> +	table_ptr = (u8 *)ACPI_HI_RSDP_WINDOW_BASE;
> +	rsdp = scan_mem_for_rsdp(table_ptr, ACPI_HI_RSDP_WINDOW_SIZE);
> +

Superfluous newline.

> +	if (rsdp) {
> +		address = (u32)(ACPI_HI_RSDP_WINDOW_BASE +
> +				ACPI_PTR_DIFF(rsdp, table_ptr));
> +		return address;
> +	}
> +	return 0;
> +}

-- 
Regards/Gruss,
    Boris.

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

Re: [PATCH v15 2/6] x86/boot: Introduce get_acpi_rsdp() to parse RSDP in cmdline from KEXEC

[Chao Fan]

On Thu, Jan 10, 2019 at 06:01:03PM +0100, Borislav Petkov wrote:
>On Mon, Jan 07, 2019 at 11:22:39AM +0800, Chao Fan wrote:
>> KASLR may randomly choose some positions which are located in movable
>> memory regions. This will make the movable memory chosen by KASLR
>> can't be removed.
>> 
>> 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..7ca5001d7639
>> --- /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 "../string.h"
>> +
>> +#include <linux/acpi.h>
>
>Ok, I corrected it to this, please replace in your version:
>
>/*
> * Max length of 64-bit hex address string is 19, prefix "0x" + 16 hex
> * digits, and '\0' for termination.
> */
>#define MAX_ADDR_LEN 19
>
>static acpi_physical_address get_acpi_rsdp(void)
>{
>	acpi_physical_address addr = 0;
>
>#ifdef CONFIG_KEXEC
>	char val[MAX_ADDR_LEN] = { };
>	int ret;
>
>	ret = cmdline_find_option("acpi_rsdp", val, MAX_ADDR_LEN);
>	if (ret < 0)
>		return 0;
>
>	if (kstrtoull(val, 16, &addr))
>		return 0;
>#endif
>	return addr;
>}
>

Thanks for your suggestion, will change it.

Thanks,
Chao Fan

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

Re: [PATCH v15 3/6] x86/boot: Introduce efi_get_rsdp_addr() to find RSDP from EFI table

[Chao Fan]

On Thu, Jan 10, 2019 at 10:15:23PM +0100, Borislav Petkov wrote:
>On Mon, Jan 07, 2019 at 11:22:40AM +0800, Chao Fan 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 | 83 +++++++++++++++++++++++++++++++++
>>  1 file changed, 83 insertions(+)
>> 
>> diff --git a/arch/x86/boot/compressed/acpi.c b/arch/x86/boot/compressed/acpi.c
>> index 7ca5001d7639..f74c5d033d79 100644
>> --- a/arch/x86/boot/compressed/acpi.c
>> +++ b/arch/x86/boot/compressed/acpi.c
>> @@ -5,6 +5,8 @@
>>  #include "../string.h"
>>  
>>  #include <linux/acpi.h>
>> +#include <linux/efi.h>
>> +#include <asm/efi.h>
>>  
>>  /*
>>   * Max length of 64-bit hex address string is 19, prefix "0x" + 16 hex
>> @@ -28,3 +30,84 @@ 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;
>
>
><---- newline here.
>

Will add it.

>> +#ifdef CONFIG_EFI
>> +	efi_system_table_t *systab;
>> +	struct efi_info *ei;
>> +	bool efi_64;
>> +	char *sig;
>> +	int size;
>> +	int i;
>> +
>> +	ei = &boot_params->efi_info;
>> +	sig = (char *)&ei->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 *)(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");
>> +		return 0;
>> +	}
>> +	systab = (efi_system_table_t *)ei->efi_systab;
>> +#endif
>> +
>> +	if (!systab)
>> +		error("EFI system table is not found.");
>
>s/is//

Will drop the 'is'.

>
>> +
>> +	/*
>> +	 * 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;
>> +			u64 table64;
>> +
>> +			tmp_table = config_tables;
>> +			guid = tmp_table->guid;
>> +			table64 = tmp_table->table;
>> +			table = table64;
>
>That table64 looks superfluous.

Yes, 'table64' looks superfluous here, but after these lines, there is:
                        if (!IS_ENABLED(CONFIG_X86_64) && table64 >> 32) {
so the 'table64' is useful here for i386. 'table' is unsigned long, it
can't do the right shift. But the 'table64' who is u64 can do that right
shift.

Thanks,
Chao Fan

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

Re: [PATCH v15 4/6] x86/boot: Introduce bios_get_rsdp_addr() to search RSDP in memory

[Chao Fan]

On Thu, Jan 10, 2019 at 10:27:47PM +0100, Borislav Petkov wrote:
>On Mon, Jan 07, 2019 at 11:22:41AM +0800, Chao Fan wrote:
>> Memory information in SRAT table 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(+)
>
>...
>
>> +/* 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;
>
>But those u8's together:
>
>	u8 *table_ptr, *rsdp;
>	u32 address;

Thanks, will change that.

>
>> +
>> +	/* Get the location of the Extended BIOS Data Area (EBDA) */
>> +	table_ptr = (u8 *)ACPI_EBDA_PTR_LOCATION;
>> +	address = *(u16 *)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 address;
>> +		}
>> +	}
>> +
>> +	/* Search upper memory: 16-byte boundaries in E0000h-FFFFFh */
>> +	table_ptr = (u8 *)ACPI_HI_RSDP_WINDOW_BASE;
>> +	rsdp = scan_mem_for_rsdp(table_ptr, ACPI_HI_RSDP_WINDOW_SIZE);
>> +
>
>Superfluous newline.

Will drop it.

Thanks,
Chao Fan

>
>> +	if (rsdp) {
>> +		address = (u32)(ACPI_HI_RSDP_WINDOW_BASE +
>> +				ACPI_PTR_DIFF(rsdp, table_ptr));
>> +		return address;
>> +	}
>> +	return 0;
>> +}
>
>-- 
>Regards/Gruss,
>    Boris.
>
>Good mailing practices for 400: avoid top-posting and trim the reply.
>
>

Re: [PATCH v15 3/6] x86/boot: Introduce efi_get_rsdp_addr() to find RSDP from EFI table

[Borislav Petkov]

On Fri, Jan 11, 2019 at 09:23:53AM +0800, Chao Fan wrote:
> Yes, 'table64' looks superfluous here, but after these lines, there is:
>                         if (!IS_ENABLED(CONFIG_X86_64) && table64 >> 32) {
> so the 'table64' is useful here for i386. 'table' is unsigned long, it
> can't do the right shift. But the 'table64' who is u64 can do that right
> shift.

Have you actually tried fixing what I suggested or you're just talking?

---
diff --git a/arch/x86/boot/compressed/acpi.c b/arch/x86/boot/compressed/acpi.c
index e9dd84f459ed..0537d46fb21f 100644
--- a/arch/x86/boot/compressed/acpi.c
+++ b/arch/x86/boot/compressed/acpi.c
@@ -77,21 +77,19 @@ static acpi_physical_address efi_get_rsdp_addr(void)
 			sizeof(efi_config_table_32_t);
 
 	for (i = 0; i < systab->nr_tables; i++) {
+		acpi_physical_address table;
 		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;
-			u64 table64;
 
 			tmp_table = config_tables;
 			guid = tmp_table->guid;
-			table64 = tmp_table->table;
-			table = table64;
+			table = tmp_table->table;
 
-			if (!IS_ENABLED(CONFIG_X86_64) && table64 >> 32) {
+			if (!IS_ENABLED(CONFIG_X86_64) && table >> 32) {
 				debug_putstr("Error getting RSDP address: EFI config table located above 4GB.\n");
 				return 0;
 			}

-- 
Regards/Gruss,
    Boris.

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

Re: [PATCH v15 3/6] x86/boot: Introduce efi_get_rsdp_addr() to find RSDP from EFI table

[Chao Fan]

On Fri, Jan 11, 2019 at 11:32:25AM +0100, Borislav Petkov wrote:
>On Fri, Jan 11, 2019 at 09:23:53AM +0800, Chao Fan wrote:
>> Yes, 'table64' looks superfluous here, but after these lines, there is:
>>                         if (!IS_ENABLED(CONFIG_X86_64) && table64 >> 32) {
>> so the 'table64' is useful here for i386. 'table' is unsigned long, it
>> can't do the right shift. But the 'table64' who is u64 can do that right
>> shift.
>
>Have you actually tried fixing what I suggested or you're just talking?

Sure, I tried what I was talking.
I ever used 'unsigned long table' directly. But that caused warning:
  CC      arch/x86/boot/compressed/acpi.o
arch/x86/boot/compressed/acpi.c: In function ‘efi_get_rsdp_addr’:
arch/x86/boot/compressed/acpi.c:106:44: warning: right shift count >= width of type [-Wshift-count-overflow]
    if (!IS_ENABLED(CONFIG_X86_64) && table >> 32) {
To avoid this warning, I add 'u64 table64' to do the right shift.

Well, the acpi_physicall_address defined as:

#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

'acpi_physical_address' could be define as u64 or u32.
In my guest machine, it's defined as u64, there is no problem as you
suggested.
I didn't find a machine where 'acpi_physical_address' defined as u32.
I thought if 'acpi_physical_address' defined as u32, it will meet
the same warning as using 'unsigned long'.
I tried to define 'table' as u32, that will also cause the warning.
So once acpi_physical_address is define as u32, that warning will
happen. We should make sure u64 to do the right shift.

Thanks,
Chao Fan

>
>---
>diff --git a/arch/x86/boot/compressed/acpi.c b/arch/x86/boot/compressed/acpi.c
>index e9dd84f459ed..0537d46fb21f 100644
>--- a/arch/x86/boot/compressed/acpi.c
>+++ b/arch/x86/boot/compressed/acpi.c
>@@ -77,21 +77,19 @@ static acpi_physical_address efi_get_rsdp_addr(void)
> 			sizeof(efi_config_table_32_t);
> 
> 	for (i = 0; i < systab->nr_tables; i++) {
>+		acpi_physical_address table;
> 		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;
>-			u64 table64;
> 
> 			tmp_table = config_tables;
> 			guid = tmp_table->guid;
>-			table64 = tmp_table->table;
>-			table = table64;
>+			table = tmp_table->table;
> 
>-			if (!IS_ENABLED(CONFIG_X86_64) && table64 >> 32) {
>+			if (!IS_ENABLED(CONFIG_X86_64) && table >> 32) {
> 				debug_putstr("Error getting RSDP address: EFI config table located above 4GB.\n");
> 				return 0;
> 			}
>
>-- 
>Regards/Gruss,
>    Boris.
>
>Good mailing practices for 400: avoid top-posting and trim the reply.
>
>

Re: [PATCH v15 3/6] x86/boot: Introduce efi_get_rsdp_addr() to find RSDP from EFI table

[Borislav Petkov]

On Sun, Jan 13, 2019 at 05:47:04PM +0800, Chao Fan wrote:
> 'acpi_physical_address' could be define as u64 or u32.

And when can acpi_physical_address be a u32?

-- 
Regards/Gruss,
    Boris.

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

Re: [PATCH v15 3/6] x86/boot: Introduce efi_get_rsdp_addr() to find RSDP from EFI table

[Chao Fan]

On Sun, Jan 13, 2019 at 12:05:45PM +0100, Borislav Petkov wrote:
>On Sun, Jan 13, 2019 at 05:47:04PM +0800, Chao Fan wrote:
>> 'acpi_physical_address' could be define as u64 or u32.
>
>And when can acpi_physical_address be a u32?
>

According to the code, I saw:
#ifdef ACPI_ASL_COMPILER
#define ACPI_32BIT_PHYSICAL_ADDRESS
#endif

and then
#ifdef ACPI_32BIT_PHYSICAL_ADDRESS
typedef u32 acpi_physical_address;

As for ACPI_ASL_COMPILER, I saw iASL in documention, but can't find more
information in the code. If I miss something, please let me know.

Thanks,
Chao Fan

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

Re: [PATCH v15 3/6] x86/boot: Introduce efi_get_rsdp_addr() to find RSDP from EFI table

[Borislav Petkov]

On Mon, Jan 14, 2019 at 09:26:42AM +0800, Chao Fan wrote:
> According to the code, I saw:
> #ifdef ACPI_ASL_COMPILER
> #define ACPI_32BIT_PHYSICAL_ADDRESS
> #endif
> 
> and then
> #ifdef ACPI_32BIT_PHYSICAL_ADDRESS
> typedef u32 acpi_physical_address;
> 
> As for ACPI_ASL_COMPILER, I saw iASL in documention, but can't find more
> information in the code. If I miss something, please let me know.

And, as a result, can acpi_physical_address ever be u32 in a kernel
build?

git annotate is also very helpful when doing git archeology like, for
example, finding the patch which added the ifdeffery and looking at its
commit message for more hints.

-- 
Regards/Gruss,
    Boris.

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

Re: [PATCH v15 3/6] x86/boot: Introduce efi_get_rsdp_addr() to find RSDP from EFI table

[Chao Fan]

On Mon, Jan 14, 2019 at 10:07:56AM +0100, Borislav Petkov wrote:
>On Mon, Jan 14, 2019 at 09:26:42AM +0800, Chao Fan wrote:
>> According to the code, I saw:
>> #ifdef ACPI_ASL_COMPILER
>> #define ACPI_32BIT_PHYSICAL_ADDRESS
>> #endif
>> 
>> and then
>> #ifdef ACPI_32BIT_PHYSICAL_ADDRESS
>> typedef u32 acpi_physical_address;
>> 
>> As for ACPI_ASL_COMPILER, I saw iASL in documention, but can't find more
>> information in the code. If I miss something, please let me know.
>
>And, as a result, can acpi_physical_address ever be u32 in a kernel
>build?

In my understanding after looking into the commit message the comments.
I thinks yes. For 32-bit OS:
32-bit OS without PAE, it's u32.
32-bit OS with PAE in 64-bit machine, it's u64.

So I thinks there is some situations where it's u32.
'acpi_physical_address' was always u32 for 32-bit OS, and then to
solve some problems, "Zheng, Lv" add the commit. So I have added to Cc
"Zheng, Lv" , I am not sure whether "Zheng, Lv" can give some suggestion
about when acpi_physical_address is defined as u32.

Thanks,
Chao Fan

>
>git annotate is also very helpful when doing git archeology like, for
>example, finding the patch which added the ifdeffery and looking at its
>commit message for more hints.
>
>-- 
>Regards/Gruss,
>    Boris.
>
>Good mailing practices for 400: avoid top-posting and trim the reply.
>
>

Re: [PATCH v15 3/6] x86/boot: Introduce efi_get_rsdp_addr() to find RSDP from EFI table

[Borislav Petkov]

On Tue, Jan 15, 2019 at 03:21:21PM +0800, Chao Fan wrote:
> In my understanding after looking into the commit message the comments.
> I thinks yes. For 32-bit OS:

And when does your "32-bit OS" define ACPI_ASL_COMPILER ?

-- 
Regards/Gruss,
    Boris.

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

Re: [PATCH v15 3/6] x86/boot: Introduce efi_get_rsdp_addr() to find RSDP from EFI table

[Chao Fan]

On Tue, Jan 15, 2019 at 10:55:02AM +0100, Borislav Petkov wrote:
>On Tue, Jan 15, 2019 at 03:21:21PM +0800, Chao Fan wrote:
>> In my understanding after looking into the commit message the comments.
>> I thinks yes. For 32-bit OS:
>
>And when does your "32-bit OS" define ACPI_ASL_COMPILER ?

Ah, I got it. You are right. Linux will never define this tag. It's
only used by other tools. I misunderstood include/acpi/platform/acenv.h
I learn much about Kernel compilation. Thanks for that.
I will change as you suggested.

Thanks,
Chao Fan

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

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

[Kairui Song]

On Mon, Jan 7, 2019 at 11:24 AM Chao Fan <fanc.fnst@cn.fujitsu.com> wrote:
>
> +
> +/* 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;
> +}

acpi_rsdp might be provided by boot_params.acpi_rsdp_addr too,
it's introduced in ae7e1238e68f2a for Xen PVH guest and later move to
boot_params in e6e094e053af,
kexec could use it to pass RSDP to second kernel as well. Please check
it as well make sure it always works.

> +
> +/* Compute SRAT address 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 *)(long)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.*/
> +       if (!root_table)
> +               return NULL;
> +       header = (struct acpi_table_header *)(long)root_table;
> +
> +       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_CAST_PTR(u32, entry);
> +               else {
> +                       address64 = *(u64 *)entry;
> +                       acpi_table = address64;
> +               }
> +
> +               if (acpi_table) {
> +                       header = (struct acpi_table_header *)(long)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;
> +       char arg[MAX_ACPI_ARG_LENGTH];
> +       unsigned long table_end;
> +       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.20.1
>
>
>


-- 
Best Regards,
Kairui Song

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

[Borislav Petkov]

On Mon, Jan 07, 2019 at 11:22:42AM +0800, Chao Fan 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_SRAT_PARSE' 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   | 138 ++++++++++++++++++++++++++++++
>  arch/x86/boot/compressed/kaslr.c  |   4 -
>  arch/x86/boot/compressed/misc.h   |  19 ++++
>  5 files changed, 171 insertions(+), 4 deletions(-)
> 
> diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
> index ba7e3464ee92..c1457609f2e3 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 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.
> +

One argument for Ingo's suggestion to have this enabled unconditionally
is all the other code needing RSDP. It might be easier to have it always
built-in instead of a long "depends on" line in Kconfig. We'll see.

>  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 e9dd84f459ed..f7a9e9cbd254 100644
> --- a/arch/x86/boot/compressed/acpi.c
> +++ b/arch/x86/boot/compressed/acpi.c
> @@ -5,6 +5,7 @@
>  #include "../string.h"
>  
>  #include <linux/acpi.h>
> +#include <linux/numa.h>
>  #include <linux/efi.h>
>  #include <asm/efi.h>
>  
> @@ -14,6 +15,19 @@
>   */
>  #define MAX_HEX_ADDRESS_STRING_LEN 19
>  
> +/*
> + * Longest parameter of 'acpi=' in cmldine is 'copy_dsdt', so max length
> + * is 10, which contains '\0' for termination.
> + */
> +#define MAX_ACPI_ARG_LENGTH 10
> +
> +/*
> + * Information of immovable memory regions.
> + * Max amount of memory regions is MAX_NUMNODES*2, so need such array
> + * to place immovable memory regions if all of the memory is immovable.
> + */
> +struct mem_vector immovable_mem[MAX_NUMNODES*2];
> +
>  static acpi_physical_address get_acpi_rsdp(void)
>  {
>  #ifdef CONFIG_KEXEC
> @@ -197,3 +211,127 @@ 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 address 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 *)(long)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.*/

This comment is wrong here - should be above.

> +	if (!root_table)
> +		return NULL;

<---- newline here.

> +	header = (struct acpi_table_header *)(long)root_table;
> +
> +	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_CAST_PTR(u32, entry);
> +		else {
> +			address64 = *(u64 *)entry;
> +			acpi_table = address64;

The same thing as in the previous patch - get rid of address64.

> +		}
> +
> +		if (acpi_table) {
> +			header = (struct acpi_table_header *)(long)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;
> +	char arg[MAX_ACPI_ARG_LENGTH];
> +	unsigned long table_end;
> +	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);

That's a lot of unnecessary casting AFAICT. You can return an unsigned
long from get_acpi_srat_table() and get rid of all that casting here.


> +	}
> +	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)
> +{
> +}

Put those on a single line:

static void get_immovable_mem(void) {}

> +#endif
> -- 
> 2.20.1
> 
> 
> 

-- 
Regards/Gruss,
    Boris.

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

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

[Borislav Petkov]

On Mon, Jan 07, 2019 at 11:22:43AM +0800, Chao Fan wrote:
> KASLR may randomly choose some positions which are located in movable
> memory regions. It will break memory hotplug feature and make the
> movable memory chosen by KASLR can't be removed.
> 
> The solution is to limit KASLR to choose memory regions in immovable
> node according to SRAT tables.
> When CONFIG_EARLY_SRAT_PARSE 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().

The commit message doesn't need to contain *what* you're doing in the
patch.

> 
> Signed-off-by: Chao Fan <fanc.fnst@cn.fujitsu.com>
> ---
>  arch/x86/boot/compressed/kaslr.c | 79 +++++++++++++++++++++++++++-----
>  1 file changed, 68 insertions(+), 11 deletions(-)
> 
> diff --git a/arch/x86/boot/compressed/kaslr.c b/arch/x86/boot/compressed/kaslr.c
> index b251572e77af..7dbec6910203 100644
> --- a/arch/x86/boot/compressed/kaslr.c
> +++ b/arch/x86/boot/compressed/kaslr.c
> @@ -97,6 +97,15 @@ 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
> +/*
> + * Information of immovable memory regions.
> + * Max amount of memory regions is MAX_NUMNODES*2, so need such array
> + * to place immovable memory regions if all of the memory is immovable.
> + */

Why is that comment repeated here?

> +extern struct mem_vector immovable_mem[MAX_NUMNODES*2];
> +#endif
> +
>  
>  enum mem_avoid_index {
>  	MEM_AVOID_ZO_RANGE = 0,
> @@ -413,6 +422,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();

get != mark.

Which should tell you that the function name needs changing.

Also, this function is void and it could very well return
num_immovable_mem and num_immovable_mem can be a static variable in
kaslr.c instead of sloppily adding yet another global one.

> +
>  #ifdef CONFIG_X86_VERBOSE_BOOTUP
>  	/* Make sure video RAM can be used. */
>  	add_identity_map(0, PMD_SIZE);
> @@ -568,9 +580,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 +658,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++) {
> +		unsigned long long start, end, entry_end, region_end;
> +		struct mem_vector entry;
> +
> +		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");

Change this error message a bit to differ from the above for easier debugging.

-- 
Regards/Gruss,
    Boris.

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

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

[Chao Fan]

On Wed, Jan 16, 2019 at 12:01:58PM +0100, Borislav Petkov wrote:
>On Mon, Jan 07, 2019 at 11:22:42AM +0800, Chao Fan wrote:
[...]
>> +	rsdp = (struct acpi_table_rsdp *)(long)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.*/
>
>This comment is wrong here - should be above.

I will change the comment
>
>> +	if (!root_table)
>> +		return NULL;
>
><---- newline here.

Will add
>
>> +	header = (struct acpi_table_header *)(long)root_table;
>> +
>> +	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_CAST_PTR(u32, entry);
>> +		else {
>> +			address64 = *(u64 *)entry;
>> +			acpi_table = address64;
>
>The same thing as in the previous patch - get rid of address64.

Got it.
>
>> +		}
>> +
>> +		if (acpi_table) {
>> +			header = (struct acpi_table_header *)(long)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;
>> +	char arg[MAX_ACPI_ARG_LENGTH];
>> +	unsigned long table_end;
>> +	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);
>
>That's a lot of unnecessary casting AFAICT. You can return an unsigned
>long from get_acpi_srat_table() and get rid of all that casting here.

I will clean the type casting
>
>
>> +	}
>> +	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)
>> +{
>> +}
>
>Put those on a single line:
>
>static void get_immovable_mem(void) {}

OK.

Thanks,
Chao Fan

>
>> +#endif
>> -- 
>> 2.20.1
>> 
>> 
>> 
>
>-- 
>Regards/Gruss,
>    Boris.
>
>Good mailing practices for 400: avoid top-posting and trim the reply.
>
>

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

[Chao Fan]

On Wed, Jan 16, 2019 at 12:15:07PM +0100, Borislav Petkov wrote:
>On Mon, Jan 07, 2019 at 11:22:43AM +0800, Chao Fan wrote:
>> KASLR may randomly choose some positions which are located in movable
>> memory regions. It will break memory hotplug feature and make the
>> movable memory chosen by KASLR can't be removed.
>> 
>> The solution is to limit KASLR to choose memory regions in immovable
>> node according to SRAT tables.
>> When CONFIG_EARLY_SRAT_PARSE 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().
>
>The commit message doesn't need to contain *what* you're doing in the
>patch.
>

Will drop it.
>> 
>> Signed-off-by: Chao Fan <fanc.fnst@cn.fujitsu.com>
>> ---
>>  arch/x86/boot/compressed/kaslr.c | 79 +++++++++++++++++++++++++++-----
>>  1 file changed, 68 insertions(+), 11 deletions(-)
>> 
>> diff --git a/arch/x86/boot/compressed/kaslr.c b/arch/x86/boot/compressed/kaslr.c
>> index b251572e77af..7dbec6910203 100644
>> --- a/arch/x86/boot/compressed/kaslr.c
>> +++ b/arch/x86/boot/compressed/kaslr.c
>> @@ -97,6 +97,15 @@ 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
>> +/*
>> + * Information of immovable memory regions.
>> + * Max amount of memory regions is MAX_NUMNODES*2, so need such array
>> + * to place immovable memory regions if all of the memory is immovable.
>> + */
>
>Why is that comment repeated here?
>

Will drop it.
>> +extern struct mem_vector immovable_mem[MAX_NUMNODES*2];
>> +#endif
>> +
>>  
>>  enum mem_avoid_index {
>>  	MEM_AVOID_ZO_RANGE = 0,
>> @@ -413,6 +422,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();
>
>get != mark.
>
>Which should tell you that the function name needs changing.
>
>Also, this function is void and it could very well return
>num_immovable_mem and num_immovable_mem can be a static variable in
>kaslr.c instead of sloppily adding yet another global one.

Will make num_immovable_mem returned, and how do you think change
the function name as "get_immovable_mem_num()" since the function
will return num_immovable_mem.
>
>> +
>>  #ifdef CONFIG_X86_VERBOSE_BOOTUP
>>  	/* Make sure video RAM can be used. */
>>  	add_identity_map(0, PMD_SIZE);
>> @@ -568,9 +580,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 +658,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++) {
>> +		unsigned long long start, end, entry_end, region_end;
>> +		struct mem_vector entry;
>> +
>> +		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");
>
>Change this error message a bit to differ from the above for easier debugging.

Will change it.

Thanks,
Chao Fan

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

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

[Chao Fan]

On Wed, Jan 16, 2019 at 03:28:52PM +0800, Kairui Song wrote:
>On Mon, Jan 7, 2019 at 11:24 AM Chao Fan <fanc.fnst@cn.fujitsu.com> wrote:
>>
>> +
>> +/* 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;
>> +}
>
>acpi_rsdp might be provided by boot_params.acpi_rsdp_addr too,
>it's introduced in ae7e1238e68f2a for Xen PVH guest and later move to
>boot_params in e6e094e053af,
>kexec could use it to pass RSDP to second kernel as well. Please check
>it as well make sure it always works.

Thanks, so in next version, should add parsing the boot_params.acpi_rsdp_addr
between get_acpi_rsdp() and efi_get_rsdp_addr().
Since it's u64, so it's easy to get the value.
Thanks for your information.

Thanks,
Chao Fan

>
>> +
>> +/* Compute SRAT address 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 *)(long)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.*/
>> +       if (!root_table)
>> +               return NULL;
>> +       header = (struct acpi_table_header *)(long)root_table;
>> +
>> +       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_CAST_PTR(u32, entry);
>> +               else {
>> +                       address64 = *(u64 *)entry;
>> +                       acpi_table = address64;
>> +               }
>> +
>> +               if (acpi_table) {
>> +                       header = (struct acpi_table_header *)(long)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;
>> +       char arg[MAX_ACPI_ARG_LENGTH];
>> +       unsigned long table_end;
>> +       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.20.1
>>
>>
>>
>
>
>-- 
>Best Regards,
>Kairui Song
>
>

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

[Chao Fan]

On Wed, Jan 16, 2019 at 12:01:58PM +0100, Borislav Petkov wrote:
>On Mon, Jan 07, 2019 at 11:22:42AM +0800, Chao Fan wrote:
>> +void get_immovable_mem(void)
>> +{
>> +	struct acpi_table_header *table_header;
>> +	struct acpi_subtable_header *table;
>> +	struct acpi_srat_mem_affinity *ma;
>> +	char arg[MAX_ACPI_ARG_LENGTH];
>> +	unsigned long table_end;
>> +	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);
>
>That's a lot of unnecessary casting AFAICT. You can return an unsigned
>long from get_acpi_srat_table() and get rid of all that casting here.
>
Hi Boris,

I have changed as you suggested, looks clear without type cast, and
we need some variable as long to calculate the address, and at same
time as the struct pointer to find it's length, so I change as below,
and get_acpi_srat_table() return an unsigned long.
How do you think of that.

int get_immovable_mem_num(void)
{
        unsigned long table_addr, table_end, table;
        struct acpi_table_header *table_header;
        struct acpi_subtable_header *sub_table;
        char arg[MAX_ACPI_ARG_LENGTH];
        int num = 0;

        if (cmdline_find_option("acpi", arg, sizeof(arg)) == 3 &&
            !strncmp(arg, "off", 3))
                return;

        table_addr = get_acpi_srat_table();
        if (!table_addr)
                return;

        table_header = (struct acpi_table_header *)table_addr;
        table_end = table_addr + table_header->length;
        table = table_addr + sizeof(struct acpi_table_srat);

        while (table + sizeof(struct acpi_subtable_header) < table_end) {
                sub_table = (struct acpi_subtable_header *)table;
                if (sub_table->type == ACPI_SRAT_TYPE_MEMORY_AFFINITY) {
                        struct acpi_srat_mem_affinity *ma;

                        ma = (struct acpi_srat_mem_affinity *)table;
                        if (!(ma->flags & ACPI_SRAT_MEM_HOT_PLUGGABLE)) {
                                immovable_mem[num].start = ma->base_address;
                                immovable_mem[num].size = ma->length;
                                num++;
                        }

                        if (num >= MAX_NUMNODES*2) {
                                debug_putstr("Too many immovable memory regions, aborting.\n");
                                return 0;
                        }
                }
                table += sub_table->length;
        }
        return num;
}

Thanks,
Chao Fan

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

[Chao Fan]

On Wed, Jan 16, 2019 at 03:28:52PM +0800, Kairui Song wrote:
>On Mon, Jan 7, 2019 at 11:24 AM Chao Fan <fanc.fnst@cn.fujitsu.com> wrote:
>>
>> +
>> +/* 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;
>> +}
>
>acpi_rsdp might be provided by boot_params.acpi_rsdp_addr too,
>it's introduced in ae7e1238e68f2a for Xen PVH guest and later move to
>boot_params in e6e094e053af,
>kexec could use it to pass RSDP to second kernel as well. Please check
>it as well make sure it always works.
>

Hi Kairui,

I saw the parsing code has been added to kernel, but I didn't see
where to fill in the 'acpi_rsdp_addr'. If only you(KEXEC) use it,
I can add "#ifdef CONFIG_KEXEC", but you said Xen will use it also,
so I didn't add ifdef to control it. I was trying to do as below:

static inline acpi_physical_address get_boot_params_rsdp(void)
{
        return boot_params->acpi_rsdp_addr;
}

static acpi_physical_address get_rsdp_addr(void)
{
	bool boot_params_rsdp_exist;
        acpi_physical_address pa;

        pa = get_acpi_rsdp();

        if (!pa)
                pa = get_boot_params_rsdp();

        if (!pa) {
                pa = efi_get_rsdp_addr();
		boot_params_rsdp_exist = false;
	}
	else
		boot_params_rsdp_exist = true;

        if (!pa)
                pa = bios_get_rsdp_addr();

	if (pa && !boot_params_rsdp_exist)
		boot_params.acpi_rsdp_addr = pa;

        return pa;
}

At the same time, I notice kernel only parses it when
"#ifdef CONFIG_ACPI", we should keep sync with kernel, but I think
we are parsing SRAT, CONFIG_ACPI is needed sure, so I am going to
update the define of EARLY_SRAT_PARSE:

config EARLY_SRAT_PARSE
        bool "EARLY SRAT parsing"
        def_bool y
        depends on RANDOMIZE_BASE && MEMORY_HOTREMOVE && ACPI

Boris, how do you think the update for the new acpi_rsdp_addr?
If I misunderstand something, please let me know.

Thanks,
Chao Fan

>> +
>> +/* Compute SRAT address 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 *)(long)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.*/
>> +       if (!root_table)
>> +               return NULL;
>> +       header = (struct acpi_table_header *)(long)root_table;
>> +
>> +       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_CAST_PTR(u32, entry);
>> +               else {
>> +                       address64 = *(u64 *)entry;
>> +                       acpi_table = address64;
>> +               }
>> +
>> +               if (acpi_table) {
>> +                       header = (struct acpi_table_header *)(long)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;
>> +       char arg[MAX_ACPI_ARG_LENGTH];
>> +       unsigned long table_end;
>> +       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.20.1
>>
>>
>>
>
>
>-- 
>Best Regards,
>Kairui Song
>
>

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

[Kairui Song]

On Thu, Jan 17, 2019 at 3:58 PM Chao Fan <fanc.fnst@cn.fujitsu.com> wrote:
>
> On Wed, Jan 16, 2019 at 03:28:52PM +0800, Kairui Song wrote:
> >On Mon, Jan 7, 2019 at 11:24 AM Chao Fan <fanc.fnst@cn.fujitsu.com> wrote:
> >>
> >> +
> >> +/* 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;
> >> +}
> >
> >acpi_rsdp might be provided by boot_params.acpi_rsdp_addr too,
> >it's introduced in ae7e1238e68f2a for Xen PVH guest and later move to
> >boot_params in e6e094e053af,
> >kexec could use it to pass RSDP to second kernel as well. Please check
> >it as well make sure it always works.
> >
>
> Hi Kairui,
>
> I saw the parsing code has been added to kernel, but I didn't see
> where to fill in the 'acpi_rsdp_addr'. If only you(KEXEC) use it,
> I can add "#ifdef CONFIG_KEXEC", but you said Xen will use it also,
> so I didn't add ifdef to control it. I was trying to do as below:
>
> static inline acpi_physical_address get_boot_params_rsdp(void)
> {
>         return boot_params->acpi_rsdp_addr;
> }
>
> static acpi_physical_address get_rsdp_addr(void)
> {
>         bool boot_params_rsdp_exist;
>         acpi_physical_address pa;
>
>         pa = get_acpi_rsdp();
>
>         if (!pa)
>                 pa = get_boot_params_rsdp();
>
>         if (!pa) {
>                 pa = efi_get_rsdp_addr();
>                 boot_params_rsdp_exist = false;
>         }
>         else
>                 boot_params_rsdp_exist = true;
>
>         if (!pa)
>                 pa = bios_get_rsdp_addr();
>
>         if (pa && !boot_params_rsdp_exist)
>                 boot_params.acpi_rsdp_addr = pa;
>
>         return pa;
> }
>
> At the same time, I notice kernel only parses it when
> "#ifdef CONFIG_ACPI", we should keep sync with kernel, but I think
> we are parsing SRAT, CONFIG_ACPI is needed sure, so I am going to
> update the define of EARLY_SRAT_PARSE:
>
> config EARLY_SRAT_PARSE
>         bool "EARLY SRAT parsing"
>         def_bool y
>         depends on RANDOMIZE_BASE && MEMORY_HOTREMOVE && ACPI
>
> Boris, how do you think the update for the new acpi_rsdp_addr?
> If I misunderstand something, please let me know.
>
> Thanks,
> Chao Fan
>

Hi, thanks for considering kexec usage,

but I think "boot_params_rsdp_exist" is not necessary,
boot_params->acpi_rsdp_addr should be either NULL or a valid value if
I, later initialization code considers it a valid value if it's not
NULL.

For the usage for Xen I'm not sure either, the info comes from commit
message of ae7e1238e68f2a that's also where boot_params.acpi_rsdp_addr
is first introduced, lets cc Juergen as well.


--
Best Regards,
Kairui Song

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

[Juergen Gross]

On 17/01/2019 09:22, Kairui Song wrote:
> On Thu, Jan 17, 2019 at 3:58 PM Chao Fan <fanc.fnst@cn.fujitsu.com> wrote:
>>
>> On Wed, Jan 16, 2019 at 03:28:52PM +0800, Kairui Song wrote:
>>> On Mon, Jan 7, 2019 at 11:24 AM Chao Fan <fanc.fnst@cn.fujitsu.com> wrote:
>>>>
>>>> +
>>>> +/* 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;
>>>> +}
>>>
>>> acpi_rsdp might be provided by boot_params.acpi_rsdp_addr too,
>>> it's introduced in ae7e1238e68f2a for Xen PVH guest and later move to
>>> boot_params in e6e094e053af,
>>> kexec could use it to pass RSDP to second kernel as well. Please check
>>> it as well make sure it always works.
>>>
>>
>> Hi Kairui,
>>
>> I saw the parsing code has been added to kernel, but I didn't see
>> where to fill in the 'acpi_rsdp_addr'. If only you(KEXEC) use it,
>> I can add "#ifdef CONFIG_KEXEC", but you said Xen will use it also,
>> so I didn't add ifdef to control it. I was trying to do as below:
>>
>> static inline acpi_physical_address get_boot_params_rsdp(void)
>> {
>>         return boot_params->acpi_rsdp_addr;
>> }
>>
>> static acpi_physical_address get_rsdp_addr(void)
>> {
>>         bool boot_params_rsdp_exist;
>>         acpi_physical_address pa;
>>
>>         pa = get_acpi_rsdp();
>>
>>         if (!pa)
>>                 pa = get_boot_params_rsdp();
>>
>>         if (!pa) {
>>                 pa = efi_get_rsdp_addr();
>>                 boot_params_rsdp_exist = false;
>>         }
>>         else
>>                 boot_params_rsdp_exist = true;
>>
>>         if (!pa)
>>                 pa = bios_get_rsdp_addr();
>>
>>         if (pa && !boot_params_rsdp_exist)
>>                 boot_params.acpi_rsdp_addr = pa;
>>
>>         return pa;
>> }
>>
>> At the same time, I notice kernel only parses it when
>> "#ifdef CONFIG_ACPI", we should keep sync with kernel, but I think
>> we are parsing SRAT, CONFIG_ACPI is needed sure, so I am going to
>> update the define of EARLY_SRAT_PARSE:
>>
>> config EARLY_SRAT_PARSE
>>         bool "EARLY SRAT parsing"
>>         def_bool y
>>         depends on RANDOMIZE_BASE && MEMORY_HOTREMOVE && ACPI
>>
>> Boris, how do you think the update for the new acpi_rsdp_addr?
>> If I misunderstand something, please let me know.
>>
>> Thanks,
>> Chao Fan
>>
> 
> Hi, thanks for considering kexec usage,
> 
> but I think "boot_params_rsdp_exist" is not necessary,
> boot_params->acpi_rsdp_addr should be either NULL or a valid value if
> I, later initialization code considers it a valid value if it's not
> NULL.
> 
> For the usage for Xen I'm not sure either, the info comes from commit
> message of ae7e1238e68f2a that's also where boot_params.acpi_rsdp_addr
> is first introduced, lets cc Juergen as well.

Thanks for adding me.

The Xen case is thought to prefer boot_params->acpi_rsdp_addr over all
other possibilities if set. This logic should be kept. It is used for
Xen PVH guests booted via grub2. Xen PVH guests don't have RSDP in low
memory, so this interface is needed.

In any case you should first try acpi_arch_get_root_pointer() as this
is the preferred way to override RSDP address.


Juergen

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

[Borislav Petkov]

On Thu, Jan 17, 2019 at 11:20:27AM +0800, Chao Fan wrote:
> I have changed as you suggested, looks clear without type cast, and
> we need some variable as long to calculate the address, and at same
> time as the struct pointer to find it's length, so I change as below,
> and get_acpi_srat_table() return an unsigned long.
> How do you think of that.
> 
> int get_immovable_mem_num(void)

I'd call that

int count_immovable_mem_regions(void)

or

int enumerate_immovable_regions(void)

or so and put a comment above it explaining what it does and what it
returns.

"mem_num" is not clear what it is.

> {
>         unsigned long table_addr, table_end, table;
>         struct acpi_table_header *table_header;
>         struct acpi_subtable_header *sub_table;
>         char arg[MAX_ACPI_ARG_LENGTH];
>         int num = 0;
> 
>         if (cmdline_find_option("acpi", arg, sizeof(arg)) == 3 &&
>             !strncmp(arg, "off", 3))
>                 return;
> 
>         table_addr = get_acpi_srat_table();
>         if (!table_addr)
>                 return;
> 
>         table_header = (struct acpi_table_header *)table_addr;
>         table_end = table_addr + table_header->length;
>         table = table_addr + sizeof(struct acpi_table_srat);
> 
>         while (table + sizeof(struct acpi_subtable_header) < table_end) {
>                 sub_table = (struct acpi_subtable_header *)table;
>                 if (sub_table->type == ACPI_SRAT_TYPE_MEMORY_AFFINITY) {
>                         struct acpi_srat_mem_affinity *ma;
> 
>                         ma = (struct acpi_srat_mem_affinity *)table;
>                         if (!(ma->flags & ACPI_SRAT_MEM_HOT_PLUGGABLE)) {
>                                 immovable_mem[num].start = ma->base_address;
>                                 immovable_mem[num].size = ma->length;
>                                 num++;
>                         }
> 
>                         if (num >= MAX_NUMNODES*2) {
>                                 debug_putstr("Too many immovable memory regions, aborting.\n");
>                                 return 0;
>                         }
>                 }
>                 table += sub_table->length;
>         }
>         return num;
> }

That looks clean.

Thx.

-- 
Regards/Gruss,
    Boris.

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

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

[Chao Fan]

On Thu, Jan 17, 2019 at 04:27:51PM +0100, Borislav Petkov wrote:
>On Thu, Jan 17, 2019 at 11:20:27AM +0800, Chao Fan wrote:
>> I have changed as you suggested, looks clear without type cast, and
>> we need some variable as long to calculate the address, and at same
>> time as the struct pointer to find it's length, so I change as below,
>> and get_acpi_srat_table() return an unsigned long.
>> How do you think of that.
>> 
>> int get_immovable_mem_num(void)
>
>I'd call that
>
>int count_immovable_mem_regions(void)
>
>or
>
>int enumerate_immovable_regions(void)
>
>or so and put a comment above it explaining what it does and what it
>returns.
>
>"mem_num" is not clear what it is.
>

Sounds good, I will rename it.

>> {
>>         unsigned long table_addr, table_end, table;
>>         struct acpi_table_header *table_header;
>>         struct acpi_subtable_header *sub_table;
>>         char arg[MAX_ACPI_ARG_LENGTH];
>>         int num = 0;
>> 
>>         if (cmdline_find_option("acpi", arg, sizeof(arg)) == 3 &&
>>             !strncmp(arg, "off", 3))
>>                 return;
>> 
>>         table_addr = get_acpi_srat_table();
>>         if (!table_addr)
>>                 return;
>> 
>>         table_header = (struct acpi_table_header *)table_addr;
>>         table_end = table_addr + table_header->length;
>>         table = table_addr + sizeof(struct acpi_table_srat);
>> 
>>         while (table + sizeof(struct acpi_subtable_header) < table_end) {
>>                 sub_table = (struct acpi_subtable_header *)table;
>>                 if (sub_table->type == ACPI_SRAT_TYPE_MEMORY_AFFINITY) {
>>                         struct acpi_srat_mem_affinity *ma;
>> 
>>                         ma = (struct acpi_srat_mem_affinity *)table;
>>                         if (!(ma->flags & ACPI_SRAT_MEM_HOT_PLUGGABLE)) {
>>                                 immovable_mem[num].start = ma->base_address;
>>                                 immovable_mem[num].size = ma->length;
>>                                 num++;
>>                         }
>> 
>>                         if (num >= MAX_NUMNODES*2) {
>>                                 debug_putstr("Too many immovable memory regions, aborting.\n");
>>                                 return 0;
>>                         }
>>                 }
>>                 table += sub_table->length;
>>         }
>>         return num;
>> }
>
>That looks clean.

Then I will change based on that.

Thanks,
Chao Fan

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

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

[Chao Fan]

On Wed, Jan 16, 2019 at 12:01:58PM +0100, Borislav Petkov wrote:
>On Mon, Jan 07, 2019 at 11:22:42AM +0800, Chao Fan wrote:
[...]
>> +
>> +/* 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;
>> +}

Hi Boris,

Talking in Kairui's thread may mislead you, let me clarify:
I mean I am going to change get_rsdp_addr(). Since I see ACPI code:

>acpi_physical_address __init acpi_os_get_root_pointer(void)
>{
>        acpi_physical_address pa;
>
>#ifdef CONFIG_KEXEC
>        if (acpi_rsdp)
>                return acpi_rsdp;
>#endif
>        pa = acpi_arch_get_root_pointer();
>        if (pa)
>                return pa;

So we need to parse boot_params->acpi_rsdp_addr also between KEXEC and
EFI:

static acpi_physical_address get_rsdp_addr(void)
{
        acpi_physical_address pa;

        pa = get_acpi_rsdp();

        if (!pa)
/* This line will be added in next version.*/
                pa = boot_params->acpi_rsdp_addr;

        if (!pa)
                pa = efi_get_rsdp_addr();

        if (!pa)
                pa = bios_get_rsdp_addr();

/* If no acpi_rsdp_addr found in boot_params, fill in it here. */

        return pa;
}

Then you said I shall fill the boot_params->acpi_rsdp_addr, so in my
understanding, if there is no acpi_rsdp_addr found in boot_params,
we can parse RSDP by EFI/BIOS then fill it, other wise if it's found
int boot_params, we need't to fill it.

Thanks,
Chao Fan


>> +
>> +/* Compute SRAT address 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 *)(long)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.*/
>
>This comment is wrong here - should be above.
>
>> +	if (!root_table)
>> +		return NULL;
>
><---- newline here.
>
>> +	header = (struct acpi_table_header *)(long)root_table;
>> +
>> +	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_CAST_PTR(u32, entry);
>> +		else {
>> +			address64 = *(u64 *)entry;
>> +			acpi_table = address64;
>
>The same thing as in the previous patch - get rid of address64.
>
>> +		}
>> +
>> +		if (acpi_table) {
>> +			header = (struct acpi_table_header *)(long)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;
>> +	char arg[MAX_ACPI_ARG_LENGTH];
>> +	unsigned long table_end;
>> +	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);
>
>That's a lot of unnecessary casting AFAICT. You can return an unsigned
>long from get_acpi_srat_table() and get rid of all that casting here.
>
>
>> +	}
>> +	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)
>> +{
>> +}
>
>Put those on a single line:
>
>static void get_immovable_mem(void) {}
>
>> +#endif
>> -- 
>> 2.20.1
>> 
>> 
>> 
>
>-- 
>Regards/Gruss,
>    Boris.
>
>Good mailing practices for 400: avoid top-posting and trim the reply.
>
>

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

[Chao Fan]

On Mon, Jan 21, 2019 at 05:33:48PM +0800, Chao Fan wrote:
>On Wed, Jan 16, 2019 at 12:01:58PM +0100, Borislav Petkov wrote:
>>On Mon, Jan 07, 2019 at 11:22:42AM +0800, Chao Fan wrote:
>[...]
>>> +
>>> +/* 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;
>>> +}
>
>Hi Boris,
>
>Talking in Kairui's thread may mislead you, let me clarify:
>I mean I am going to change get_rsdp_addr(). Since I see ACPI code:
>
>>acpi_physical_address __init acpi_os_get_root_pointer(void)
>>{
>>        acpi_physical_address pa;
>>
>>#ifdef CONFIG_KEXEC
>>        if (acpi_rsdp)
>>                return acpi_rsdp;
>>#endif
>>        pa = acpi_arch_get_root_pointer();
>>        if (pa)
>>                return pa;
>
>So we need to parse boot_params->acpi_rsdp_addr also between KEXEC and
>EFI:
>
>static acpi_physical_address get_rsdp_addr(void)
>{
>        acpi_physical_address pa;
>
>        pa = get_acpi_rsdp();
>
>        if (!pa)
>/* This line will be added in next version.*/
>                pa = boot_params->acpi_rsdp_addr;
>
>        if (!pa)
>                pa = efi_get_rsdp_addr();
>
>        if (!pa)
>                pa = bios_get_rsdp_addr();
>
>/* If no acpi_rsdp_addr found in boot_params, fill in it here. */
>
>        return pa;
>}

Or I clear this function as:
static acpi_physical_address get_rsdp_addr(void)
{
        acpi_physical_address pa;

        pa = get_acpi_rsdp();

        if (!pa)
                pa = boot_params->acpi_rsdp_addr;

        if (!pa)
                pa = efi_get_rsdp_addr();

        if (!pa)
                pa = bios_get_rsdp_addr();

        if (pa && !boot_params->acpi_rsdp_addr)
                boot_params->acpi_rsdp_addr = pa;

        return pa;
}

This version may look better, it does not need a bool.

Thanks,
Chao Fan

>
>Then you said I shall fill the boot_params->acpi_rsdp_addr, so in my
>understanding, if there is no acpi_rsdp_addr found in boot_params,
>we can parse RSDP by EFI/BIOS then fill it, other wise if it's found
>int boot_params, we need't to fill it.
>
>Thanks,
>Chao Fan

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

[Borislav Petkov]

On Mon, Jan 21, 2019 at 05:42:14PM +0800, Chao Fan wrote:
> Or I clear this function as:
> static acpi_physical_address get_rsdp_addr(void)
> {
>         acpi_physical_address pa;
> 
>         pa = get_acpi_rsdp();
> 
>         if (!pa)
>                 pa = boot_params->acpi_rsdp_addr;
> 
>         if (!pa)
>                 pa = efi_get_rsdp_addr();
> 
>         if (!pa)
>                 pa = bios_get_rsdp_addr();
> 
>         if (pa && !boot_params->acpi_rsdp_addr)
>                 boot_params->acpi_rsdp_addr = pa;

Or simply:

	if (pa)
		boot_params->acpi_rsdp_addr = pa;

-- 
Regards/Gruss,
    Boris.

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

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

[Chao Fan]

On Mon, Jan 21, 2019 at 10:45:24AM +0100, Borislav Petkov wrote:
>On Mon, Jan 21, 2019 at 05:42:14PM +0800, Chao Fan wrote:
>> Or I clear this function as:
>> static acpi_physical_address get_rsdp_addr(void)
>> {
>>         acpi_physical_address pa;
>> 
>>         pa = get_acpi_rsdp();
>> 
>>         if (!pa)
>>                 pa = boot_params->acpi_rsdp_addr;
>> 
>>         if (!pa)
>>                 pa = efi_get_rsdp_addr();
>> 
>>         if (!pa)
>>                 pa = bios_get_rsdp_addr();
>> 
>>         if (pa && !boot_params->acpi_rsdp_addr)
>>                 boot_params->acpi_rsdp_addr = pa;
>
>Or simply:
>
>	if (pa)
>		boot_params->acpi_rsdp_addr = pa;
>

Make sense. If pa is not zero, boot_params->acpi_rsdp_addr is zero or
the same as pa, so will change it.

Thanks,
Chao Fan

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