linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v11 0/5] x86/boot/KASLR: Parse ACPI table and limit kaslr in immovable memory
@ 2018-11-12  9:46 Chao Fan
  2018-11-12  9:46 ` [PATCH v11 1/5] x86/boot: Add efi_get_rsdp_addr() to dig out RSDP from EFI table Chao Fan
                   ` (4 more replies)
  0 siblings, 5 replies; 35+ messages in thread
From: Chao Fan @ 2018-11-12  9:46 UTC (permalink / raw)
  To: linux-kernel, x86, linux-efi, linux-acpi, bp, tglx, mingo, hpa,
	keescook, bhe, msys.mizuma
  Cc: indou.takao, caoj.fnst, fanc.fnst

***Background:
People reported that kaslr may randomly chooses 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:
There should be a method to limit kaslr to choosing immovable memory
regions, so there are 2 solutions:
1) Add a kernel parameter to specify the memory regions.
2) Get the information of memory hot-remove, then kaslr will know the
   right regions.
In method 2, information about memory hot-remove is in ACPI
tables, which will be parsed after start_kernel(), kaslr can't get
the information.
In method 1, users should know the regions address and specify in
kernel parameter.

In the earliest time, I tried to dig ACPI tabls to solve this problem.
But I didn't splite the code in 'compressed/' and ACPI code, so the patch
is hard to follow so refused by community.
Somebody suggest to add a kernel parameter to specify the
immovable memory so that limit kaslr in these regions. Then I make
a new 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/7 Add efi_get_rsdp_addr() to dig out RSDP from EFI table when
          booting from EFI.
PATCH 2/7 Add bios_get_rsdp_addr() to search RSDP in memory when EFI
          table not found.
PATCH 3/7 Add get_acpi_rsdp() to parse RSDP in cmdline from kexec
PATCH 4/7 Dig out SRAT table from RSDP and walk SRAT table to store
          the immovable memory regions.
PATCH 5/7 Calculate the intersection between memory regions from e820/efi
          memory table and immovable memory regions. Limit KASLR choose
          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 suggetsion:
 - 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 wraped function.

Any comments will be welcome.

Chao Fan (5):
  x86/boot: Add efi_get_rsdp_addr() to dig out RSDP from EFI table
  x86/boot: Add bios_get_rsdp_addr() to search RSDP in memory
  x86/boot: Add get_acpi_rsdp() to parse RSDP in cmdlien from kexec
  x86/boot: Dig out SRAT table from RSDP and find immovable memory
  x86/boot/KASLR: Walk srat tables to filter immovable memory

 arch/x86/boot/compressed/Makefile |   4 +
 arch/x86/boot/compressed/acpitb.c | 367 ++++++++++++++++++++++++++++++
 arch/x86/boot/compressed/kaslr.c  |  81 +++++--
 arch/x86/boot/compressed/misc.h   |  15 ++
 arch/x86/boot/string.h            |   4 +
 lib/kstrtox.c                     |   4 +
 6 files changed, 460 insertions(+), 15 deletions(-)
 create mode 100644 arch/x86/boot/compressed/acpitb.c

-- 
2.19.1




^ permalink raw reply	[flat|nested] 35+ messages in thread

* [PATCH v11 1/5] x86/boot: Add efi_get_rsdp_addr() to dig out RSDP from EFI table
  2018-11-12  9:46 [PATCH v11 0/5] x86/boot/KASLR: Parse ACPI table and limit kaslr in immovable memory Chao Fan
@ 2018-11-12  9:46 ` Chao Fan
  2018-11-12 14:54   ` Borislav Petkov
  2018-11-12  9:46 ` [PATCH v11 2/5] x86/boot: Add bios_get_rsdp_addr() to search RSDP in memory Chao Fan
                   ` (3 subsequent siblings)
  4 siblings, 1 reply; 35+ messages in thread
From: Chao Fan @ 2018-11-12  9:46 UTC (permalink / raw)
  To: linux-kernel, x86, linux-efi, linux-acpi, bp, tglx, mingo, hpa,
	keescook, bhe, msys.mizuma
  Cc: indou.takao, caoj.fnst, fanc.fnst

In order to parse SRAT table and get memory information, RSDP pointer
should be found. In kernel, there are three methods to get RSDP:
EFI condition, BIOS condition and KEXEC condition. The first works
for EFI condition.

Imitate ACPI code and EFI code to dig RSDP pointer from EFI tables.
Process: boot_param->systab->efi_config_table->RSDP.
Based on efi_init(), efi_config_init(), efi_config_parse_tables().

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

diff --git a/arch/x86/boot/compressed/acpitb.c b/arch/x86/boot/compressed/acpitb.c
new file mode 100644
index 000000000000..56b54b0e0889
--- /dev/null
+++ b/arch/x86/boot/compressed/acpitb.c
@@ -0,0 +1,96 @@
+// SPDX-License-Identifier: GPL-2.0
+#define BOOT_CTYPE_H
+#include "misc.h"
+#include "error.h"
+
+#include <linux/efi.h>
+#include <asm/efi.h>
+#include <linux/numa.h>
+#include <linux/acpi.h>
+
+/* Search EFI table for RSDP table. */
+static void efi_get_rsdp_addr(acpi_physical_address *rsdp_addr)
+{
+#ifdef CONFIG_EFI
+	efi_system_table_t *systab;
+	bool efi_64 = false;
+	void *config_tables;
+	struct efi_info *e;
+	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;
+	}
+
+	/* 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("Table located above 4GB. EFI should be disabled.\n");
+		return;
+	}
+	systab = (efi_system_table_t *)e->efi_systab;
+#endif
+
+	if (!systab)
+		return;
+
+	/*
+	 * Get EFI tables from systab. Based on efi_config_init() and
+	 * efi_config_parse_tables(). Only dig out the config_table.
+	 */
+	size = efi_64 ? sizeof(efi_config_table_64_t) :
+			sizeof(efi_config_table_32_t);
+
+	for (i = 0; i < systab->nr_tables; i++) {
+		efi_guid_t guid;
+		unsigned long table;
+
+		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;
+#ifndef CONFIG_64BIT
+			if (table >> 32) {
+				debug_putstr("Table located above 4G. EFI should be disabled.\n");
+				return;
+			}
+#endif
+		} 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;
+		}
+
+		/*
+		 * Get RSDP from EFI tables.
+		 * If ACPI20 table found, use it.
+		 * If ACPI20 table not found, but ACPI table found,
+		 * use the ACPI table.
+		 */
+		if (!(efi_guidcmp(guid, ACPI_TABLE_GUID))) {
+			*rsdp_addr = (acpi_physical_address)table;
+		} else if (!(efi_guidcmp(guid, ACPI_20_TABLE_GUID))) {
+			*rsdp_addr = (acpi_physical_address)table;
+			return;
+		}
+	}
+#endif
+}
-- 
2.19.1




^ permalink raw reply	[flat|nested] 35+ messages in thread

* [PATCH v11 2/5] x86/boot: Add bios_get_rsdp_addr() to search RSDP in memory
  2018-11-12  9:46 [PATCH v11 0/5] x86/boot/KASLR: Parse ACPI table and limit kaslr in immovable memory Chao Fan
  2018-11-12  9:46 ` [PATCH v11 1/5] x86/boot: Add efi_get_rsdp_addr() to dig out RSDP from EFI table Chao Fan
@ 2018-11-12  9:46 ` Chao Fan
  2018-11-12 15:27   ` Borislav Petkov
  2018-11-12  9:46 ` [PATCH v11 3/5] x86/boot: Add get_acpi_rsdp() to parse RSDP in cmdlien from kexec Chao Fan
                   ` (2 subsequent siblings)
  4 siblings, 1 reply; 35+ messages in thread
From: Chao Fan @ 2018-11-12  9:46 UTC (permalink / raw)
  To: linux-kernel, x86, linux-efi, linux-acpi, bp, tglx, mingo, hpa,
	keescook, bhe, msys.mizuma
  Cc: indou.takao, caoj.fnst, fanc.fnst

Imitate ACPI code to search RSDP pointer from memory.
Walk memory and check the signature until get the RSDP signature.
Based on acpi_tb_scan_memory_for_rsdp() and acpi_find_root_pointer().
If didn't get RSDP from EFI table, will run this function.

Used for later patch to dig out SRAT table and get the memory
information. And figure out the immovable memory regions
to avoid KASLR extracts kernel on movable memory, slove the
conflict between KASLR and movable_node feature.

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

diff --git a/arch/x86/boot/compressed/acpitb.c b/arch/x86/boot/compressed/acpitb.c
index 56b54b0e0889..50fa65cf824d 100644
--- a/arch/x86/boot/compressed/acpitb.c
+++ b/arch/x86/boot/compressed/acpitb.c
@@ -94,3 +94,109 @@ static void efi_get_rsdp_addr(acpi_physical_address *rsdp_addr)
 	}
 #endif
 }
+
+static u8 compute_checksum(u8 *buffer, u32 length)
+{
+	u8 sum = 0;
+	u8 *end = buffer + length;
+
+	while (buffer < end)
+		sum = (u8)(sum + *(buffer++));
+
+	return sum;
+}
+
+/*
+ * Used to search a block of memory for the RSDP signature.
+ * Return Pointer to the RSDP if found, otherwise NULL.
+ * Based on acpi_tb_scan_memory_for_rsdp().
+ */
+static u8 *scan_mem_for_rsdp(u8 *start, u32 length)
+{
+	struct acpi_table_rsdp *rsdp;
+	u8 *end;
+	u8 *rover;
+
+	end = start + length;
+
+	/* Search from given start address for the requested length */
+	for (rover = start; rover < end; rover += ACPI_RSDP_SCAN_STEP) {
+		/*
+		 * The RSDP signature and checksum must both 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 *)rover;
+
+		/* Nope, 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;
+
+		/* Sig and checksum valid, we have found a real RSDP */
+		return rover;
+	}
+	return NULL;
+}
+
+/*
+ * Used to search RSDP physical address.
+ * Based on acpi_find_root_pointer(). Since only use physical address
+ * in this period, so there is no need to do the memory map jobs.
+ */
+static void bios_get_rsdp_addr(acpi_physical_address *rsdp_addr)
+{
+	struct acpi_table_rsdp *rsdp;
+	u8 *table_ptr;
+	u8 *mem_rover;
+	u32 address;
+
+	/*
+	 * Get the location of the Extended BIOS Data Area (EBDA)
+	 * Since we use physical address directely, so
+	 * acpi_os_map_memory() and acpi_os_unmap_memory() are
+	 * not needed here.
+	 */
+	table_ptr = (u8 *)ACPI_EBDA_PTR_LOCATION;
+	*(u32 *)(void *)&address = *(u16 *)(void *)table_ptr;
+	address <<= 4;
+	table_ptr = (u8 *)address;
+
+	/*
+	 * Search EBDA paragraphs (EBDA is required to be a minimum of
+	 * 1K length)
+	 */
+	if (address > 0x400) {
+		mem_rover = scan_mem_for_rsdp(table_ptr, ACPI_EBDA_WINDOW_SIZE);
+
+		if (mem_rover) {
+			address += (u32)ACPI_PTR_DIFF(mem_rover, table_ptr);
+			*rsdp_addr = (acpi_physical_address)address;
+			return;
+		}
+	}
+
+	table_ptr = (u8 *)ACPI_HI_RSDP_WINDOW_BASE;
+	mem_rover = scan_mem_for_rsdp(table_ptr, ACPI_HI_RSDP_WINDOW_SIZE);
+
+	/*
+	 * Search upper memory: 16-byte boundaries in E0000h-FFFFFh
+	 * Since we use physical address directely, so
+	 * acpi_os_map_memory() and acpi_os_unmap_memory() are
+	 * not needed here.
+	 */
+	if (mem_rover) {
+		address = (u32)(ACPI_HI_RSDP_WINDOW_BASE +
+				ACPI_PTR_DIFF(mem_rover, table_ptr));
+		*rsdp_addr = (acpi_physical_address)address;
+	}
+}
-- 
2.19.1




^ permalink raw reply	[flat|nested] 35+ messages in thread

* [PATCH v11 3/5] x86/boot: Add get_acpi_rsdp() to parse RSDP in cmdlien from kexec
  2018-11-12  9:46 [PATCH v11 0/5] x86/boot/KASLR: Parse ACPI table and limit kaslr in immovable memory Chao Fan
  2018-11-12  9:46 ` [PATCH v11 1/5] x86/boot: Add efi_get_rsdp_addr() to dig out RSDP from EFI table Chao Fan
  2018-11-12  9:46 ` [PATCH v11 2/5] x86/boot: Add bios_get_rsdp_addr() to search RSDP in memory Chao Fan
@ 2018-11-12  9:46 ` Chao Fan
  2018-11-12  9:50   ` Chao Fan
                     ` (2 more replies)
  2018-11-12  9:46 ` [PATCH v11 4/5] x86/boot: Dig out SRAT table from RSDP and find immovable memory Chao Fan
  2018-11-12  9:46 ` [PATCH v11 5/5] x86/boot/KASLR: Walk srat tables to filter " Chao Fan
  4 siblings, 3 replies; 35+ messages in thread
From: Chao Fan @ 2018-11-12  9:46 UTC (permalink / raw)
  To: linux-kernel, x86, linux-efi, linux-acpi, bp, tglx, mingo, hpa,
	keescook, bhe, msys.mizuma
  Cc: indou.takao, caoj.fnst, fanc.fnst

Imitate setup_acpi_rsdp() for the early_param of 'acpi_rsdp'.
KEXEC writes the RSDP pointer to cmdline for EFI booting.
So if 'acpi_rsdp' found in cmdline, use it directely.

Since function kstrtoull() is needed, include it in
arch/x86/boot/string.h. To solve the definition conflict
problem, set BOOT_STRING tag to expose only kstrtoull() and
functions used by it. Other functions in lib/kstrtox.c will
be covered.

Signed-off-by: Chao Fan <fanc.fnst@cn.fujitsu.com>
---
 arch/x86/boot/compressed/acpitb.c | 26 ++++++++++++++++++++++++++
 arch/x86/boot/string.h            |  4 ++++
 lib/kstrtox.c                     |  4 ++++
 3 files changed, 34 insertions(+)

diff --git a/arch/x86/boot/compressed/acpitb.c b/arch/x86/boot/compressed/acpitb.c
index 50fa65cf824d..5cfb4efa5a19 100644
--- a/arch/x86/boot/compressed/acpitb.c
+++ b/arch/x86/boot/compressed/acpitb.c
@@ -8,6 +8,12 @@
 #include <linux/numa.h>
 #include <linux/acpi.h>
 
+#define STATIC
+#include <linux/decompress/mm.h>
+
+#define BOOT_STRING
+#include "../string.h"
+
 /* Search EFI table for RSDP table. */
 static void efi_get_rsdp_addr(acpi_physical_address *rsdp_addr)
 {
@@ -200,3 +206,23 @@ static void bios_get_rsdp_addr(acpi_physical_address *rsdp_addr)
 		*rsdp_addr = (acpi_physical_address)address;
 	}
 }
+
+static void get_acpi_rsdp(acpi_physical_address *rsdp_addr)
+{
+#ifdef CONFIG_KEXEC
+	unsigned long long res;
+	int len = 0;
+	char *val;
+
+	val = malloc(19);
+	len = cmdline_find_option("acpi_rsdp", val, 19);
+
+	if (len == -1)
+		return;
+
+	if (len > 0) {
+		val[len] = 0;
+		*rsdp_addr = (acpi_physical_address)kstrtoull(val, 16, &res);
+	}
+#endif
+}
diff --git a/arch/x86/boot/string.h b/arch/x86/boot/string.h
index 3d78e27077f4..0ff3edb888e4 100644
--- a/arch/x86/boot/string.h
+++ b/arch/x86/boot/string.h
@@ -30,3 +30,7 @@ extern unsigned long long simple_strtoull(const char *cp, char **endp,
 					  unsigned int base);
 
 #endif /* BOOT_STRING_H */
+
+#ifdef BOOT_STRING
+#include "../../../lib/kstrtox.c"
+#endif
diff --git a/lib/kstrtox.c b/lib/kstrtox.c
index 1006bf70bf74..3804db9eed56 100644
--- a/lib/kstrtox.c
+++ b/lib/kstrtox.c
@@ -126,6 +126,8 @@ int kstrtoull(const char *s, unsigned int base, unsigned long long *res)
 }
 EXPORT_SYMBOL(kstrtoull);
 
+#ifndef BOOT_STRING
+
 /**
  * kstrtoll - convert a string to a long long
  * @s: The start of the string. The string must be null-terminated, and may also
@@ -408,3 +410,5 @@ kstrto_from_user(kstrtou16_from_user,	kstrtou16,	u16);
 kstrto_from_user(kstrtos16_from_user,	kstrtos16,	s16);
 kstrto_from_user(kstrtou8_from_user,	kstrtou8,	u8);
 kstrto_from_user(kstrtos8_from_user,	kstrtos8,	s8);
+
+#endif
-- 
2.19.1




^ permalink raw reply	[flat|nested] 35+ messages in thread

* [PATCH v11 4/5] x86/boot: Dig out SRAT table from RSDP and find immovable memory
  2018-11-12  9:46 [PATCH v11 0/5] x86/boot/KASLR: Parse ACPI table and limit kaslr in immovable memory Chao Fan
                   ` (2 preceding siblings ...)
  2018-11-12  9:46 ` [PATCH v11 3/5] x86/boot: Add get_acpi_rsdp() to parse RSDP in cmdlien from kexec Chao Fan
@ 2018-11-12  9:46 ` Chao Fan
  2018-11-12 20:52   ` Masayoshi Mizuma
                     ` (2 more replies)
  2018-11-12  9:46 ` [PATCH v11 5/5] x86/boot/KASLR: Walk srat tables to filter " Chao Fan
  4 siblings, 3 replies; 35+ messages in thread
From: Chao Fan @ 2018-11-12  9:46 UTC (permalink / raw)
  To: linux-kernel, x86, linux-efi, linux-acpi, bp, tglx, mingo, hpa,
	keescook, bhe, msys.mizuma
  Cc: indou.takao, caoj.fnst, fanc.fnst

To avoid KASLR extracting kernel on movable memory, slove the
conflict between KASLR and movable_node feature, dig the SRAT tables
from RSDP pointer. Walk the SRAT tables and store the immovable
memory regions in immovable_mem[].

There are three methods to get RSDP pointer: KEXEC condition,
EFI confition, BIOS condition.
If KEXEC add 'acpi_rsdp' to cmdline, use it.
Otherwise, parse EFI table for RSDP.
Then, search memory for RSDP.

Imitate from ACPI code, based on acpi_os_get_root_pointer().
Process: RSDP->RSDT/XSDT->ACPI root table->SRAT.

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

diff --git a/arch/x86/boot/compressed/Makefile b/arch/x86/boot/compressed/Makefile
index 466f66c8a7f8..b51f7629b8ef 100644
--- a/arch/x86/boot/compressed/Makefile
+++ b/arch/x86/boot/compressed/Makefile
@@ -84,6 +84,10 @@ ifdef CONFIG_X86_64
 	vmlinux-objs-y += $(obj)/pgtable_64.o
 endif
 
+#if (defined CONFIG_MEMORY_HOTREMOVE) && (defined CONFIG_RANDOMIZE_BASE)
+vmlinux-objs-$(CONFIG_RANDOMIZE_BASE) += $(obj)/acpitb.o
+#endif
+
 $(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/acpitb.c b/arch/x86/boot/compressed/acpitb.c
index 5cfb4efa5a19..161f21a7fb3b 100644
--- a/arch/x86/boot/compressed/acpitb.c
+++ b/arch/x86/boot/compressed/acpitb.c
@@ -14,6 +14,11 @@
 #define BOOT_STRING
 #include "../string.h"
 
+#ifdef CONFIG_MEMORY_HOTREMOVE
+/* Store the immovable memory regions */
+struct mem_vector immovable_mem[MAX_NUMNODES*2];
+#endif
+
 /* Search EFI table for RSDP table. */
 static void efi_get_rsdp_addr(acpi_physical_address *rsdp_addr)
 {
@@ -226,3 +231,137 @@ static void get_acpi_rsdp(acpi_physical_address *rsdp_addr)
 	}
 #endif
 }
+
+/*
+ * Used to dig RSDP table from EFI table or BIOS.
+ * If RSDP table found in EFI table, use it. Or search BIOS.
+ * Based on acpi_os_get_root_pointer().
+ */
+static acpi_physical_address get_rsdp_addr(void)
+{
+	acpi_physical_address pa = 0;
+
+	get_acpi_rsdp(&pa);
+
+	if (!pa)
+		efi_get_rsdp_addr(&pa);
+
+	if (!pa)
+		bios_get_rsdp_addr(&pa);
+
+	return pa;
+}
+
+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;
+	bool acpi_use_rsdt = false;
+	char *signature;
+	char arg[10];
+	u8 *entry;
+	u32 count;
+	u32 size;
+	int i, j;
+	int ret;
+	u32 len;
+
+	rsdp = (struct acpi_table_rsdp *)get_rsdp_addr();
+	if (!rsdp)
+		return NULL;
+
+	ret = cmdline_find_option("acpi", arg, sizeof(arg));
+	if (ret == 4 && !strncmp(arg, "rsdt", 4))
+		acpi_use_rsdt = true;
+
+	/* Get RSDT or XSDT from RSDP. */
+	if (!acpi_use_rsdt &&
+	    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;
+	len = header->length;
+	count = (u32)((len - sizeof(struct acpi_table_header)) / size);
+	entry = ACPI_ADD_PTR(u8, header, sizeof(struct acpi_table_header));
+
+	for (i = 0; i < count; i++) {
+		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;
+			signature = header->signature;
+
+			if (ACPI_COMPARE_NAME(header->signature, ACPI_SIG_SRAT))
+				return header;
+		}
+		entry += size;
+	}
+	return NULL;
+}
+
+/*
+ * According to ACPI table, filter the immvoable 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;
+	int ret;
+
+	ret = cmdline_find_option("acpi", arg, sizeof(arg));
+	if (ret == 3 && !strncmp(arg, "off", 3))
+		return;
+
+	if (!cmdline_find_option_bool("movable_node"))
+		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, aborted.\n");
+				break;
+			}
+		}
+		table = (struct acpi_subtable_header *)
+			((unsigned long)table + table->length);
+	}
+	num_immovable_mem = i;
+}
diff --git a/arch/x86/boot/compressed/kaslr.c b/arch/x86/boot/compressed/kaslr.c
index 9ed9709d9947..b251572e77af 100644
--- a/arch/x86/boot/compressed/kaslr.c
+++ b/arch/x86/boot/compressed/kaslr.c
@@ -87,10 +87,6 @@ static unsigned long get_boot_seed(void)
 #define KASLR_COMPRESSED_BOOT
 #include "../../lib/kaslr.c"
 
-struct mem_vector {
-	unsigned long long start;
-	unsigned long long size;
-};
 
 /* Only supporting at most 4 unusable memmap regions with kaslr */
 #define MAX_MEMMAP_REGIONS	4
