This patch set narrows the valid space addressed by the page table
level2_kernel_pgt to only contain ranges checked against the "usable
RAM" list provided by the BIOS.
Prior to this, some larger than needed mappings were occasionally
crossing over into spaces marked reserved, allowing the processor to
access these reserved spaces, which were caught by the hardware and
caused BIOS to halt on our platform (UV).
Changes since v1:
* Cover letter added because there's now two patches.
* Patch 1: Added comment and re-worked changelog text.
* Patch 2: New change requested by Dave Hansen to handle the case that
the mapping of the last PMD page for the kernel image could cross a
reserved region boundary.
Steve Wahl (2):
x86/boot/64: Make level2_kernel_pgt pages invalid outside kernel area.
x86/boot/64: round memory hole size up to next PMD page.
arch/x86/boot/compressed/misc.c | 25 +++++++++++++++++++------
arch/x86/kernel/head64.c | 18 ++++++++++++++++--
2 files changed, 35 insertions(+), 8 deletions(-)
--
2.21.0
--
Steve Wahl, Hewlett Packard Enterprise
The kernel image map is created using PMD pages, which can include
some extra space beyond what's actually needed. Round the size of the
memory hole we search for up to the next PMD boundary, to be certain
all of the space to be mapped is usable RAM and includes no reserved
areas.
Signed-off-by: Steve Wahl <[email protected]>
Cc: [email protected]
---
Changes since v1:
* This patch is completely new to this verison.
arch/x86/boot/compressed/misc.c | 25 +++++++++++++++++++------
1 file changed, 19 insertions(+), 6 deletions(-)
diff --git a/arch/x86/boot/compressed/misc.c b/arch/x86/boot/compressed/misc.c
index 53ac0cb2396d..9652d5c2afda 100644
--- a/arch/x86/boot/compressed/misc.c
+++ b/arch/x86/boot/compressed/misc.c
@@ -345,6 +345,7 @@ asmlinkage __visible void *extract_kernel(void *rmode, memptr heap,
{
const unsigned long kernel_total_size = VO__end - VO__text;
unsigned long virt_addr = LOAD_PHYSICAL_ADDR;
+ unsigned long needed_size;
/* Retain x86 boot parameters pointer passed from startup_32/64. */
boot_params = rmode;
@@ -379,26 +380,38 @@ asmlinkage __visible void *extract_kernel(void *rmode, memptr heap,
free_mem_ptr = heap; /* Heap */
free_mem_end_ptr = heap + BOOT_HEAP_SIZE;
+ /*
+ * The memory hole needed for the kernel is the larger of either
+ * the entire decompressed kernel plus relocation table, or the
+ * entire decompressed kernel plus .bss and .brk sections.
+ *
+ * On X86_64, the memory is mapped with PMD pages. Round the
+ * size up so that the full extent of PMD pages mapped is
+ * included in the check against the valid memory table
+ * entries. This ensures the full mapped area is usable RAM
+ * and doesn't include any reserved areas.
+ */
+ needed_size = max(output_len, kernel_total_size);
+#ifdef CONFIG_X86_64
+ needed_size = ALIGN(needed_size, MIN_KERNEL_ALIGN);
+#endif
+
/* Report initial kernel position details. */
debug_putaddr(input_data);
debug_putaddr(input_len);
debug_putaddr(output);
debug_putaddr(output_len);
debug_putaddr(kernel_total_size);
+ debug_putaddr(needed_size);
#ifdef CONFIG_X86_64
/* Report address of 32-bit trampoline */
debug_putaddr(trampoline_32bit);
#endif
- /*
- * The memory hole needed for the kernel is the larger of either
- * the entire decompressed kernel plus relocation table, or the
- * entire decompressed kernel plus .bss and .brk sections.
- */
choose_random_location((unsigned long)input_data, input_len,
(unsigned long *)&output,
- max(output_len, kernel_total_size),
+ needed_size,
&virt_addr);
/* Validate memory location choices. */
--
2.21.0
--
Steve Wahl, Hewlett Packard Enterprise
Our hardware (UV aka Superdome Flex) has address ranges marked
reserved by the BIOS. Access to these ranges is caught as an error,
causing the BIOS to halt the system.
Initial page tables mapped a large range of physical addresses that
were not checked against the list of BIOS reserved addresses, and
sometimes included reserved addresses in part of the mapped range.
Including the reserved range in the map allowed processor speculative
accesses to the reserved range, triggering a BIOS halt.
Used early in booting, the page table level2_kernel_pgt addresses 1
GiB divided into 2 MiB pages, and it was set up to linearly map a full
1 GiB of physical addresses that included the physical address range
of the kernel image, as chosen by KASLR. But this also included a
large range of unused addresses on either side of the kernel image.
And unlike the kernel image's physical address range, this extra
mapped space was not checked against the BIOS tables of usable RAM
addresses. So there were times when the addresses chosen by KASLR
would result in processor accessible mappings of BIOS reserved
physical addresses.
The kernel code did not directly access any of this extra mapped
space, but having it mapped allowed the processor to issue speculative
accesses into reserved memory, causing system halts.
This was encountered somewhat rarely on a normal system boot, and much
more often when starting the crash kernel if "crashkernel=512M,high"
was specified on the command line (this heavily restricts the physical
address of the crash kernel, in our case usually within 1 GiB of
reserved space).
The solution is to invalidate the pages of this table outside the
kernel image's space before the page table is activated. This patch
has been validated to fix this problem on our hardware.
Signed-off-by: Steve Wahl <[email protected]>
Cc: [email protected]
---
Changes since v1:
* Added comment.
* Reworked changelog text.
arch/x86/kernel/head64.c | 18 ++++++++++++++++--
1 file changed, 16 insertions(+), 2 deletions(-)
diff --git a/arch/x86/kernel/head64.c b/arch/x86/kernel/head64.c
index 29ffa495bd1c..ee9d0e3e0c46 100644
--- a/arch/x86/kernel/head64.c
+++ b/arch/x86/kernel/head64.c
@@ -222,13 +222,27 @@ unsigned long __head __startup_64(unsigned long physaddr,
* we might write invalid pmds, when the kernel is relocated
* cleanup_highmap() fixes this up along with the mappings
* beyond _end.
+ *
+ * Only the region occupied by the kernel image has so far
+ * been checked against the table of usable memory regions
+ * provided by the firmware, so invalidate pages outside that
+ * region. A page table entry that maps to a reserved area of
+ * memory would allow processor speculation into that area,
+ * and on some hardware (particularly the UV platform) even
+ * speculative access to some reserved areas is caught as an
+ * error, causing the BIOS to halt the system.
*/
pmd = fixup_pointer(level2_kernel_pgt, physaddr);
- for (i = 0; i < PTRS_PER_PMD; i++) {
+ for (i = 0; i < pmd_index((unsigned long)_text); i++)
+ pmd[i] &= ~_PAGE_PRESENT;
+
+ for (; i <= pmd_index((unsigned long)_end); i++)
if (pmd[i] & _PAGE_PRESENT)
pmd[i] += load_delta;
- }
+
+ for (; i < PTRS_PER_PMD; i++)
+ pmd[i] &= ~_PAGE_PRESENT;
/*
* Fixup phys_base - remove the memory encryption mask to obtain
--
2.21.0
--
Steve Wahl, Hewlett Packard Enterprise
On 9/23/19 11:15 AM, Steve Wahl wrote:
> The kernel image map is created using PMD pages, which can include
> some extra space beyond what's actually needed. Round the size of the
> memory hole we search for up to the next PMD boundary, to be certain
> all of the space to be mapped is usable RAM and includes no reserved
> areas.
This looks good. It also fully closes any possibility that anyone's
future hardware will hit issues like this as long as they mark the
memory reserved, right?
Reviewed-by: Dave Hansen <[email protected]>
On 9/23/19 11:15 AM, Steve Wahl wrote:
> pmd = fixup_pointer(level2_kernel_pgt, physaddr);
> - for (i = 0; i < PTRS_PER_PMD; i++) {
> + for (i = 0; i < pmd_index((unsigned long)_text); i++)
> + pmd[i] &= ~_PAGE_PRESENT;
> +
> + for (; i <= pmd_index((unsigned long)_end); i++)
> if (pmd[i] & _PAGE_PRESENT)
> pmd[i] += load_delta;
> - }
> +
> + for (; i < PTRS_PER_PMD; i++)
> + pmd[i] &= ~_PAGE_PRESENT;
This is looking a bunch better. The broken-up loop could probably use
some comments, or you could combine it back to a single loop like this:
int text_start_pmd_index = pmd_index((unsigned long)_text);
int text_end_pmd_index = pmd_index((unsigned long)_end);
for (i = 0; i < PTRS_PER_PMD; i++) {
if ((i < text_start_pmd_index) ||
(i > text_end_pmd_index)) {
/* Unmap entries not mapping the kernel image */
pmd[i] &= ~_PAGE_PRESENT;
} else if (pmd[i] & _PAGE_PRESENT)
pmd[i] += load_delta;
}
}
Although I'd prefer it get commented or rewritten, it's passable like
this, so:
Reviewed-by: Dave Hansen <[email protected]>
On Mon, Sep 23, 2019 at 02:20:35PM -0700, Dave Hansen wrote:
> On 9/23/19 11:15 AM, Steve Wahl wrote:
> > The kernel image map is created using PMD pages, which can include
> > some extra space beyond what's actually needed. Round the size of the
> > memory hole we search for up to the next PMD boundary, to be certain
> > all of the space to be mapped is usable RAM and includes no reserved
> > areas.
>
> This looks good. It also fully closes any possibility that anyone's
> future hardware will hit issues like this as long as they mark the
> memory reserved, right?
I believe that is true. Thanks!
> Reviewed-by: Dave Hansen <[email protected]>
--> Steve Wahl
--
Steve Wahl, Hewlett Packard Enterprise
On Mon, Sep 23, 2019 at 02:19:46PM -0700, Dave Hansen wrote:
> On 9/23/19 11:15 AM, Steve Wahl wrote:
> > pmd = fixup_pointer(level2_kernel_pgt, physaddr);
> > - for (i = 0; i < PTRS_PER_PMD; i++) {
> > + for (i = 0; i < pmd_index((unsigned long)_text); i++)
> > + pmd[i] &= ~_PAGE_PRESENT;
> > +
> > + for (; i <= pmd_index((unsigned long)_end); i++)
> > if (pmd[i] & _PAGE_PRESENT)
> > pmd[i] += load_delta;
> > - }
> > +
> > + for (; i < PTRS_PER_PMD; i++)
> > + pmd[i] &= ~_PAGE_PRESENT;
>
> This is looking a bunch better. The broken-up loop could probably use
> some comments, or you could combine it back to a single loop like this:
>
> int text_start_pmd_index = pmd_index((unsigned long)_text);
> int text_end_pmd_index = pmd_index((unsigned long)_end);
>
> for (i = 0; i < PTRS_PER_PMD; i++) {
> if ((i < text_start_pmd_index) ||
> (i > text_end_pmd_index)) {
> /* Unmap entries not mapping the kernel image */
> pmd[i] &= ~_PAGE_PRESENT;
> } else if (pmd[i] & _PAGE_PRESENT)
> pmd[i] += load_delta;
> }
> }
That's funny, I wrote it very close to that way initially, and
re-wrote it broken-up loop style because I thought it better conveyed
my intention. (Mark pages before the kernel image as invalid, fixup
the pages that are in kernel image range, mark pages after the kernel
image as invalid.)
Given that, would you feel better with A) the way I have it, B) your
rewrite, or C) with an inline comment for each part of the loop:
pmd = fixup_pointer(level2_kernel_pgt, physaddr);
/* invalidate pages before the kernel image */
for (i = 0; i < pmd_index((unsigned long)_text); i++)
pmd[i] &= ~_PAGE_PRESENT;
/* fixup pages that are part of the kernel image */
for (; i <= pmd_index((unsigned long)_end); i++)
if (pmd[i] & _PAGE_PRESENT)
pmd[i] += load_delta;
/* invalidate pages after the kernel image */
for (; i < PTRS_PER_PMD; i++)
pmd[i] &= ~_PAGE_PRESENT;
> Although I'd prefer it get commented or rewritten, it's passable like
> this, so:
>
> Reviewed-by: Dave Hansen <[email protected]>
Thanks for your input!
--> Steve Wahl
--
Steve Wahl, Hewlett Packard Enterprise
On 9/24/19 1:04 PM, Steve Wahl wrote:
> Given that, would you feel better with A) the way I have it, B) your
> rewrite, or C) with an inline comment for each part of the loop:
>
> pmd = fixup_pointer(level2_kernel_pgt, physaddr);
>
> /* invalidate pages before the kernel image */
> for (i = 0; i < pmd_index((unsigned long)_text); i++)
> pmd[i] &= ~_PAGE_PRESENT;
>
> /* fixup pages that are part of the kernel image */
> for (; i <= pmd_index((unsigned long)_end); i++)
> if (pmd[i] & _PAGE_PRESENT)
> pmd[i] += load_delta;
>
> /* invalidate pages after the kernel image */
> for (; i < PTRS_PER_PMD; i++)
> pmd[i] &= ~_PAGE_PRESENT;
I don't feel super strongly either way, but I'd prefer doing B or C over
nothing.