Re: [PATCH v8 0/3] x86/boot/KASLR: Parse ACPI table and limit kaslr in immovable memory
On Thu, Oct 11, 2018 at 08:29:55AM +0800, Baoquan He wrote: >On 10/10/18 at 03:44pm, Masayoshi Mizuma wrote: >> On Wed, Oct 10, 2018 at 05:30:57PM +0800, Baoquan He wrote: >> > On 10/10/18 at 11:19am, Borislav Petkov wrote: >> > > On Wed, Oct 10, 2018 at 11:14:26AM +0200, Thomas Gleixner wrote: >> > > > Yes, it's different, but if the SRAT information is available early, >> > > > then >> > > > the command line parameter can go away because then the required >> > > > information for Masa's problem is available as well. >> > > >> > > Exactly. And I'd prefer we delayed the command line parameter until we >> > > figure out we really need it and not expose it to upstream and then >> > > remove it shortly after. >> > > >> > > So I'd suggest we move Masa's patches to a separate branch and not send >> > > it up this round. >> > >> > Yes, sounds more reasonable if we can reuse functions in Chao's patch 1/3 >> > to solve the padding issue. >> >> Thanks for your comments! Yes, immovable_mem[num_immovable_mem] in Chao's >> patch may be useful for calculating the padding size. If so, we don't >> need the new kernel parameter. It's nice! >> >> Do you happen to have ideas how we access immovable_mem[num_immovable_mem] >> from arch/x86/mm/kaslr.c ? It is located to arch/x86/boot/compressed/*, so >> I suppose it is not easy to access it... >> I would appreciate if you could give some advice. > >Hmm, they are living in different life cycle and space. So we can only >reuse the code from Chao's patch, but not the variable. Means we need >go through the SRAT accessing again in arch/x86/mm/kaslr.c and fill >immovable_mem[num_immovable_mem] for mm/kaslr.c usage, if we decide to >do like that. Reading three times is redundant, but reading two times is needed. Becasue the ACPI code run very stable, but we need more information before that. As for Masa's issue, I am wondering whether we can tranfer the information or only the address of SRAT table from compressed period to the period after start_kernel(). Thanks, Chao Fan > >Thanks >Baoquan > >
Re: [PATCH v8 0/3] x86/boot/KASLR: Parse ACPI table and limit kaslr in immovable memory
On 10/10/18 at 03:44pm, Masayoshi Mizuma wrote: > On Wed, Oct 10, 2018 at 05:30:57PM +0800, Baoquan He wrote: > > On 10/10/18 at 11:19am, Borislav Petkov wrote: > > > On Wed, Oct 10, 2018 at 11:14:26AM +0200, Thomas Gleixner wrote: > > > > Yes, it's different, but if the SRAT information is available early, > > > > then > > > > the command line parameter can go away because then the required > > > > information for Masa's problem is available as well. > > > > > > Exactly. And I'd prefer we delayed the command line parameter until we > > > figure out we really need it and not expose it to upstream and then > > > remove it shortly after. > > > > > > So I'd suggest we move Masa's patches to a separate branch and not send > > > it up this round. > > > > Yes, sounds more reasonable if we can reuse functions in Chao's patch 1/3 > > to solve the padding issue. > > Thanks for your comments! Yes, immovable_mem[num_immovable_mem] in Chao's > patch may be useful for calculating the padding size. If so, we don't > need the new kernel parameter. It's nice! > > Do you happen to have ideas how we access immovable_mem[num_immovable_mem] > from arch/x86/mm/kaslr.c ? It is located to arch/x86/boot/compressed/*, so > I suppose it is not easy to access it... > I would appreciate if you could give some advice. Hmm, they are living in different life cycle and space. So we can only reuse the code from Chao's patch, but not the variable. Means we need go through the SRAT accessing again in arch/x86/mm/kaslr.c and fill immovable_mem[num_immovable_mem] for mm/kaslr.c usage, if we decide to do like that. Thanks Baoquan
Re: [PATCH v8 0/3] x86/boot/KASLR: Parse ACPI table and limit kaslr in immovable memory
On Wed, Oct 10, 2018 at 05:30:57PM +0800, Baoquan He wrote: > On 10/10/18 at 11:19am, Borislav Petkov wrote: > > On Wed, Oct 10, 2018 at 11:14:26AM +0200, Thomas Gleixner wrote: > > > Yes, it's different, but if the SRAT information is available early, then > > > the command line parameter can go away because then the required > > > information for Masa's problem is available as well. > > > > Exactly. And I'd prefer we delayed the command line parameter until we > > figure out we really need it and not expose it to upstream and then > > remove it shortly after. > > > > So I'd suggest we move Masa's patches to a separate branch and not send > > it up this round. > > Yes, sounds more reasonable if we can reuse functions in Chao's patch 1/3 > to solve the padding issue. Thanks for your comments! Yes, immovable_mem[num_immovable_mem] in Chao's patch may be useful for calculating the padding size. If so, we don't need the new kernel parameter. It's nice! Do you happen to have ideas how we access immovable_mem[num_immovable_mem] from arch/x86/mm/kaslr.c ? It is located to arch/x86/boot/compressed/*, so I suppose it is not easy to access it... I would appreciate if you could give some advice. Thanks! Masa
Re: [PATCH v8 0/3] x86/boot/KASLR: Parse ACPI table and limit kaslr in immovable memory
On Wed, Oct 10, 2018 at 04:41:16PM +0800, Chao Fan wrote: > 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 copy the codes > and functions to 'compressed/' directory, so that kaslr won't > influence the initialization of ACPI. You say "copy". I'm still about to look at the code but can those functions be carved out in a separate compilation unit which ACPI *and* KASLR can both link with so that there's no duplication? -- Regards/Gruss, Boris. Good mailing practices for 400: avoid top-posting and trim the reply.
Re: [PATCH v8 0/3] x86/boot/KASLR: Parse ACPI table and limit kaslr in immovable memory
On 10/10/18 at 11:19am, Borislav Petkov wrote: > On Wed, Oct 10, 2018 at 11:14:26AM +0200, Thomas Gleixner wrote: > > Yes, it's different, but if the SRAT information is available early, then > > the command line parameter can go away because then the required > > information for Masa's problem is available as well. > > Exactly. And I'd prefer we delayed the command line parameter until we > figure out we really need it and not expose it to upstream and then > remove it shortly after. > > So I'd suggest we move Masa's patches to a separate branch and not send > it up this round. Yes, sounds more reasonable if we can reuse functions in Chao's patch 1/3 to solve the padding issue. Thanks Baoquan
Re: [PATCH v8 0/3] x86/boot/KASLR: Parse ACPI table and limit kaslr in immovable memory
On 10/10/18 at 05:12pm, Chao Fan wrote: > On Wed, Oct 10, 2018 at 05:06:20PM +0800, Baoquan He wrote: > >Hi Boris, > > > >On 10/10/18 at 10:59am, Borislav Petkov wrote: > >> ... and we just picked up > >> > >> https://lkml.kernel.org/r/20181001140843.26137-1-msys.miz...@gmail.com > >> > >> and without having looked at the rest of your stuff, if people accept > >> your solution, we don't need the silly parameter anymore, right? > >> > >> Which means, we should not rush the whole thing yet until the whole > >> KASLR vs movable memory gets solved properly. > > > >Masa's patches solves the problem in memory region KASLR which later hot > >added memory may be big than the default padding 10 TB. > > > >Chao's patches is trying to fix a conflict between 'movable_node' and > >kernel text KASLR. If 'movable_node' specified, we rely on SRAT to get > >which memory region is movable or immovable, and movable region can be > >hot removed. But if kernel is randomized into movable memory, it can't > >be hot removed any more, this is a regression after KASLR introduced. > >So this is a different issue than Masa's. > > Yes, they are two issues. > But if we can get more memory information by the function in > the new file acpi.c, semms it's helfpul to Masa's issue. Hmm, reading SRAT three times during x86 kernel boot? Maybe we try this after the function has run a time and proved very stable? Thanks Baoquan
Re: [PATCH v8 0/3] x86/boot/KASLR: Parse ACPI table and limit kaslr in immovable memory
On Wed, Oct 10, 2018 at 11:14:26AM +0200, Thomas Gleixner wrote: > Yes, it's different, but if the SRAT information is available early, then > the command line parameter can go away because then the required > information for Masa's problem is available as well. Exactly. And I'd prefer we delayed the command line parameter until we figure out we really need it and not expose it to upstream and then remove it shortly after. So I'd suggest we move Masa's patches to a separate branch and not send it up this round. -- Regards/Gruss, Boris. Good mailing practices for 400: avoid top-posting and trim the reply.
Re: [PATCH v8 0/3] x86/boot/KASLR: Parse ACPI table and limit kaslr in immovable memory
On Wed, 10 Oct 2018, Baoquan He wrote: > Hi Boris, > > On 10/10/18 at 10:59am, Borislav Petkov wrote: > > ... and we just picked up > > > > https://lkml.kernel.org/r/20181001140843.26137-1-msys.miz...@gmail.com > > > > and without having looked at the rest of your stuff, if people accept > > your solution, we don't need the silly parameter anymore, right? > > > > Which means, we should not rush the whole thing yet until the whole > > KASLR vs movable memory gets solved properly. > > Masa's patches solves the problem in memory region KASLR which later hot > added memory may be big than the default padding 10 TB. > > Chao's patches is trying to fix a conflict between 'movable_node' and > kernel text KASLR. If 'movable_node' specified, we rely on SRAT to get > which memory region is movable or immovable, and movable region can be > hot removed. But if kernel is randomized into movable memory, it can't > be hot removed any more, this is a regression after KASLR introduced. > So this is a different issue than Masa's. Yes, it's different, but if the SRAT information is available early, then the command line parameter can go away because then the required information for Masa's problem is available as well. Thanks, tglx
Re: [PATCH v8 0/3] x86/boot/KASLR: Parse ACPI table and limit kaslr in immovable memory
On Wed, Oct 10, 2018 at 05:06:20PM +0800, Baoquan He wrote: >Hi Boris, > >On 10/10/18 at 10:59am, Borislav Petkov wrote: >> ... and we just picked up >> >> https://lkml.kernel.org/r/20181001140843.26137-1-msys.miz...@gmail.com >> >> and without having looked at the rest of your stuff, if people accept >> your solution, we don't need the silly parameter anymore, right? >> >> Which means, we should not rush the whole thing yet until the whole >> KASLR vs movable memory gets solved properly. > >Masa's patches solves the problem in memory region KASLR which later hot >added memory may be big than the default padding 10 TB. > >Chao's patches is trying to fix a conflict between 'movable_node' and >kernel text KASLR. If 'movable_node' specified, we rely on SRAT to get >which memory region is movable or immovable, and movable region can be >hot removed. But if kernel is randomized into movable memory, it can't >be hot removed any more, this is a regression after KASLR introduced. >So this is a different issue than Masa's. Yes, they are two issues. But if we can get more memory information by the function in the new file acpi.c, semms it's helfpul to Masa's issue. Thanks, Chao Fan > >Thanks >Baoquan > >
Re: [PATCH v8 0/3] x86/boot/KASLR: Parse ACPI table and limit kaslr in immovable memory
Hi Boris, On 10/10/18 at 10:59am, Borislav Petkov wrote: > ... and we just picked up > > https://lkml.kernel.org/r/20181001140843.26137-1-msys.miz...@gmail.com > > and without having looked at the rest of your stuff, if people accept > your solution, we don't need the silly parameter anymore, right? > > Which means, we should not rush the whole thing yet until the whole > KASLR vs movable memory gets solved properly. Masa's patches solves the problem in memory region KASLR which later hot added memory may be big than the default padding 10 TB. Chao's patches is trying to fix a conflict between 'movable_node' and kernel text KASLR. If 'movable_node' specified, we rely on SRAT to get which memory region is movable or immovable, and movable region can be hot removed. But if kernel is randomized into movable memory, it can't be hot removed any more, this is a regression after KASLR introduced. So this is a different issue than Masa's. Thanks Baoquan
Re: [PATCH v8 0/3] x86/boot/KASLR: Parse ACPI table and limit kaslr in immovable memory
On Wed, Oct 10, 2018 at 04:41:16PM +0800, Chao Fan wrote: > ***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 copy the codes > and functions to 'compressed/' directory, so that kaslr won't > influence the initialization of ACPI. ... and we just picked up https://lkml.kernel.org/r/20181001140843.26137-1-msys.miz...@gmail.com and without having looked at the rest of your stuff, if people accept your solution, we don't need the silly parameter anymore, right? Which means, we should not rush the whole thing yet until the whole KASLR vs movable memory gets solved properly. Ingo, Thomas? -- Regards/Gruss, Boris. Good mailing practices for 400: avoid top-posting and trim the reply.
[PATCH v8 2/3] x86/boot/KASLR: Walk srat tables to filter immovable memory
If CONFIG_MEMORY_HOTREMOVE enabled, walk through the acpi srat memory tables and store those immovable memory regions so that kaslr can get where to choose for randomization. Signed-off-by: Chao Fan --- arch/x86/boot/compressed/kaslr.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/arch/x86/boot/compressed/kaslr.c b/arch/x86/boot/compressed/kaslr.c index 9ed9709d9947..573e582e8709 100644 --- a/arch/x86/boot/compressed/kaslr.c +++ b/arch/x86/boot/compressed/kaslr.c @@ -417,6 +417,9 @@ static void mem_avoid_init(unsigned long input, unsigned long input_size, /* Mark the memmap regions we need to avoid */ handle_mem_options(); + /* Mark the immovable regions we need to choose */ + get_immovable_mem(); + #ifdef CONFIG_X86_VERBOSE_BOOTUP /* Make sure video RAM can be used. */ add_identity_map(0, PMD_SIZE); -- 2.17.1
[PATCH v8 0/3] x86/boot/KASLR: Parse ACPI table and limit kaslr in immovable memory
***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 copy the codes and functions to 'compressed/' directory, so that kaslr won't influence the initialization of ACPI. PATCH 1/3 Add acpitb.c to provide functions to parse ACPI code. PATCH 2/3 If CONFIG_MEMORY_HOTREMOVE enabled, walk all nodes and store the information of immovable memory regions. PATCH 3/3 According to the immovable memory regions, filter the immovable regions which KASLR can choose. ***Test results: - I did a very simple test, and it can get the memory information in bios and efi KVM guest machine, and put it by early printk. But no more tests, so it's with RFC tag. 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. - Simplify two functions in head file. 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. Any comments will be welcome. Chao Fan (3): x86/boot: Add acpitb.c to parse acpi tables x86/boot/KASLR: Walk srat tables to filter immovable memory x86/boot/KASLR: Limit kaslr to choosing the immovable memory arch/x86/boot/compressed/Makefile | 2 + arch/x86/boot/compressed/acpitb.c | 405 ++ arch/x86/boot/compressed/kaslr.c | 75 +- arch/x86/boot/compressed/misc.h | 8 + 4 files changed, 479 insertions(+), 11 deletions(-) create mode 100644 arch/x86/boot/compressed/acpitb.c -- 2.17.1
[PATCH v8 3/3] x86/boot/KASLR: Limit kaslr to choosing the immovable memory
If CONFIG_MEMORY_HOTREMOVE enabled and the amount of immovable
memory regions is not zero. Calculate the intersection between memory
regions from e820/efi memory table and immovable memory regions.
Signed-off-by: Chao Fan
---
arch/x86/boot/compressed/kaslr.c | 72 +++-
1 file changed, 61 insertions(+), 11 deletions(-)
diff --git a/arch/x86/boot/compressed/kaslr.c b/arch/x86/boot/compressed/kaslr.c
index 573e582e8709..61486aad39b6 100644
--- a/arch/x86/boot/compressed/kaslr.c
+++ b/arch/x86/boot/compressed/kaslr.c
@@ -101,6 +101,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,
@@ -575,9 +580,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;
@@ -653,6 +658,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, _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(, 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
@@ -718,11 +774,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(, minimum, image_size);
- if (slot_area_index == MAX_SLOT_AREA) {
- debug_putstr("Aborted EFI scan (slot_areas full)!\n");
+ if (process_mem_region(, minimum, image_size))
break;
- }
}
return true;
}
@@ -749,11 +802,8 @@ static void process_e820_entries(unsigned long minimum,
continue;
region.start = entry->addr;
region.size = entry->size;
- process_mem_region(, minimum, image_size);
- if (slot_area_index == MAX_SLOT_AREA) {
- debug_putstr("Aborted e820 scan (slot_areas full)!\n");
+ if (process_mem_region(, minimum, image_size))
break;
- }
}
}
--
2.17.1