diff --git a/arch/x86/boot/compressed/misc.h b/arch/x86/boot/compressed/misc.h
index a1d5918765f3..4a3645fda0ed 100644
--- a/arch/x86/boot/compressed/misc.h
+++ b/arch/x86/boot/compressed/misc.h
@@ -77,6 +77,11 @@ void choose_random_location(unsigned long input,
 			    unsigned long *output,
 			    unsigned long output_size,
 			    unsigned long *virt_addr);
+struct mem_vector {
+	unsigned long long start;
+	unsigned long long size;
+};
+
 /* cpuflags.c */
 bool has_cpuflag(int flag);
 #else
@@ -116,3 +121,13 @@ static inline void console_init(void)
 void set_sev_encryption_mask(void);
 
 #endif
+
+/* acpitb.c */
+#ifdef CONFIG_RANDOMIZE_BASE
+int num_immovable_mem;
+#ifdef CONFIG_MEMORY_HOTREMOVE
+/* Store the amount of immovable memory regions */
+#define ACPI_MAX_TABLES                128
+void get_immovable_mem(void);
+#endif
+#endif
-- 
2.19.1




^ permalink raw reply	[flat|nested] 35+ messages in thread

* [PATCH v11 5/5] x86/boot/KASLR: Walk srat tables to filter immovable memory
  2018-11-12  9:46 [PATCH v11 0/5] x86/boot/KASLR: Parse ACPI table and limit kaslr in immovable memory Chao Fan
                   ` (3 preceding siblings ...)
  2018-11-12  9:46 ` [PATCH v11 4/5] x86/boot: Dig out SRAT table from RSDP and find immovable memory Chao Fan
@ 2018-11-12  9:46 ` Chao Fan
  2018-11-16 13:50   ` Borislav Petkov
  4 siblings, 1 reply; 35+ messages in thread
From: Chao Fan @ 2018-11-12  9:46 UTC (permalink / raw)
  To: linux-kernel, x86, linux-efi, linux-acpi, bp, tglx, mingo, hpa,
	keescook, bhe, msys.mizuma
  Cc: indou.takao, caoj.fnst, fanc.fnst

KASLR may randomly chooses 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.

The solution is limite KASLR to choose memory regions in immovable
node according to SRAT tables.

If CONFIG_MEMORY_HOTREMOVE enabled, walk through the SRAT memory
tables and store those immovable memory regions so that KASLR can get
where to choose for randomization.

If the amount of immovable memory regions is not zero, which
means the immovable memory regions existing. Calculate the intersection
between memory regions from e820/efi memory table and immovable memory
regions.

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

diff --git a/arch/x86/boot/compressed/kaslr.c b/arch/x86/boot/compressed/kaslr.c
index b251572e77af..174d2114045e 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_MEMORY_HOTREMOVE
+/* Store 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,11 @@ static void mem_avoid_init(unsigned long input, unsigned long input_size,
 	/* Mark the memmap regions we need to avoid */
 	handle_mem_options();
 
+#ifdef CONFIG_MEMORY_HOTREMOVE
+	/* Mark the immovable regions we need to choose */
+	get_immovable_mem();
+#endif
+
 #ifdef CONFIG_X86_VERBOSE_BOOTUP
 	/* Make sure video RAM can be used. */
 	add_identity_map(0, PMD_SIZE);
@@ -568,9 +578,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 slots_count(struct mem_vector *entry,
+			unsigned long minimum,
+			unsigned long image_size)
 {
 	struct mem_vector region, overlap;
 	unsigned long start_orig, end;
@@ -646,6 +656,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 directely.
+	 */
+	if (num_immovable_mem == 0) {
+		slots_count(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_MEMORY_HOTREMOVE
+	/*
+	 * If immovable memory found, filter the intersection between
+	 * immovable memory and region to slots_count.
+	 * 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;
+
+		slots_count(&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 +772,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 +800,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.1




^ permalink raw reply	[flat|nested] 35+ messages in thread

* Re: [PATCH v11 3/5] x86/boot: Add get_acpi_rsdp() to parse RSDP in cmdlien from kexec
  2018-11-12  9:46 ` [PATCH v11 3/5] x86/boot: Add get_acpi_rsdp() to parse RSDP in cmdlien from kexec Chao Fan
@ 2018-11-12  9:50   ` Chao Fan
  2018-11-12 17:43   ` Masayoshi Mizuma
  2018-11-13 17:51   ` Borislav Petkov
  2 siblings, 0 replies; 35+ messages in thread
From: Chao Fan @ 2018-11-12  9:50 UTC (permalink / raw)
  To: linux-kernel, x86, linux-efi, linux-acpi, bp, tglx, mingo, hpa,
	keescook, bhe, msys.mizuma
  Cc: indou.takao, caoj.fnst

Hi Boris,

I try to include lib/kstrtox.c in arch/x86/boot/string.c and
define the kstrtoull() function in arch/x86/boot/string.h.
But the definition problem is hard to solve, so I include
it in arch/x86/boot/string.c directely.
Then use BOOT_STRING tag to cover other functions and only
kstrtoull() is exposed.
I am not sure whether this is OK.

Thanks,
Chao Fan

On Mon, Nov 12, 2018 at 05:46:43PM +0800, Chao Fan wrote:
>Imitate setup_acpi_rsdp() for the early_param of 'acpi_rsdp'.
>KEXEC writes the RSDP pointer to cmdline for EFI booting.
>So if 'acpi_rsdp' found in cmdline, use it directely.
>
>Since function kstrtoull() is needed, include it in
>arch/x86/boot/string.h. To solve the definition conflict
>problem, set BOOT_STRING tag to expose only kstrtoull() and
>functions used by it. Other functions in lib/kstrtox.c will
>be covered.
>
>Signed-off-by: Chao Fan <fanc.fnst@cn.fujitsu.com>
>---
> arch/x86/boot/compressed/acpitb.c | 26 ++++++++++++++++++++++++++
> arch/x86/boot/string.h            |  4 ++++
> lib/kstrtox.c                     |  4 ++++
> 3 files changed, 34 insertions(+)
>
>diff --git a/arch/x86/boot/compressed/acpitb.c b/arch/x86/boot/compressed/acpitb.c
>index 50fa65cf824d..5cfb4efa5a19 100644
>--- a/arch/x86/boot/compressed/acpitb.c
>+++ b/arch/x86/boot/compressed/acpitb.c
>@@ -8,6 +8,12 @@
> #include <linux/numa.h>
> #include <linux/acpi.h>
> 
>+#define STATIC
>+#include <linux/decompress/mm.h>
>+
>+#define BOOT_STRING
>+#include "../string.h"
>+
> /* Search EFI table for RSDP table. */
> static void efi_get_rsdp_addr(acpi_physical_address *rsdp_addr)
> {
>@@ -200,3 +206,23 @@ static void bios_get_rsdp_addr(acpi_physical_address *rsdp_addr)
> 		*rsdp_addr = (acpi_physical_address)address;
> 	}
> }
>+
>+static void get_acpi_rsdp(acpi_physical_address *rsdp_addr)
>+{
>+#ifdef CONFIG_KEXEC
>+	unsigned long long res;
>+	int len = 0;
>+	char *val;
>+
>+	val = malloc(19);
>+	len = cmdline_find_option("acpi_rsdp", val, 19);
>+
>+	if (len == -1)
>+		return;
>+
>+	if (len > 0) {
>+		val[len] = 0;
>+		*rsdp_addr = (acpi_physical_address)kstrtoull(val, 16, &res);
>+	}
>+#endif
>+}
>diff --git a/arch/x86/boot/string.h b/arch/x86/boot/string.h
>index 3d78e27077f4..0ff3edb888e4 100644
>--- a/arch/x86/boot/string.h
>+++ b/arch/x86/boot/string.h
>@@ -30,3 +30,7 @@ extern unsigned long long simple_strtoull(const char *cp, char **endp,
> 					  unsigned int base);
> 
> #endif /* BOOT_STRING_H */
>+
>+#ifdef BOOT_STRING
>+#include "../../../lib/kstrtox.c"
>+#endif
>diff --git a/lib/kstrtox.c b/lib/kstrtox.c
>index 1006bf70bf74..3804db9eed56 100644
>--- a/lib/kstrtox.c
>+++ b/lib/kstrtox.c
>@@ -126,6 +126,8 @@ int kstrtoull(const char *s, unsigned int base, unsigned long long *res)
> }
> EXPORT_SYMBOL(kstrtoull);
> 
>+#ifndef BOOT_STRING
>+
> /**
>  * kstrtoll - convert a string to a long long
>  * @s: The start of the string. The string must be null-terminated, and may also
>@@ -408,3 +410,5 @@ kstrto_from_user(kstrtou16_from_user,	kstrtou16,	u16);
> kstrto_from_user(kstrtos16_from_user,	kstrtos16,	s16);
> kstrto_from_user(kstrtou8_from_user,	kstrtou8,	u8);
> kstrto_from_user(kstrtos8_from_user,	kstrtos8,	s8);
>+
>+#endif
>-- 
>2.19.1
>



^ permalink raw reply	[flat|nested] 35+ messages in thread

* Re: [PATCH v11 1/5] x86/boot: Add efi_get_rsdp_addr() to dig out RSDP from EFI table
  2018-11-12  9:46 ` [PATCH v11 1/5] x86/boot: Add efi_get_rsdp_addr() to dig out RSDP from EFI table Chao Fan
@ 2018-11-12 14:54   ` Borislav Petkov
  2018-11-13  1:57     ` Chao Fan
  0 siblings, 1 reply; 35+ messages in thread
From: Borislav Petkov @ 2018-11-12 14:54 UTC (permalink / raw)
  To: Chao Fan
  Cc: linux-kernel, x86, linux-efi, linux-acpi, tglx, mingo, hpa,
	keescook, bhe, msys.mizuma, indou.takao, caoj.fnst

On Mon, Nov 12, 2018 at 05:46:41PM +0800, Chao Fan wrote:
> In order to parse SRAT table and get memory information, RSDP pointer
> should be found. In kernel, there are three methods to get RSDP:
> EFI condition, BIOS condition and KEXEC condition. The first works
> for EFI condition.

"condition"?

Also, please explain shortly what all those abbreviations mean: think
of a person reading your commit message who doesn't have any clue from
ACPI.

> Imitate ACPI code and EFI code to dig RSDP pointer from EFI tables.
> Process: boot_param->systab->efi_config_table->RSDP.
> Based on efi_init(), efi_config_init(), efi_config_parse_tables().
> 
> Signed-off-by: Chao Fan <fanc.fnst@cn.fujitsu.com>
> ---
>  arch/x86/boot/compressed/acpitb.c | 96 +++++++++++++++++++++++++++++++
>  1 file changed, 96 insertions(+)
>  create mode 100644 arch/x86/boot/compressed/acpitb.c
> 
> diff --git a/arch/x86/boot/compressed/acpitb.c b/arch/x86/boot/compressed/acpitb.c
> new file mode 100644
> index 000000000000..56b54b0e0889
> --- /dev/null
> +++ b/arch/x86/boot/compressed/acpitb.c
> @@ -0,0 +1,96 @@
> +// SPDX-License-Identifier: GPL-2.0
> +#define BOOT_CTYPE_H
> +#include "misc.h"
> +#include "error.h"
> +
> +#include <linux/efi.h>
> +#include <asm/efi.h>
> +#include <linux/numa.h>
> +#include <linux/acpi.h>
> +
> +/* Search EFI table for RSDP table. */
> +static void efi_get_rsdp_addr(acpi_physical_address *rsdp_addr)

This is just silly: the function returns void and has a single parameter
which is an *output* parameter?!

Why isn't the signature

static acpi_physical_address *efi_get_rsdp_addr(void)

instead?

> +{
> +#ifdef CONFIG_EFI
> +	efi_system_table_t *systab;
> +	bool efi_64 = false;

You're setting it below already, why here too?

> +	void *config_tables;
> +	struct efi_info *e;
> +	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;
> +	}
> +
> +	/* 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));

No ugly line breaks with open braces trailing like that, pls - just let
it stick out.

> +#else
> +	if (e->efi_systab_hi || e->efi_memmap_hi) {
> +		debug_putstr("Table located above 4GB. EFI should be disabled.\n");

You need to say here what really happens here:

		debug_putstr("Error getting RSDP address: EFI system table located above 4GB.\n");

The same below.

> +		return;
> +	}
> +	systab = (efi_system_table_t *)e->efi_systab;
> +#endif
> +
> +	if (!systab)
> +		return;
> +
> +	/*
> +	 * Get EFI tables from systab. Based on efi_config_init() and
> +	 * efi_config_parse_tables(). Only dig out the config_table.
> +	 */
> +	size = efi_64 ? sizeof(efi_config_table_64_t) :
> +			sizeof(efi_config_table_32_t);
> +
> +	for (i = 0; i < systab->nr_tables; i++) {
> +		efi_guid_t guid;
> +		unsigned long table;

Put the void *config_tables declaration here.

> +
> +		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;
> +#ifndef CONFIG_64BIT

Above you have CONFIG_X86_64, here CONFIG_64BIT. Please use one only.

Also, use IS_ENABLED() instead.

> +			if (table >> 32) {
> +				debug_putstr("Table located above 4G. EFI should be disabled.\n");
> +				return;
> +			}
> +#endif
> +		} 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;
> +		}
> +
> +		/*
> +		 * Get RSDP from EFI tables.
> +		 * If ACPI20 table found, use it.
> +		 * If ACPI20 table not found, but ACPI table found,
> +		 * use the ACPI table.
> +		 */

That comment is the opposite of what the code does. Also, why is that
comment needed at all? If anything, it should say *why* ACPI_TABLE_GUID
is preferred and then the fallback to ACPI_20_TABLE_GUID is done - not
*what* it does. That's easily visible in the code.

Thx.

-- 
Regards/Gruss,
    Boris.

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

^ permalink raw reply	[flat|nested] 35+ messages in thread

* Re: [PATCH v11 2/5] x86/boot: Add bios_get_rsdp_addr() to search RSDP in memory
  2018-11-12  9:46 ` [PATCH v11 2/5] x86/boot: Add bios_get_rsdp_addr() to search RSDP in memory Chao Fan
@ 2018-11-12 15:27   ` Borislav Petkov
  2018-11-13  2:10     ` Chao Fan
  0 siblings, 1 reply; 35+ messages in thread
From: Borislav Petkov @ 2018-11-12 15:27 UTC (permalink / raw)
  To: Chao Fan
  Cc: linux-kernel, x86, linux-efi, linux-acpi, tglx, mingo, hpa,
	keescook, bhe, msys.mizuma, indou.takao, caoj.fnst

On Mon, Nov 12, 2018 at 05:46:42PM +0800, Chao Fan wrote:
> Imitate ACPI code to search RSDP pointer from memory.
> Walk memory and check the signature until get the RSDP signature.
> Based on acpi_tb_scan_memory_for_rsdp() and acpi_find_root_pointer().
> If didn't get RSDP from EFI table, will run this function.

That's some very strange english. Please improve.

> Used for later patch to dig out SRAT table and get the memory
> information. And figure out the immovable memory regions
> to avoid KASLR extracts kernel on movable memory, slove the
						    ^^^^^^

Please introduce a spellchecker into your patch creation workflow.

> conflict between KASLR and movable_node feature.

Btw, this paragraph could be used for a CONFIG_ item you could define
for your particular use case. Because right now you have funnies like:

+#if (defined CONFIG_MEMORY_HOTREMOVE) && (defined CONFIG_RANDOMIZE_BASE)
+vmlinux-objs-$(CONFIG_RANDOMIZE_BASE) += $(obj)/acpitb.o
+#endif

where CONFIG_RANDOMIZE_BASE is repeated for no good reason.

But we'll see - need to get to the end of your patch series first.

> Signed-off-by: Chao Fan <fanc.fnst@cn.fujitsu.com>
> ---
>  arch/x86/boot/compressed/acpitb.c | 106 ++++++++++++++++++++++++++++++
>  1 file changed, 106 insertions(+)
> 
> diff --git a/arch/x86/boot/compressed/acpitb.c b/arch/x86/boot/compressed/acpitb.c
> index 56b54b0e0889..50fa65cf824d 100644
> --- a/arch/x86/boot/compressed/acpitb.c
> +++ b/arch/x86/boot/compressed/acpitb.c
> @@ -94,3 +94,109 @@ static void efi_get_rsdp_addr(acpi_physical_address *rsdp_addr)
>  	}
>  #endif
>  }
> +
> +static u8 compute_checksum(u8 *buffer, u32 length)
> +{
> +	u8 sum = 0;
> +	u8 *end = buffer + length;
> +
> +	while (buffer < end)
> +		sum = (u8)(sum + *(buffer++));

What's that cast for?

Ah, this is the version in acpi_tb_checksum(). Well, I'd write this
simply as:

		sum += *(buffer++);

> +
> +	return sum;
> +}
> +
> +/*
> + * Used to search a block of memory for the RSDP signature.
> + * Return Pointer to the RSDP if found, otherwise NULL.

     "Returns pointer... "

> + * Based on acpi_tb_scan_memory_for_rsdp().
> + */
> +static u8 *scan_mem_for_rsdp(u8 *start, u32 length)
> +{
> +	struct acpi_table_rsdp *rsdp;
> +	u8 *end;
> +	u8 *rover;

rover?

> +
> +	end = start + length;
> +
> +	/* Search from given start address for the requested length */
> +	for (rover = start; rover < end; rover += ACPI_RSDP_SCAN_STEP) {
> +		/*
> +		 * The RSDP signature and checksum must both 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 *)rover;
> +
> +		/* Nope, 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;
> +
> +		/* Sig and checksum valid, we have found a real RSDP */
> +		return rover;
> +	}
> +	return NULL;
> +}
> +
> +/*
> + * Used to search RSDP physical address.
> + * Based on acpi_find_root_pointer(). Since only use physical address
> + * in this period, so there is no need to do the memory map jobs.

You mean: "All addresses used here are physical."?

"memory map jobs"?

Please be more careful when writing comments which are going to be read
by other people. "jobs" means a lot of things and you don't want "jobs"
in that context here.

> + */
> +static void bios_get_rsdp_addr(acpi_physical_address *rsdp_addr)

Same remark as before: the function is void and you're returning through
its parameter. Make it return acpi_physical_address instead.

> +{
> +	struct acpi_table_rsdp *rsdp;
> +	u8 *table_ptr;
> +	u8 *mem_rover;

rover?

> +	u32 address;
> +
> +	/*
> +	 * Get the location of the Extended BIOS Data Area (EBDA)
> +	 * Since we use physical address directely, so

It is "directly" - what about that spellchecker?

> +	 * acpi_os_map_memory() and acpi_os_unmap_memory() are
> +	 * not needed here.

Why do you even need to say that here?

> +	 */
> +	table_ptr = (u8 *)ACPI_EBDA_PTR_LOCATION;
> +	*(u32 *)(void *)&address = *(u16 *)(void *)table_ptr;
> +	address <<= 4;
> +	table_ptr = (u8 *)address;

arch/x86/boot/compressed/acpitb.c: In function ‘bios_get_rsdp_addr’:
arch/x86/boot/compressed/acpitb.c:172:14: warning: cast to pointer from integer of different size [-Wint-to-pointer-cast]
  table_ptr = (u8 *)address;
              ^

Also, that is some crazy casting here and I think you could use
unsigned longs here for all the address arithmetic and cast to
acpi_physical_address only at the end.

> +
> +	/*
> +	 * Search EBDA paragraphs (EBDA is required to be a minimum of
> +	 * 1K length)
> +	 */
> +	if (address > 0x400) {
> +		mem_rover = scan_mem_for_rsdp(table_ptr, ACPI_EBDA_WINDOW_SIZE);
> +

Superfluous new line.

> +		if (mem_rover) {
> +			address += (u32)ACPI_PTR_DIFF(mem_rover, table_ptr);
> +			*rsdp_addr = (acpi_physical_address)address;
> +			return;
> +		}
> +	}
> +
> +	table_ptr = (u8 *)ACPI_HI_RSDP_WINDOW_BASE;
> +	mem_rover = scan_mem_for_rsdp(table_ptr, ACPI_HI_RSDP_WINDOW_SIZE);
> +
> +	/*
> +	 * Search upper memory: 16-byte boundaries in E0000h-FFFFFh
> +	 * Since we use physical address directely, so
> +	 * acpi_os_map_memory() and acpi_os_unmap_memory() are
> +	 * not needed here.
> +	 */

And this comment needs to be repeated here because... ?

> +	if (mem_rover) {
> +		address = (u32)(ACPI_HI_RSDP_WINDOW_BASE +
> +				ACPI_PTR_DIFF(mem_rover, table_ptr));
> +		*rsdp_addr = (acpi_physical_address)address;
> +	}
> +}
> -- 

-- 
Regards/Gruss,
    Boris.

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

^ permalink raw reply	[flat|nested] 35+ messages in thread

* Re: [PATCH v11 3/5] x86/boot: Add get_acpi_rsdp() to parse RSDP in cmdlien from kexec
  2018-11-12  9:46 ` [PATCH v11 3/5] x86/boot: Add get_acpi_rsdp() to parse RSDP in cmdlien from kexec Chao Fan
  2018-11-12  9:50   ` Chao Fan
@ 2018-11-12 17:43   ` Masayoshi Mizuma
  2018-11-13  2:12     ` Chao Fan
  2018-11-13 17:51   ` Borislav Petkov
  2 siblings, 1 reply; 35+ messages in thread
From: Masayoshi Mizuma @ 2018-11-12 17:43 UTC (permalink / raw)
  To: Chao Fan
  Cc: linux-kernel, x86, linux-efi, linux-acpi, bp, tglx, mingo, hpa,
	keescook, bhe, indou.takao, caoj.fnst

Hi Chao,

