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

[PATCH v13 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 break memory
hotplug feature and make the movable memory chosen by KASLR can't be
removed.

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

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

PATCH 1/6 Copy kstrtoull() to boot/string.c
PATCH 2/6 Add get_acpi_rsdp() to parse RSDP in cmdline from KEXEC
PATCH 3/6 Add efi_get_rsdp_addr() to find RSDP from EFI table when
          booting from EFI.
PATCH 4/6 Add bios_get_rsdp_addr() to search RSDP in memory when EFI
          table not found.
PATCH 5/6 Compute SRAT from RSDP and walk SRAT to 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.

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

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

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

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

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

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

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

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

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

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

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()

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

Any comments will be welcome.

Chao Fan (6):
  x86/boot: Introduce kstrtoull() to boot directory instead of
    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 from RSDP and store immovable memory
  x86/boot/KASLR: Limit KASLR to extracting kernel in immovable memory

 arch/x86/Kconfig                  |  10 +
 arch/x86/boot/compressed/Makefile |   2 +
 arch/x86/boot/compressed/acpi.c   | 322 ++++++++++++++++++++++++++++++
 arch/x86/boot/compressed/kaslr.c  |  79 ++++++--
 arch/x86/boot/compressed/misc.h   |  21 ++
 arch/x86/boot/string.c            | 137 +++++++++++++
 arch/x86/boot/string.h            |   2 +
 7 files changed, 558 insertions(+), 15 deletions(-)
 create mode 100644 arch/x86/boot/compressed/acpi.c

-- 
2.19.2

[PATCH v13 1/6] x86/boot: Introduce kstrtoull() to boot directory instead of simple_strtoull()

[Chao Fan]

Introduce kstrtoull() from lib/kstrtox.c to boot directory so that code
in boot/ can use kstrtoull() and the old simple_strtoull() can be
replaced.

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..e02405f20f98 100644
--- a/arch/x86/boot/string.c
+++ b/arch/x86/boot/string.c
@@ -12,7 +12,10 @@
  * Very basic string functions
  */
 
+#define _LINUX_CTYPE_H
 #include <linux/types.h>
+#include <linux/kernel.h>
+#include <linux/errno.h>
 #include <asm/asm.h>
 #include "ctype.h"
 #include "string.h"
@@ -187,3 +190,137 @@ 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.19.2

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

[Chao Fan]

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

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

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

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

Signed-off-by: Chao Fan <fanc.fnst@cn.fujitsu.com>
---
 arch/x86/Kconfig                | 10 ++++++++++
 arch/x86/boot/compressed/acpi.c | 30 ++++++++++++++++++++++++++++++
 arch/x86/boot/compressed/misc.h |  6 ++++++
 3 files changed, 46 insertions(+)
 create mode 100644 arch/x86/boot/compressed/acpi.c

diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
index ba7e3464ee92..455da382fa9e 100644
--- a/arch/x86/Kconfig
+++ b/arch/x86/Kconfig
@@ -2149,6 +2149,16 @@ config X86_NEED_RELOCS
 	def_bool y
 	depends on RANDOMIZE_BASE || (X86_32 && RELOCATABLE)
 
+config EARLY_PARSE_RSDP
+	bool "Parse RSDP pointer on compressed period for KASLR"
+	def_bool y
+	depends on RANDOMIZE_BASE && MEMORY_HOTREMOVE
+	help
+	  This option parses RSDP in compressed period. Works
+	  for KASLR to get memory information from SRAT table and choose
+	  immovable memory to extract kernel.
+	  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/acpi.c b/arch/x86/boot/compressed/acpi.c
new file mode 100644
index 000000000000..cad15686f82c
--- /dev/null
+++ b/arch/x86/boot/compressed/acpi.c
@@ -0,0 +1,30 @@
+// SPDX-License-Identifier: GPL-2.0
+#define BOOT_CTYPE_H
+#include "misc.h"
+#include "error.h"
+
+#include <linux/efi.h>
+#include <linux/numa.h>
+#include <linux/acpi.h>
+#include <asm/efi.h>
+
+#define STATIC
+#include <linux/decompress/mm.h>
+
+#include "../string.h"
+
+static acpi_physical_address get_acpi_rsdp(void)
+{
+#ifdef CONFIG_KEXEC
+	unsigned long long res;
+	int len = 0;
+	char val[MAX_ADDRESS_LENGTH+1];
+
+	len = cmdline_find_option("acpi_rsdp", val, MAX_ADDRESS_LENGTH+1);
+	if (len > 0) {
+		val[len] = 0;
+		return (acpi_physical_address)kstrtoull(val, 16, &res);
+	}
+	return 0;
+#endif
+}
diff --git a/arch/x86/boot/compressed/misc.h b/arch/x86/boot/compressed/misc.h
index a1d5918765f3..72fcfbfec3c6 100644
--- a/arch/x86/boot/compressed/misc.h
+++ b/arch/x86/boot/compressed/misc.h
@@ -116,3 +116,9 @@ static inline void console_init(void)
 void set_sev_encryption_mask(void);
 
 #endif
+
+/* acpi.c */
+#ifdef CONFIG_EARLY_PARSE_RSDP
+/* Max length of 64-bit hex address string is 18, prefix "0x" + 16 hex digit. */
+#define MAX_ADDRESS_LENGTH 18
+#endif
-- 
2.19.2

[PATCH v13 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 | 79 +++++++++++++++++++++++++++++++++
 1 file changed, 79 insertions(+)

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

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

[Chao Fan]

To fix the conflict between KASLR and memory-hotremove, memory
Memory information in SRAT 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 | 85 +++++++++++++++++++++++++++++++++
 1 file changed, 85 insertions(+)

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

[PATCH v13 5/6] x86/boot: Parse SRAT 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.

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

diff --git a/arch/x86/boot/compressed/Makefile b/arch/x86/boot/compressed/Makefile
index 466f66c8a7f8..bcc6560fb2ec 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_PARSE_RSDP) += $(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 c546a463b8ba..6fa0a1ff5ba0 100644
--- a/arch/x86/boot/compressed/acpi.c
+++ b/arch/x86/boot/compressed/acpi.c
@@ -13,6 +13,9 @@
 
 #include "../string.h"
 
+/* Store the immovable memory regions. */
+struct mem_vector immovable_mem[MAX_NUMNODES*2];
+
 static acpi_physical_address get_acpi_rsdp(void)
 {
 #ifdef CONFIG_KEXEC
@@ -192,3 +195,128 @@ static acpi_physical_address bios_get_rsdp_addr(void)
 		return (acpi_physical_address)address;
 	}
 }
+
+/* Determine RSDP, based on acpi_os_get_root_pointer(). */
+static acpi_physical_address get_rsdp_addr(void)
+{
+	acpi_physical_address pa = 0;
+
+	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 *)get_rsdp_addr();
+	if (!rsdp)
+		return NULL;
+
+	/* Get RSDT or XSDT from RSDP. */
+	if (!(cmdline_find_option("acpi", arg, sizeof(arg)) == 4 &&
+	    !strncmp(arg, "rsdt", 4)) &&
+	    rsdp->xsdt_physical_address &&
+	    rsdp->revision > 1) {
+		root_table = rsdp->xsdt_physical_address;
+		size = ACPI_XSDT_ENTRY_SIZE;
+	} else {
+		root_table = rsdp->rsdt_physical_address;
+		size = ACPI_RSDT_ENTRY_SIZE;
+	}
+
+	/* Get ACPI root table from RSDT or XSDT.*/
+	header = (struct acpi_table_header *)root_table;
+	if (!header)
+		return NULL;
+
+	len = header->length;
+	if (len < sizeof(struct acpi_table_header) + size)
+		return NULL;
+
+	num_entries = (u32)((len - sizeof(struct acpi_table_header)) / size);
+	entry = ACPI_ADD_PTR(u8, header, sizeof(struct acpi_table_header));
+
+	while (num_entries--) {
+		u64 address64;
+
+		if (size == ACPI_RSDT_ENTRY_SIZE)
+			acpi_table = ((acpi_physical_address)
+				      (*ACPI_CAST_PTR(u32, entry)));
+		else {
+			*(u64 *)(void *)&address64 = *(u64 *)(void *)entry;
+			acpi_table = (acpi_physical_address) address64;
+		}
+
+		if (acpi_table) {
+			header = (struct acpi_table_header *)acpi_table;
+
+			if (ACPI_COMPARE_NAME(header->signature, ACPI_SIG_SRAT))
+				return header;
+		}
+		entry += size;
+	}
+	return NULL;
+}
+
+/*
+ * According to ACPI table, filter the immovable memory regions
+ * and store them in immovable_mem[].
+ */
+void get_immovable_mem(void)
+{
+	struct acpi_table_header *table_header;
+	struct acpi_subtable_header *table;
+	struct acpi_srat_mem_affinity *ma;
+	unsigned long table_end;
+	char arg[10];
+	int i = 0;
+
+	if (cmdline_find_option("acpi", arg, sizeof(arg)) == 3 &&
+	    !strncmp(arg, "off", 3))
+		return;
+
+	table_header = get_acpi_srat_table();
+	if (!table_header)
+		return;
+
+	table_end = (unsigned long)table_header + table_header->length;
+	table = (struct acpi_subtable_header *)
+		((unsigned long)table_header + sizeof(struct acpi_table_srat));
+
+	while (((unsigned long)table) +
+		       sizeof(struct acpi_subtable_header) < table_end) {
+		if (table->type == ACPI_SRAT_TYPE_MEMORY_AFFINITY) {
+			ma = (struct acpi_srat_mem_affinity *)table;
+			if (!(ma->flags & ACPI_SRAT_MEM_HOT_PLUGGABLE)) {
+				immovable_mem[i].start = ma->base_address;
+				immovable_mem[i].size = ma->length;
+				i++;
+			}
+
+			if (i >= MAX_NUMNODES*2) {
+				debug_putstr("Too many immovable memory regions, aborting.\n");
+				return;
+			}
+		}
+		table = (struct acpi_subtable_header *)
+			((unsigned long)table + table->length);
+	}
+	num_immovable_mem = i;
+}
diff --git a/arch/x86/boot/compressed/kaslr.c b/arch/x86/boot/compressed/kaslr.c
index 9ed9709d9947..b251572e77af 100644
--- a/arch/x86/boot/compressed/kaslr.c
+++ b/arch/x86/boot/compressed/kaslr.c
@@ -87,10 +87,6 @@ static unsigned long get_boot_seed(void)
 #define KASLR_COMPRESSED_BOOT
 #include "../../lib/kaslr.c"
 
