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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20181217172510.GA90818@gmail.com>
Date:   Mon, 17 Dec 2018 18:25:10 +0100
From:   Ingo Molnar <mingo@...nel.org>
To:     Chao Fan <fanc.fnst@...fujitsu.com>
Cc:     linux-kernel@...r.kernel.org, x86@...nel.org, bp@...en8.de,
        tglx@...utronix.de, mingo@...hat.com, hpa@...or.com,
        keescook@...omium.org, bhe@...hat.com, msys.mizuma@...il.com,
        indou.takao@...fujitsu.com, caoj.fnst@...fujitsu.com
Subject: Re: [PATCH v14 1/5] x86/boot: Introduce get_acpi_rsdp() to parse
 RSDP in cmdline from KEXEC


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

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

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

Some minor nits:

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

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

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

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

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

Thanks,

	Ingo