On Mon, Nov 12, 2018 at 05:46:43PM +0800, Chao Fan wrote:
> Imitate setup_acpi_rsdp() for the early_param of 'acpi_rsdp'.
> KEXEC writes the RSDP pointer to cmdline for EFI booting.
> So if 'acpi_rsdp' found in cmdline, use it directely.
> 

> Since function kstrtoull() is needed, include it in
> arch/x86/boot/string.h. To solve the definition conflict
> problem, set BOOT_STRING tag to expose only kstrtoull() and
> functions used by it. Other functions in lib/kstrtox.c will
> be covered.

How about the following get_acpi_rsdp()...? It doesn't use kstrtoull().

static void get_acpi_rsdp(acpi_physical_address *rsdp_addr)
{
#ifdef CONFIG_KEXEC
        unsigned long addr;
        char val[32];

        if (cmdline_find_option("acpi_rsdp", val, sizeof(val)) > 0) {
                char *e;

                if (!strncmp(val, "0x", 2)) {
                        addr = simple_strtoull(val + 2, &e, 16);
                        if ((addr == 0) || ((val + 2) == e))
                                return;
                        *rsdp_addr = (acpi_physical_address)addr;
                }
        }
#endif
}

Thanks,
Masa

> 
> Signed-off-by: Chao Fan <fanc.fnst@cn.fujitsu.com>
> ---
>  arch/x86/boot/compressed/acpitb.c | 26 ++++++++++++++++++++++++++
>  arch/x86/boot/string.h            |  4 ++++
>  lib/kstrtox.c                     |  4 ++++
>  3 files changed, 34 insertions(+)
> 
> diff --git a/arch/x86/boot/compressed/acpitb.c b/arch/x86/boot/compressed/acpitb.c
> index 50fa65cf824d..5cfb4efa5a19 100644
> --- a/arch/x86/boot/compressed/acpitb.c
> +++ b/arch/x86/boot/compressed/acpitb.c
> @@ -8,6 +8,12 @@
>  #include <linux/numa.h>
>  #include <linux/acpi.h>
>  
> +#define STATIC
> +#include <linux/decompress/mm.h>
> +
> +#define BOOT_STRING
> +#include "../string.h"
> +
>  /* Search EFI table for RSDP table. */
>  static void efi_get_rsdp_addr(acpi_physical_address *rsdp_addr)
>  {
> @@ -200,3 +206,23 @@ static void bios_get_rsdp_addr(acpi_physical_address *rsdp_addr)
>  		*rsdp_addr = (acpi_physical_address)address;
>  	}
>  }
> +
> +static void get_acpi_rsdp(acpi_physical_address *rsdp_addr)
> +{
> +#ifdef CONFIG_KEXEC
> +	unsigned long long res;
> +	int len = 0;
> +	char *val;
> +
> +	val = malloc(19);
> +	len = cmdline_find_option("acpi_rsdp", val, 19);
> +
> +	if (len == -1)
> +		return;
> +
> +	if (len > 0) {
> +		val[len] = 0;
> +		*rsdp_addr = (acpi_physical_address)kstrtoull(val, 16, &res);
> +	}
> +#endif
> +}
> diff --git a/arch/x86/boot/string.h b/arch/x86/boot/string.h
> index 3d78e27077f4..0ff3edb888e4 100644
> --- a/arch/x86/boot/string.h
> +++ b/arch/x86/boot/string.h
> @@ -30,3 +30,7 @@ extern unsigned long long simple_strtoull(const char *cp, char **endp,
>  					  unsigned int base);
>  
>  #endif /* BOOT_STRING_H */
> +
> +#ifdef BOOT_STRING
> +#include "../../../lib/kstrtox.c"
> +#endif
> diff --git a/lib/kstrtox.c b/lib/kstrtox.c
> index 1006bf70bf74..3804db9eed56 100644
> --- a/lib/kstrtox.c
> +++ b/lib/kstrtox.c
> @@ -126,6 +126,8 @@ int kstrtoull(const char *s, unsigned int base, unsigned long long *res)
>  }
>  EXPORT_SYMBOL(kstrtoull);
>  
> +#ifndef BOOT_STRING
> +
>  /**
>   * kstrtoll - convert a string to a long long
>   * @s: The start of the string. The string must be null-terminated, and may also
> @@ -408,3 +410,5 @@ kstrto_from_user(kstrtou16_from_user,	kstrtou16,	u16);
>  kstrto_from_user(kstrtos16_from_user,	kstrtos16,	s16);
>  kstrto_from_user(kstrtou8_from_user,	kstrtou8,	u8);
>  kstrto_from_user(kstrtos8_from_user,	kstrtos8,	s8);
> +
> +#endif
> -- 
> 2.19.1
> 
> 
> 

^ permalink raw reply	[flat|nested] 35+ messages in thread

* Re: [PATCH v11 4/5] x86/boot: Dig out SRAT table from RSDP and find immovable memory
  2018-11-12  9:46 ` [PATCH v11 4/5] x86/boot: Dig out SRAT table from RSDP and find immovable memory Chao Fan
@ 2018-11-12 20:52   ` Masayoshi Mizuma
  2018-11-13  2:43     ` Chao Fan
  2018-11-12 21:51   ` Masayoshi Mizuma
  2018-11-16 11:16   ` Borislav Petkov
  2 siblings, 1 reply; 35+ messages in thread
From: Masayoshi Mizuma @ 2018-11-12 20:52 UTC (permalink / raw)
  To: Chao Fan
  Cc: linux-kernel, x86, linux-efi, linux-acpi, bp, tglx, mingo, hpa,
	keescook, bhe, indou.takao, caoj.fnst

On Mon, Nov 12, 2018 at 05:46:44PM +0800, Chao Fan wrote:
> To avoid KASLR extracting kernel on movable memory, slove the
> conflict between KASLR and movable_node feature, dig the SRAT tables
> from RSDP pointer. Walk the SRAT tables and store the immovable
> memory regions in immovable_mem[].
> 
> There are three methods to get RSDP pointer: KEXEC condition,
> EFI confition, BIOS condition.
> If KEXEC add 'acpi_rsdp' to cmdline, use it.
> Otherwise, parse EFI table for RSDP.
> Then, search memory for RSDP.
> 
> Imitate from ACPI code, based on acpi_os_get_root_pointer().
> Process: RSDP->RSDT/XSDT->ACPI root table->SRAT.
> 
> Signed-off-by: Chao Fan <fanc.fnst@cn.fujitsu.com>
> ---
>  arch/x86/boot/compressed/Makefile |   4 +
>  arch/x86/boot/compressed/acpitb.c | 139 ++++++++++++++++++++++++++++++
>  arch/x86/boot/compressed/kaslr.c  |   4 -
>  arch/x86/boot/compressed/misc.h   |  15 ++++
>  4 files changed, 158 insertions(+), 4 deletions(-)
> 
> diff --git a/arch/x86/boot/compressed/Makefile b/arch/x86/boot/compressed/Makefile
> index 466f66c8a7f8..b51f7629b8ef 100644
> --- a/arch/x86/boot/compressed/Makefile
> +++ b/arch/x86/boot/compressed/Makefile
> @@ -84,6 +84,10 @@ ifdef CONFIG_X86_64
>  	vmlinux-objs-y += $(obj)/pgtable_64.o
>  endif
>  
> +#if (defined CONFIG_MEMORY_HOTREMOVE) && (defined CONFIG_RANDOMIZE_BASE)
> +vmlinux-objs-$(CONFIG_RANDOMIZE_BASE) += $(obj)/acpitb.o
> +#endif
> +
>  $(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/acpitb.c b/arch/x86/boot/compressed/acpitb.c
> index 5cfb4efa5a19..161f21a7fb3b 100644
> --- a/arch/x86/boot/compressed/acpitb.c
> +++ b/arch/x86/boot/compressed/acpitb.c
> @@ -14,6 +14,11 @@
>  #define BOOT_STRING
>  #include "../string.h"
>  
> +#ifdef CONFIG_MEMORY_HOTREMOVE
> +/* Store the immovable memory regions */
> +struct mem_vector immovable_mem[MAX_NUMNODES*2];
> +#endif
> +
>  /* Search EFI table for RSDP table. */
>  static void efi_get_rsdp_addr(acpi_physical_address *rsdp_addr)
>  {
> @@ -226,3 +231,137 @@ static void get_acpi_rsdp(acpi_physical_address *rsdp_addr)
>  	}
>  #endif
>  }
> +
> +/*
> + * Used to dig RSDP table from EFI table or BIOS.
> + * If RSDP table found in EFI table, use it. Or search BIOS.
> + * Based on acpi_os_get_root_pointer().
> + */
> +static acpi_physical_address get_rsdp_addr(void)
> +{
> +	acpi_physical_address pa = 0;
> +
> +	get_acpi_rsdp(&pa);
> +
> +	if (!pa)
> +		efi_get_rsdp_addr(&pa);
> +
> +	if (!pa)
> +		bios_get_rsdp_addr(&pa);
> +
> +	return pa;
> +}
> +
> +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;
> +	bool acpi_use_rsdt = false;
> +	char *signature;
> +	char arg[10];
> +	u8 *entry;
> +	u32 count;
> +	u32 size;
> +	int i, j;
> +	int ret;
> +	u32 len;
> +
> +	rsdp = (struct acpi_table_rsdp *)get_rsdp_addr();
> +	if (!rsdp)
> +		return NULL;

> +
> +	ret = cmdline_find_option("acpi", arg, sizeof(arg));
> +	if (ret == 4 && !strncmp(arg, "rsdt", 4))
> +		acpi_use_rsdt = true;

All have to do for ret is checking whether it's above 0, right?
So how about the following?

	if ((cmdline_find_option("acpi", arg, sizeof(arg)) > 0) &&
		!strncmp(arg, "rsdt", 4))
		acpi_use_rsdt = true;

> +
> +	/* Get RSDT or XSDT from RSDP. */
> +	if (!acpi_use_rsdt &&
> +	    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;
> +	len = header->length;
> +	count = (u32)((len - sizeof(struct acpi_table_header)) / size);
> +	entry = ACPI_ADD_PTR(u8, header, sizeof(struct acpi_table_header));
> +
> +	for (i = 0; i < count; i++) {
> +		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;

> +			signature = header->signature;

this isn't needed.

> +
> +			if (ACPI_COMPARE_NAME(header->signature, ACPI_SIG_SRAT))
> +				return header;
> +		}
> +		entry += size;
> +	}
> +	return NULL;
> +}
> +
> +/*
> + * According to ACPI table, filter the immvoable 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;
> +	int ret;
> +

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

Same as above.

	if ((cmdline_find_option("acpi", arg, sizeof(arg)) > 0) &&
		!strncmp(arg, "off", 3))
		return;
> +
> +	if (!cmdline_find_option_bool("movable_node"))
> +		return;

I think this check isn't needed.
The SRAT parsing is needed to the kaslr issue which I'm trying
to fix. The issue may happen even if movable_node isn't set.

> +
> +	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, aborted.\n");

I'm not sure this statement gets true actually,
but if so, should it be set 0 to fallback in process_mem_region()?

Thanks,
Masa

> +				break;
> +			}
> +		}
> +		table = (struct acpi_subtable_header *)
> +			((unsigned long)table + table->length);
> +	}
> +	num_immovable_mem = i;
> +}
> diff --git a/arch/x86/boot/compressed/kaslr.c b/arch/x86/boot/compressed/kaslr.c
> index 9ed9709d9947..b251572e77af 100644
> --- a/arch/x86/boot/compressed/kaslr.c
> +++ b/arch/x86/boot/compressed/kaslr.c
> @@ -87,10 +87,6 @@ static unsigned long get_boot_seed(void)
>  #define KASLR_COMPRESSED_BOOT
>  #include "../../lib/kaslr.c"
>  
> -struct mem_vector {
> -	unsigned long long start;
> -	unsigned long long size;
> -};
>  
>  /* Only supporting at most 4 unusable memmap regions with kaslr */
>  #define MAX_MEMMAP_REGIONS	4
> diff --git a/arch/x86/boot/compressed/misc.h b/arch/x86/boot/compressed/misc.h
> index a1d5918765f3..4a3645fda0ed 100644
> --- a/arch/x86/boot/compressed/misc.h
> +++ b/arch/x86/boot/compressed/misc.h
> @@ -77,6 +77,11 @@ void choose_random_location(unsigned long input,
>  			    unsigned long *output,
>  			    unsigned long output_size,
>  			    unsigned long *virt_addr);
> +struct mem_vector {
> +	unsigned long long start;
> +	unsigned long long size;
> +};
> +
>  /* cpuflags.c */
>  bool has_cpuflag(int flag);
>  #else
> @@ -116,3 +121,13 @@ static inline void console_init(void)
>  void set_sev_encryption_mask(void);
>  
>  #endif
> +
> +/* acpitb.c */
> +#ifdef CONFIG_RANDOMIZE_BASE
> +int num_immovable_mem;
> +#ifdef CONFIG_MEMORY_HOTREMOVE
> +/* Store the amount of immovable memory regions */
> +#define ACPI_MAX_TABLES                128
> +void get_immovable_mem(void);
> +#endif
> +#endif
> -- 
> 2.19.1
> 
> 
> 

^ permalink raw reply	[flat|nested] 35+ messages in thread

* Re: [PATCH v11 4/5] x86/boot: Dig out SRAT table from RSDP and find immovable memory
  2018-11-12  9:46 ` [PATCH v11 4/5] x86/boot: Dig out SRAT table from RSDP and find immovable memory Chao Fan
  2018-11-12 20:52   ` Masayoshi Mizuma
@ 2018-11-12 21:51   ` Masayoshi Mizuma
  2018-11-13  2:45     ` Chao Fan
  2018-11-16 11:16   ` Borislav Petkov
  2 siblings, 1 reply; 35+ messages in thread
From: Masayoshi Mizuma @ 2018-11-12 21:51 UTC (permalink / raw)
  To: Chao Fan
  Cc: linux-kernel, x86, linux-efi, linux-acpi, bp, tglx, mingo, hpa,
	keescook, bhe, indou.takao, caoj.fnst

On Mon, Nov 12, 2018 at 05:46:44PM +0800, Chao Fan wrote:
> To avoid KASLR extracting kernel on movable memory, slove the
> conflict between KASLR and movable_node feature, dig the SRAT tables
> from RSDP pointer. Walk the SRAT tables and store the immovable
> memory regions in immovable_mem[].
> 
> There are three methods to get RSDP pointer: KEXEC condition,
> EFI confition, BIOS condition.
> If KEXEC add 'acpi_rsdp' to cmdline, use it.
> Otherwise, parse EFI table for RSDP.
> Then, search memory for RSDP.
> 
> Imitate from ACPI code, based on acpi_os_get_root_pointer().
> Process: RSDP->RSDT/XSDT->ACPI root table->SRAT.
> 
> Signed-off-by: Chao Fan <fanc.fnst@cn.fujitsu.com>
> ---
>  arch/x86/boot/compressed/Makefile |   4 +
>  arch/x86/boot/compressed/acpitb.c | 139 ++++++++++++++++++++++++++++++
>  arch/x86/boot/compressed/kaslr.c  |   4 -
>  arch/x86/boot/compressed/misc.h   |  15 ++++
>  4 files changed, 158 insertions(+), 4 deletions(-)
> 
> diff --git a/arch/x86/boot/compressed/Makefile b/arch/x86/boot/compressed/Makefile
> index 466f66c8a7f8..b51f7629b8ef 100644
> --- a/arch/x86/boot/compressed/Makefile
> +++ b/arch/x86/boot/compressed/Makefile
> @@ -84,6 +84,10 @@ ifdef CONFIG_X86_64
>  	vmlinux-objs-y += $(obj)/pgtable_64.o
>  endif
>  
> +#if (defined CONFIG_MEMORY_HOTREMOVE) && (defined CONFIG_RANDOMIZE_BASE)
> +vmlinux-objs-$(CONFIG_RANDOMIZE_BASE) += $(obj)/acpitb.o
> +#endif

'#' in Makefile means comment out...

ifdef CONFIG_MEMORY_HOTREMOVE
        vmlinux-objs-$(CONFIG_RANDOMIZE_BASE) += $(obj)/acpitb.o
endif

> +
>  $(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/acpitb.c b/arch/x86/boot/compressed/acpitb.c
> index 5cfb4efa5a19..161f21a7fb3b 100644
> --- a/arch/x86/boot/compressed/acpitb.c
> +++ b/arch/x86/boot/compressed/acpitb.c
> @@ -14,6 +14,11 @@
>  #define BOOT_STRING
>  #include "../string.h"
>  

> +#ifdef CONFIG_MEMORY_HOTREMOVE
> +/* Store the immovable memory regions */
> +struct mem_vector immovable_mem[MAX_NUMNODES*2];
> +#endif

This #ifdef isn't needed.

Could you please check the other #ifdef statement as well?

Thanks,
Masa

> +
>  /* Search EFI table for RSDP table. */
>  static void efi_get_rsdp_addr(acpi_physical_address *rsdp_addr)
>  {
> @@ -226,3 +231,137 @@ static void get_acpi_rsdp(acpi_physical_address *rsdp_addr)
>  	}
>  #endif
>  }
> +
> +/*
> + * Used to dig RSDP table from EFI table or BIOS.
> + * If RSDP table found in EFI table, use it. Or search BIOS.
> + * Based on acpi_os_get_root_pointer().
> + */
> +static acpi_physical_address get_rsdp_addr(void)
> +{
> +	acpi_physical_address pa = 0;
> +
> +	get_acpi_rsdp(&pa);
> +
> +	if (!pa)
> +		efi_get_rsdp_addr(&pa);
> +
> +	if (!pa)
> +		bios_get_rsdp_addr(&pa);
> +
> +	return pa;
> +}
> +
> +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;
> +	bool acpi_use_rsdt = false;
> +	char *signature;
> +	char arg[10];
> +	u8 *entry;
> +	u32 count;
> +	u32 size;
> +	int i, j;
> +	int ret;
> +	u32 len;
> +
> +	rsdp = (struct acpi_table_rsdp *)get_rsdp_addr();
> +	if (!rsdp)
> +		return NULL;
> +
> +	ret = cmdline_find_option("acpi", arg, sizeof(arg));
> +	if (ret == 4 && !strncmp(arg, "rsdt", 4))
> +		acpi_use_rsdt = true;
> +
> +	/* Get RSDT or XSDT from RSDP. */
> +	if (!acpi_use_rsdt &&
> +	    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;
> +	len = header->length;
> +	count = (u32)((len - sizeof(struct acpi_table_header)) / size);
> +	entry = ACPI_ADD_PTR(u8, header, sizeof(struct acpi_table_header));
> +
> +	for (i = 0; i < count; i++) {
> +		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;
> +			signature = header->signature;
> +
> +			if (ACPI_COMPARE_NAME(header->signature, ACPI_SIG_SRAT))
> +				return header;
> +		}
> +		entry += size;
> +	}
> +	return NULL;
> +}
> +
> +/*
> + * According to ACPI table, filter the immvoable 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;
> +	int ret;
> +
> +	ret = cmdline_find_option("acpi", arg, sizeof(arg));
> +	if (ret == 3 && !strncmp(arg, "off", 3))
> +		return;
> +
> +	if (!cmdline_find_option_bool("movable_node"))
> +		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, aborted.\n");
> +				break;
> +			}
> +		}
> +		table = (struct acpi_subtable_header *)
> +			((unsigned long)table + table->length);
> +	}
> +	num_immovable_mem = i;
> +}
> diff --git a/arch/x86/boot/compressed/kaslr.c b/arch/x86/boot/compressed/kaslr.c
> index 9ed9709d9947..b251572e77af 100644
> --- a/arch/x86/boot/compressed/kaslr.c
> +++ b/arch/x86/boot/compressed/kaslr.c
> @@ -87,10 +87,6 @@ static unsigned long get_boot_seed(void)
>  #define KASLR_COMPRESSED_BOOT
>  #include "../../lib/kaslr.c"
>  
> -struct mem_vector {
> -	unsigned long long start;
> -	unsigned long long size;
> -};
>  
>  /* Only supporting at most 4 unusable memmap regions with kaslr */
>  #define MAX_MEMMAP_REGIONS	4
> diff --git a/arch/x86/boot/compressed/misc.h b/arch/x86/boot/compressed/misc.h
> index a1d5918765f3..4a3645fda0ed 100644
> --- a/arch/x86/boot/compressed/misc.h
> +++ b/arch/x86/boot/compressed/misc.h
> @@ -77,6 +77,11 @@ void choose_random_location(unsigned long input,
>  			    unsigned long *output,
>  			    unsigned long output_size,
>  			    unsigned long *virt_addr);
> +struct mem_vector {
> +	unsigned long long start;
> +	unsigned long long size;
> +};
> +
>  /* cpuflags.c */
>  bool has_cpuflag(int flag);
>  #else
> @@ -116,3 +121,13 @@ static inline void console_init(void)
>  void set_sev_encryption_mask(void);
>  
>  #endif
> +
> +/* acpitb.c */
> +#ifdef CONFIG_RANDOMIZE_BASE
> +int num_immovable_mem;
> +#ifdef CONFIG_MEMORY_HOTREMOVE
> +/* Store the amount of immovable memory regions */
> +#define ACPI_MAX_TABLES                128
> +void get_immovable_mem(void);
> +#endif
> +#endif
> -- 
> 2.19.1
> 
> 
> 

^ permalink raw reply	[flat|nested] 35+ messages in thread

* Re: [PATCH v11 1/5] x86/boot: Add efi_get_rsdp_addr() to dig out RSDP from EFI table
  2018-11-12 14:54   ` Borislav Petkov