-struct mem_vector {
-	unsigned long long start;
-	unsigned long long size;
-};
 
 /* Only supporting at most 4 unusable memmap regions with kaslr */
 #define MAX_MEMMAP_REGIONS	4
diff --git a/arch/x86/boot/compressed/misc.h b/arch/x86/boot/compressed/misc.h
index 72fcfbfec3c6..c6b4c8c3c2fc 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
@@ -118,7 +123,17 @@ 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_PARSE_RSDP
+void get_immovable_mem(void);
 /* Max length of 64-bit hex address string is 18, prefix "0x" + 16 hex digit. */
 #define MAX_ADDRESS_LENGTH 18
+#else
+static void get_immovable_mem(void)
+{
+}
 #endif
-- 
2.19.2

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

[Chao Fan]

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

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

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

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

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

Re: [PATCH v13 1/6] x86/boot: Introduce kstrtoull() to boot directory instead of simple_strtoull()

[Chao Fan]

On Wed, Dec 12, 2018 at 11:10:48AM +0800, Chao Fan wrote:
>Introduce kstrtoull() from lib/kstrtox.c to boot directory so that code
>in boot/ can use kstrtoull() and the old simple_strtoull() can be
>replaced.
>

Hi all,

Thanks for Boris, Baoquan and Masa's help, this PATCHSET has proceeded to
this step. With the talking in community, the key problem has been turned
from ACPI issue to kstrtoull() issue.
In this version, following the suggestion of Boris, I copy the kstrtoull()
to boot/string.c
But from last week, I was working on kstrtoull() issue in different methods
and try many times, there are several methods:
1. Copy kstrtoull() to boot/string.c
2. Include kstrtoull() to boot/string.c.
3. Use existing simple_strtoull() for now, and proceed to include kstrtoull()
   as a next work.

For method 1, PATCH is here, it's easy and clear.
For method 2, I tried and met many ifdeffery problems:
	First, the conflict of ctype.h header file.
	Second, redefine of isdigit() and memcpy() and other functions.
	Third, 'CONFIG_MODVERSIONS=y' causes ld error
	Forth, div_u64 causes make error.
Thanks to Baoquan and Masa, these problems have been fixed.
I success to make, but more warning issues is waiting to be fixed.
And I think several weeks are needed to handle the warning issue
since I am not so familiar to fix these problems.
But it blocked the main job for a long time, so if people think
the method 1 is not so good, I think method 3 is a choice.

Thanks,
Chao Fan

>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..e02405f20f98 100644
>--- a/arch/x86/boot/string.c
>+++ b/arch/x86/boot/string.c
>@@ -12,7 +12,10 @@
>  * Very basic string functions
>  */
> 
>+#define _LINUX_CTYPE_H
> #include <linux/types.h>
>+#include <linux/kernel.h>
>+#include <linux/errno.h>
> #include <asm/asm.h>
> #include "ctype.h"
> #include "string.h"
>@@ -187,3 +190,137 @@ 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.19.2
>

Re: [PATCH v13 1/6] x86/boot: Introduce kstrtoull() to boot directory instead of simple_strtoull()

[Baoquan He]

On 12/12/18 at 11:10am, Chao Fan wrote:
> Introduce kstrtoull() from lib/kstrtox.c to boot directory so that code

It's not introducing kstrtoull(), just copying kstrtoull() from
lib/kstrtox.c to boot.

> in boot/ can use kstrtoull() and the old simple_strtoull() can be
> replaced.
> 
> 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..e02405f20f98 100644
> --- a/arch/x86/boot/string.c
> +++ b/arch/x86/boot/string.c
> @@ -12,7 +12,10 @@
>   * Very basic string functions
>   */
>  
> +#define _LINUX_CTYPE_H
>  #include <linux/types.h>
> +#include <linux/kernel.h>
> +#include <linux/errno.h>
>  #include <asm/asm.h>
>  #include "ctype.h"
>  #include "string.h"
> @@ -187,3 +190,137 @@ 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.19.2
> 
> 
> 

Re: [PATCH v13 1/6] x86/boot: Introduce kstrtoull() to boot directory instead of simple_strtoull()

[Baoquan He]

On 12/12/18 at 12:12pm, Chao Fan wrote:
> On Wed, Dec 12, 2018 at 11:10:48AM +0800, Chao Fan wrote:
> >Introduce kstrtoull() from lib/kstrtox.c to boot directory so that code
> >in boot/ can use kstrtoull() and the old simple_strtoull() can be
> >replaced.
> >
> 
> Hi all,
> 
> Thanks for Boris, Baoquan and Masa's help, this PATCHSET has proceeded to
> this step. With the talking in community, the key problem has been turned
> from ACPI issue to kstrtoull() issue.
> In this version, following the suggestion of Boris, I copy the kstrtoull()
> to boot/string.c
> But from last week, I was working on kstrtoull() issue in different methods
> and try many times, there are several methods:
> 1. Copy kstrtoull() to boot/string.c
> 2. Include kstrtoull() to boot/string.c.
> 3. Use existing simple_strtoull() for now, and proceed to include kstrtoull()
>    as a next work.

If can incalude it to boot/string.c, that's surely the best. Since we
don't need to worry about update kstrtoull() update from /lib/kstrtox.c.
Currently simple_strtoull() is called in arch/x86/boot/compressed/kaslr.c
and arch/x86/boot/early_serial_console.c, if not easy to include kstrtoull()
to boot/string.c, copying it is also fine. 

Surely, using the old simple_strtoull() is fine too, we can take its
replacement into TODO list. This fix has blocked KASLR&hotplug
combination long time, now we have to ask customsers to add 'nokaslr'
always if they want to do memory hot add/remove on bare metal system.

See what other reviewers will say.

Thanks
Baoquan

Re: [PATCH v13 1/6] x86/boot: Introduce kstrtoull() to boot directory instead of simple_strtoull()

[Chao Fan]

