Re: [PATCH v8 0/3] x86/boot/KASLR: Parse ACPI table and limit kaslr in immovable memory
From: Masayoshi Mizuma
Date: Fri Feb 08 2019 - 19:24:14 EST
Hi Boris,
Thank you for your review.
On Fri, Feb 08, 2019 at 07:26:00PM +0100, Borislav Petkov wrote:
> On Tue, Feb 05, 2019 at 10:05:16AM -0500, Masayoshi Mizuma wrote:
> > From: Masayoshi Mizuma <m.mizuma@xxxxxxxxxxxxxx>
> > Date: Tue, 5 Feb 2019 10:00:59 -0500
> > Subject: [PATCH] x86/mm: Introduce adjustment the padding size for KASLR
>
> "Adjust the padding size for KASLR"
>
> > If the physical memory layout has huge space for hotplug, the padding
> > used for the physical memory mapping section is not enough.
> > So, such system may crash while memory hot-adding on KASLR enabled system.
>
> Crash why?
>
> Why is the padding not enough?
>
> > For example, SRAT has the following layout, the maximum possible memory
> > size is 32TB, and the memory is installed as 2TB actually, then the padding
> > size should set 30TB (== possible memory size - actual memory size).
> >
> > SRAT: Node 3 PXM 7 [mem 0x1c0000000000-0x1fffffffffff] hotplug
>
> What is that supposed to exemplify: that range is 3T not 2 and that
> range start is not at 2T but 28T. So I have absolutely no clue what
> you're trying to explain here.
>
> Please go back, take your time and structure your commit message like
> this:
>
> Problem is A.
>
> It happens because of B.
>
> Fix it by doing C.
>
> (Potentially do D).
>
> For more detailed info, see
> Documentation/process/submitting-patches.rst, Section "2) Describe your
> changes".
Got it, thanks.
>
> > This patch introduces adjustment the padding size if the default
>
> Avoid having "This patch" or "This commit" in the commit message. It is
> tautologically useless.
>
> Also, do
>
> $ git grep 'This patch' Documentation/process
>
> for more details.
Thanks. I see.
>
> > padding size isn't enough.
> >
> > Signed-off-by: Masayoshi Mizuma <m.mizuma@xxxxxxxxxxxxxx>
> > ---
> > Documentation/x86/zero-page.txt | 1 +
> > arch/x86/boot/compressed/acpi.c | 19 +++++++++++++++----
> > arch/x86/include/uapi/asm/bootparam.h | 2 +-
> > arch/x86/mm/kaslr.c | 26 +++++++++++++++++++++++++-
> > 4 files changed, 42 insertions(+), 6 deletions(-)
> >
> > diff --git a/Documentation/x86/zero-page.txt b/Documentation/x86/zero-page.txt
> > index 68aed077f..343fe1a90 100644
> > --- a/Documentation/x86/zero-page.txt
> > +++ b/Documentation/x86/zero-page.txt
> > @@ -15,6 +15,7 @@ Offset Proto Name Meaning
> > 058/008 ALL tboot_addr Physical address of tboot shared page
> > 060/010 ALL ist_info Intel SpeedStep (IST) BIOS support information
> > (struct ist_info)
> > +078/010 ALL possible_mem_addr The possible maximum physical memory address.
>
> Why isn't this called max_phys_addr then?
>
> Also, please explain what it means at the end of this file.
>
> > 080/010 ALL hd0_info hd0 disk parameter, OBSOLETE!!
> > 090/010 ALL hd1_info hd1 disk parameter, OBSOLETE!!
> > 0A0/010 ALL sys_desc_table System description table (struct sys_desc_table),
> > diff --git a/arch/x86/boot/compressed/acpi.c b/arch/x86/boot/compressed/acpi.c
> > index c5a949335..7dd61b943 100644
> > --- a/arch/x86/boot/compressed/acpi.c
> > +++ b/arch/x86/boot/compressed/acpi.c
> > @@ -288,6 +288,7 @@ int count_immovable_mem_regions(void)
> > struct acpi_subtable_header *sub_table;
> > struct acpi_table_header *table_header;
> > char arg[MAX_ACPI_ARG_LENGTH];
> > + unsigned long long possible_addr, max_possible_addr = 0;
> > int num = 0;
> >
> > if (cmdline_find_option("acpi", arg, sizeof(arg)) == 3 &&
> > @@ -308,10 +309,19 @@ int count_immovable_mem_regions(void)
> > struct acpi_srat_mem_affinity *ma;
> >
> > ma = (struct acpi_srat_mem_affinity *)sub_table;
> > - if (!(ma->flags & ACPI_SRAT_MEM_HOT_PLUGGABLE) && ma->length) {
> > - immovable_mem[num].start = ma->base_address;
> > - immovable_mem[num].size = ma->length;
> > - num++;
> > + if (ma->length) {
> > + if (ma->flags & ACPI_SRAT_MEM_HOT_PLUGGABLE) {
> > + possible_addr =
> > + ma->base_address + ma->length;
> > + if (possible_addr > max_possible_addr)
> > + max_possible_addr =
> > + possible_addr;
> > + } else {
> > + immovable_mem[num].start =
> > + ma->base_address;
> > + immovable_mem[num].size = ma->length;
> > + num++;
> > + }
>
> This piece has some ugly linebreaks which makes it impossible to read.
> Perhaps because the variable names you're adding are too long.
>
> You can carve it out in a separate function, for example.
Thanks. I will add a separate function like as:
static void subtable_parse(struct acpi_subtable_header *sub_table, int *num,
unsigned long *max_addr)
{
struct acpi_srat_mem_affinity *ma;
unsigned long addr;
ma = (struct acpi_srat_mem_affinity *)sub_table;
if (ma->length) {
if (ma->flags & ACPI_SRAT_MEM_HOT_PLUGGABLE) {
addr = ma->base_address + ma->length;
if (addr > *max_addr)
*max_addr = addr;
} else {
immovable_mem[*num].start = ma->base_address;
immovable_mem[*num].size = ma->length;
(*num)++;
}
}
}
>
> > if (num >= MAX_NUMNODES*2) {
> > @@ -320,6 +330,7 @@ int count_immovable_mem_regions(void)
> > }
> > }
> > table += sub_table->length;
> > + boot_params->possible_mem_addr = max_possible_addr;
>
> This assignment is inside the loop and happens potentially a bunch of
> times. Why?
I will move the assigment out of the loop, thanks.
>
> > }
> > return num;
> > }
> > diff --git a/arch/x86/include/uapi/asm/bootparam.h b/arch/x86/include/uapi/asm/bootparam.h
> > index 60733f137..5b64b606e 100644
> > --- a/arch/x86/include/uapi/asm/bootparam.h
> > +++ b/arch/x86/include/uapi/asm/bootparam.h
> > @@ -156,7 +156,7 @@ struct boot_params {
> > __u64 tboot_addr; /* 0x058 */
> > struct ist_info ist_info; /* 0x060 */
> > __u64 acpi_rsdp_addr; /* 0x070 */
> > - __u8 _pad3[8]; /* 0x078 */
> > + __u64 possible_mem_addr; /* 0x078 */
> > __u8 hd0_info[16]; /* obsolete! */ /* 0x080 */
> > __u8 hd1_info[16]; /* obsolete! */ /* 0x090 */
> > struct sys_desc_table sys_desc_table; /* obsolete! */ /* 0x0a0 */
> > diff --git a/arch/x86/mm/kaslr.c b/arch/x86/mm/kaslr.c
> > index 3f452ffed..71fc28570 100644
> > --- a/arch/x86/mm/kaslr.c
> > +++ b/arch/x86/mm/kaslr.c
> > @@ -70,6 +70,30 @@ static inline bool kaslr_memory_enabled(void)
> > return kaslr_enabled() && !IS_ENABLED(CONFIG_KASAN);
> > }
> >
> > +static unsigned int __init kaslr_padding(void)
> > +{
> > + unsigned int rand_mem_physical_padding =
> > + CONFIG_RANDOMIZE_MEMORY_PHYSICAL_PADDING;
> > +#ifdef CONFIG_MEMORY_HOTPLUG
> > + unsigned long long max_possible_phys, max_actual_phys, threshold;
> > +
> > + if (!boot_params.possible_mem_addr)
> > + goto out;
> > +
> > + max_actual_phys = roundup(PFN_PHYS(max_pfn), 1ULL << TB_SHIFT);
> > + max_possible_phys = roundup(boot_params.possible_mem_addr,
> > + 1ULL << TB_SHIFT);
> > + threshold = max_actual_phys +
> > + ((unsigned long long)rand_mem_physical_padding << TB_SHIFT);
> > +
> > + if (max_possible_phys > threshold)
> > + rand_mem_physical_padding =
> > + (max_possible_phys - max_actual_phys) >> TB_SHIFT;
> > +out:
> > +#endif
> > + return rand_mem_physical_padding;
>
> Same problem: local variables with unnecessarily long names make the
> code hard to read. Pls shorten.
>
> Also, the types in that function are a total mess. An unsigned int which
> you cast to unsigned long long and return an unsigned int again to add
> into a sum with unsigned longs. Are you selecting the types randomly?
> Why aren't you using plain unsigned longs like the rest of the file does
> with memory addresses?
Sorry. I will unify the type as unsinged long.
>
> Also, this function could have a comment above it explaining what all
> that threshold and actual and possible address arithmetic is supposed to
> do.
Got it.
Thanks!
Masa