@ 2018-11-13  1:57     ` Chao Fan
  0 siblings, 0 replies; 35+ messages in thread
From: Chao Fan @ 2018-11-13  1:57 UTC (permalink / raw)
  To: Borislav Petkov
  Cc: linux-kernel, x86, linux-efi, linux-acpi, tglx, mingo, hpa,
	keescook, bhe, msys.mizuma, indou.takao, caoj.fnst

On Mon, Nov 12, 2018 at 03:54:33PM +0100, Borislav Petkov wrote:
>On Mon, Nov 12, 2018 at 05:46:41PM +0800, Chao Fan wrote:
>> In order to parse SRAT table and get memory information, RSDP pointer
>> should be found. In kernel, there are three methods to get RSDP:
>> EFI condition, BIOS condition and KEXEC condition. The first works
>> for EFI condition.
>
>"condition"?
>
>Also, please explain shortly what all those abbreviations mean: think
>of a person reading your commit message who doesn't have any clue from
>ACPI.

I will try to improve in next version.
>
>> Imitate ACPI code and EFI code to dig RSDP pointer from EFI tables.
>> Process: boot_param->systab->efi_config_table->RSDP.
>> Based on efi_init(), efi_config_init(), efi_config_parse_tables().
>> 
>> Signed-off-by: Chao Fan <fanc.fnst@cn.fujitsu.com>
>> ---
>>  arch/x86/boot/compressed/acpitb.c | 96 +++++++++++++++++++++++++++++++
>>  1 file changed, 96 insertions(+)
>>  create mode 100644 arch/x86/boot/compressed/acpitb.c
>> 
>> diff --git a/arch/x86/boot/compressed/acpitb.c b/arch/x86/boot/compressed/acpitb.c
>> new file mode 100644
>> index 000000000000..56b54b0e0889
>> --- /dev/null
>> +++ b/arch/x86/boot/compressed/acpitb.c
>> @@ -0,0 +1,96 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +#define BOOT_CTYPE_H
>> +#include "misc.h"
>> +#include "error.h"
>> +
>> +#include <linux/efi.h>
>> +#include <asm/efi.h>
>> +#include <linux/numa.h>
>> +#include <linux/acpi.h>
>> +
>> +/* Search EFI table for RSDP table. */
>> +static void efi_get_rsdp_addr(acpi_physical_address *rsdp_addr)
>
>This is just silly: the function returns void and has a single parameter
>which is an *output* parameter?!
>
>Why isn't the signature
>
>static acpi_physical_address *efi_get_rsdp_addr(void)
>
>instead?

I will change the function style.

>
>> +{
>> +#ifdef CONFIG_EFI
>> +	efi_system_table_t *systab;
>> +	bool efi_64 = false;
>
>You're setting it below already, why here too?

I will drop.

>
>> +	void *config_tables;
>> +	struct efi_info *e;
>> +	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;
>> +	}
>> +
>> +	/* 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));
>
>No ugly line breaks with open braces trailing like that, pls - just let
>it stick out.

Thanks for this suggestion.

>
>> +#else
>> +	if (e->efi_systab_hi || e->efi_memmap_hi) {
>> +		debug_putstr("Table located above 4GB. EFI should be disabled.\n");
>
>You need to say here what really happens here:
>
>		debug_putstr("Error getting RSDP address: EFI system table located above 4GB.\n");
>
>The same below.

I will change all the all the same message.
>
>> +		return;
>> +	}
>> +	systab = (efi_system_table_t *)e->efi_systab;
>> +#endif
>> +
>> +	if (!systab)
>> +		return;
>> +
>> +	/*
>> +	 * Get EFI tables from systab. Based on efi_config_init() and
>> +	 * efi_config_parse_tables(). Only dig out the config_table.
>> +	 */
>> +	size = efi_64 ? sizeof(efi_config_table_64_t) :
>> +			sizeof(efi_config_table_32_t);
>> +
>> +	for (i = 0; i < systab->nr_tables; i++) {
>> +		efi_guid_t guid;
>> +		unsigned long table;
>
>Put the void *config_tables declaration here.

OK.

>
>> +
>> +		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;
>> +#ifndef CONFIG_64BIT
>
>Above you have CONFIG_X86_64, here CONFIG_64BIT. Please use one only.
>
>Also, use IS_ENABLED() instead.

OK.

>
>> +			if (table >> 32) {
>> +				debug_putstr("Table located above 4G. EFI should be disabled.\n");
>> +				return;
>> +			}
>> +#endif
>> +		} 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;
>> +		}
>> +
>> +		/*
>> +		 * Get RSDP from EFI tables.
>> +		 * If ACPI20 table found, use it.
>> +		 * If ACPI20 table not found, but ACPI table found,
>> +		 * use the ACPI table.
>> +		 */
>
>That comment is the opposite of what the code does. Also, why is that
>comment needed at all? If anything, it should say *why* ACPI_TABLE_GUID
>is preferred and then the fallback to ACPI_20_TABLE_GUID is done - not
>*what* it does. That's easily visible in the code.

I will drop the comment.

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



^ permalink raw reply	[flat|nested] 35+ messages in thread

* Re: [PATCH v11 2/5] x86/boot: Add bios_get_rsdp_addr() to search RSDP in memory
  2018-11-12 15:27   ` Borislav Petkov
@ 2018-11-13  2:10     ` Chao Fan
  2018-11-13 10:09       ` Borislav Petkov
  0 siblings, 1 reply; 35+ messages in thread
From: Chao Fan @ 2018-11-13  2:10 UTC (permalink / raw)
  To: Borislav Petkov
  Cc: linux-kernel, x86, linux-efi, linux-acpi, tglx, mingo, hpa,
	keescook, bhe, msys.mizuma, indou.takao, caoj.fnst

On Mon, Nov 12, 2018 at 04:27:44PM +0100, Borislav Petkov wrote:
>On Mon, Nov 12, 2018 at 05:46:42PM +0800, Chao Fan wrote:
>> Imitate ACPI code to search RSDP pointer from memory.
>> Walk memory and check the signature until get the RSDP signature.
>> Based on acpi_tb_scan_memory_for_rsdp() and acpi_find_root_pointer().
>> If didn't get RSDP from EFI table, will run this function.
>
>That's some very strange english. Please improve.
>
>> Used for later patch to dig out SRAT table and get the memory
>> information. And figure out the immovable memory regions
>> to avoid KASLR extracts kernel on movable memory, slove the
>						    ^^^^^^
>
>Please introduce a spellchecker into your patch creation workflow.
>

Thanks.

>> conflict between KASLR and movable_node feature.
>
>Btw, this paragraph could be used for a CONFIG_ item you could define
>for your particular use case. Because right now you have funnies like:
>
>+#if (defined CONFIG_MEMORY_HOTREMOVE) && (defined CONFIG_RANDOMIZE_BASE)
>+vmlinux-objs-$(CONFIG_RANDOMIZE_BASE) += $(obj)/acpitb.o
>+#endif
>
>where CONFIG_RANDOMIZE_BASE is repeated for no good reason.
>
>But we'll see - need to get to the end of your patch series first.
>
>> Signed-off-by: Chao Fan <fanc.fnst@cn.fujitsu.com>
>> ---
>>  arch/x86/boot/compressed/acpitb.c | 106 ++++++++++++++++++++++++++++++
>>  1 file changed, 106 insertions(+)
>> 
>> diff --git a/arch/x86/boot/compressed/acpitb.c b/arch/x86/boot/compressed/acpitb.c
>> index 56b54b0e0889..50fa65cf824d 100644
>> --- a/arch/x86/boot/compressed/acpitb.c
>> +++ b/arch/x86/boot/compressed/acpitb.c
>> @@ -94,3 +94,109 @@ static void efi_get_rsdp_addr(acpi_physical_address *rsdp_addr)
>>  	}
>>  #endif
>>  }
>> +
>> +static u8 compute_checksum(u8 *buffer, u32 length)
>> +{
>> +	u8 sum = 0;
>> +	u8 *end = buffer + length;
>> +
>> +	while (buffer < end)
>> +		sum = (u8)(sum + *(buffer++));
>
>What's that cast for?
>
>Ah, this is the version in acpi_tb_checksum(). Well, I'd write this
>simply as:
>
>		sum += *(buffer++);

Thanks for your suggestion.

>
>> +
>> +	return sum;
>> +}
>> +
>> +/*
>> + * Used to search a block of memory for the RSDP signature.
>> + * Return Pointer to the RSDP if found, otherwise NULL.
>
>     "Returns pointer... "
>
>> + * Based on acpi_tb_scan_memory_for_rsdp().
>> + */
>> +static u8 *scan_mem_for_rsdp(u8 *start, u32 length)
>> +{
>> +	struct acpi_table_rsdp *rsdp;
>> +	u8 *end;
>> +	u8 *rover;
>
>rover?
>
>> +
>> +	end = start + length;
>> +
>> +	/* Search from given start address for the requested length */
>> +	for (rover = start; rover < end; rover += ACPI_RSDP_SCAN_STEP) {

The 'rover' was named as 'mem_rover', but the length of this line is too
long. So shorten it as 'rever' so that they can keep in one line.

>> +		/*
>> +		 * The RSDP signature and checksum must both 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 *)rover;
>> +
>> +		/* Nope, 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;
>> +
>> +		/* Sig and checksum valid, we have found a real RSDP */
>> +		return rover;
>> +	}
>> +	return NULL;
>> +}
>> +
>> +/*
>> + * Used to search RSDP physical address.
>> + * Based on acpi_find_root_pointer(). Since only use physical address
>> + * in this period, so there is no need to do the memory map jobs.
>
>You mean: "All addresses used here are physical."?
>
>"memory map jobs"?
>
>Please be more careful when writing comments which are going to be read
>by other people. "jobs" means a lot of things and you don't want "jobs"
>in that context here.

OK.

>
>> + */
>> +static void bios_get_rsdp_addr(acpi_physical_address *rsdp_addr)
>
>Same remark as before: the function is void and you're returning through
>its parameter. Make it return acpi_physical_address instead.
>

I will change all these functions.