On Wed, Dec 12, 2018 at 03:46:15PM +0800, Baoquan He wrote:
>On 12/12/18 at 11:10am, Chao Fan wrote:
>> Introduce kstrtoull() from lib/kstrtox.c to boot directory so that code
>
>It's not introducing kstrtoull(), just copying kstrtoull() from
>lib/kstrtox.c to boot.

Oops, maybe I misunderstand 'introduce'.

Thanks,
Chao Fan

>
>> in boot/ can use kstrtoull() and the old simple_strtoull() can be
>> replaced.
>> 
>> 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..e02405f20f98 100644
>> --- a/arch/x86/boot/string.c
>> +++ b/arch/x86/boot/string.c
>> @@ -12,7 +12,10 @@
>>   * Very basic string functions
>>   */
>>  
>> +#define _LINUX_CTYPE_H
>>  #include <linux/types.h>
>> +#include <linux/kernel.h>
>> +#include <linux/errno.h>
>>  #include <asm/asm.h>
>>  #include "ctype.h"
>>  #include "string.h"
>> @@ -187,3 +190,137 @@ 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.19.2
>> 
>> 
>> 
>
>

Re: [PATCH v13 1/6] x86/boot: Introduce kstrtoull() to boot directory instead of simple_strtoull()

[Borislav Petkov]

On Wed, Dec 12, 2018 at 11:10:48AM +0800, Chao Fan wrote:
> Introduce kstrtoull() from lib/kstrtox.c to boot directory so that code
> in boot/ can use kstrtoull() and the old simple_strtoull() can be
> replaced.
> 
> 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..e02405f20f98 100644
> --- a/arch/x86/boot/string.c
> +++ b/arch/x86/boot/string.c
> @@ -12,7 +12,10 @@
>   * Very basic string functions
>   */
>  
> +#define _LINUX_CTYPE_H
>  #include <linux/types.h>
> +#include <linux/kernel.h>
> +#include <linux/errno.h>
>  #include <asm/asm.h>
>  #include "ctype.h"
>  #include "string.h"
> @@ -187,3 +190,137 @@ 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;
> +}

Why aren't you using the simple one from include/linux/math64.h ?

Rest looks ok.

-- 
Regards/Gruss,
    Boris.

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

Re: [PATCH v13 1/6] x86/boot: Introduce kstrtoull() to boot directory instead of simple_strtoull()

[Borislav Petkov]

On Wed, Dec 12, 2018 at 04:03:12PM +0800, Baoquan He wrote:
> Surely, using the old simple_strtoull() is fine too, we can take its
> replacement into TODO list.

Yes, that would be nice.

-- 
Regards/Gruss,
    Boris.

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

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

[Masayoshi Mizuma]

Hi Chao,

Great work! Let me say some trivial comments.

On Wed, Dec 12, 2018 at 11:10:49AM +0800, Chao Fan wrote:
> Memory information in SRAT is necessary to fix the conflict between
> KASLR and memory-hotremove.
> 
> ACPI SRAT (System/Static Resource Affinity Table) shows the details
> about memory ranges, including ranges of memory provided by hot-added
> memory devices. SRAT is introduced by Root System Description
> Pointer(RSDP). So RSDP should be found firstly.
> 
> When booting form KEXEC/EFI/BIOS, the methods to find RSDP
> are different. When booting from KEXEC, 'acpi_rsdp' may have been
> added to cmdline, so parse cmdline to find RSDP.
> 
> Since 'RANDOMIZE_BASE' && 'MEMORY_HOTREMOVE' is needed, introduce
> 'CONFIG_EARLY_PARSE_RSDP' to make ifdeffery clear.
> 
> Signed-off-by: Chao Fan <fanc.fnst@cn.fujitsu.com>
> ---
>  arch/x86/Kconfig                | 10 ++++++++++
>  arch/x86/boot/compressed/acpi.c | 30 ++++++++++++++++++++++++++++++
>  arch/x86/boot/compressed/misc.h |  6 ++++++
>  3 files changed, 46 insertions(+)
>  create mode 100644 arch/x86/boot/compressed/acpi.c
> 
> diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
> index ba7e3464ee92..455da382fa9e 100644
> --- a/arch/x86/Kconfig
> +++ b/arch/x86/Kconfig
> @@ -2149,6 +2149,16 @@ config X86_NEED_RELOCS
>  	def_bool y
>  	depends on RANDOMIZE_BASE || (X86_32 && RELOCATABLE)
>  
> +config EARLY_PARSE_RSDP
> +	bool "Parse RSDP pointer on compressed period for KASLR"
> +	def_bool y
> +	depends on RANDOMIZE_BASE && MEMORY_HOTREMOVE
> +	help
> +	  This option parses RSDP in compressed period. Works
> +	  for KASLR to get memory information from SRAT table and choose
> +	  immovable memory to extract kernel.
> +	  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/acpi.c b/arch/x86/boot/compressed/acpi.c
> new file mode 100644
> index 000000000000..cad15686f82c
> --- /dev/null
> +++ b/arch/x86/boot/compressed/acpi.c
> @@ -0,0 +1,30 @@
> +// SPDX-License-Identifier: GPL-2.0
> +#define BOOT_CTYPE_H
> +#include "misc.h"
> +#include "error.h"
> +
> +#include <linux/efi.h>
> +#include <linux/numa.h>
> +#include <linux/acpi.h>
> +#include <asm/efi.h>
> +
> +#define STATIC
> +#include <linux/decompress/mm.h>
> +
> +#include "../string.h"
> +
> +static acpi_physical_address get_acpi_rsdp(void)
> +{
> +#ifdef CONFIG_KEXEC
> +	unsigned long long res;
> +	int len = 0;
> +	char val[MAX_ADDRESS_LENGTH+1];
> +

> +	len = cmdline_find_option("acpi_rsdp", val, MAX_ADDRESS_LENGTH+1);

sizeof() is better here.

	len = cmdline_find_option("acpi_rsdp", val, sizeof(var));

> +	if (len > 0) {
> +		val[len] = 0;

'\0' should be fine here not 0.

		val[len] = '\0';

> +		return (acpi_physical_address)kstrtoull(val, 16, &res);
> +	}
> +	return 0;
> +#endif
> +}
> diff --git a/arch/x86/boot/compressed/misc.h b/arch/x86/boot/compressed/misc.h
> index a1d5918765f3..72fcfbfec3c6 100644
> --- a/arch/x86/boot/compressed/misc.h
> +++ b/arch/x86/boot/compressed/misc.h
> @@ -116,3 +116,9 @@ static inline void console_init(void)
>  void set_sev_encryption_mask(void);
>  
>  #endif
> +
> +/* acpi.c */
> +#ifdef CONFIG_EARLY_PARSE_RSDP
> +/* Max length of 64-bit hex address string is 18, prefix "0x" + 16 hex digit. */
> +#define MAX_ADDRESS_LENGTH 18
> +#endif
> -- 
> 2.19.2
> 
> 

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

[Borislav Petkov]

On Thu, Dec 13, 2018 at 02:25:30PM -0500, Masayoshi Mizuma wrote:
> > +	len = cmdline_find_option("acpi_rsdp", val, MAX_ADDRESS_LENGTH+1);
> 
> sizeof() is better here.
> 
> 	len = cmdline_find_option("acpi_rsdp", val, sizeof(var));

Why is it better?

That makes you go look for the "val" variable and see what it's size is.
MAX_ADDRESS_LENGTH+1 is OTOH explicit.

