Re: [PATCH v8 1/3] x86/boot: Add acpitb.c to parse acpi tables

From: Chao Fan
Date: Thu Oct 11 2018 - 21:57:00 EST

On Thu, Oct 11, 2018 at 12:57:08PM +0200, Borislav Petkov wrote:
>On Wed, Oct 10, 2018 at 04:41:17PM +0800, Chao Fan wrote:
>> There is a bug 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. So dig SRAT table from ACPI tables to get memory information.
>>
>> Imitate the ACPI code of parsing ACPI tables to dig and read ACPI
>> tables. Since some operations are not needed here, functions are
>> simplified. Functions will be used to dig only SRAT tables to get
>> information of memory, so that KASLR can the memory in immovable node.
>>
>> And also, these functions won't influence the initialization of
>> ACPI after start_kernel().
>>
>> Since use physical address directely, so acpi_os_map_memory()
>> and acpi_os_unmap_memory() are not needed.
>>
>> Signed-off-by: Chao Fan <fanc.fnst@xxxxxxxxxxxxxx>
>> ---
>> arch/x86/boot/compressed/Makefile | 2 +
>> arch/x86/boot/compressed/acpitb.c | 405 ++++++++++++++++++++++++++++++
>> arch/x86/boot/compressed/misc.h | 8 +
>> 3 files changed, 415 insertions(+)
>> create mode 100644 arch/x86/boot/compressed/acpitb.c
>>
>> diff --git a/arch/x86/boot/compressed/Makefile b/arch/x86/boot/compressed/Makefile
>> index 28764dacf018..1609e4efcaed 100644
>> --- a/arch/x86/boot/compressed/Makefile
>> +++ b/arch/x86/boot/compressed/Makefile
>> @@ -83,6 +83,8 @@ ifdef CONFIG_X86_64
>> vmlinux-objs-y += $(obj)/pgtable_64.o
>> endif
>>
>> +vmlinux-objs-$(CONFIG_RANDOMIZE_BASE) += $(obj)/acpitb.o
>

So many thanks for your review.

>This should be CONFIG_MEMORY_HOTREMOVE *and* CONFIG_RANDOMIZE_BASE.
>Otherwise we don't need all that code.

Thanks, I will add CONFIG_RANDOMIZE_BASE.
In V7, I ever added CONFIG_MEMORY_HOTREMOVE, then I need add in kaslr.c:

+#ifdef CONFIG_MEMORY_HOTREMOVE
+ /* Mark the immovable regions we need to choose */
+ get_immovable_mem();
+#endif

Then in V8, follow Kees Cook's suggestion, change the #ifdef to the
definition of get_immovable_mem() in acpitb.c
So I drop the CONFIG_MEMORY_HOTREMOVE.

I will splite it to more patch in next version.

Thanks,
Chao Fan