>> +{
>> +	struct acpi_table_rsdp *rsdp;
>> +	u8 *table_ptr;
>> +	u8 *mem_rover;
>
>rover?

This name came from ACPI driver code, acpi_find_root_pointer().
Used for the loop. If you have a better name, please tell me.

>
>> +	u32 address;
>> +
>> +	/*
>> +	 * Get the location of the Extended BIOS Data Area (EBDA)
>> +	 * Since we use physical address directely, so
>
>It is "directly" - what about that spellchecker?
>
>> +	 * acpi_os_map_memory() and acpi_os_unmap_memory() are
>> +	 * not needed here.
>
>Why do you even need to say that here?

I will try to improve all the comment.
>
>> +	 */
>> +	table_ptr = (u8 *)ACPI_EBDA_PTR_LOCATION;
>> +	*(u32 *)(void *)&address = *(u16 *)(void *)table_ptr;
>> +	address <<= 4;
>> +	table_ptr = (u8 *)address;
>
>arch/x86/boot/compressed/acpitb.c: In function ‘bios_get_rsdp_addr’:
>arch/x86/boot/compressed/acpitb.c:172:14: warning: cast to pointer from integer of different size [-Wint-to-pointer-cast]
>  table_ptr = (u8 *)address;
>              ^
>
>Also, that is some crazy casting here and I think you could use
>unsigned longs here for all the address arithmetic and cast to
>acpi_physical_address only at the end.

That's a good suggestion.

>
>> +
>> +	/*
>> +	 * Search EBDA paragraphs (EBDA is required to be a minimum of
>> +	 * 1K length)
>> +	 */
>> +	if (address > 0x400) {
>> +		mem_rover = scan_mem_for_rsdp(table_ptr, ACPI_EBDA_WINDOW_SIZE);
>> +
>
>Superfluous new line.
>
>> +		if (mem_rover) {
>> +			address += (u32)ACPI_PTR_DIFF(mem_rover, table_ptr);
>> +			*rsdp_addr = (acpi_physical_address)address;
>> +			return;
>> +		}
>> +	}
>> +
>> +	table_ptr = (u8 *)ACPI_HI_RSDP_WINDOW_BASE;
>> +	mem_rover = scan_mem_for_rsdp(table_ptr, ACPI_HI_RSDP_WINDOW_SIZE);
>> +
>> +	/*
>> +	 * Search upper memory: 16-byte boundaries in E0000h-FFFFFh
>> +	 * Since we use physical address directely, so
>> +	 * acpi_os_map_memory() and acpi_os_unmap_memory() are
>> +	 * not needed here.
>> +	 */
>
>And this comment needs to be repeated here because... ?
I will try to improve all the comment.

Thanks,
Chao Fan

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



^ permalink raw reply	[flat|nested] 35+ messages in thread

* Re: [PATCH v11 3/5] x86/boot: Add get_acpi_rsdp() to parse RSDP in cmdlien from kexec
  2018-11-12 17:43   ` Masayoshi Mizuma
@ 2018-11-13  2:12     ` Chao Fan
  2018-11-13 16:11       ` Masayoshi Mizuma
  0 siblings, 1 reply; 35+ messages in thread
From: Chao Fan @ 2018-11-13  2:12 UTC (permalink / raw)
  To: Masayoshi Mizuma
  Cc: linux-kernel, x86, linux-efi, linux-acpi, bp, tglx, mingo, hpa,
	keescook, bhe, indou.takao, caoj.fnst

On Mon, Nov 12, 2018 at 12:43:44PM -0500, Masayoshi Mizuma wrote:
>Hi Chao,
>
>On Mon, Nov 12, 2018 at 05:46:43PM +0800, Chao Fan wrote:
>> Imitate setup_acpi_rsdp() for the early_param of 'acpi_rsdp'.
>> KEXEC writes the RSDP pointer to cmdline for EFI booting.
>> So if 'acpi_rsdp' found in cmdline, use it directely.
>> 
>
>> Since function kstrtoull() is needed, include it in
>> arch/x86/boot/string.h. To solve the definition conflict
>> problem, set BOOT_STRING tag to expose only kstrtoull() and
>> functions used by it. Other functions in lib/kstrtox.c will
>> be covered.
>
>How about the following get_acpi_rsdp()...? It doesn't use kstrtoull().
>
>static void get_acpi_rsdp(acpi_physical_address *rsdp_addr)
>{
>#ifdef CONFIG_KEXEC
>        unsigned long addr;
>        char val[32];
>
>        if (cmdline_find_option("acpi_rsdp", val, sizeof(val)) > 0) {
>                char *e;
>
>                if (!strncmp(val, "0x", 2)) {
>                        addr = simple_strtoull(val + 2, &e, 16);
>                        if ((addr == 0) || ((val + 2) == e))
>                                return;
>                        *rsdp_addr = (acpi_physical_address)addr;
>                }
>        }
>#endif
>}

Thanks for the suggestion.
I used this function. In the old version, Boris said simple_strtoull()
is the old function and told me use the new kstrtoull().

Thanks,
Chao Fan

>
>Thanks,
>Masa
>
>> 
>> Signed-off-by: Chao Fan <fanc.fnst@cn.fujitsu.com>
>> ---
>>  arch/x86/boot/compressed/acpitb.c | 26 ++++++++++++++++++++++++++
>>  arch/x86/boot/string.h            |  4 ++++
>>  lib/kstrtox.c                     |  4 ++++
>>  3 files changed, 34 insertions(+)
>> 
>> diff --git a/arch/x86/boot/compressed/acpitb.c b/arch/x86/boot/compressed/acpitb.c
>> index 50fa65cf824d..5cfb4efa5a19 100644
>> --- a/arch/x86/boot/compressed/acpitb.c
>> +++ b/arch/x86/boot/compressed/acpitb.c
>> @@ -8,6 +8,12 @@
>>  #include <linux/numa.h>
>>  #include <linux/acpi.h>
>>  
>> +#define STATIC
>> +#include <linux/decompress/mm.h>
>> +
>> +#define BOOT_STRING
>> +#include "../string.h"
>> +
>>  /* Search EFI table for RSDP table. */
>>  static void efi_get_rsdp_addr(acpi_physical_address *rsdp_addr)
>>  {
>> @@ -200,3 +206,23 @@ static void bios_get_rsdp_addr(acpi_physical_address *rsdp_addr)
>>  		*rsdp_addr = (acpi_physical_address)address;
>>  	}
>>  }
>> +
>> +static void get_acpi_rsdp(acpi_physical_address *rsdp_addr)
>> +{
>> +#ifdef CONFIG_KEXEC
>> +	unsigned long long res;
>> +	int len = 0;
>> +	char *val;
>> +
>> +	val = malloc(19);
>> +	len = cmdline_find_option("acpi_rsdp", val, 19);
>> +
>> +	if (len == -1)
>> +		return;
>> +
>> +	if (len > 0) {
>> +		val[len] = 0;
>> +		*rsdp_addr = (acpi_physical_address)kstrtoull(val, 16, &res);
>> +	}
>> +#endif
>> +}
>> diff --git a/arch/x86/boot/string.h b/arch/x86/boot/string.h
>> index 3d78e27077f4..0ff3edb888e4 100644
>> --- a/arch/x86/boot/string.h
>> +++ b/arch/x86/boot/string.h
>> @@ -30,3 +30,7 @@ extern unsigned long long simple_strtoull(const char *cp, char **endp,
>>  					  unsigned int base);
>>  
>>  #endif /* BOOT_STRING_H */
>> +
>> +#ifdef BOOT_STRING
>> +#include "../../../lib/kstrtox.c"
>> +#endif
>> diff --git a/lib/kstrtox.c b/lib/kstrtox.c
>> index 1006bf70bf74..3804db9eed56 100644
>> --- a/lib/kstrtox.c
>> +++ b/lib/kstrtox.c
>> @@ -126,6 +126,8 @@ int kstrtoull(const char *s, unsigned int base, unsigned long long *res)
>>  }
>>  EXPORT_SYMBOL(kstrtoull);
>>  
>> +#ifndef BOOT_STRING
>> +
>>  /**
>>   * kstrtoll - convert a string to a long long
>>   * @s: The start of the string. The string must be null-terminated, and may also
>> @@ -408,3 +410,5 @@ kstrto_from_user(kstrtou16_from_user,	kstrtou16,	u16);
>>  kstrto_from_user(kstrtos16_from_user,	kstrtos16,	s16);
>>  kstrto_from_user(kstrtou8_from_user,	kstrtou8,	u8);
>>  kstrto_from_user(kstrtos8_from_user,	kstrtos8,	s8);
>> +
>> +#endif
>> -- 
>> 2.19.1
>> 
>> 
>> 
>
>



^ permalink raw reply	[flat|nested] 35+ messages in thread

* Re: [PATCH v11 4/5] x86/boot: Dig out SRAT table from RSDP and find immovable memory
  2018-11-12 20:52   ` Masayoshi Mizuma
@ 2018-11-13  2:43     ` Chao Fan
  0 siblings, 0 replies; 35+ messages in thread
From: Chao Fan @ 2018-11-13  2:43 UTC (permalink / raw)
  To: Masayoshi Mizuma
  Cc: linux-kernel, x86, linux-efi, linux-acpi, bp, tglx, mingo, hpa,
	keescook, bhe, indou.takao, caoj.fnst

On Mon, Nov 12, 2018 at 03:52:54PM -0500, Masayoshi Mizuma wrote:
[...]
>
>> +
>> +	ret = cmdline_find_option("acpi", arg, sizeof(arg));
>> +	if (ret == 4 && !strncmp(arg, "rsdt", 4))
>> +		acpi_use_rsdt = true;
>
>All have to do for ret is checking whether it's above 0, right?
>So how about the following?
>
>	if ((cmdline_find_option("acpi", arg, sizeof(arg)) > 0) &&
>		!strncmp(arg, "rsdt", 4))
>		acpi_use_rsdt = true;

Maybe:
>	if ((cmdline_find_option("acpi", arg, sizeof(arg)) == 4) &&
>		!strncmp(arg, "rsdt", 4))
>		acpi_use_rsdt = true;
looks better. Thanks for your suggestion.

>
>> +
>> +	/* Get RSDT or XSDT from RSDP. */
>> +	if (!acpi_use_rsdt &&
>> +	    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;
>> +	len = header->length;
>> +	count = (u32)((len - sizeof(struct acpi_table_header)) / size);
>> +	entry = ACPI_ADD_PTR(u8, header, sizeof(struct acpi_table_header));
>> +
>> +	for (i = 0; i < count; i++) {
>> +		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;
>
>> +			signature = header->signature;
>
>this isn't needed.

Yes, will change it.

>
>> +
>> +			if (ACPI_COMPARE_NAME(header->signature, ACPI_SIG_SRAT))
>> +				return header;
>> +		}
>> +		entry += size;
>> +	}
>> +	return NULL;
>> +}
>> +
>> +/*
>> + * According to ACPI table, filter the immvoable 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;
>> +	int ret;
>> +
>
>> +	ret = cmdline_find_option("acpi", arg, sizeof(arg));
>> +	if (ret == 3 && !strncmp(arg, "off", 3))
>> +		return;
>
>Same as above.
>
>	if ((cmdline_find_option("acpi", arg, sizeof(arg)) > 0) &&
>		!strncmp(arg, "off", 3))
>		return;
>> +
>> +	if (!cmdline_find_option_bool("movable_node"))
>> +		return;
>
>I think this check isn't needed.
>The SRAT parsing is needed to the kaslr issue which I'm trying
>to fix. The issue may happen even if movable_node isn't set.

Got it.

>
>> +
>> +	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, aborted.\n");
>
>I'm not sure this statement gets true actually,
>but if so, should it be set 0 to fallback in process_mem_region()?

That's a good problem.
Well, I don't know what will cause "i >= MAX_NUMNODES*2", maybe some
error in SRAT table or failed to parse table.
Anyway, it's a good idea to set it 0.

Thanks,
Chao Fan

>
>Thanks,
>Masa
>
>> +				break;
>> +			}
>> +		}
>> +		table = (struct acpi_subtable_header *)
>> +			((unsigned long)table + table->length);
>> +	}
>> +	num_immovable_mem = i;
>> +}
>> diff --git a/arch/x86/boot/compressed/kaslr.c b/arch/x86/boot/compressed/kaslr.c
>> index 9ed9709d9947..b251572e77af 100644
>> --- a/arch/x86/boot/compressed/kaslr.c
>> +++ b/arch/x86/boot/compressed/kaslr.c
>> @@ -87,10 +87,6 @@ static unsigned long get_boot_seed(void)
>>  #define KASLR_COMPRESSED_BOOT
>>  #include "../../lib/kaslr.c"
>>  
>> -struct mem_vector {
>> -	unsigned long long start;
>> -	unsigned long long size;
>> -};
>>  
>>  /* Only supporting at most 4 unusable memmap regions with kaslr */
>>  #define MAX_MEMMAP_REGIONS	4
>> diff --git a/arch/x86/boot/compressed/misc.h b/arch/x86/boot/compressed/misc.h
>> index a1d5918765f3..4a3645fda0ed 100644
>> --- a/arch/x86/boot/compressed/misc.h
>> +++ b/arch/x86/boot/compressed/misc.h
>> @@ -77,6 +77,11 @@ void choose_random_location(unsigned long input,
>>  			    unsigned long *output,
>>  			    unsigned long output_size,
>>  			    unsigned long *virt_addr);
>> +struct mem_vector {
>> +	unsigned long long start;
>> +	unsigned long long size;
>> +};
>> +
>>  /* cpuflags.c */
>>  bool has_cpuflag(int flag);
>>  #else
>> @@ -116,3 +121,13 @@ static inline void console_init(void)
>>  void set_sev_encryption_mask(void);
>>  
>>  #endif
>> +
>> +/* acpitb.c */
>> +#ifdef CONFIG_RANDOMIZE_BASE
>> +int num_immovable_mem;
>> +#ifdef CONFIG_MEMORY_HOTREMOVE
>> +/* Store the amount of immovable memory regions */
>> +#define ACPI_MAX_TABLES                128
>> +void get_immovable_mem(void);
>> +#endif
>> +#endif
>> -- 
>> 2.19.1
>> 
>> 
>> 
>
>



^ permalink raw reply	[flat|nested] 35+ messages in thread

* Re: [PATCH v11 4/5] x86/boot: Dig out SRAT table from RSDP and find immovable memory
  2018-11-12 21:51   ` Masayoshi Mizuma
@ 2018-11-13  2:45     ` Chao Fan
  0 siblings, 0 replies; 35+ messages in thread
From: Chao Fan @ 2018-11-13  2:45 UTC (permalink / raw)
  To: Masayoshi Mizuma
  Cc: linux-kernel, x86, linux-efi, linux-acpi, bp, tglx, mingo, hpa,
	keescook, bhe, indou.takao, caoj.fnst

On Mon, Nov 12, 2018 at 04:51:59PM -0500, Masayoshi Mizuma wrote:
>On Mon, Nov 12, 2018 at 05:46:44PM +0800, Chao Fan wrote:
>> To avoid KASLR extracting kernel on movable memory, slove the
>> conflict between KASLR and movable_node feature, dig the SRAT tables
>> from RSDP pointer. Walk the SRAT tables and store the immovable
>> memory regions in immovable_mem[].
>> 
>> There are three methods to get RSDP pointer: KEXEC condition,
>> EFI confition, BIOS condition.
>> If KEXEC add 'acpi_rsdp' to cmdline, use it.
>> Otherwise, parse EFI table for RSDP.
>> Then, search memory for RSDP.
>> 
>> Imitate from ACPI code, based on acpi_os_get_root_pointer().
>> Process: RSDP->RSDT/XSDT->ACPI root table->SRAT.
>> 
>> Signed-off-by: Chao Fan <fanc.fnst@cn.fujitsu.com>
>> ---
>>  arch/x86/boot/compressed/Makefile |   4 +
>>  arch/x86/boot/compressed/acpitb.c | 139 ++++++++++++++++++++++++++++++
>>  arch/x86/boot/compressed/kaslr.c  |   4 -
>>  arch/x86/boot/compressed/misc.h   |  15 ++++
>>  4 files changed, 158 insertions(+), 4 deletions(-)
>> 
>> diff --git a/arch/x86/boot/compressed/Makefile b/arch/x86/boot/compressed/Makefile
>> index 466f66c8a7f8..b51f7629b8ef 100644
>> --- a/arch/x86/boot/compressed/Makefile
>> +++ b/arch/x86/boot/compressed/Makefile
>> @@ -84,6 +84,10 @@ ifdef CONFIG_X86_64
>>  	vmlinux-objs-y += $(obj)/pgtable_64.o
>>  endif
>>  
>> +#if (defined CONFIG_MEMORY_HOTREMOVE) && (defined CONFIG_RANDOMIZE_BASE)
>> +vmlinux-objs-$(CONFIG_RANDOMIZE_BASE) += $(obj)/acpitb.o
>> +#endif
>
>'#' in Makefile means comment out...
>
>ifdef CONFIG_MEMORY_HOTREMOVE
>        vmlinux-objs-$(CONFIG_RANDOMIZE_BASE) += $(obj)/acpitb.o
>endif
>

Thanks,

>> +
>>  $(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/acpitb.c b/arch/x86/boot/compressed/acpitb.c
>> index 5cfb4efa5a19..161f21a7fb3b 100644
>> --- a/arch/x86/boot/compressed/acpitb.c
>> +++ b/arch/x86/boot/compressed/acpitb.c
>> @@ -14,6 +14,11 @@
>>  #define BOOT_STRING
>>  #include "../string.h"
>>  
>
>> +#ifdef CONFIG_MEMORY_HOTREMOVE
>> +/* Store the immovable memory regions */
>> +struct mem_vector immovable_mem[MAX_NUMNODES*2];
>> +#endif
>
>This #ifdef isn't needed.
>
>Could you please check the other #ifdef statement as well?

Sure, I will check all the #ifdef.

Thanks,
Chao Fan

>
>Thanks,
>Masa
>
>> +
>>  /* Search EFI table for RSDP table. */
>>  static void efi_get_rsdp_addr(acpi_physical_address *rsdp_addr)
>>  {
>> @@ -226,3 +231,137 @@ static void get_acpi_rsdp(acpi_physical_address *rsdp_addr)
>>  	}
>>  #endif
>>  }
>> +
>> +/*
>> + * Used to dig RSDP table from EFI table or BIOS.
>> + * If RSDP table found in EFI table, use it. Or search BIOS.
>> + * Based on acpi_os_get_root_pointer().
>> + */
>> +static acpi_physical_address get_rsdp_addr(void)
>> +{
>> +	acpi_physical_address pa = 0;
>> +
>> +	get_acpi_rsdp(&pa);
>> +
>> +	if (!pa)
>> +		efi_get_rsdp_addr(&pa);
>> +
>> +	if (!pa)
>> +		bios_get_rsdp_addr(&pa);
>> +
>> +	return pa;
>> +}
>> +
>> +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;
>> +	bool acpi_use_rsdt = false;
>> +	char *signature;
>> +	char arg[10];
>> +	u8 *entry;
>> +	u32 count;
>> +	u32 size;
>> +	int i, j;
>> +	int ret;
>> +	u32 len;
>> +
>> +	rsdp = (struct acpi_table_rsdp *)get_rsdp_addr();
>> +	if (!rsdp)
>> +		return NULL;
>> +
>> +	ret = cmdline_find_option("acpi", arg, sizeof(arg));
>> +	if (ret == 4 && !strncmp(arg, "rsdt", 4))
>> +		acpi_use_rsdt = true;
>> +
>> +	/* Get RSDT or XSDT from RSDP. */
>> +	if (!acpi_use_rsdt &&
>> +	    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;
>> +	len = header->length;
>> +	count = (u32)((len - sizeof(struct acpi_table_header)) / size);
>> +	entry = ACPI_ADD_PTR(u8, header, sizeof(struct acpi_table_header));
>> +
>> +	for (i = 0; i < count; i++) {
>> +		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;
>> +			signature = header->signature;
>> +
>> +			if (ACPI_COMPARE_NAME(header->signature, ACPI_SIG_SRAT))
>> +				return header;
>> +		}
>> +		entry += size;
>> +	}
>> +	return NULL;
>> +}
>> +
>> +/*
>> + * According to ACPI table, filter the immvoable 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;
>> +	int ret;
>> +
>> +	ret = cmdline_find_option("acpi", arg, sizeof(arg));
>> +	if (ret == 3 && !strncmp(arg, "off", 3))
>> +		return;
>> +
>> +	if (!cmdline_find_option_bool("movable_node"))
>> +		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, aborted.\n");
>> +				break;
>> +			}
>> +		}
>> +		table = (struct acpi_subtable_header *)
>> +			((unsigned long)table + table->length);
>> +	}
>> +	num_immovable_mem = i;
>> +}
>> diff --git a/arch/x86/boot/compressed/kaslr.c b/arch/x86/boot/compressed/kaslr.c
>> index 9ed9709d9947..b251572e77af 100644
>> --- a/arch/x86/boot/compressed/kaslr.c
>> +++ b/arch/x86/boot/compressed/kaslr.c
>> @@ -87,10 +87,6 @@ static unsigned long get_boot_seed(void)
>>  #define KASLR_COMPRESSED_BOOT
>>  #include "../../lib/kaslr.c"
>>  
>> -struct mem_vector {
>> -	unsigned long long start;
>> -	unsigned long long size;
>> -};
>>  
>>  /* Only supporting at most 4 unusable memmap regions with kaslr */
>>  #define MAX_MEMMAP_REGIONS	4
>> diff --git a/arch/x86/boot/compressed/misc.h b/arch/x86/boot/compressed/misc.h
>> index a1d5918765f3..4a3645fda0ed 100644
>> --- a/arch/x86/boot/compressed/misc.h
>> +++ b/arch/x86/boot/compressed/misc.h
>> @@ -77,6 +77,11 @@ void choose_random_location(unsigned long input,
>>  			    unsigned long *output,
>>  			    unsigned long output_size,
>>  			    unsigned long *virt_addr);
>> +struct mem_vector {
>> +	unsigned long long start;
>> +	unsigned long long size;
>> +};
>> +
>>  /* cpuflags.c */
>>  bool has_cpuflag(int flag);
>>  #else
>> @@ -116,3 +121,13 @@ static inline void console_init(void)
>>  void set_sev_encryption_mask(void);
>>  
>>  #endif
>> +
>> +/* acpitb.c */
>> +#ifdef CONFIG_RANDOMIZE_BASE
>> +int num_immovable_mem;
>> +#ifdef CONFIG_MEMORY_HOTREMOVE
>> +/* Store the amount of immovable memory regions */
>> +#define ACPI_MAX_TABLES                128
>> +void get_immovable_mem(void);
>> +#endif
>> +#endif
>> -- 
>> 2.19.1
>> 
>> 
>> 
>
>



^ permalink raw reply	[flat|nested] 35+ messages in thread

* Re: [PATCH v11 2/5] x86/boot: Add bios_get_rsdp_addr() to search RSDP in memory
  2018-11-13  2:10     ` Chao Fan
@ 2018-11-13 10:09       ` Borislav Petkov
  0 siblings, 0 replies; 35+ messages in thread
From: Borislav Petkov @ 2018-11-13 10:09 UTC (permalink / raw)
  To: Chao Fan
  Cc: linux-kernel, x86, linux-efi, linux-acpi, tglx, mingo, hpa,
	keescook, bhe, msys.mizuma, indou.takao, caoj.fnst

On Tue, Nov 13, 2018 at 10:10:16AM +0800, Chao Fan wrote:
> The 'rover' was named as 'mem_rover', but the length of this line is too
> long. So shorten it as 'rever' so that they can keep in one line.

I still have no clue what "rover" or "rever" means...

> This name came from ACPI driver code, acpi_find_root_pointer().
> Used for the loop. If you have a better name, please tell me.

I would if I knew what it meant. If you don't know either, then name it
to something descriptive so that it is clear what it is when reading the
code.

Thx.

-- 
Regards/Gruss,
    Boris.

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

^ permalink raw reply	[flat|nested] 35+ messages in thread

* Re: [PATCH v11 3/5] x86/boot: Add get_acpi_rsdp() to parse RSDP in cmdlien from kexec
  2018-11-13  2:12     ` Chao Fan
@ 2018-11-13 16:11       ` Masayoshi Mizuma
  2018-11-13 17:22         ` Borislav Petkov
  2018-11-13 17:54         ` Borislav Petkov
  0 siblings, 2 replies; 35+ messages in thread
From: Masayoshi Mizuma @ 2018-11-13 16:11 UTC (permalink / raw)
  To: Chao Fan, bp
  Cc: linux-kernel, x86, linux-efi, linux-acpi, tglx, mingo, hpa,
	keescook, bhe, indou.takao, caoj.fnst

Hi Chao and Boris,

On Tue, Nov 13, 2018 at 10:12:18AM +0800, Chao Fan wrote:
> On Mon, Nov 12, 2018 at 12:43:44PM -0500, Masayoshi Mizuma wrote:
> >How about the following get_acpi_rsdp()...? It doesn't use kstrtoull().
> >
> >static void get_acpi_rsdp(acpi_physical_address *rsdp_addr)
> >{
> >#ifdef CONFIG_KEXEC
> >        unsigned long addr;
> >        char val[32];
> >
> >        if (cmdline_find_option("acpi_rsdp", val, sizeof(val)) > 0) {
> >                char *e;
> >
> >                if (!strncmp(val, "0x", 2)) {
> >                        addr = simple_strtoull(val + 2, &e, 16);
> >                        if ((addr == 0) || ((val + 2) == e))
> >                                return;
> >                        *rsdp_addr = (acpi_physical_address)addr;
> >                }
> >        }
> >#endif
> >}
> 
> Thanks for the suggestion.
> I used this function. In the old version, Boris said simple_strtoull()
> is the old function and told me use the new kstrtoull().

I think it's not very good idea to use kstrtoull() in
arch/x86/boot/compressed/* because some tricks are needed to
use the function, looks like Chao is trying...
It is the simple way here to use simple_strtoull() defined
in arch/x86/boot/boot.h, I think.

I know checkpatch.pl says an warning about simple_strtoull(),
however, I believe the warning is for simple_strtoull() defined
in lib/vsprintf.c.

Thanks,
Masa

^ permalink raw reply	[flat|nested] 35+ messages in thread

* Re: [PATCH v11 3/5] x86/boot: Add get_acpi_rsdp() to parse RSDP in cmdlien from kexec
  2018-11-13 16:11       ` Masayoshi Mizuma
@ 2018-11-13 17:22         ` Borislav Petkov
  2018-11-13 17:54         ` Borislav Petkov
  1 sibling, 0 replies; 35+ messages in thread
From: Borislav Petkov @ 2018-11-13 17:22 UTC (permalink / raw)
  To: Masayoshi Mizuma
  Cc: Chao Fan, linux-kernel, x86, linux-efi, linux-acpi, tglx, mingo,
	hpa, keescook, bhe, indou.takao, caoj.fnst

On Tue, Nov 13, 2018 at 11:11:11AM -0500, Masayoshi Mizuma wrote:
> I know checkpatch.pl says an warning about simple_strtoull(),
> however, I believe the warning is for simple_strtoull() defined
> in lib/vsprintf.c.

simple_strtoull is deprecated for various reasons. I'll take a look at
Chao's patch soon.

-- 
Regards/Gruss,
    Boris.

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

^ permalink raw reply	[flat|nested] 35+ messages in thread

* Re: [PATCH v11 3/5] x86/boot: Add get_acpi_rsdp() to parse RSDP in cmdlien from kexec
  2018-11-12  9:46 ` [PATCH v11 3/5] x86/boot: Add get_acpi_rsdp() to parse RSDP in cmdlien from kexec Chao Fan
  2018-11-12  9:50   ` Chao Fan
  2018-11-12 17:43   ` Masayoshi Mizuma
@ 2018-11-13 17:51   ` Borislav Petkov
  2018-11-14  1:54     ` Chao Fan
  2 siblings, 1 reply; 35+ messages in thread
From: Borislav Petkov @ 2018-11-13 17:51 UTC (permalink / raw)
  To: Chao Fan
  Cc: linux-kernel, x86, linux-efi, linux-acpi, tglx, mingo, hpa,
	keescook, bhe, msys.mizuma, indou.takao, caoj.fnst

On Mon, Nov 12, 2018 at 05:46:43PM +0800, Chao Fan wrote:
> Imitate setup_acpi_rsdp() for the early_param of 'acpi_rsdp'.
> KEXEC writes the RSDP pointer to cmdline for EFI booting.
> So if 'acpi_rsdp' found in cmdline, use it directely.
> 
> Since function kstrtoull() is needed, include it in
> arch/x86/boot/string.h. To solve the definition conflict
> problem, set BOOT_STRING tag to expose only kstrtoull() and
> functions used by it. Other functions in lib/kstrtox.c will
> be covered.
> 
> Signed-off-by: Chao Fan <fanc.fnst@cn.fujitsu.com>
> ---
>  arch/x86/boot/compressed/acpitb.c | 26 ++++++++++++++++++++++++++
>  arch/x86/boot/string.h            |  4 ++++
>  lib/kstrtox.c                     |  4 ++++
>  3 files changed, 34 insertions(+)
> 
> diff --git a/arch/x86/boot/compressed/acpitb.c b/arch/x86/boot/compressed/acpitb.c
> index 50fa65cf824d..5cfb4efa5a19 100644
> --- a/arch/x86/boot/compressed/acpitb.c
> +++ b/arch/x86/boot/compressed/acpitb.c
> @@ -8,6 +8,12 @@
>  #include <linux/numa.h>
>  #include <linux/acpi.h>
>  
> +#define STATIC
> +#include <linux/decompress/mm.h>
> +
> +#define BOOT_STRING
> +#include "../string.h"
> +
>  /* Search EFI table for RSDP table. */
>  static void efi_get_rsdp_addr(acpi_physical_address *rsdp_addr)
>  {
> @@ -200,3 +206,23 @@ static void bios_get_rsdp_addr(acpi_physical_address *rsdp_addr)
>  		*rsdp_addr = (acpi_physical_address)address;
>  	}
>  }
> +
> +static void get_acpi_rsdp(acpi_physical_address *rsdp_addr)
> +{
> +#ifdef CONFIG_KEXEC

Ok, why is that CONFIG_KEXEC dependency needed now too?

Ok, let's recap: so far, for your use case you need:

CONFIG_MEMORY_HOTREMOVE
CONFIG_RANDOMIZE_BASE
and now
CONFIG_KEXEC

So, you can clean up all that ifdeffery by defining a new config item
CONFIG_EARLY_PARSE_RSDP or so which depends on all those three items and
then you can do

vmlinux-objs-$(CONFIG_EARLY_PARSE_RSDP) += $(obj)/acpitb.o

and get rid of the most of the ifdeffery.

Yes?

> +	unsigned long long res;
> +	int len = 0;
> +	char *val;
> +
> +	val = malloc(19);
> +	len = cmdline_find_option("acpi_rsdp", val, 19);
> +

^ Superfluous newline.

> +	if (len == -1)
> +		return;

That check is not needed since you do > 0 below.

> +
> +	if (len > 0) {
> +		val[len] = 0;
> +		*rsdp_addr = (acpi_physical_address)kstrtoull(val, 16, &res);
> +	}
> +#endif
> +}
> diff --git a/arch/x86/boot/string.h b/arch/x86/boot/string.h
> index 3d78e27077f4..0ff3edb888e4 100644
> --- a/arch/x86/boot/string.h
> +++ b/arch/x86/boot/string.h
> @@ -30,3 +30,7 @@ extern unsigned long long simple_strtoull(const char *cp, char **endp,
>  					  unsigned int base);
>  
>  #endif /* BOOT_STRING_H */
> +
> +#ifdef BOOT_STRING
> +#include "../../../lib/kstrtox.c"
> +#endif
> diff --git a/lib/kstrtox.c b/lib/kstrtox.c
> index 1006bf70bf74..3804db9eed56 100644
> --- a/lib/kstrtox.c
> +++ b/lib/kstrtox.c
> @@ -126,6 +126,8 @@ int kstrtoull(const char *s, unsigned int base, unsigned long long *res)
>  }
>  EXPORT_SYMBOL(kstrtoull);

This needs a comment to explain what is that guard used for.

> +#ifndef BOOT_STRING
> +
>  /**
>   * kstrtoll - convert a string to a long long
>   * @s: The start of the string. The string must be null-terminated, and may also
> @@ -408,3 +410,5 @@ kstrto_from_user(kstrtou16_from_user,	kstrtou16,	u16);
>  kstrto_from_user(kstrtos16_from_user,	kstrtos16,	s16);
>  kstrto_from_user(kstrtou8_from_user,	kstrtou8,	u8);
>  kstrto_from_user(kstrtos8_from_user,	kstrtos8,	s8);
> +
> +#endif

#endif /* BOOT_STRING */

-- 
Regards/Gruss,
    Boris.

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

^ permalink raw reply	[flat|nested] 35+ messages in thread

* Re: [PATCH v11 3/5] x86/boot: Add get_acpi_rsdp() to parse RSDP in cmdlien from kexec
  2018-11-13 16:11       ` Masayoshi Mizuma
  2018-11-13 17:22         ` Borislav Petkov
@ 2018-11-13 17:54         ` Borislav Petkov
  2018-11-13 20:06           ` Masayoshi Mizuma
  1 sibling, 1 reply; 35+ messages in thread
From: Borislav Petkov @ 2018-11-13 17:54 UTC (permalink / raw)
  To: Masayoshi Mizuma
  Cc: Chao Fan, linux-kernel, x86, linux-efi, linux-acpi, tglx, mingo,
	hpa, keescook, bhe, indou.takao, caoj.fnst

On Tue, Nov 13, 2018 at 11:11:11AM -0500, Masayoshi Mizuma wrote:
> I think it's not very good idea to use kstrtoull() in
> arch/x86/boot/compressed/* because some tricks are needed to
> use the function, looks like Chao is trying...

Ok, I had a look at the patch. And frankly, I don't see anything wrong
with the aspect of using kstrtoull() in the compressed stage too and
getting rid of simple_strtoull().

So what are your reservations?

-- 
Regards/Gruss,
    Boris.

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

^ permalink raw reply	[flat|nested] 35+ messages in thread

* Re: [PATCH v11 3/5] x86/boot: Add get_acpi_rsdp() to parse RSDP in cmdlien from kexec
  2018-11-13 17:54         ` Borislav Petkov
@ 2018-11-13 20:06           ` Masayoshi Mizuma
  2018-11-13 21:51             ` Borislav Petkov
  0 siblings, 1 reply; 35+ messages in thread
From: Masayoshi Mizuma @ 2018-11-13 20:06 UTC (permalink / raw)
  To: Borislav Petkov
  Cc: Chao Fan, linux-kernel, x86, linux-efi, linux-acpi, tglx, mingo,
	hpa, keescook, bhe, indou.takao, caoj.fnst

On Tue, Nov 13, 2018 at 06:54:13PM +0100, Borislav Petkov wrote:
> On Tue, Nov 13, 2018 at 11:11:11AM -0500, Masayoshi Mizuma wrote:
> > I think it's not very good idea to use kstrtoull() in
> > arch/x86/boot/compressed/* because some tricks are needed to
> > use the function, looks like Chao is trying...
> 
> Ok, I had a look at the patch. And frankly, I don't see anything wrong
> with the aspect of using kstrtoull() in the compressed stage too and
> getting rid of simple_strtoull().
> 
> So what are your reservations?

Thank you for your checking.
I just felt the BOOT_STRING thing in lib/kstrtox.c confuses...
I'm OK for now if it's applied your below comment.

> > @@ -126,6 +126,8 @@ int kstrtoull(const char *s, unsigned int base, unsigned long long *res)
> >  }
> >  EXPORT_SYMBOL(kstrtoull);
> 
> This needs a comment to explain what is that guard used for.

Thanks,
Masa

^ permalink raw reply	[flat|nested] 35+ messages in thread

* Re: [PATCH v11 3/5] x86/boot: Add get_acpi_rsdp() to parse RSDP in cmdlien from kexec
  2018-11-13 20:06           ` Masayoshi Mizuma
@ 2018-11-13 21:51             ` Borislav Petkov
  2018-11-14  6:12               ` Chao Fan
  0 siblings, 1 reply; 35+ messages in thread
From: Borislav Petkov @ 2018-11-13 21:51 UTC (permalink / raw)
  To: Masayoshi Mizuma, Chao Fan
  Cc: linux-kernel, x86, linux-efi, linux-acpi, tglx, mingo, hpa,
	keescook, bhe, indou.takao, caoj.fnst

On Tue, Nov 13, 2018 at 03:06:16PM -0500, Masayoshi Mizuma wrote:
> I just felt the BOOT_STRING thing in lib/kstrtox.c confuses...
> I'm OK for now if it's applied your below comment.

Well, actually, upon a second look, I don't think that including a .c
file into a header is ok:

diff --git a/arch/x86/boot/string.h b/arch/x86/boot/string.h
index 3d78e27077f4..0ff3edb888e4 100644
--- a/arch/x86/boot/string.h
+++ b/arch/x86/boot/string.h
@@ -30,3 +30,7 @@ extern unsigned long long simple_strtoull(const char *cp, char **endp,
                                          unsigned int base);

 #endif /* BOOT_STRING_H */
+
+#ifdef BOOT_STRING
+#include "../../../lib/kstrtox.c"
+#endif

Chao, why isn't this part of arch/x86/boot/compressed/misc.c ?

-- 
Regards/Gruss,
    Boris.

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

^ permalink raw reply	[flat|nested] 35+ messages in thread

* Re: [PATCH v11 3/5] x86/boot: Add get_acpi_rsdp() to parse RSDP in cmdlien from kexec
  2018-11-13 17:51   ` Borislav Petkov
@ 2018-11-14  1:54     ` Chao Fan
  2018-11-14  1:59       ` Chao Fan
  2018-11-14 18:33       ` Borislav Petkov
  0 siblings, 2 replies; 35+ messages in thread
From: Chao Fan @ 2018-11-14  1:54 UTC (permalink / raw)
  To: Borislav Petkov
  Cc: linux-kernel, x86, linux-efi, linux-acpi, tglx, mingo, hpa,
	keescook, bhe, msys.mizuma, indou.takao, caoj.fnst

On Tue, Nov 13, 2018 at 06:51:50PM +0100, Borislav Petkov wrote:
>On Mon, Nov 12, 2018 at 05:46:43PM +0800, Chao Fan wrote:
>> Imitate setup_acpi_rsdp() for the early_param of 'acpi_rsdp'.
>> KEXEC writes the RSDP pointer to cmdline for EFI booting.
>> So if 'acpi_rsdp' found in cmdline, use it directely.
>> 
>> Since function kstrtoull() is needed, include it in
>> arch/x86/boot/string.h. To solve the definition conflict
>> problem, set BOOT_STRING tag to expose only kstrtoull() and
>> functions used by it. Other functions in lib/kstrtox.c will
>> be covered.
>> 
>> Signed-off-by: Chao Fan <fanc.fnst@cn.fujitsu.com>
>> ---
>>  arch/x86/boot/compressed/acpitb.c | 26 ++++++++++++++++++++++++++
>>  arch/x86/boot/string.h            |  4 ++++
>>  lib/kstrtox.c                     |  4 ++++
>>  3 files changed, 34 insertions(+)
>> 
>> diff --git a/arch/x86/boot/compressed/acpitb.c b/arch/x86/boot/compressed/acpitb.c
>> index 50fa65cf824d..5cfb4efa5a19 100644
>> --- a/arch/x86/boot/compressed/acpitb.c
>> +++ b/arch/x86/boot/compressed/acpitb.c
>> @@ -8,6 +8,12 @@
>>  #include <linux/numa.h>
>>  #include <linux/acpi.h>
>>  
>> +#define STATIC
>> +#include <linux/decompress/mm.h>
>> +
>> +#define BOOT_STRING
>> +#include "../string.h"
>> +
>>  /* Search EFI table for RSDP table. */
>>  static void efi_get_rsdp_addr(acpi_physical_address *rsdp_addr)
>>  {
>> @@ -200,3 +206,23 @@ static void bios_get_rsdp_addr(acpi_physical_address *rsdp_addr)
>>  		*rsdp_addr = (acpi_physical_address)address;
>>  	}
>>  }
>> +
>> +static void get_acpi_rsdp(acpi_physical_address *rsdp_addr)
>> +{
>> +#ifdef CONFIG_KEXEC
>
>Ok, why is that CONFIG_KEXEC dependency needed now too?
>

CONFIG_KEXEC is only needed in this function.

When searching RSDP, there are three methods in order:
1. When booting from KEXEC, 'acpi_rsdp' is added to cmdline by KEXEC,
   so it can be parsed and used. CONFIG_KEXEC is needed here.
2. When booting from EFI, parse EFI table and find RSDP.
3. When booting from BIOS, search memory for RSDP just like
   acpi_find_root_pointer() in drivers/acpi/acpica/tbxfroot.c did.

So, CONFIG_KEXEC is only needed in 1, exactly in this function
get_acpi_rsdp() of my PATCH.

Thanks,
Chao Fan

>Ok, let's recap: so far, for your use case you need:
>
>CONFIG_MEMORY_HOTREMOVE
>CONFIG_RANDOMIZE_BASE
>and now
>CONFIG_KEXEC
>
>So, you can clean up all that ifdeffery by defining a new config item
>CONFIG_EARLY_PARSE_RSDP or so which depends on all those three items and
>then you can do
>
>vmlinux-objs-$(CONFIG_EARLY_PARSE_RSDP) += $(obj)/acpitb.o
>
>and get rid of the most of the ifdeffery.
>
>Yes?
>
>> +	unsigned long long res;
>> +	int len = 0;
>> +	char *val;
>> +
>> +	val = malloc(19);
>> +	len = cmdline_find_option("acpi_rsdp", val, 19);
>> +
>
>^ Superfluous newline.
>
>> +	if (len == -1)
>> +		return;
>
>That check is not needed since you do > 0 below.
>
>> +
>> +	if (len > 0) {
>> +		val[len] = 0;
>> +		*rsdp_addr = (acpi_physical_address)kstrtoull(val, 16, &res);
>> +	}
>> +#endif
>> +}
>> diff --git a/arch/x86/boot/string.h b/arch/x86/boot/string.h
>> index 3d78e27077f4..0ff3edb888e4 100644
>> --- a/arch/x86/boot/string.h
>> +++ b/arch/x86/boot/string.h
>> @@ -30,3 +30,7 @@ extern unsigned long long simple_strtoull(const char *cp, char **endp,
>>  					  unsigned int base);
>>  
>>  #endif /* BOOT_STRING_H */
>> +
>> +#ifdef BOOT_STRING
>> +#include "../../../lib/kstrtox.c"
>> +#endif
>> diff --git a/lib/kstrtox.c b/lib/kstrtox.c
>> index 1006bf70bf74..3804db9eed56 100644
>> --- a/lib/kstrtox.c
>> +++ b/lib/kstrtox.c
>> @@ -126,6 +126,8 @@ int kstrtoull(const char *s, unsigned int base, unsigned long long *res)
>>  }
>>  EXPORT_SYMBOL(kstrtoull);
>
>This needs a comment to explain what is that guard used for.
>
>> +#ifndef BOOT_STRING
>> +
>>  /**
>>   * kstrtoll - convert a string to a long long
>>   * @s: The start of the string. The string must be null-terminated, and may also
>> @@ -408,3 +410,5 @@ kstrto_from_user(kstrtou16_from_user,	kstrtou16,	u16);
>>  kstrto_from_user(kstrtos16_from_user,	kstrtos16,	s16);
>>  kstrto_from_user(kstrtou8_from_user,	kstrtou8,	u8);
>>  kstrto_from_user(kstrtos8_from_user,	kstrtos8,	s8);
>> +
>> +#endif
>
>#endif /* BOOT_STRING */
>
>-- 
>Regards/Gruss,
>    Boris.
>
>Good mailing practices for 400: avoid top-posting and trim the reply.
>
>