> 
> > +	if (len > 0) {
> > +		val[len] = 0;
> 
> '\0' should be fine here not 0.
> 
> 		val[len] = '\0';

Yes.

-- 
Regards/Gruss,
    Boris.

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

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

[Masayoshi Mizuma]

On Thu, Dec 13, 2018 at 08:29:50PM +0100, Borislav Petkov wrote:
> On Thu, Dec 13, 2018 at 02:25:30PM -0500, Masayoshi Mizuma wrote:
> > > +	len = cmdline_find_option("acpi_rsdp", val, MAX_ADDRESS_LENGTH+1);
> > 
> > sizeof() is better here.
> > 
> > 	len = cmdline_find_option("acpi_rsdp", val, sizeof(var));
> 
> Why is it better?
> 
> That makes you go look for the "val" variable and see what it's size is.
> MAX_ADDRESS_LENGTH+1 is OTOH explicit.

Ah, thanks, make sense.

- Masa

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

[Borislav Petkov]

On Wed, Dec 12, 2018 at 11:10:49AM +0800, Chao Fan wrote:
> Memory information in SRAT is necessary to fix the conflict between
> KASLR and memory-hotremove.
> 
> ACPI SRAT (System/Static Resource Affinity Table) shows the details
> about memory ranges, including ranges of memory provided by hot-added
> memory devices. SRAT is introduced by Root System Description
> Pointer(RSDP). So RSDP should be found firstly.
> 
> When booting form KEXEC/EFI/BIOS, the methods to find RSDP
> are different. When booting from KEXEC, 'acpi_rsdp' may have been
> added to cmdline, so parse cmdline to find RSDP.
> 
> Since 'RANDOMIZE_BASE' && 'MEMORY_HOTREMOVE' is needed, introduce
> 'CONFIG_EARLY_PARSE_RSDP' to make ifdeffery clear.
> 
> Signed-off-by: Chao Fan <fanc.fnst@cn.fujitsu.com>
> ---
>  arch/x86/Kconfig                | 10 ++++++++++
>  arch/x86/boot/compressed/acpi.c | 30 ++++++++++++++++++++++++++++++
>  arch/x86/boot/compressed/misc.h |  6 ++++++
>  3 files changed, 46 insertions(+)
>  create mode 100644 arch/x86/boot/compressed/acpi.c
> 
> diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
> index ba7e3464ee92..455da382fa9e 100644
> --- a/arch/x86/Kconfig
> +++ b/arch/x86/Kconfig
> @@ -2149,6 +2149,16 @@ config X86_NEED_RELOCS
>  	def_bool y
>  	depends on RANDOMIZE_BASE || (X86_32 && RELOCATABLE)
>  
> +config EARLY_PARSE_RSDP

That should be EARLY_SRAT_PARSE or so

> +	bool "Parse RSDP pointer on compressed period for KASLR"

and that should be

	bool "Early SRAT table parsing"

> +	def_bool y
> +	depends on RANDOMIZE_BASE && MEMORY_HOTREMOVE
> +	help
> +	  This option parses RSDP in compressed period. Works

do s/compressed period/compressed stage/g

I think "compressed stage" or "compressed boot stage" is more precise.

> +	  for KASLR to get memory information from SRAT table and choose
> +	  immovable memory to extract kernel.
> +	  Say Y if you want to use both KASLR and memory-hotremove.

This help text needs to explain what the functionality is for. "This
option parses RSDP in compressed period" doesn't tell a whole lot to the
normal user wondering whether she should enable this or not. You need to
explain whether people would need this or not so should start along the
lines of:

"This option enables early SRAT parsing so that memory hot remove ranges do not
overlap with KASLR chosen ranges..."

Basically, the "***Background" in your 0th message.

> +
>  config PHYSICAL_ALIGN
>  	hex "Alignment value to which kernel should be aligned"
>  	default "0x200000"
> diff --git a/arch/x86/boot/compressed/acpi.c b/arch/x86/boot/compressed/acpi.c
> new file mode 100644
> index 000000000000..cad15686f82c
> --- /dev/null
> +++ b/arch/x86/boot/compressed/acpi.c
> @@ -0,0 +1,30 @@
> +// SPDX-License-Identifier: GPL-2.0
> +#define BOOT_CTYPE_H
> +#include "misc.h"
> +#include "error.h"
> +
> +#include <linux/efi.h>
> +#include <linux/numa.h>
> +#include <linux/acpi.h>
> +#include <asm/efi.h>
> +
> +#define STATIC
> +#include <linux/decompress/mm.h>
> +
> +#include "../string.h"
> +
> +static acpi_physical_address get_acpi_rsdp(void)
> +{
> +#ifdef CONFIG_KEXEC
> +	unsigned long long res;
> +	int len = 0;
> +	char val[MAX_ADDRESS_LENGTH+1];

Please sort function local variables declaration in a reverse christmas
tree order:

	<type A> longest_variable_name;
	<type B> shorter_var_name;
	<type C> even_shorter;
	<type D> i;

> +
> +	len = cmdline_find_option("acpi_rsdp", val, MAX_ADDRESS_LENGTH+1);

That MAX_ADDRESS_LENGTH define is used only here, right? So put it at
the top in this file - no need to have it in a header.

> +	if (len > 0) {
> +		val[len] = 0;
> +		return (acpi_physical_address)kstrtoull(val, 16, &res);
> +	}
> +	return 0;
> +#endif

Those two lines need to be the other way around:

#endif
	return 0;

because in the !CONFIG_KEXEC case that function won't return anything.
gcc is smart enough to optimize that function away so that there's no
build error but still.

Ditto for efi_get_rsdp_addr() in your next patch.

> +}
> diff --git a/arch/x86/boot/compressed/misc.h b/arch/x86/boot/compressed/misc.h
> index a1d5918765f3..72fcfbfec3c6 100644
> --- a/arch/x86/boot/compressed/misc.h
> +++ b/arch/x86/boot/compressed/misc.h
> @@ -116,3 +116,9 @@ static inline void console_init(void)
>  void set_sev_encryption_mask(void);
>  
>  #endif
> +
> +/* acpi.c */
> +#ifdef CONFIG_EARLY_PARSE_RSDP
> +/* Max length of 64-bit hex address string is 18, prefix "0x" + 16 hex digit. */
> +#define MAX_ADDRESS_LENGTH 18
> +#endif
> -- 

Yeah, just put that define in acpi.c.

-- 
Regards/Gruss,
    Boris.

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

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

[Borislav Petkov]

On Wed, Dec 12, 2018 at 11:10:50AM +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 | 79 +++++++++++++++++++++++++++++++++
>  1 file changed, 79 insertions(+)
> 
> diff --git a/arch/x86/boot/compressed/acpi.c b/arch/x86/boot/compressed/acpi.c
> index cad15686f82c..c96008712ec9 100644
> --- a/arch/x86/boot/compressed/acpi.c
> +++ b/arch/x86/boot/compressed/acpi.c
> @@ -28,3 +28,82 @@ static acpi_physical_address get_acpi_rsdp(void)
>  	return 0;
>  #endif
>  }
> +
> +/* Search EFI table for RSDP. */
> +static acpi_physical_address efi_get_rsdp_addr(void)
> +{
> +#ifdef CONFIG_EFI
> +	acpi_physical_address rsdp_addr = 0;
> +	efi_system_table_t *systab;
> +	struct efi_info *e;
> +	bool efi_64;
> +	char *sig;
> +	int size;
> +	int i;
> +
> +	e = &boot_params->efi_info;
> +	sig = (char *)&e->efi_loader_signature;
> +
> +	if (!strncmp(sig, EFI64_LOADER_SIGNATURE, 4))
> +		efi_64 = true;
> +	else if (!strncmp(sig, EFI32_LOADER_SIGNATURE, 4))
> +		efi_64 = false;
> +	else {
> +		debug_putstr("Wrong EFI loader signature.\n");
> +		return 0;
> +	}

Use curly braces for all three branches above.

> +
> +	/* Get systab from boot params. Based on efi_init(). */
> +#ifdef CONFIG_X86_64
> +	systab = (efi_system_table_t *)(e->efi_systab | ((__u64)e->efi_systab_hi<<32));
> +#else
> +	if (e->efi_systab_hi || e->efi_memmap_hi) {
> +		debug_putstr("Error getting RSDP address: EFI system table located above 4GB.\n");
> +		return 0;
> +	}
> +	systab = (efi_system_table_t *)e->efi_systab;
> +#endif
> +
> +	if (!systab)
> +		return 0;
> +
> +	/*
> +	 * Get EFI tables from systab. Based on efi_config_init() and
> +	 * efi_config_parse_tables().
> +	 */
> +	size = efi_64 ? sizeof(efi_config_table_64_t) :
> +			sizeof(efi_config_table_32_t);
> +
> +	for (i = 0; i < systab->nr_tables; i++) {
> +		void *config_tables;
> +		unsigned long table;
> +		efi_guid_t guid;
> +
> +		config_tables = (void *)(systab->tables + size * i);
> +		if (efi_64) {
> +			efi_config_table_64_t *tmp_table;
> +
> +			tmp_table = (efi_config_table_64_t *)config_tables;
> +			guid = tmp_table->guid;
> +			table = tmp_table->table;
> +
> +			if (!IS_ENABLED(CONFIG_X86_64) && table >> 32) {
> +				debug_putstr("Error getting RSDP address: EFI system table located above 4GB.\n");

Do s/system/config/ here so that this error message is different from
the one above.

> +				return 0;
> +			}
> +		} else {
> +			efi_config_table_32_t *tmp_table;
> +
> +			tmp_table = (efi_config_table_32_t *)config_tables;
> +			guid = tmp_table->guid;
> +			table = tmp_table->table;
> +		}
> +
> +		if (!(efi_guidcmp(guid, ACPI_TABLE_GUID)))
> +			rsdp_addr = (acpi_physical_address)table;
> +		else if (!(efi_guidcmp(guid, ACPI_20_TABLE_GUID)))
> +			return (acpi_physical_address)table;
> +	}
> +	return rsdp_addr;
> +#endif

	}
