Re: [PATCH v11 4/5] x86/boot: Dig out SRAT table from RSDP and find immovable memory
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
>> ---
>> 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();
>> +
>> +if (!pa)
>> +efi_get_rsdp_addr();
>> +
>> +if (!pa)
>> +bios_get_rsdp_addr();
>> +
>> +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
Re: [PATCH v11 5/5] x86/boot/KASLR: Walk srat tables to filter immovable memory
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
>> ---
>> 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.
>
>
Re: [PATCH v11 3/5] x86/boot: Add get_acpi_rsdp() to parse RSDP in cmdlien from kexec
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.
>
>
Re: [GIT PULL] EFI fixes
The pull request you sent on Sat, 17 Nov 2018 11:51:15 +0100: > git://git.kernel.org/pub/scm/linux/kernel/git/tip/tip.git efi-urgent-for-linus has been merged into torvalds/linux.git: https://git.kernel.org/torvalds/c/743a4863fddc4fdd591e1cbf4157e981a71b0f09 Thank you! -- Deet-doot-dot, I am a bot. https://korg.wiki.kernel.org/userdoc/prtracker