^ permalink raw reply	[flat|nested] 35+ messages in thread

* Re: [PATCH v11 3/5] x86/boot: Add get_acpi_rsdp() to parse RSDP in cmdlien from kexec
  2018-11-14  1:54     ` Chao Fan
@ 2018-11-14  1:59       ` Chao Fan
  2018-11-14 18:33       ` Borislav Petkov
  1 sibling, 0 replies; 35+ messages in thread
From: Chao Fan @ 2018-11-14  1:59 UTC (permalink / raw)
  To: Borislav Petkov
  Cc: linux-kernel, x86, linux-efi, linux-acpi, tglx, mingo, hpa,
	keescook, bhe, msys.mizuma, indou.takao, caoj.fnst

On Wed, Nov 14, 2018 at 09:54:50AM +0800, Chao Fan wrote:
>On Tue, Nov 13, 2018 at 06:51:50PM +0100, Borislav Petkov wrote:
>>On Mon, Nov 12, 2018 at 05:46:43PM +0800, Chao Fan wrote:
>>> Imitate setup_acpi_rsdp() for the early_param of 'acpi_rsdp'.
>>> KEXEC writes the RSDP pointer to cmdline for EFI booting.
>>> So if 'acpi_rsdp' found in cmdline, use it directely.
>>> 
>>> Since function kstrtoull() is needed, include it in
>>> arch/x86/boot/string.h. To solve the definition conflict
>>> problem, set BOOT_STRING tag to expose only kstrtoull() and
>>> functions used by it. Other functions in lib/kstrtox.c will
>>> be covered.
>>> 
>>> Signed-off-by: Chao Fan <fanc.fnst@cn.fujitsu.com>
>>> ---
>>>  arch/x86/boot/compressed/acpitb.c | 26 ++++++++++++++++++++++++++
>>>  arch/x86/boot/string.h            |  4 ++++
>>>  lib/kstrtox.c                     |  4 ++++
>>>  3 files changed, 34 insertions(+)
>>> 
>>> diff --git a/arch/x86/boot/compressed/acpitb.c b/arch/x86/boot/compressed/acpitb.c
>>> index 50fa65cf824d..5cfb4efa5a19 100644
>>> --- a/arch/x86/boot/compressed/acpitb.c
>>> +++ b/arch/x86/boot/compressed/acpitb.c
>>> @@ -8,6 +8,12 @@
>>>  #include <linux/numa.h>
>>>  #include <linux/acpi.h>
>>>  
>>> +#define STATIC
>>> +#include <linux/decompress/mm.h>
>>> +
>>> +#define BOOT_STRING
>>> +#include "../string.h"
>>> +
>>>  /* Search EFI table for RSDP table. */
>>>  static void efi_get_rsdp_addr(acpi_physical_address *rsdp_addr)
>>>  {
>>> @@ -200,3 +206,23 @@ static void bios_get_rsdp_addr(acpi_physical_address *rsdp_addr)
>>>  		*rsdp_addr = (acpi_physical_address)address;
>>>  	}
>>>  }
>>> +
>>> +static void get_acpi_rsdp(acpi_physical_address *rsdp_addr)
>>> +{
>>> +#ifdef CONFIG_KEXEC
>>
>>Ok, why is that CONFIG_KEXEC dependency needed now too?
>>
>
>CONFIG_KEXEC is only needed in this function.
>
>When searching RSDP, there are three methods in order:
>1. When booting from KEXEC, 'acpi_rsdp' is added to cmdline by KEXEC,
>   so it can be parsed and used. CONFIG_KEXEC is needed here.
>2. When booting from EFI, parse EFI table and find RSDP.
>3. When booting from BIOS, search memory for RSDP just like
>   acpi_find_root_pointer() in drivers/acpi/acpica/tbxfroot.c did.
>
>So, CONFIG_KEXEC is only needed in 1, exactly in this function
>get_acpi_rsdp() of my PATCH.
>
>Thanks,
>Chao Fan
>

That means, CONFIG_KEXEC is needed by a little part of the whole PATCHSET.
Without CONFIG_KEXEC, RSDP can only be found in other methods.

Thanks,
Chao Fan

>>Ok, let's recap: so far, for your use case you need:
>>
>>CONFIG_MEMORY_HOTREMOVE
>>CONFIG_RANDOMIZE_BASE
>>and now
>>CONFIG_KEXEC
>>
>>So, you can clean up all that ifdeffery by defining a new config item
>>CONFIG_EARLY_PARSE_RSDP or so which depends on all those three items and
>>then you can do
>>
>>vmlinux-objs-$(CONFIG_EARLY_PARSE_RSDP) += $(obj)/acpitb.o
>>
>>and get rid of the most of the ifdeffery.
>>
>>Yes?
>>
>>> +	unsigned long long res;
>>> +	int len = 0;
>>> +	char *val;
>>> +
>>> +	val = malloc(19);
>>> +	len = cmdline_find_option("acpi_rsdp", val, 19);
>>> +
>>
>>^ Superfluous newline.
>>
>>> +	if (len == -1)
>>> +		return;
>>
>>That check is not needed since you do > 0 below.
>>
>>> +
>>> +	if (len > 0) {
>>> +		val[len] = 0;
>>> +		*rsdp_addr = (acpi_physical_address)kstrtoull(val, 16, &res);
>>> +	}
>>> +#endif
>>> +}
>>> diff --git a/arch/x86/boot/string.h b/arch/x86/boot/string.h
>>> index 3d78e27077f4..0ff3edb888e4 100644
>>> --- a/arch/x86/boot/string.h
>>> +++ b/arch/x86/boot/string.h
>>> @@ -30,3 +30,7 @@ extern unsigned long long simple_strtoull(const char *cp, char **endp,
>>>  					  unsigned int base);
>>>  
>>>  #endif /* BOOT_STRING_H */
>>> +
>>> +#ifdef BOOT_STRING
>>> +#include "../../../lib/kstrtox.c"
>>> +#endif
>>> diff --git a/lib/kstrtox.c b/lib/kstrtox.c
>>> index 1006bf70bf74..3804db9eed56 100644
>>> --- a/lib/kstrtox.c
>>> +++ b/lib/kstrtox.c
>>> @@ -126,6 +126,8 @@ int kstrtoull(const char *s, unsigned int base, unsigned long long *res)
>>>  }
>>>  EXPORT_SYMBOL(kstrtoull);
>>
>>This needs a comment to explain what is that guard used for.
>>
>>> +#ifndef BOOT_STRING
>>> +
>>>  /**
>>>   * kstrtoll - convert a string to a long long
>>>   * @s: The start of the string. The string must be null-terminated, and may also
>>> @@ -408,3 +410,5 @@ kstrto_from_user(kstrtou16_from_user,	kstrtou16,	u16);
>>>  kstrto_from_user(kstrtos16_from_user,	kstrtos16,	s16);
>>>  kstrto_from_user(kstrtou8_from_user,	kstrtou8,	u8);
>>>  kstrto_from_user(kstrtos8_from_user,	kstrtos8,	s8);
>>> +
>>> +#endif
>>
>>#endif /* BOOT_STRING */
>>
>>-- 
>>Regards/Gruss,
>>    Boris.
>>
>>Good mailing practices for 400: avoid top-posting and trim the reply.
>>
>>



^ permalink raw reply	[flat|nested] 35+ messages in thread

* Re: [PATCH v11 3/5] x86/boot: Add get_acpi_rsdp() to parse RSDP in cmdlien from kexec
  2018-11-13 21:51             ` Borislav Petkov
@ 2018-11-14  6:12               ` Chao Fan
  2018-11-14 18:30                 ` Borislav Petkov
  0 siblings, 1 reply; 35+ messages in thread
From: Chao Fan @ 2018-11-14  6:12 UTC (permalink / raw)
  To: Borislav Petkov
  Cc: Masayoshi Mizuma, linux-kernel, x86, linux-efi, linux-acpi, tglx,
	mingo, hpa, keescook, bhe, indou.takao, caoj.fnst

Hi Boris, Masa, and Baoquan,

On Tue, Nov 13, 2018 at 10:51:56PM +0100, Borislav Petkov wrote:
>On Tue, Nov 13, 2018 at 03:06:16PM -0500, Masayoshi Mizuma wrote:
>> I just felt the BOOT_STRING thing in lib/kstrtox.c confuses...
>> I'm OK for now if it's applied your below comment.
>
>Well, actually, upon a second look, I don't think that including a .c
>file into a header is ok:
>
>diff --git a/arch/x86/boot/string.h b/arch/x86/boot/string.h
>index 3d78e27077f4..0ff3edb888e4 100644
>--- a/arch/x86/boot/string.h
>+++ b/arch/x86/boot/string.h
>@@ -30,3 +30,7 @@ extern unsigned long long simple_strtoull(const char *cp, char **endp,
>                                          unsigned int base);
>
> #endif /* BOOT_STRING_H */
>+
>+#ifdef BOOT_STRING
>+#include "../../../lib/kstrtox.c"
>+#endif
>
>Chao, why isn't this part of arch/x86/boot/compressed/misc.c ?
>

Fine, I have put it to misc.c:

diff --git a/arch/x86/boot/compressed/misc.c b/arch/x86/boot/compressed/misc.c
index 8dd1d5ccae58..714b05b65a33 100644
--- a/arch/x86/boot/compressed/misc.c
+++ b/arch/x86/boot/compressed/misc.c
@@ -426,3 +426,7 @@ void fortify_panic(const char *name)
 {
        error("detected buffer overflow");
 }
+
+#ifdef BOOT_STRING
+#include "../../../../lib/kstrtox.c"
+#endif

And define it in misc.h:

diff --git a/arch/x86/boot/compressed/misc.h b/arch/x86/boot/compressed/misc.h
index 4a3645fda0ed..98e28c4281ee 100644
--- a/arch/x86/boot/compressed/misc.h
+++ b/arch/x86/boot/compressed/misc.h
@@ -131,3 +131,5 @@ int num_immovable_mem;
 void get_immovable_mem(void);
 #endif
 #endif
+#define BOOT_STRING
+extern int kstrtoull(const char *s, unsigned int base, unsigned long long *res);

But isdigit() would be redefine, so:

diff --git a/include/linux/ctype.h b/include/linux/ctype.h
index 363b004426db..aba01c385232 100644
--- a/include/linux/ctype.h
+++ b/include/linux/ctype.h
@@ -23,10 +23,12 @@ extern const unsigned char _ctype[];
 #define isalnum(c)     ((__ismask(c)&(_U|_L|_D)) != 0)
 #define isalpha(c)     ((__ismask(c)&(_U|_L)) != 0)
 #define iscntrl(c)     ((__ismask(c)&(_C)) != 0)
+#ifndef BOOT_STRING
 static inline int isdigit(int c)
 {
        return '0' <= c && c <= '9';
 }
+#endif
 #define isgraph(c)     ((__ismask(c)&(_P|_U|_L|_D)) != 0)
 #define islower(c)     ((__ismask(c)&(_L)) != 0)
 #define isprint(c)     ((__ismask(c)&(_P|_U|_L|_D|_SP)) != 0)

Now I can make it.
I wonder whether this is OK to cover isdigit() with 'BOOT_STRING' tag.

Thanks,
Chao Fan

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



^ permalink raw reply	[flat|nested] 35+ messages in thread

* Re: [PATCH v11 3/5] x86/boot: Add get_acpi_rsdp() to parse RSDP in cmdlien from kexec
  2018-11-14  6:12               ` Chao Fan
@ 2018-11-14 18:30                 ` Borislav Petkov
  2018-11-19  1:16                   ` Chao Fan
  0 siblings, 1 reply; 35+ messages in thread
From: Borislav Petkov @ 2018-11-14 18:30 UTC (permalink / raw)
  To: Chao Fan
  Cc: Masayoshi Mizuma, linux-kernel, x86, linux-efi, linux-acpi, tglx,
	mingo, hpa, keescook, bhe, indou.takao, caoj.fnst

On Wed, Nov 14, 2018 at 02:12:16PM +0800, Chao Fan wrote:
> But isdigit() would be redefine, so:
> 
> diff --git a/include/linux/ctype.h b/include/linux/ctype.h
> index 363b004426db..aba01c385232 100644
> --- a/include/linux/ctype.h
> +++ b/include/linux/ctype.h
> @@ -23,10 +23,12 @@ extern const unsigned char _ctype[];
>  #define isalnum(c)     ((__ismask(c)&(_U|_L|_D)) != 0)
>  #define isalpha(c)     ((__ismask(c)&(_U|_L)) != 0)
>  #define iscntrl(c)     ((__ismask(c)&(_C)) != 0)
> +#ifndef BOOT_STRING
>  static inline int isdigit(int c)
>  {
>         return '0' <= c && c <= '9';
>  }
> +#endif
>  #define isgraph(c)     ((__ismask(c)&(_P|_U|_L|_D)) != 0)
>  #define islower(c)     ((__ismask(c)&(_L)) != 0)
>  #define isprint(c)     ((__ismask(c)&(_P|_U|_L|_D|_SP)) != 0)
> 
> Now I can make it.
> I wonder whether this is OK to cover isdigit() with 'BOOT_STRING' tag.

See the beginning of arch/x86/boot/compressed/kaslr.c for a possible way
to disable boot/ctype.h

-- 
Regards/Gruss,
    Boris.

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

^ permalink raw reply	[flat|nested] 35+ messages in thread

* Re: [PATCH v11 3/5] x86/boot: Add get_acpi_rsdp() to parse RSDP in cmdlien from kexec
  2018-11-14  1:54     ` Chao Fan
  2018-11-14  1:59       ` Chao Fan
@ 2018-11-14 18:33       ` Borislav Petkov
  1 sibling, 0 replies; 35+ messages in thread
From: Borislav Petkov @ 2018-11-14 18:33 UTC (permalink / raw)
  To: Chao Fan
  Cc: linux-kernel, x86, linux-efi, linux-acpi, tglx, mingo, hpa,
	keescook, bhe, msys.mizuma, indou.takao, caoj.fnst