#endif
	return rsdp_addr;
}

IOW, you have:

static acpi_physical_address efi_get_rsdp_addr(void)
{
        acpi_physical_address rsdp_addr = 0;
#ifdef CONFIG_EFI
	...
#endif
	return rsdp_addr;
}

-- 
Regards/Gruss,
    Boris.

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

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

[Chao Fan]

On Thu, Dec 13, 2018 at 09:23:04PM +0100, Borislav Petkov wrote:
>On Wed, Dec 12, 2018 at 11:10:50AM +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 | 79 +++++++++++++++++++++++++++++++++
>>  1 file changed, 79 insertions(+)
>> 
>> diff --git a/arch/x86/boot/compressed/acpi.c b/arch/x86/boot/compressed/acpi.c
>> index cad15686f82c..c96008712ec9 100644
>> --- a/arch/x86/boot/compressed/acpi.c
>> +++ b/arch/x86/boot/compressed/acpi.c
>> @@ -28,3 +28,82 @@ static acpi_physical_address get_acpi_rsdp(void)
>>  	return 0;
>>  #endif
>>  }
>> +
>> +/* Search EFI table for RSDP. */
>> +static acpi_physical_address efi_get_rsdp_addr(void)
>> +{
>> +#ifdef CONFIG_EFI
>> +	acpi_physical_address rsdp_addr = 0;
>> +	efi_system_table_t *systab;
>> +	struct efi_info *e;
>> +	bool efi_64;
>> +	char *sig;
>> +	int size;
>> +	int i;
>> +
>> +	e = &boot_params->efi_info;
>> +	sig = (char *)&e->efi_loader_signature;
>> +
>> +	if (!strncmp(sig, EFI64_LOADER_SIGNATURE, 4))
>> +		efi_64 = true;
>> +	else if (!strncmp(sig, EFI32_LOADER_SIGNATURE, 4))
>> +		efi_64 = false;
>> +	else {
>> +		debug_putstr("Wrong EFI loader signature.\n");
>> +		return 0;
>> +	}
>
>Use curly braces for all three branches above.
>

OK, I will add in the first and second.

>> +
>> +	/* Get systab from boot params. Based on efi_init(). */
>> +#ifdef CONFIG_X86_64
>> +	systab = (efi_system_table_t *)(e->efi_systab | ((__u64)e->efi_systab_hi<<32));
>> +#else
>> +	if (e->efi_systab_hi || e->efi_memmap_hi) {
>> +		debug_putstr("Error getting RSDP address: EFI system table located above 4GB.\n");
>> +		return 0;
>> +	}
>> +	systab = (efi_system_table_t *)e->efi_systab;
>> +#endif
>> +
>> +	if (!systab)
>> +		return 0;
>> +
>> +	/*
>> +	 * Get EFI tables from systab. Based on efi_config_init() and
>> +	 * efi_config_parse_tables().
>> +	 */
>> +	size = efi_64 ? sizeof(efi_config_table_64_t) :
>> +			sizeof(efi_config_table_32_t);
>> +
>> +	for (i = 0; i < systab->nr_tables; i++) {
>> +		void *config_tables;
>> +		unsigned long table;
>> +		efi_guid_t guid;
>> +
>> +		config_tables = (void *)(systab->tables + size * i);
>> +		if (efi_64) {
>> +			efi_config_table_64_t *tmp_table;
>> +
>> +			tmp_table = (efi_config_table_64_t *)config_tables;
>> +			guid = tmp_table->guid;
>> +			table = tmp_table->table;
>> +
>> +			if (!IS_ENABLED(CONFIG_X86_64) && table >> 32) {
>> +				debug_putstr("Error getting RSDP address: EFI system table located above 4GB.\n");
>
>Do s/system/config/ here so that this error message is different from
>the one above.

Yes, will change it.

>
>> +				return 0;
>> +			}
>> +		} else {
>> +			efi_config_table_32_t *tmp_table;
>> +
>> +			tmp_table = (efi_config_table_32_t *)config_tables;
>> +			guid = tmp_table->guid;
>> +			table = tmp_table->table;
>> +		}
>> +
>> +		if (!(efi_guidcmp(guid, ACPI_TABLE_GUID)))
>> +			rsdp_addr = (acpi_physical_address)table;
>> +		else if (!(efi_guidcmp(guid, ACPI_20_TABLE_GUID)))
>> +			return (acpi_physical_address)table;
>> +	}
>> +	return rsdp_addr;
>> +#endif
>
>	}
>#endif
>	return rsdp_addr;
>}
>
>IOW, you have:
>
>static acpi_physical_address efi_get_rsdp_addr(void)
>{
>        acpi_physical_address rsdp_addr = 0;
>#ifdef CONFIG_EFI
>	...
>#endif
>	return rsdp_addr;
>}

Make sense. Thanks.

Thanks,
Chao Fan

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

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

[Chao Fan]

On Thu, Dec 13, 2018 at 08:42:30PM +0100, Borislav Petkov wrote:
>On Wed, Dec 12, 2018 at 11:10:49AM +0800, Chao Fan wrote:
>> Memory information in SRAT is necessary to fix the conflict between
>> KASLR and memory-hotremove.
>> 
>> ACPI SRAT (System/Static Resource Affinity Table) shows the details
>> about memory ranges, including ranges of memory provided by hot-added
>> memory devices. SRAT is introduced by Root System Description
>> Pointer(RSDP). So RSDP should be found firstly.
>> 
>> When booting form KEXEC/EFI/BIOS, the methods to find RSDP
>> are different. When booting from KEXEC, 'acpi_rsdp' may have been
>> added to cmdline, so parse cmdline to find RSDP.
>> 
>> Since 'RANDOMIZE_BASE' && 'MEMORY_HOTREMOVE' is needed, introduce
>> 'CONFIG_EARLY_PARSE_RSDP' to make ifdeffery clear.
>> 
>> Signed-off-by: Chao Fan <fanc.fnst@cn.fujitsu.com>
>> ---
>>  arch/x86/Kconfig                | 10 ++++++++++
>>  arch/x86/boot/compressed/acpi.c | 30 ++++++++++++++++++++++++++++++
>>  arch/x86/boot/compressed/misc.h |  6 ++++++
>>  3 files changed, 46 insertions(+)
>>  create mode 100644 arch/x86/boot/compressed/acpi.c
>> 
>> diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
>> index ba7e3464ee92..455da382fa9e 100644
>> --- a/arch/x86/Kconfig
>> +++ b/arch/x86/Kconfig
>> @@ -2149,6 +2149,16 @@ config X86_NEED_RELOCS
>>  	def_bool y
>>  	depends on RANDOMIZE_BASE || (X86_32 && RELOCATABLE)
>>  
>> +config EARLY_PARSE_RSDP
>
>That should be EARLY_SRAT_PARSE or so
>
>> +	bool "Parse RSDP pointer on compressed period for KASLR"
>
>and that should be
>
>	bool "Early SRAT table parsing"
>
>> +	def_bool y
>> +	depends on RANDOMIZE_BASE && MEMORY_HOTREMOVE
>> +	help
>> +	  This option parses RSDP in compressed period. Works
>
>do s/compressed period/compressed stage/g
>
>I think "compressed stage" or "compressed boot stage" is more precise.
>
>> +	  for KASLR to get memory information from SRAT table and choose
>> +	  immovable memory to extract kernel.
>> +	  Say Y if you want to use both KASLR and memory-hotremove.
>
>This help text needs to explain what the functionality is for. "This
>option parses RSDP in compressed period" doesn't tell a whole lot to the
>normal user wondering whether she should enable this or not. You need to
>explain whether people would need this or not so should start along the
>lines of:
>
>"This option enables early SRAT parsing so that memory hot remove ranges do not
>overlap with KASLR chosen ranges..."
>
>Basically, the "***Background" in your 0th message.
>