>
>> $(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
>> new file mode 100644
>> index 000000000000..6b869e3f9780
>> --- /dev/null
>> +++ b/arch/x86/boot/compressed/acpitb.c
>> @@ -0,0 +1,405 @@
>> +// 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>
>> +
>> +extern unsigned long get_cmd_line_ptr(void);
>> +
>> +#define STATIC
>> +#include <linux/decompress/mm.h>
>> +
>> +#ifdef CONFIG_MEMORY_HOTREMOVE
>> +struct mem_vector {
>> + unsigned long long start;
>> + unsigned long long size;
>> +};
>> +/* Store the immovable memory regions */
>> +struct mem_vector immovable_mem[MAX_NUMNODES*2];
>> +#endif
>> +
>> +#ifdef CONFIG_EFI
>> +/* Search EFI table for rsdp table. */
>> +static bool efi_get_rsdp_addr(acpi_physical_address *rsdp_addr)
>> +{
>> + efi_system_table_t *systab;
>> + bool find_rsdp = false;
>> + 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 false;
>> + }
>> +
>> + /* Get systab from boot params. Based on efi_init(). */
>> +#ifdef CONFIG_X86_32
>
>Why the efi_64 detection above but the ifdeffery here? Why not test
>efi_64 instead?

The efi_64 is used for the efi table size.

>
>> + if (e->efi_systab_hi || e->efi_memmap_hi) {
>> + debug_putstr("Table located above 4GB, disabling EFI.\n");
>
>Are you disabling EFI? Where?
>
>Ah, I see, this code is copied from arch/x86/platform/efi/efi.c.
>
>So when copying, fix the user-visible strings too.

Will change it.

>
>> + return false;
>> + }
>> + systab = (efi_system_table_t *)e->efi_systab;
>> +#else
>> + systab = (efi_system_table_t *)(
>> + e->efi_systab | ((__u64)e->efi_systab_hi<<32));
>> +#endif
>> +
>> + if (systab == NULL)
>
> if (!systab)
>
>Fix all other occurrences.
>
>> + return false;
>> +
>> + /*
>> + * Get EFI tables from systab. Based on efi_config_init() and
>> + * efi_config_parse_tables(). Only dig the config_table.
>
> dig out
>
>> + */
>> + 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, disabling EFI.\n");
>
>Fix that.
>
>> + return false;
>> + }
>> +#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 and return true.
>
>You don't have to say "return true" in the comment - that is in the code
>already.
>
>Also, this function - just like get_acpi_rsdp() doesn't need to return
>bool but the pa directly.
>
>> + * If ACPI20 table not found, but ACPI table found,
>> + * use the ACPI table and return true.
>> + * If neither ACPI table nor ACPI20 table found,
>> + * return false.
>> + */
>> + if (!(efi_guidcmp(guid, ACPI_TABLE_GUID))) {
>> + *rsdp_addr = (acpi_physical_address)table;
>> + find_rsdp = true;
>> + } else if (!(efi_guidcmp(guid, ACPI_20_TABLE_GUID))) {
>> + *rsdp_addr = (acpi_physical_address)table;
>> + return true;
>> + }
>> + }
>> + return find_rsdp;
>> +}
>> +#else
>> +static bool efi_get_rsdp_addr(acpi_physical_address *rsdp_addr)
>> +{
>> + return false;
>> +}
>> +#endif
>
>Instead of doing this, move the ifdef inside the function:
>
>static bool efi_get_rsdp_addr(acpi_physical_address *rsdp_addr)
>{
>#ifdef CONFIG_EFI
>
> /* function body */
>#endif
>}
>
>
>> +
>> +static u8 checksum(u8 *buffer, u32 length)
>
>compute_checksum(...)
>
>> +{
>> + 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_address, u32 length)
>> +{
>> + struct acpi_table_rsdp *rsdp;
>> + u8 *end_address;
>> + u8 *mem_rover;
>> +
>> + end_address = start_address + length;
>> +
>> + /* Search from given start address for the requested length */
>> + for (mem_rover = start_address; mem_rover < end_address;
>> + mem_rover += ACPI_RSDP_SCAN_STEP) {
>
>Shorten those variable names so that the loop fits on 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 *)mem_rover;
>> +
>> + /* Nope, BAD Signature */
>> + if (!ACPI_VALIDATE_RSDP_SIG(rsdp->signature))
>> + continue;
>> +
>> + /* Check the standard checksum */
>> + if (checksum((u8 *) rsdp, ACPI_RSDP_CHECKSUM_LENGTH) != 0)
>
>No need for "!= 0" at the end. Fix all other tests too.
>
>> + continue;
>> +
>> + /* Check extended checksum if table version >= 2 */
>> + if ((rsdp->revision >= 2) &&
>> + (checksum((u8 *) rsdp, ACPI_RSDP_XCHECKSUM_LENGTH) != 0))
>> + continue;
>> +
>> + /* Sig and checksum valid, we have found a real RSDP */
>> + return mem_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;
>> + return;
>
>We will return anyway, without that statement. :)
>
>> + }
>> +}
>> +
>> +#ifdef CONFIG_KEXEC
>> +static bool get_acpi_rsdp(acpi_physical_address *rsdp_addr)
>> +{
>> + char *args = (char *)get_cmd_line_ptr();
>> + size_t len = strlen((char *)args);
>> + char *tmp_cmdline, *param, *val;
>> + unsigned long long addr = 0;
>> + char *endptr;
>> +
>> + if (!strstr(args, "acpi_rsdp="))
>> + return false;
>> +
>> + tmp_cmdline = malloc(len+1);
>> + if (!tmp_cmdline)
>> + error("Failed to allocate space for tmp_cmdline");
>
>Why do you even need to allocate a tmp cmdline?
>
>Ah, I see what you've done - you've copied handle_mem_options() in
>kaslr.c. Well no, not really.
>
>That functionality needs to get extracted into a separate facility. Oh
>look, there's arch/x86/boot/compressed/cmdline.c which is begging to get
>extended.
>
>:-)
>
>> +
>> + memcpy(tmp_cmdline, args, len);
>> + tmp_cmdline[len] = 0;
>> + args = tmp_cmdline;
>> +
>> + args = skip_spaces(args);
>> +
>> + while (*args) {
>> + args = next_arg(args, &param, &val);
>> +
>> + if (!val && strcmp(param, "--") == 0) {
>> + warn("Only '--' specified in cmdline");
>> + free(tmp_cmdline);
>> + return false;
>> + }
>> +
>> + if (!strcmp(param, "acpi_rsdp")) {
>> + addr = simple_strtoull(val, &endptr, 0);
>
>WARNING: simple_strtoull is obsolete, use kstrtoull instead
>#321: FILE: arch/x86/boot/compressed/acpitb.c:262:
>+ addr = simple_strtoull(val, &endptr, 0);
>
>
>Please integrate scripts/checkpatch.pl into your patch creation
>workflow. Some of the warnings/errors *actually* make sense.
>
>> +
>> + if (addr == 0)
>> + return false;
>> +
>> + *rsdp_addr = (acpi_physical_address)addr;
>> + return true;
>> + }
>> + }
>> + return false;
>> +}
>> +#else
>> +static bool get_acpi_rsdp(acpi_physical_address *rsdp_addr)
>> +{
>> + return false;
>> +}
>> +#endif
>> +
>> +/*
>> + * Used to dig rsdp table from EFI table or BIOS.
>
>Write "rsdp" in all caps in all comments.
>
>> + * 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;
>> + bool status = false;
>> +
>> + status = get_acpi_rsdp(&pa);
>
>Why does this function return bool if pa == 0 is already an invalid
>address. You don't need the initialization to 0 above either.
>
>> +
>> + if (!status || pa == 0)
>
> if (!status || !pa)
>
>Fix all other tests.
>
>> + status = efi_get_rsdp_addr(&pa);
>> +
>> + if (!status || pa == 0)
>> + bios_get_rsdp_addr(&pa);
>> +
>> + return pa;
>> +}
>> +
>> +static struct acpi_table_header *get_acpi_srat_table(void)
>> +{
>> + char *args = (char *)get_cmd_line_ptr();
>> + acpi_physical_address acpi_table;
>> + acpi_physical_address root_table;
>> + struct acpi_table_header *header;
>> + struct acpi_table_rsdp *rsdp;
>> + char *signature;
>> + u8 *entry;
>> + u32 count;
>> + u32 size;
>> + int i, j;
>> + u32 len;
>> +
>> + rsdp = (struct acpi_table_rsdp *)get_rsdp_addr();
>> + if (!rsdp)
>> + return NULL;
>> +
>> + /* Get rsdt or xsdt from rsdp. */
>> + if (!strstr(args, "acpi=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;
>> + }
>
>Please rework the cmdline parsing so that the functions can call helpers
>only.
>
>> +
>> + /* 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 (!strncmp(signature, "SRAT", 4))
>> + return header;
>> + }
>> + entry += size;
>> + }
>> + return NULL;
>> +}
>> +
>> +#ifdef CONFIG_MEMORY_HOTREMOVE
>> +/*
>> + * According to ACPI table, filter the immvoable memory regions
>> + * and store them in immovable_mem[].
>> + */
>> +void get_immovable_mem(void)
>> +{
>> + char *args = (char *)get_cmd_line_ptr();
>> + struct acpi_table_header *table_header;
>> + struct acpi_subtable_header *table;
>> + struct acpi_srat_mem_affinity *ma;
>> + unsigned long table_end;
>> + int i = 0;
>> +
>> + if (!strstr(args, "movable_node") || strstr(args, "acpi=off"))
>> + return;
>
>Ditto.
>
>> +
>> + 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) + table->length < table_end) {
>> + if (table->type == 1) {
>> + 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)
>> + break;
>> + }
>> + table = (struct acpi_subtable_header *)
>> + ((unsigned long)table + table->length);
>> + }
>> + num_immovable_mem = i;
>> +}
>> +#else
>> +void get_immovable_mem(void)
>> +{
>> +}
>> +#endif
>
>This patch is a pain to review - pls split it into patches adding:
>
>* get_acpi_rsdp
>* efi_get_rsdp_addr
>* bios_get_rsdp_addr
>
>and the needed functionality.
>
>As a prepatch refactor the cmdline parsing pls.
>
>Thx.
>
>--
>Regards/Gruss,
> Boris.
>
>Good mailing practices for 400: avoid top-posting and trim the reply.
>
>