On Wed, Nov 14, 2018 at 09:54:50AM +0800, Chao Fan wrote:
> CONFIG_KEXEC is only needed in this function.
> 
> When searching RSDP, there are three methods in order:
> 1. When booting from KEXEC, 'acpi_rsdp' is added to cmdline by KEXEC,
>    so it can be parsed and used. CONFIG_KEXEC is needed here.

But theoretically acpi_rsdp can be supplied by the first kernel too,
right?

So you don't need that CONFIG_KEXEC here at all?

> >Ok, let's recap: so far, for your use case you need:
> >
> >CONFIG_MEMORY_HOTREMOVE
> >CONFIG_RANDOMIZE_BASE
> >and now
> >CONFIG_KEXEC
> >
> >So, you can clean up all that ifdeffery by defining a new config item
> >CONFIG_EARLY_PARSE_RSDP or so which depends on all those three items and
> >then you can do
> >
> >vmlinux-objs-$(CONFIG_EARLY_PARSE_RSDP) += $(obj)/acpitb.o
> >
> >and get rid of the most of the ifdeffery.

Regardless of CONFIG_KEXEC - you still need to define a CONFIG_ symbol
for your use case. We won't be parsing RSDP early on !MEMORY_HOTREMOVE
machines, which is the majority.

-- 
Regards/Gruss,
    Boris.

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

^ permalink raw reply	[flat|nested] 35+ messages in thread

* Re: [PATCH v11 4/5] x86/boot: Dig out SRAT table from RSDP and find immovable memory
  2018-11-12  9:46 ` [PATCH v11 4/5] x86/boot: Dig out SRAT table from RSDP and find immovable memory Chao Fan
  2018-11-12 20:52   ` Masayoshi Mizuma
  2018-11-12 21:51   ` Masayoshi Mizuma
@ 2018-11-16 11:16   ` Borislav Petkov
  2018-11-19  2:08     ` Chao Fan
  2018-11-20  6:18     ` Chao Fan
  2 siblings, 2 replies; 35+ messages in thread
From: Borislav Petkov @ 2018-11-16 11:16 UTC (permalink / raw)
  To: Chao Fan
  Cc: linux-kernel, x86, linux-efi, linux-acpi, tglx, mingo, hpa,
	keescook, bhe, msys.mizuma, indou.takao, caoj.fnst

On Mon, Nov 12, 2018 at 05:46:44PM +0800, Chao Fan wrote:
> To avoid KASLR extracting kernel on movable memory, slove the
						      ^^^^^

Please introduce a spellchecker into your patch creation workflow.

> conflict between KASLR and movable_node feature, dig the SRAT tables

s/dig/determine/ or "compute SRAT table's address" or so.

Also, replace "dig" with a more suitable verb in all your patches.

> from RSDP pointer. Walk the SRAT tables and store the immovable
> memory regions in immovable_mem[].

		"... in an array called immovable_mem[]."

> There are three methods to get RSDP pointer: KEXEC condition,
> EFI confition, BIOS condition.

"condition" is not the right word here.

> If KEXEC add 'acpi_rsdp' to cmdline, use it.
> Otherwise, parse EFI table for RSDP.
> Then, search memory for RSDP.
> 
> Imitate from ACPI code, based on acpi_os_get_root_pointer().
> Process: RSDP->RSDT/XSDT->ACPI root table->SRAT.

What?!

This looks like a comment you've added as a note for yourself but not
part of the final commit message. If you wanna explain the process, then
write it out in plain english as if you're explaining it to someone who
doesn't know what you're doing.

> 
> Signed-off-by: Chao Fan <fanc.fnst@cn.fujitsu.com>
> ---
>  arch/x86/boot/compressed/Makefile |   4 +
>  arch/x86/boot/compressed/acpitb.c | 139 ++++++++++++++++++++++++++++++
>  arch/x86/boot/compressed/kaslr.c  |   4 -
>  arch/x86/boot/compressed/misc.h   |  15 ++++
>  4 files changed, 158 insertions(+), 4 deletions(-)
> 
> diff --git a/arch/x86/boot/compressed/Makefile b/arch/x86/boot/compressed/Makefile
> index 466f66c8a7f8..b51f7629b8ef 100644
> --- a/arch/x86/boot/compressed/Makefile
> +++ b/arch/x86/boot/compressed/Makefile
> @@ -84,6 +84,10 @@ ifdef CONFIG_X86_64
>  	vmlinux-objs-y += $(obj)/pgtable_64.o
>  endif
>  
> +#if (defined CONFIG_MEMORY_HOTREMOVE) && (defined CONFIG_RANDOMIZE_BASE)
> +vmlinux-objs-$(CONFIG_RANDOMIZE_BASE) += $(obj)/acpitb.o
> +#endif

Right, as previously pointed out, this needs that CONFIG_ symbol and
then you can save yourself most (if not all) of the ifdeffery in the
rest of the patchset.

> +
>  $(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/acpitb.c b/arch/x86/boot/compressed/acpitb.c
> index 5cfb4efa5a19..161f21a7fb3b 100644
> --- a/arch/x86/boot/compressed/acpitb.c
> +++ b/arch/x86/boot/compressed/acpitb.c
> @@ -14,6 +14,11 @@
>  #define BOOT_STRING
>  #include "../string.h"
>  
> +#ifdef CONFIG_MEMORY_HOTREMOVE
> +/* Store the immovable memory regions */
> +struct mem_vector immovable_mem[MAX_NUMNODES*2];
> +#endif
> +
>  /* Search EFI table for RSDP table. */
>  static void efi_get_rsdp_addr(acpi_physical_address *rsdp_addr)
>  {
> @@ -226,3 +231,137 @@ static void get_acpi_rsdp(acpi_physical_address *rsdp_addr)
>  	}
>  #endif
>  }
> +
> +/*
> + * Used to dig RSDP table from EFI table or BIOS.
> + * If RSDP table found in EFI table, use it. Or search BIOS.
> + * Based on acpi_os_get_root_pointer().
> + */
> +static acpi_physical_address get_rsdp_addr(void)
> +{
> +	acpi_physical_address pa = 0;
> +
> +	get_acpi_rsdp(&pa);
> +
> +	if (!pa)
> +		efi_get_rsdp_addr(&pa);
> +
> +	if (!pa)
> +		bios_get_rsdp_addr(&pa);
> +
> +	return pa;
> +}
> +
> +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;
> +	bool acpi_use_rsdt = false;
> +	char *signature;
> +	char arg[10];
> +	u8 *entry;
> +	u32 count;
> +	u32 size;
> +	int i, j;
> +	int ret;
> +	u32 len;
> +
> +	rsdp = (struct acpi_table_rsdp *)get_rsdp_addr();
> +	if (!rsdp)
> +		return NULL;
> +
> +	ret = cmdline_find_option("acpi", arg, sizeof(arg));
> +	if (ret == 4 && !strncmp(arg, "rsdt", 4))
> +		acpi_use_rsdt = true;
> +
> +	/* Get RSDT or XSDT from RSDP. */
> +	if (!acpi_use_rsdt &&
> +	    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;
> +	}

Reorganize that code here to get rid of acpi_use_rsdt.

> +
> +	/* Get ACPI root table from RSDT or XSDT.*/
> +	header = (struct acpi_table_header *)root_table;
> +	len = header->length;

No checking of that header pointer before dereffing it?

If it is NUL, that gives you a very nasty bug to try to debug in the
early code.

> +	count = (u32)((len - sizeof(struct acpi_table_header)) / size);

Uuh, no checking for count wrapping around here due to wrong len? That
would give you a *lot* of looping below if it wraps.

IOW, you need to verify all those values before doing arithmetic with
them - it is early code and it is BIOS - there's no trusting it.

Also, it is not "count" but "num_entries" or so.

> +	entry = ACPI_ADD_PTR(u8, header, sizeof(struct acpi_table_header));
> +
> +	for (i = 0; i < count; i++) {

That variable i is not needed, right?

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

Now can acpi_table be NUL here?

> +			header = (struct acpi_table_header *)acpi_table;
> +			signature = header->signature;
> +
> +			if (ACPI_COMPARE_NAME(header->signature, ACPI_SIG_SRAT))
> +				return header;
> +		}
> +		entry += size;
> +	}
> +	return NULL;
> +}
> +
> +/*
> + * According to ACPI table, filter the immvoable memory regions
					  ^^^^^^^^^
Typo.

> + * 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;
> +	int ret;
> +
> +	ret = cmdline_find_option("acpi", arg, sizeof(arg));
> +	if (ret == 3 && !strncmp(arg, "off", 3))
> +		return;
> +
> +	if (!cmdline_find_option_bool("movable_node"))
> +		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, aborted.\n");

"..., aborting."

> +				break;
> +			}
> +		}
> +		table = (struct acpi_subtable_header *)
> +			((unsigned long)table + table->length);
> +	}
> +	num_immovable_mem = i;
> +}
> diff --git a/arch/x86/boot/compressed/kaslr.c b/arch/x86/boot/compressed/kaslr.c
> index 9ed9709d9947..b251572e77af 100644
> --- a/arch/x86/boot/compressed/kaslr.c
> +++ b/arch/x86/boot/compressed/kaslr.c
> @@ -87,10 +87,6 @@ static unsigned long get_boot_seed(void)
>  #define KASLR_COMPRESSED_BOOT
>  #include "../../lib/kaslr.c"
>  
> -struct mem_vector {
> -	unsigned long long start;
> -	unsigned long long size;
> -};
>  
>  /* Only supporting at most 4 unusable memmap regions with kaslr */
>  #define MAX_MEMMAP_REGIONS	4
> diff --git a/arch/x86/boot/compressed/misc.h b/arch/x86/boot/compressed/misc.h
> index a1d5918765f3..4a3645fda0ed 100644
> --- a/arch/x86/boot/compressed/misc.h
> +++ b/arch/x86/boot/compressed/misc.h
> @@ -77,6 +77,11 @@ void choose_random_location(unsigned long input,
>  			    unsigned long *output,
>  			    unsigned long output_size,
>  			    unsigned long *virt_addr);
> +struct mem_vector {
> +	unsigned long long start;
> +	unsigned long long size;
> +};
> +
>  /* cpuflags.c */
>  bool has_cpuflag(int flag);
>  #else
> @@ -116,3 +121,13 @@ static inline void console_init(void)
>  void set_sev_encryption_mask(void);
>  
>  #endif
> +
> +/* acpitb.c */
> +#ifdef CONFIG_RANDOMIZE_BASE
> +int num_immovable_mem;
> +#ifdef CONFIG_MEMORY_HOTREMOVE
> +/* Store the amount of immovable memory regions */

Above says "regions" but define below is "TABLES". Hmmm?

> +#define ACPI_MAX_TABLES                128
> +void get_immovable_mem(void);
> +#endif
> +#endif
> -- 

-- 
Regards/Gruss,
    Boris.

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

^ permalink raw reply	[flat|nested] 35+ messages in thread

* Re: [PATCH v11 5/5] x86/boot/KASLR: Walk srat tables to filter immovable memory
  2018-11-12  9:46 ` [PATCH v11 5/5] x86/boot/KASLR: Walk srat tables to filter " Chao Fan
@ 2018-11-16 13:50   ` Borislav Petkov
  2018-11-19  1:31     ` Chao Fan
  0 siblings, 1 reply; 35+ messages in thread
From: Borislav Petkov @ 2018-11-16 13:50 UTC (permalink / raw)
  To: Chao Fan
  Cc: linux-kernel, x86, linux-efi, linux-acpi, tglx, mingo, hpa,
	keescook, bhe, msys.mizuma, indou.takao, caoj.fnst

 Subject: Re: [PATCH v11 5/5] x86/boot/KASLR: Walk srat tables to filter immovable memory

s/srat/SRAT/g

On Mon, Nov 12, 2018 at 05:46:45PM +0800, Chao Fan wrote:
> KASLR may randomly chooses some positions which are located in movable

		    choose

> memory regions. This will break memory hotplug feature and make the
> movable memory chosen by KASLR can't be removed.

			by KASLR practically immovable.

:)

> The solution is limite KASLR to choose memory regions in immovable

limite?

"to limit"

> node according to SRAT tables.
> 
> If CONFIG_MEMORY_HOTREMOVE enabled, walk through the SRAT memory

			   *is* enabled,

> tables and store those immovable memory regions so that KASLR can get
> where to choose for randomization.
> 
> If the amount of immovable memory regions is not zero, which
> means the immovable memory regions existing. Calculate the intersection
> between memory regions from e820/efi memory table and immovable memory
> regions.

This is explaining *what* the patch does and generally doesn't need to
be in the commit messge as people can read it in the patch itself.

> Signed-off-by: Chao Fan <fanc.fnst@cn.fujitsu.com>
> ---
>  arch/x86/boot/compressed/kaslr.c | 77 +++++++++++++++++++++++++++-----
>  1 file changed, 66 insertions(+), 11 deletions(-)
> 
> diff --git a/arch/x86/boot/compressed/kaslr.c b/arch/x86/boot/compressed/kaslr.c
> index b251572e77af..174d2114045e 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_MEMORY_HOTREMOVE
> +/* Store the immovable memory regions */
> +extern struct mem_vector immovable_mem[MAX_NUMNODES*2];
> +#endif

For this and the other occurrences of ifdef CONFIG_MEMORY_HOTREMOVE,
define empty stubs for those functions in a header and remove the
ifdeffery at the call sites.

> +
>  
>  enum mem_avoid_index {
>  	MEM_AVOID_ZO_RANGE = 0,
> @@ -413,6 +418,11 @@ static void mem_avoid_init(unsigned long input, unsigned long input_size,
>  	/* Mark the memmap regions we need to avoid */
>  	handle_mem_options();
>  
> +#ifdef CONFIG_MEMORY_HOTREMOVE
> +	/* Mark the immovable regions we need to choose */
> +	get_immovable_mem();
> +#endif
> +
>  #ifdef CONFIG_X86_VERBOSE_BOOTUP
>  	/* Make sure video RAM can be used. */
>  	add_identity_map(0, PMD_SIZE);
> @@ -568,9 +578,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 slots_count(struct mem_vector *entry,

That's a strange rename.

__process_mem_region() makes more sense to me.

> +			unsigned long minimum,
> +			unsigned long image_size)
>  {
>  	struct mem_vector region, overlap;
>  	unsigned long start_orig, end;
> @@ -646,6 +656,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 directely.

"directly"

> +	 */
> +	if (num_immovable_mem == 0) {

	if (!...

> +		slots_count(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;
> +	}
> +

-- 
Regards/Gruss,
    Boris.

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

^ permalink raw reply	[flat|nested] 35+ messages in thread

* Re: [PATCH v11 3/5] x86/boot: Add get_acpi_rsdp() to parse RSDP in cmdlien from kexec
  2018-11-14 18:30                 ` Borislav Petkov
@ 2018-11-19  1:16                   ` Chao Fan
  0 siblings, 0 replies; 35+ messages in thread
From: Chao Fan @ 2018-11-19  1:16 UTC (permalink / raw)
  To: Borislav Petkov
  Cc: Masayoshi Mizuma, linux-kernel, x86, linux-efi, linux-acpi, tglx,
	mingo, hpa, keescook, bhe, indou.takao, caoj.fnst

On Wed, Nov 14, 2018 at 07:30:17PM +0100, Borislav Petkov wrote:
>On Wed, Nov 14, 2018 at 02:12:16PM +0800, Chao Fan wrote:
>> But isdigit() would be redefine, so:
>> 
>> diff --git a/include/linux/ctype.h b/include/linux/ctype.h
>> index 363b004426db..aba01c385232 100644
>> --- a/include/linux/ctype.h
>> +++ b/include/linux/ctype.h
>> @@ -23,10 +23,12 @@ extern const unsigned char _ctype[];
>>  #define isalnum(c)     ((__ismask(c)&(_U|_L|_D)) != 0)
>>  #define isalpha(c)     ((__ismask(c)&(_U|_L)) != 0)
>>  #define iscntrl(c)     ((__ismask(c)&(_C)) != 0)
>> +#ifndef BOOT_STRING
>>  static inline int isdigit(int c)
>>  {
>>         return '0' <= c && c <= '9';
>>  }
>> +#endif
>>  #define isgraph(c)     ((__ismask(c)&(_P|_U|_L|_D)) != 0)
>>  #define islower(c)     ((__ismask(c)&(_L)) != 0)
>>  #define isprint(c)     ((__ismask(c)&(_P|_U|_L|_D|_SP)) != 0)
>> 
>> Now I can make it.
>> I wonder whether this is OK to cover isdigit() with 'BOOT_STRING' tag.
>
>See the beginning of arch/x86/boot/compressed/kaslr.c for a possible way
>to disable boot/ctype.h

I have done this with BOOT_CTYPE_H.
So misc.c can only use isdigit() and isxdigit() in
include/linux/ctype.h.

diff --git a/arch/x86/boot/compressed/misc.c b/arch/x86/boot/compressed/misc.c
index 8dd1d5ccae58..e51713fe3add 100644
--- a/arch/x86/boot/compressed/misc.c
+++ b/arch/x86/boot/compressed/misc.c
@@ -12,6 +12,7 @@
  * High loaded stuff by Hans Lermen & Werner Almesberger, Feb. 1996
  */

+#define BOOT_CTYPE_H
 #include "misc.h"
 #include "error.h"
 #include "pgtable.h"
@@ -426,3 +427,7 @@ void fortify_panic(const char *name)
 {
        error("detected buffer overflow");
 }
+
+#ifdef BOOT_STRING
+#include "../../../../lib/kstrtox.c"
+#endif

This looks better than before.

Thanks,
Chao Fan

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



^ permalink raw reply	[flat|nested] 35+ messages in thread

* Re: [PATCH v11 5/5] x86/boot/KASLR: Walk srat tables to filter immovable memory
  2018-11-16 13:50   ` Borislav Petkov
@ 2018-11-19  1:31     ` Chao Fan
  0 siblings, 0 replies; 35+ messages in thread
From: Chao Fan @ 2018-11-19  1:31 UTC (permalink / raw)
  To: Borislav Petkov
  Cc: linux-kernel, x86, linux-efi, linux-acpi, tglx, mingo, hpa,
	keescook, bhe, msys.mizuma, indou.takao, caoj.fnst

On Fri, Nov 16, 2018 at 02:50:39PM +0100, Borislav Petkov wrote:
> Subject: Re: [PATCH v11 5/5] x86/boot/KASLR: Walk srat tables to filter immovable memory
>
>s/srat/SRAT/g
>
>On Mon, Nov 12, 2018 at 05:46:45PM +0800, Chao Fan wrote:
>> KASLR may randomly chooses some positions which are located in movable
>
>		    choose
>
>> memory regions. This will break memory hotplug feature and make the
>> movable memory chosen by KASLR can't be removed.
>
>			by KASLR practically immovable.

Thanks,

>
>:)
>
>> The solution is limite KASLR to choose memory regions in immovable
>
>limite?
>
>"to limit"
>
>> node according to SRAT tables.
>> 
>> If CONFIG_MEMORY_HOTREMOVE enabled, walk through the SRAT memory
>
>			   *is* enabled,
>
>> tables and store those immovable memory regions so that KASLR can get
>> where to choose for randomization.
>> 
>> If the amount of immovable memory regions is not zero, which
>> means the immovable memory regions existing. Calculate the intersection
>> between memory regions from e820/efi memory table and immovable memory
>> regions.
>
>This is explaining *what* the patch does and generally doesn't need to
>be in the commit messge as people can read it in the patch itself.

OK,

>
>> Signed-off-by: Chao Fan <fanc.fnst@cn.fujitsu.com>
>> ---
>>  arch/x86/boot/compressed/kaslr.c | 77 +++++++++++++++++++++++++++-----
>>  1 file changed, 66 insertions(+), 11 deletions(-)
>> 
>> diff --git a/arch/x86/boot/compressed/kaslr.c b/arch/x86/boot/compressed/kaslr.c
>> index b251572e77af..174d2114045e 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_MEMORY_HOTREMOVE
>> +/* Store the immovable memory regions */
>> +extern struct mem_vector immovable_mem[MAX_NUMNODES*2];
>> +#endif
>
>For this and the other occurrences of ifdef CONFIG_MEMORY_HOTREMOVE,
>define empty stubs for those functions in a header and remove the
>ifdeffery at the call sites.

OK,

>
>> +
>>  
>>  enum mem_avoid_index {
>>  	MEM_AVOID_ZO_RANGE = 0,
>> @@ -413,6 +418,11 @@ static void mem_avoid_init(unsigned long input, unsigned long input_size,
>>  	/* Mark the memmap regions we need to avoid */
>>  	handle_mem_options();
>>  
>> +#ifdef CONFIG_MEMORY_HOTREMOVE
>> +	/* Mark the immovable regions we need to choose */
>> +	get_immovable_mem();
>> +#endif
>> +
>>  #ifdef CONFIG_X86_VERBOSE_BOOTUP
>>  	/* Make sure video RAM can be used. */
>>  	add_identity_map(0, PMD_SIZE);
>> @@ -568,9 +578,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 slots_count(struct mem_vector *entry,
>
>That's a strange rename.
>
I will change it.

Thanks,
Chao Fan

>__process_mem_region() makes more sense to me.
>
>> +			unsigned long minimum,
>> +			unsigned long image_size)
>>  {
>>  	struct mem_vector region, overlap;
>>  	unsigned long start_orig, end;
>> @@ -646,6 +656,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 directely.
>
>"directly"
>
>> +	 */
>> +	if (num_immovable_mem == 0) {
>
>	if (!...
>
>> +		slots_count(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;
>> +	}
>> +
>
>-- 
>Regards/Gruss,
>    Boris.
>
>Good mailing practices for 400: avoid top-posting and trim the reply.
>
>



^ permalink raw reply	[flat|nested] 35+ messages in thread

* Re: [PATCH v11 4/5] x86/boot: Dig out SRAT table from RSDP and find immovable memory
  2018-11-16 11:16   ` Borislav Petkov
@ 2018-11-19  2:08     ` Chao Fan
  2018-11-20  6:18     ` Chao Fan
  1 sibling, 0 replies; 35+ messages in thread
From: Chao Fan @ 2018-11-19  2:08 UTC (permalink / raw)
  To: Borislav Petkov
  Cc: linux-kernel, x86, linux-efi, linux-acpi, tglx, mingo, hpa,
	keescook, bhe, msys.mizuma, indou.takao, caoj.fnst