Thanks, I will rewrite this part.

>> +
>>  config PHYSICAL_ALIGN
>>  	hex "Alignment value to which kernel should be aligned"
>>  	default "0x200000"
>> diff --git a/arch/x86/boot/compressed/acpi.c b/arch/x86/boot/compressed/acpi.c
>> new file mode 100644
>> index 000000000000..cad15686f82c
>> --- /dev/null
>> +++ b/arch/x86/boot/compressed/acpi.c
>> @@ -0,0 +1,30 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +#define BOOT_CTYPE_H
>> +#include "misc.h"
>> +#include "error.h"
>> +
>> +#include <linux/efi.h>
>> +#include <linux/numa.h>
>> +#include <linux/acpi.h>
>> +#include <asm/efi.h>
>> +
>> +#define STATIC
>> +#include <linux/decompress/mm.h>
>> +
>> +#include "../string.h"
>> +
>> +static acpi_physical_address get_acpi_rsdp(void)
>> +{
>> +#ifdef CONFIG_KEXEC
>> +	unsigned long long res;
>> +	int len = 0;
>> +	char val[MAX_ADDRESS_LENGTH+1];
>
>Please sort function local variables declaration in a reverse christmas
>tree order:
>
>	<type A> longest_variable_name;
>	<type B> shorter_var_name;
>	<type C> even_shorter;
>	<type D> i;

I will change the position and check other places like this.

>
>> +
>> +	len = cmdline_find_option("acpi_rsdp", val, MAX_ADDRESS_LENGTH+1);
>
>That MAX_ADDRESS_LENGTH define is used only here, right? So put it at
>the top in this file - no need to have it in a header.
>
>> +	if (len > 0) {
>> +		val[len] = 0;
>> +		return (acpi_physical_address)kstrtoull(val, 16, &res);
>> +	}
>> +	return 0;
>> +#endif
>
>Those two lines need to be the other way around:
>
>#endif
>	return 0;
>
>because in the !CONFIG_KEXEC case that function won't return anything.
>gcc is smart enough to optimize that function away so that there's no
>build error but still.
>
>Ditto for efi_get_rsdp_addr() in your next patch.

Yes, I will change all places like this.

Thanks,
Chao Fan

>
>> +}
>> diff --git a/arch/x86/boot/compressed/misc.h b/arch/x86/boot/compressed/misc.h
>> index a1d5918765f3..72fcfbfec3c6 100644
>> --- a/arch/x86/boot/compressed/misc.h
>> +++ b/arch/x86/boot/compressed/misc.h
>> @@ -116,3 +116,9 @@ static inline void console_init(void)
>>  void set_sev_encryption_mask(void);
>>  
>>  #endif
>> +
>> +/* acpi.c */
>> +#ifdef CONFIG_EARLY_PARSE_RSDP
>> +/* Max length of 64-bit hex address string is 18, prefix "0x" + 16 hex digit. */
>> +#define MAX_ADDRESS_LENGTH 18
>> +#endif
>> -- 
>
>Yeah, just put that define in acpi.c.
>
>-- 
>Regards/Gruss,
>    Boris.
>
>Good mailing practices for 400: avoid top-posting and trim the reply.
>
>

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

[Chao Fan]

On Thu, Dec 13, 2018 at 02:25:30PM -0500, Masayoshi Mizuma wrote:
>Hi Chao,
>
>Great work! Let me say some trivial comments.

Thanks for your help, any comments will be welcome.

>
>On Wed, Dec 12, 2018 at 11:10:49AM +0800, Chao Fan wrote:
>> Memory information in SRAT is necessary to fix the conflict between
>> KASLR and memory-hotremove.
>> 
>> ACPI SRAT (System/Static Resource Affinity Table) shows the details
>> about memory ranges, including ranges of memory provided by hot-added
>> memory devices. SRAT is introduced by Root System Description
>> Pointer(RSDP). So RSDP should be found firstly.
>> 
>> When booting form KEXEC/EFI/BIOS, the methods to find RSDP
>> are different. When booting from KEXEC, 'acpi_rsdp' may have been
>> added to cmdline, so parse cmdline to find RSDP.
>> 
>> Since 'RANDOMIZE_BASE' && 'MEMORY_HOTREMOVE' is needed, introduce
>> 'CONFIG_EARLY_PARSE_RSDP' to make ifdeffery clear.
>> 
>> Signed-off-by: Chao Fan <fanc.fnst@cn.fujitsu.com>
>> ---
>>  arch/x86/Kconfig                | 10 ++++++++++
>>  arch/x86/boot/compressed/acpi.c | 30 ++++++++++++++++++++++++++++++
>>  arch/x86/boot/compressed/misc.h |  6 ++++++
>>  3 files changed, 46 insertions(+)
>>  create mode 100644 arch/x86/boot/compressed/acpi.c
>> 
>> diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
>> index ba7e3464ee92..455da382fa9e 100644
>> --- a/arch/x86/Kconfig
>> +++ b/arch/x86/Kconfig
>> @@ -2149,6 +2149,16 @@ config X86_NEED_RELOCS
>>  	def_bool y
>>  	depends on RANDOMIZE_BASE || (X86_32 && RELOCATABLE)
>>  
>> +config EARLY_PARSE_RSDP
>> +	bool "Parse RSDP pointer on compressed period for KASLR"
>> +	def_bool y
>> +	depends on RANDOMIZE_BASE && MEMORY_HOTREMOVE
>> +	help
>> +	  This option parses RSDP in compressed period. Works
>> +	  for KASLR to get memory information from SRAT table and choose
>> +	  immovable memory to extract kernel.
>> +	  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/acpi.c b/arch/x86/boot/compressed/acpi.c
>> new file mode 100644
>> index 000000000000..cad15686f82c
>> --- /dev/null
>> +++ b/arch/x86/boot/compressed/acpi.c
>> @@ -0,0 +1,30 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +#define BOOT_CTYPE_H
>> +#include "misc.h"
>> +#include "error.h"
>> +
>> +#include <linux/efi.h>
>> +#include <linux/numa.h>
>> +#include <linux/acpi.h>
>> +#include <asm/efi.h>
>> +
>> +#define STATIC
>> +#include <linux/decompress/mm.h>
>> +
>> +#include "../string.h"
>> +
>> +static acpi_physical_address get_acpi_rsdp(void)
>> +{
>> +#ifdef CONFIG_KEXEC
>> +	unsigned long long res;
>> +	int len = 0;
>> +	char val[MAX_ADDRESS_LENGTH+1];
>> +
>
>> +	len = cmdline_find_option("acpi_rsdp", val, MAX_ADDRESS_LENGTH+1);
>
>sizeof() is better here.
>
>	len = cmdline_find_option("acpi_rsdp", val, sizeof(var));
>
>> +	if (len > 0) {
>> +		val[len] = 0;
>
>'\0' should be fine here not 0.
>
>		val[len] = '\0';
Will change it.

Thanks,
Chao Fan
>
>> +		return (acpi_physical_address)kstrtoull(val, 16, &res);
>> +	}
>> +	return 0;
>> +#endif
>> +}
>> diff --git a/arch/x86/boot/compressed/misc.h b/arch/x86/boot/compressed/misc.h
>> index a1d5918765f3..72fcfbfec3c6 100644
>> --- a/arch/x86/boot/compressed/misc.h
>> +++ b/arch/x86/boot/compressed/misc.h
>> @@ -116,3 +116,9 @@ static inline void console_init(void)
>>  void set_sev_encryption_mask(void);
>>  
>>  #endif
>> +
>> +/* acpi.c */
>> +#ifdef CONFIG_EARLY_PARSE_RSDP
>> +/* Max length of 64-bit hex address string is 18, prefix "0x" + 16 hex digit. */
>> +#define MAX_ADDRESS_LENGTH 18
>> +#endif
>> -- 
>> 2.19.2
>> 
>> 
>
>

Re: [PATCH v13 1/6] x86/boot: Introduce kstrtoull() to boot directory instead of simple_strtoull()

[Chao Fan]