On Fri, Nov 16, 2018 at 12:16:54PM +0100, Borislav Petkov wrote:
>On Mon, Nov 12, 2018 at 05:46:44PM +0800, Chao Fan wrote:
>> To avoid KASLR extracting kernel on movable memory, slove the
>						      ^^^^^
>
>Please introduce a spellchecker into your patch creation workflow.

OK.

>
>> conflict between KASLR and movable_node feature, dig the SRAT tables
>
>s/dig/determine/ or "compute SRAT table's address" or so.
>
>Also, replace "dig" with a more suitable verb in all your patches.

How about "search RSDP pointer"
>
>> from RSDP pointer. Walk the SRAT tables and store the immovable
>> memory regions in immovable_mem[].
>
>		"... in an array called immovable_mem[]."

Looks good.

>
>> There are three methods to get RSDP pointer: KEXEC condition,
>> EFI confition, BIOS condition.
>
>"condition" is not the right word here.
>
>> If KEXEC add 'acpi_rsdp' to cmdline, use it.
>> Otherwise, parse EFI table for RSDP.
>> Then, search memory for RSDP.
>> 
>> Imitate from ACPI code, based on acpi_os_get_root_pointer().
>> Process: RSDP->RSDT/XSDT->ACPI root table->SRAT.
>
>What?!
>
>This looks like a comment you've added as a note for yourself but not
>part of the final commit message. If you wanna explain the process, then
>write it out in plain english as if you're explaining it to someone who
>doesn't know what you're doing.

OK.

>
>> 
>> Signed-off-by: Chao Fan <fanc.fnst@cn.fujitsu.com>
>> ---
>>  arch/x86/boot/compressed/Makefile |   4 +
>>  arch/x86/boot/compressed/acpitb.c | 139 ++++++++++++++++++++++++++++++
>>  arch/x86/boot/compressed/kaslr.c  |   4 -
>>  arch/x86/boot/compressed/misc.h   |  15 ++++
>>  4 files changed, 158 insertions(+), 4 deletions(-)
>> 
>> diff --git a/arch/x86/boot/compressed/Makefile b/arch/x86/boot/compressed/Makefile
>> index 466f66c8a7f8..b51f7629b8ef 100644
>> --- a/arch/x86/boot/compressed/Makefile
>> +++ b/arch/x86/boot/compressed/Makefile
>> @@ -84,6 +84,10 @@ ifdef CONFIG_X86_64
>>  	vmlinux-objs-y += $(obj)/pgtable_64.o
>>  endif
>>  
>> +#if (defined CONFIG_MEMORY_HOTREMOVE) && (defined CONFIG_RANDOMIZE_BASE)
>> +vmlinux-objs-$(CONFIG_RANDOMIZE_BASE) += $(obj)/acpitb.o
>> +#endif
>
>Right, as previously pointed out, this needs that CONFIG_ symbol and
>then you can save yourself most (if not all) of the ifdeffery in the
>rest of the patchset.

That makes sense, I will do that.

>
>> +
>>  $(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/acpitb.c b/arch/x86/boot/compressed/acpitb.c
>> index 5cfb4efa5a19..161f21a7fb3b 100644
>> --- a/arch/x86/boot/compressed/acpitb.c
>> +++ b/arch/x86/boot/compressed/acpitb.c
>> @@ -14,6 +14,11 @@
>>  #define BOOT_STRING
>>  #include "../string.h"
>>  
>> +#ifdef CONFIG_MEMORY_HOTREMOVE
>> +/* Store the immovable memory regions */
>> +struct mem_vector immovable_mem[MAX_NUMNODES*2];
>> +#endif
>> +
>>  /* Search EFI table for RSDP table. */
>>  static void efi_get_rsdp_addr(acpi_physical_address *rsdp_addr)
>>  {
>> @@ -226,3 +231,137 @@ static void get_acpi_rsdp(acpi_physical_address *rsdp_addr)
>>  	}
>>  #endif
>>  }
>> +
>> +/*
>> + * Used to dig RSDP table from EFI table or BIOS.
>> + * If RSDP table found in EFI table, use it. Or search BIOS.
>> + * Based on acpi_os_get_root_pointer().
>> + */
>> +static acpi_physical_address get_rsdp_addr(void)
>> +{
>> +	acpi_physical_address pa = 0;
>> +
>> +	get_acpi_rsdp(&pa);
>> +
>> +	if (!pa)
>> +		efi_get_rsdp_addr(&pa);
>> +
>> +	if (!pa)
>> +		bios_get_rsdp_addr(&pa);
>> +
>> +	return pa;
>> +}
>> +
>> +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;
>> +	bool acpi_use_rsdt = false;
>> +	char *signature;
>> +	char arg[10];
>> +	u8 *entry;
>> +	u32 count;
>> +	u32 size;
>> +	int i, j;
>> +	int ret;
>> +	u32 len;
>> +
>> +	rsdp = (struct acpi_table_rsdp *)get_rsdp_addr();
>> +	if (!rsdp)
>> +		return NULL;
>> +
>> +	ret = cmdline_find_option("acpi", arg, sizeof(arg));
>> +	if (ret == 4 && !strncmp(arg, "rsdt", 4))
>> +		acpi_use_rsdt = true;
>> +
>> +	/* Get RSDT or XSDT from RSDP. */
>> +	if (!acpi_use_rsdt &&
>> +	    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;
>> +	}
>
>Reorganize that code here to get rid of acpi_use_rsdt.

OK.

>
>> +
>> +	/* Get ACPI root table from RSDT or XSDT.*/
>> +	header = (struct acpi_table_header *)root_table;
>> +	len = header->length;
>
>No checking of that header pointer before dereffing it?
>
>If it is NUL, that gives you a very nasty bug to try to debug in the
>early code.
>
>> +	count = (u32)((len - sizeof(struct acpi_table_header)) / size);
>
>Uuh, no checking for count wrapping around here due to wrong len? That
>would give you a *lot* of looping below if it wraps.
>
>IOW, you need to verify all those values before doing arithmetic with
>them - it is early code and it is BIOS - there's no trusting it.

I will add the check.

>
>Also, it is not "count" but "num_entries" or so.
>
>> +	entry = ACPI_ADD_PTR(u8, header, sizeof(struct acpi_table_header));
>> +
>> +	for (i = 0; i < count; i++) {
>
>That variable i is not needed, right?
>
>	while (num_entries--)
>
>?

Yes

>
>> +		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) {
>
>Now can acpi_table be NUL here?
>

Thank you,
I will change all of these.

Thanks,
Chao Fan

>> +			header = (struct acpi_table_header *)acpi_table;
>> +			signature = header->signature;
>> +
>> +			if (ACPI_COMPARE_NAME(header->signature, ACPI_SIG_SRAT))
>> +				return header;
>> +		}
>> +		entry += size;
>> +	}
>> +	return NULL;
>> +}
>> +
>> +/*
>> + * According to ACPI table, filter the immvoable memory regions
>					  ^^^^^^^^^
>Typo.
>
>> + * 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;
>> +	int ret;
>> +
>> +	ret = cmdline_find_option("acpi", arg, sizeof(arg));
>> +	if (ret == 3 && !strncmp(arg, "off", 3))
>> +		return;
>> +
>> +	if (!cmdline_find_option_bool("movable_node"))
>> +		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, aborted.\n");
>
>"..., aborting."
>
>> +				break;
>> +			}
>> +		}
>> +		table = (struct acpi_subtable_header *)
>> +			((unsigned long)table + table->length);
>> +	}
>> +	num_immovable_mem = i;
>> +}
>> diff --git a/arch/x86/boot/compressed/kaslr.c b/arch/x86/boot/compressed/kaslr.c
>> index 9ed9709d9947..b251572e77af 100644
>> --- a/arch/x86/boot/compressed/kaslr.c
>> +++ b/arch/x86/boot/compressed/kaslr.c
>> @@ -87,10 +87,6 @@ static unsigned long get_boot_seed(void)
>>  #define KASLR_COMPRESSED_BOOT
>>  #include "../../lib/kaslr.c"
>>  
>> -struct mem_vector {
>> -	unsigned long long start;
>> -	unsigned long long size;
>> -};
>>  
>>  /* Only supporting at most 4 unusable memmap regions with kaslr */
>>  #define MAX_MEMMAP_REGIONS	4
>> diff --git a/arch/x86/boot/compressed/misc.h b/arch/x86/boot/compressed/misc.h
>> index a1d5918765f3..4a3645fda0ed 100644
>> --- a/arch/x86/boot/compressed/misc.h
>> +++ b/arch/x86/boot/compressed/misc.h
>> @@ -77,6 +77,11 @@ void choose_random_location(unsigned long input,
>>  			    unsigned long *output,
>>  			    unsigned long output_size,
>>  			    unsigned long *virt_addr);
>> +struct mem_vector {
>> +	unsigned long long start;
>> +	unsigned long long size;
>> +};
>> +
>>  /* cpuflags.c */
>>  bool has_cpuflag(int flag);
>>  #else
>> @@ -116,3 +121,13 @@ static inline void console_init(void)
>>  void set_sev_encryption_mask(void);
>>  
>>  #endif
>> +
>> +/* acpitb.c */
>> +#ifdef CONFIG_RANDOMIZE_BASE
>> +int num_immovable_mem;
>> +#ifdef CONFIG_MEMORY_HOTREMOVE
>> +/* Store the amount of immovable memory regions */
>
>Above says "regions" but define below is "TABLES". Hmmm?
>
>> +#define ACPI_MAX_TABLES                128
>> +void get_immovable_mem(void);
>> +#endif
>> +#endif
>> -- 
>
>-- 
>Regards/Gruss,
>    Boris.
>
>Good mailing practices for 400: avoid top-posting and trim the reply.
>
>



^ permalink raw reply	[flat|nested] 35+ messages in thread

* Re: [PATCH v11 4/5] x86/boot: Dig out SRAT table from RSDP and find immovable memory
  2018-11-16 11:16   ` Borislav Petkov
  2018-11-19  2:08     ` Chao Fan
@ 2018-11-20  6:18     ` Chao Fan
  1 sibling, 0 replies; 35+ messages in thread
From: Chao Fan @ 2018-11-20  6:18 UTC (permalink / raw)
  To: Borislav Petkov
  Cc: linux-kernel, x86, linux-efi, linux-acpi, tglx, mingo, hpa,
	keescook, bhe, msys.mizuma, indou.takao, caoj.fnst

Hi Boris,

On Fri, Nov 16, 2018 at 12:16:54PM +0100, Borislav Petkov wrote:
>On Mon, Nov 12, 2018 at 05:46:44PM +0800, Chao Fan wrote:
>> To avoid KASLR extracting kernel on movable memory, slove the
>						      ^^^^^
>
>Please introduce a spellchecker into your patch creation workflow.
>
>> conflict between KASLR and movable_node feature, dig the SRAT tables
>
>s/dig/determine/ or "compute SRAT table's address" or so.
>
>Also, replace "dig" with a more suitable verb in all your patches.
>
>> from RSDP pointer. Walk the SRAT tables and store the immovable
>> memory regions in immovable_mem[].
>
>		"... in an array called immovable_mem[]."
>
>> There are three methods to get RSDP pointer: KEXEC condition,
>> EFI confition, BIOS condition.
>
>"condition" is not the right word here.
>
>> If KEXEC add 'acpi_rsdp' to cmdline, use it.
>> Otherwise, parse EFI table for RSDP.
>> Then, search memory for RSDP.
>> 
>> Imitate from ACPI code, based on acpi_os_get_root_pointer().
>> Process: RSDP->RSDT/XSDT->ACPI root table->SRAT.
>
>What?!
>
>This looks like a comment you've added as a note for yourself but not
>part of the final commit message. If you wanna explain the process, then
>write it out in plain english as if you're explaining it to someone who
>doesn't know what you're doing.
>
>> 
>> Signed-off-by: Chao Fan <fanc.fnst@cn.fujitsu.com>
>> ---
>>  arch/x86/boot/compressed/Makefile |   4 +
>>  arch/x86/boot/compressed/acpitb.c | 139 ++++++++++++++++++++++++++++++
>>  arch/x86/boot/compressed/kaslr.c  |   4 -
>>  arch/x86/boot/compressed/misc.h   |  15 ++++
>>  4 files changed, 158 insertions(+), 4 deletions(-)
>> 
>> diff --git a/arch/x86/boot/compressed/Makefile b/arch/x86/boot/compressed/Makefile
>> index 466f66c8a7f8..b51f7629b8ef 100644
>> --- a/arch/x86/boot/compressed/Makefile
>> +++ b/arch/x86/boot/compressed/Makefile
>> @@ -84,6 +84,10 @@ ifdef CONFIG_X86_64
>>  	vmlinux-objs-y += $(obj)/pgtable_64.o
>>  endif
>>  
>> +#if (defined CONFIG_MEMORY_HOTREMOVE) && (defined CONFIG_RANDOMIZE_BASE)
>> +vmlinux-objs-$(CONFIG_RANDOMIZE_BASE) += $(obj)/acpitb.o
>> +#endif
>
>Right, as previously pointed out, this needs that CONFIG_ symbol and
>then you can save yourself most (if not all) of the ifdeffery in the
>rest of the patchset.
>
>> +
>>  $(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/acpitb.c b/arch/x86/boot/compressed/acpitb.c
>> index 5cfb4efa5a19..161f21a7fb3b 100644
>> --- a/arch/x86/boot/compressed/acpitb.c
>> +++ b/arch/x86/boot/compressed/acpitb.c
>> @@ -14,6 +14,11 @@
>>  #define BOOT_STRING
>>  #include "../string.h"
>>  
>> +#ifdef CONFIG_MEMORY_HOTREMOVE
>> +/* Store the immovable memory regions */
>> +struct mem_vector immovable_mem[MAX_NUMNODES*2];
>> +#endif
>> +
>>  /* Search EFI table for RSDP table. */
>>  static void efi_get_rsdp_addr(acpi_physical_address *rsdp_addr)
>>  {
>> @@ -226,3 +231,137 @@ static void get_acpi_rsdp(acpi_physical_address *rsdp_addr)
>>  	}
>>  #endif
>>  }
>> +
>> +/*
>> + * Used to dig RSDP table from EFI table or BIOS.
>> + * If RSDP table found in EFI table, use it. Or search BIOS.
>> + * Based on acpi_os_get_root_pointer().
>> + */
>> +static acpi_physical_address get_rsdp_addr(void)
>> +{
>> +	acpi_physical_address pa = 0;
>> +
>> +	get_acpi_rsdp(&pa);
>> +
>> +	if (!pa)
>> +		efi_get_rsdp_addr(&pa);
>> +
>> +	if (!pa)
>> +		bios_get_rsdp_addr(&pa);
>> +
>> +	return pa;
>> +}
>> +
>> +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;
>> +	bool acpi_use_rsdt = false;
>> +	char *signature;
>> +	char arg[10];
>> +	u8 *entry;
>> +	u32 count;
>> +	u32 size;
>> +	int i, j;
>> +	int ret;
>> +	u32 len;
>> +
>> +	rsdp = (struct acpi_table_rsdp *)get_rsdp_addr();
>> +	if (!rsdp)
>> +		return NULL;
>> +
>> +	ret = cmdline_find_option("acpi", arg, sizeof(arg));
>> +	if (ret == 4 && !strncmp(arg, "rsdt", 4))
>> +		acpi_use_rsdt = true;
>> +
>> +	/* Get RSDT or XSDT from RSDP. */
>> +	if (!acpi_use_rsdt &&
>> +	    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;
>> +	}
>
>Reorganize that code here to get rid of acpi_use_rsdt.
>
>> +
>> +	/* Get ACPI root table from RSDT or XSDT.*/
>> +	header = (struct acpi_table_header *)root_table;
>> +	len = header->length;
>
>No checking of that header pointer before dereffing it?
>
>If it is NUL, that gives you a very nasty bug to try to debug in the
>early code.
>
>> +	count = (u32)((len - sizeof(struct acpi_table_header)) / size);
>
>Uuh, no checking for count wrapping around here due to wrong len? That
>would give you a *lot* of looping below if it wraps.
>
>IOW, you need to verify all those values before doing arithmetic with
>them - it is early code and it is BIOS - there's no trusting it.
>
>Also, it is not "count" but "num_entries" or so.
>
>> +	entry = ACPI_ADD_PTR(u8, header, sizeof(struct acpi_table_header));
>> +
>> +	for (i = 0; i < count; i++) {
>
>That variable i is not needed, right?
>
>	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) {
>
>Now can acpi_table be NUL here?

In iacpi_tb_parse_root_table() of drivers/acpi/acpica/tbutils.c:
                /* Skip NULL entries in RSDT/XSDT */

                if (!address) {
                        goto next_table;
                }
So I think the table might be NULL and did the similar operation.

Thanks,
Chao Fan

>
>> +			header = (struct acpi_table_header *)acpi_table;
>> +			signature = header->signature;
>> +
>> +			if (ACPI_COMPARE_NAME(header->signature, ACPI_SIG_SRAT))
>> +				return header;
>> +		}
>> +		entry += size;
>> +	}
>> +	return NULL;
>> +}
>> +
>> +/*
>> + * According to ACPI table, filter the immvoable memory regions
>					  ^^^^^^^^^
>Typo.
>
>> + * 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;
>> +	int ret;
>> +
>> +	ret = cmdline_find_option("acpi", arg, sizeof(arg));
>> +	if (ret == 3 && !strncmp(arg, "off", 3))
>> +		return;
>> +
>> +	if (!cmdline_find_option_bool("movable_node"))
>> +		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, aborted.\n");
>
>"..., aborting."
>
>> +				break;
>> +			}
>> +		}
>> +		table = (struct acpi_subtable_header *)
>> +			((unsigned long)table + table->length);
>> +	}
>> +	num_immovable_mem = i;
>> +}
>> diff --git a/arch/x86/boot/compressed/kaslr.c b/arch/x86/boot/compressed/kaslr.c
>> index 9ed9709d9947..b251572e77af 100644
>> --- a/arch/x86/boot/compressed/kaslr.c
>> +++ b/arch/x86/boot/compressed/kaslr.c
>> @@ -87,10 +87,6 @@ static unsigned long get_boot_seed(void)
>>  #define KASLR_COMPRESSED_BOOT
>>  #include "../../lib/kaslr.c"
>>  
>> -struct mem_vector {
>> -	unsigned long long start;
>> -	unsigned long long size;
>> -};
>>  
>>  /* Only supporting at most 4 unusable memmap regions with kaslr */
>>  #define MAX_MEMMAP_REGIONS	4
>> diff --git a/arch/x86/boot/compressed/misc.h b/arch/x86/boot/compressed/misc.h
>> index a1d5918765f3..4a3645fda0ed 100644
>> --- a/arch/x86/boot/compressed/misc.h
>> +++ b/arch/x86/boot/compressed/misc.h
>> @@ -77,6 +77,11 @@ void choose_random_location(unsigned long input,
>>  			    unsigned long *output,
>>  			    unsigned long output_size,
>>  			    unsigned long *virt_addr);
>> +struct mem_vector {
>> +	unsigned long long start;
>> +	unsigned long long size;
>> +};
>> +
>>  /* cpuflags.c */
>>  bool has_cpuflag(int flag);
>>  #else
>> @@ -116,3 +121,13 @@ static inline void console_init(void)
>>  void set_sev_encryption_mask(void);
>>  
>>  #endif
>> +
>> +/* acpitb.c */
>> +#ifdef CONFIG_RANDOMIZE_BASE
>> +int num_immovable_mem;
>> +#ifdef CONFIG_MEMORY_HOTREMOVE
>> +/* Store the amount of immovable memory regions */
>
>Above says "regions" but define below is "TABLES". Hmmm?
>
>> +#define ACPI_MAX_TABLES                128
>> +void get_immovable_mem(void);
>> +#endif
>> +#endif
>> -- 
>
>-- 
>Regards/Gruss,
>    Boris.
>
>Good mailing practices for 400: avoid top-posting and trim the reply.
>
>



^ permalink raw reply	[flat|nested] 35+ messages in thread

end of thread, other threads:[~2018-11-20  6:18 UTC | newest]

Thread overview: 35+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-11-12  9:46 [PATCH v11 0/5] x86/boot/KASLR: Parse ACPI table and limit kaslr in immovable memory Chao Fan
2018-11-12  9:46 ` [PATCH v11 1/5] x86/boot: Add efi_get_rsdp_addr() to dig out RSDP from EFI table Chao Fan
2018-11-12 14:54   ` Borislav Petkov
2018-11-13  1:57     ` Chao Fan
2018-11-12  9:46 ` [PATCH v11 2/5] x86/boot: Add bios_get_rsdp_addr() to search RSDP in memory Chao Fan
2018-11-12 15:27   ` Borislav Petkov
2018-11-13  2:10     ` Chao Fan
2018-11-13 10:09       ` Borislav Petkov
2018-11-12  9:46 ` [PATCH v11 3/5] x86/boot: Add get_acpi_rsdp() to parse RSDP in cmdlien from kexec Chao Fan
2018-11-12  9:50   ` Chao Fan
2018-11-12 17:43   ` Masayoshi Mizuma
2018-11-13  2:12     ` Chao Fan
2018-11-13 16:11       ` Masayoshi Mizuma
2018-11-13 17:22         ` Borislav Petkov
2018-11-13 17:54         ` Borislav Petkov
2018-11-13 20:06           ` Masayoshi Mizuma
2018-11-13 21:51             ` Borislav Petkov
2018-11-14  6:12               ` Chao Fan
2018-11-14 18:30                 ` Borislav Petkov
2018-11-19  1:16                   ` Chao Fan
2018-11-13 17:51   ` Borislav Petkov
2018-11-14  1:54     ` Chao Fan
2018-11-14  1:59       ` Chao Fan
2018-11-14 18:33       ` Borislav Petkov
2018-11-12  9:46 ` [PATCH v11 4/5] x86/boot: Dig out SRAT table from RSDP and find immovable memory Chao Fan
2018-11-12 20:52   ` Masayoshi Mizuma
2018-11-13  2:43     ` Chao Fan
2018-11-12 21:51   ` Masayoshi Mizuma
2018-11-13  2:45     ` Chao Fan
2018-11-16 11:16   ` Borislav Petkov
2018-11-19  2:08     ` Chao Fan
2018-11-20  6:18     ` Chao Fan
2018-11-12  9:46 ` [PATCH v11 5/5] x86/boot/KASLR: Walk srat tables to filter " Chao Fan
2018-11-16 13:50   ` Borislav Petkov
2018-11-19  1:31     ` Chao Fan

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).