On Thu, Dec 13, 2018 at 02:26:03PM +0100, Borislav Petkov wrote:
>On Wed, Dec 12, 2018 at 04:03:12PM +0800, Baoquan He wrote:
>> Surely, using the old simple_strtoull() is fine too, we can take its
>> replacement into TODO list.
>
>Yes, that would be nice.

Thanks, then I can use simple_strtoull() for now.

Thanks,
Chao Fan

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

Re: [PATCH v13 1/6] x86/boot: Introduce kstrtoull() to boot directory instead of simple_strtoull()

[Chao Fan]

>> +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;
>> +}
>
>Why aren't you using the simple one from include/linux/math64.h ?

Cant't use div_u64() in math64.h directly, because  that will cause ld error.

Even in 64-bit environment, arch/x86/boot/*.o will be built as 32-bit
binary. In 64-bit binary, the dividend is handled as 64-bit value,
that's OK, but in 32-bit binary, that's wrong. So it's necessary
to separate the dividend to upper and lower in 32-bit binary.
That's why copy the needed div_u64() here.

So, when copying kstrtoull(), we should copy div_u64().
When trying to include lib/kstrtox.c, even if other error in make period
are solved, we will also meet this error. We should also copy div_u64
also. And then cover the math64.h in boot/string.c, and hanlde the other
error and warining who comes together.

So just like Baoquan said, it's a good choice to use simple_strtoull()
for now.

Thanks,
Chao Fan

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

Re: [PATCH v13 1/6] x86/boot: Introduce kstrtoull() to boot directory instead of simple_strtoull()

[Borislav Petkov]

On Fri, Dec 14, 2018 at 09:34:21AM +0800, Chao Fan wrote:
> On Thu, Dec 13, 2018 at 02:26:03PM +0100, Borislav Petkov wrote:
> >On Wed, Dec 12, 2018 at 04:03:12PM +0800, Baoquan He wrote:
> >> Surely, using the old simple_strtoull() is fine too, we can take its
> >> replacement into TODO list.
> >
> >Yes, that would be nice.
> 
> Thanks, then I can use simple_strtoull() for now.

I don't understand - patch 1 in your v13 has kstrtoull() added to
boot/string.c. Why are you reverting back to simple_strtoull()?

-- 
Regards/Gruss,
    Boris.

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

Re: [PATCH v13 1/6] x86/boot: Introduce kstrtoull() to boot directory instead of simple_strtoull()

[Borislav Petkov]

On Fri, Dec 14, 2018 at 10:59:17AM +0800, Chao Fan wrote:
> Cant't use div_u64() in math64.h directly, because  that will cause ld error.

Ok, I see it now.

> Even in 64-bit environment, arch/x86/boot/*.o will be built as 32-bit
> binary. In 64-bit binary, the dividend is handled as 64-bit value,
> that's OK, but in 32-bit binary, that's wrong. So it's necessary
> to separate the dividend to upper and lower in 32-bit binary.
> That's why copy the needed div_u64() here.

You could've written this in the commit message. This is an important
piece of information.

> So just like Baoquan said, it's a good choice to use simple_strtoull()
> for now.

So, we tried a little, we encountered a problem and now we're giving up?
Is that what you're trying to tell me?

Please explain what are we in a hurry for and why aren't we doing it
right?

Btw, one more thing I noticed:

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

This would've never worked because the parsed integer gets returned in
res but you're returning the kstrtoull() return value, which is

"Returns 0 on success, -ERANGE on overflow and -EINVAL on parsing error."

So this could not have been tested successfully, if at all...

So let's please slow down, think about it good, do it right and test it
properly before sending next time, ok?

Thanks.

-- 
Regards/Gruss,
    Boris.

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

Re: [PATCH v13 1/6] x86/boot: Introduce kstrtoull() to boot directory instead of simple_strtoull()

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 4179 bytes --]

Hi Chao,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on linus/master]
[also build test ERROR on v4.20-rc6 next-20181214]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Chao-Fan/x86-boot-KASLR-Parse-ACPI-table-and-limit-KASLR-to-choosing-immovable-memory/20181214-082218
config: i386-allmodconfig (attached as .config)
compiler: gcc-7 (Debian 7.3.0-1) 7.3.0
reproduce:
        # save the attached .config to linux build tree
        make ARCH=i386 

All error/warnings (new ones prefixed by >>):

   In file included from include/linux/kernel.h:207:0,
                    from arch/x86/boot/string.c:17:
>> arch/x86/include/asm/div64.h:61:21: error: redefinition of 'div_u64_rem'
    #define div_u64_rem div_u64_rem
                        ^
>> arch/x86/boot/string.c:194:19: note: in expansion of macro 'div_u64_rem'
    static inline u64 div_u64_rem(u64 dividend, u32 divisor, u32 *remainder)
                      ^~~~~~~~~~~
   arch/x86/include/asm/div64.h:43:19: note: previous definition of 'div_u64_rem' was here
    static inline u64 div_u64_rem(u64 dividend, u32 divisor, u32 *remainder)
                      ^~~~~~~~~~~
--
   In file included from include/linux/kernel.h:207:0,
                    from arch/x86/boot/compressed/../string.c:17,
                    from arch/x86/boot/compressed/string.c:11:
>> arch/x86/include/asm/div64.h:61:21: error: redefinition of 'div_u64_rem'
    #define div_u64_rem div_u64_rem
                        ^
>> arch/x86/boot/compressed/../string.c:194:19: note: in expansion of macro 'div_u64_rem'
    static inline u64 div_u64_rem(u64 dividend, u32 divisor, u32 *remainder)
                      ^~~~~~~~~~~
   arch/x86/include/asm/div64.h:43:19: note: previous definition of 'div_u64_rem' was here
    static inline u64 div_u64_rem(u64 dividend, u32 divisor, u32 *remainder)
                      ^~~~~~~~~~~

vim +/div_u64_rem +61 arch/x86/include/asm/div64.h

428c5a23 include/asm-x86/div64.h Chris Snook  2007-10-20  42  
2418f4f2 include/asm-x86/div64.h Roman Zippel 2008-05-01  43  static inline u64 div_u64_rem(u64 dividend, u32 divisor, u32 *remainder)
2418f4f2 include/asm-x86/div64.h Roman Zippel 2008-05-01  44  {
2418f4f2 include/asm-x86/div64.h Roman Zippel 2008-05-01  45  	union {
2418f4f2 include/asm-x86/div64.h Roman Zippel 2008-05-01  46  		u64 v64;
2418f4f2 include/asm-x86/div64.h Roman Zippel 2008-05-01  47  		u32 v32[2];
2418f4f2 include/asm-x86/div64.h Roman Zippel 2008-05-01  48  	} d = { dividend };
2418f4f2 include/asm-x86/div64.h Roman Zippel 2008-05-01  49  	u32 upper;
2418f4f2 include/asm-x86/div64.h Roman Zippel 2008-05-01  50  
2418f4f2 include/asm-x86/div64.h Roman Zippel 2008-05-01  51  	upper = d.v32[1];
2418f4f2 include/asm-x86/div64.h Roman Zippel 2008-05-01  52  	d.v32[1] = 0;
2418f4f2 include/asm-x86/div64.h Roman Zippel 2008-05-01  53  	if (upper >= divisor) {
2418f4f2 include/asm-x86/div64.h Roman Zippel 2008-05-01  54  		d.v32[1] = upper / divisor;
2418f4f2 include/asm-x86/div64.h Roman Zippel 2008-05-01  55  		upper %= divisor;
2418f4f2 include/asm-x86/div64.h Roman Zippel 2008-05-01  56  	}
2418f4f2 include/asm-x86/div64.h Roman Zippel 2008-05-01  57  	asm ("divl %2" : "=a" (d.v32[0]), "=d" (*remainder) :
2418f4f2 include/asm-x86/div64.h Roman Zippel 2008-05-01  58  		"rm" (divisor), "0" (d.v32[0]), "1" (upper));
2418f4f2 include/asm-x86/div64.h Roman Zippel 2008-05-01  59  	return d.v64;
2418f4f2 include/asm-x86/div64.h Roman Zippel 2008-05-01  60  }
2418f4f2 include/asm-x86/div64.h Roman Zippel 2008-05-01 @61  #define div_u64_rem	div_u64_rem
2418f4f2 include/asm-x86/div64.h Roman Zippel 2008-05-01  62  

:::::: The code at line 61 was first introduced by commit
:::::: 2418f4f28f8467b92a6177af32d05737ebf6206c introduce explicit signed/unsigned 64bit divide

:::::: TO: Roman Zippel <zippel@linux-m68k.org>
:::::: CC: Linus Torvalds <torvalds@linux-foundation.org>

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 66054 bytes --]

Re: [PATCH v13 1/6] x86/boot: Introduce kstrtoull() to boot directory instead of simple_strtoull()

[Borislav Petkov]

On Mon, Dec 17, 2018 at 03:22:39AM +0800, kbuild test robot wrote:
> Hi Chao,
> 
> Thank you for the patch! Yet something to improve:
> 
> [auto build test ERROR on linus/master]
> [also build test ERROR on v4.20-rc6 next-20181214]
> [if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
> 
> url:    https://github.com/0day-ci/linux/commits/Chao-Fan/x86-boot-KASLR-Parse-ACPI-table-and-limit-KASLR-to-choosing-immovable-memory/20181214-082218
> config: i386-allmodconfig (attached as .config)
> compiler: gcc-7 (Debian 7.3.0-1) 7.3.0
> reproduce:
>         # save the attached .config to linux build tree
>         make ARCH=i386 
> 
> All error/warnings (new ones prefixed by >>):
> 
>    In file included from include/linux/kernel.h:207:0,
>                     from arch/x86/boot/string.c:17:
> >> arch/x86/include/asm/div64.h:61:21: error: redefinition of 'div_u64_rem'

Thanks for the report. We need something like this I guess:

---
diff --git a/arch/x86/boot/string.c b/arch/x86/boot/string.c
index 771971efc0d6..4a823481e4e1 100644
--- a/arch/x86/boot/string.c
+++ b/arch/x86/boot/string.c
@@ -191,7 +191,7 @@ char *strchr(const char *s, int c)
 	return (char *)s;
 }
 
-static inline u64 div_u64_rem(u64 dividend, u32 divisor, u32 *remainder)
+static inline u64 __div_u64_rem(u64 dividend, u32 divisor, u32 *remainder)
 {
 	union {
 		u64 v64;
@@ -213,7 +213,7 @@ static inline u64 div_u64_rem(u64 dividend, u32 divisor, u32 *remainder)
 static inline u64 div_u64(u64 dividend, u32 divisor)
 {
 	u32 remainder;
-	return div_u64_rem(dividend, divisor, &remainder);
+	return __div_u64_rem(dividend, divisor, &remainder);
 }
 
 static inline char _tolower(const char c)

-- 
Regards/Gruss,
    Boris.

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

Re: [PATCH v13 1/6] x86/boot: Introduce kstrtoull() to boot directory instead of simple_strtoull()

[Chao Fan]

On Thu, Dec 13, 2018 at 02:26:03PM +0100, Borislav Petkov wrote:
>On Wed, Dec 12, 2018 at 04:03:12PM +0800, Baoquan He wrote:
>> Surely, using the old simple_strtoull() is fine too, we can take its
>> replacement into TODO list.
>
>Yes, that would be nice.

Oops, I thought you agree with what Baoquan said, so I send the new version
and use simple_strtoull() back.

Thanks,
Chao Fan

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

Re: [PATCH v13 1/6] x86/boot: Introduce kstrtoull() to boot directory instead of simple_strtoull()

[Borislav Petkov]

On Mon, Dec 17, 2018 at 09:27:28AM +0800, Chao Fan wrote:
> Oops, I thought you agree with what Baoquan said, so I send the new version
> and use simple_strtoull() back.

I could've misunderstood him. Lemme repeat what I mean:

- we should make kstrtoull() work because it is the right function to
use. Which means, include this too:

https://lkml.kernel.org/r/20181216203113.GA23823@zn.tnic

- we should gradually start replacing simple_strtoull() in
arch/x86/boot/ and eventually get rid of it. But that's for the TODO
list, to take care of later.

- Take our time and stop with the hurrying and do the whole thing nice
and clean.

Ok?

Or have I forgotten something?

If not, I'll continue looking at your v13.

Thx.

-- 
Regards/Gruss,
    Boris.

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

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

[Masayoshi Mizuma]

On Wed, Dec 12, 2018 at 11:10:50AM +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 | 79 +++++++++++++++++++++++++++++++++
>  1 file changed, 79 insertions(+)
> 
> diff --git a/arch/x86/boot/compressed/acpi.c b/arch/x86/boot/compressed/acpi.c
> index cad15686f82c..c96008712ec9 100644
> --- a/arch/x86/boot/compressed/acpi.c
> +++ b/arch/x86/boot/compressed/acpi.c
> @@ -28,3 +28,82 @@ static acpi_physical_address get_acpi_rsdp(void)
>  	return 0;
>  #endif
>  }
> +
> +/* Search EFI table for RSDP. */
> +static acpi_physical_address efi_get_rsdp_addr(void)
> +{
> +#ifdef CONFIG_EFI
> +	acpi_physical_address rsdp_addr = 0;
> +	efi_system_table_t *systab;
> +	struct efi_info *e;
> +	bool efi_64;
> +	char *sig;
> +	int size;
> +	int i;
> +
> +	e = &boot_params->efi_info;
> +	sig = (char *)&e->efi_loader_signature;
> +
> +	if (!strncmp(sig, EFI64_LOADER_SIGNATURE, 4))
> +		efi_64 = true;
> +	else if (!strncmp(sig, EFI32_LOADER_SIGNATURE, 4))
> +		efi_64 = false;
> +	else {
> +		debug_putstr("Wrong EFI loader signature.\n");
> +		return 0;
> +	}
> +
> +	/* Get systab from boot params. Based on efi_init(). */
> +#ifdef CONFIG_X86_64
> +	systab = (efi_system_table_t *)(e->efi_systab | ((__u64)e->efi_systab_hi<<32));
> +#else
> +	if (e->efi_systab_hi || e->efi_memmap_hi) {
> +		debug_putstr("Error getting RSDP address: EFI system table located above 4GB.\n");
> +		return 0;
> +	}
> +	systab = (efi_system_table_t *)e->efi_systab;
> +#endif
> +
> +	if (!systab)
> +		return 0;
> +
> +	/*
> +	 * Get EFI tables from systab. Based on efi_config_init() and
> +	 * efi_config_parse_tables().
> +	 */
> +	size = efi_64 ? sizeof(efi_config_table_64_t) :
> +			sizeof(efi_config_table_32_t);
> +
> +	for (i = 0; i < systab->nr_tables; i++) {
> +		void *config_tables;

> +		unsigned long table;

		u64 table;

Otherwise, the following build warning happen in ARCH=i386.
===
arch/x86/boot/compressed/acpi.c:96:44: warning: right shift count >= width of type [-Wshift-count-overflow]
    if (!IS_ENABLED(CONFIG_X86_64) && table >> 32) {
===

Thanks,
Masa

Re: [PATCH v13 1/6] x86/boot: Introduce kstrtoull() to boot directory instead of simple_strtoull()

[Chao Fan]

On Mon, Dec 17, 2018 at 04:45:51PM +0100, Borislav Petkov wrote:
>On Mon, Dec 17, 2018 at 09:27:28AM +0800, Chao Fan wrote:
>> Oops, I thought you agree with what Baoquan said, so I send the new version
>> and use simple_strtoull() back.
>
>I could've misunderstood him. Lemme repeat what I mean:
>
>- we should make kstrtoull() work because it is the right function to
>use. Which means, include this too:
>
>https://lkml.kernel.org/r/20181216203113.GA23823@zn.tnic
>
>- we should gradually start replacing simple_strtoull() in
>arch/x86/boot/ and eventually get rid of it. But that's for the TODO
>list, to take care of later.
>
OK, got it.
And I tested your solution(rename div_u64 as __div_u64, and div_u64_rem
as __div_u64_rem)for the issue from test rebot, that works.
Thank you very much.

Thanks,
Chao Fan
>- Take our time and stop with the hurrying and do the whole thing nice
>and clean.
>
>Ok?
>
>Or have I forgotten something?
>
>If not, I'll continue looking at your v13.
>
>Thx.
>
>-- 
>Regards/Gruss,
>    Boris.
>
>Good mailing practices for 400: avoid top-posting and trim the reply.
>
>