[GIT PULL v3 0/6] EFI page table isolation

[GIT PULL v3 0/6] EFI page table isolation

[Matt Fleming]

Folks,

This patch series is a response to the report that the EFI region
mappings trigger warnings when booting with CONFIG_DEBUG_WX enabled.
They allocate a new page table structure and ensure that all the
mappings we require during EFI runtime calls are only setup there.

It turns out that it still makes sense to share some page table
entries with 'swapper_pg_dir', just not the entries where we need to
allow security lax permissions. Sharing entries is useful for memory
hotplug, for example.

When writing this series I discovered a number of bugs in the existing
code that only became apparent when we stopped using 'trampoline_pgd'
which already mapped a bunch of things for us. I've put those bug
fixes at the start of the series.

Further testing would be very much appreciated as this is a
notoriously funky area of the EFI code.

Changes in v3:

 - PFN_ALIGN() _text and _end when calculating npages to map

 - Drop the hunk that mapped the stack in efi_setup_page_tables(), now
   that we're mapping RAM we know the stack is already mapped.

 - Update the wording in Documentation/<..>/mm.txt to make it clear we
   carve out a 64Gb *size* of virtual address space, we don't use the
   first 64Gb virtual addresses.

Changes in v2:

 - Folded PATCH 1 and 2 together because they both fall under the
   umbrella of "making sure cpa->pfn is really a page frame number".

 - Fixed some checkpatch warnings about mixing spaces and tabs and
   made some stylistic changes per Borislav's comments.

 - Moved efi_alloc_page_tables() earlier in __efi_enter_virtual_mode()
   so that we fail early if we can't allocate memory for the page
   tables.

The following changes since commit 2c66e24d75d424919c42288b418d2e593fa818b1:

  x86/efi: Fix kernel panic when CONFIG_DEBUG_VIRTUAL is enabled (2015-10-25 10:22:25 +0000)

are available in the git repository at:

  git://git.kernel.org/pub/scm/linux/kernel/git/mfleming/efi.git tags/efi-next

for you to fetch changes up to fac3e3c52017a5d974c7d8168e8e43c8f68af82a:

  Documentation/x86: Update EFI memory region description (2015-11-23 12:33:09 +0000)

----------------------------------------------------------------
 * Use completely separate page tables for EFI runtime service calls
   so that the security-lax mapping permissions (RWX) do not leak into
   the standard kernel page tables and trigger warnings when
   CONFIG_DEBUG_WX is enabled.

----------------------------------------------------------------
Matt Fleming (6):
      x86/efi: PFN_ALIGN() _text and _end when calculating number of pages
      x86/mm/pageattr: Ensure cpa->pfn only contains page frame numbers
      x86/efi: Map RAM into the identity page table for mixed mode
      x86/efi: Hoist page table switching code into efi_call_virt()
      x86/efi: Build our own page table structures
      Documentation/x86: Update EFI memory region description

 Documentation/x86/x86_64/mm.txt     |  12 +--
 arch/x86/include/asm/efi.h          |  26 ++++++
 arch/x86/mm/pageattr.c              |  17 ++--
 arch/x86/platform/efi/efi.c         |  39 ++++-----
 arch/x86/platform/efi/efi_32.c      |   5 ++
 arch/x86/platform/efi/efi_64.c      | 159 ++++++++++++++++++++++++++++--------
 arch/x86/platform/efi/efi_stub_64.S |  43 ----------
 7 files changed, 181 insertions(+), 120 deletions(-)

[PATCH 1/6] x86/efi: PFN_ALIGN() _text and _end when calculating number of pages

[Matt Fleming]

While _text is currently aligned to PAGE_SIZE in the vmlinux linker
script because it's based on CONFIG_PHYSICAL_ALIGN, it's always better
to be explicit about these things to be sure no alignment bugs are
lurking. There's no analogous enforcement for _end.

Dave provided an example of why the 'npages' calculation is wrong,

 "Just for fun, imagine that _end=0xfff and _text=0x1001.  npages
  would be 0."

Use PFN_ALIGN() to be sure the calculation is correctly aligned to
PAGE_SIZE.

Reported-by: Dave Hansen <dave.hansen@intel.com>
Reported-by: Thomas Gleixner <tglx@linutronix.de>
Cc: Borislav Petkov <bp@alien8.de>
Cc: Sai Praneeth Prakhya <sai.praneeth.prakhya@intel.com>
Cc: Ingo Molnar <mingo@kernel.org>
Cc: <stable@vger.kernel.org>
Signed-off-by: Matt Fleming <matt@codeblueprint.co.uk>
---
 arch/x86/platform/efi/efi_64.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/x86/platform/efi/efi_64.c b/arch/x86/platform/efi/efi_64.c
index a0ac0f9c307f..3a90eb72d153 100644
--- a/arch/x86/platform/efi/efi_64.c
+++ b/arch/x86/platform/efi/efi_64.c
@@ -183,7 +183,7 @@ int __init efi_setup_page_tables(unsigned long pa_memmap, unsigned num_pages)
 	efi_scratch.phys_stack = virt_to_phys(page_address(page));
 	efi_scratch.phys_stack += PAGE_SIZE; /* stack grows down */
 
-	npages = (_end - _text) >> PAGE_SHIFT;
+	npages = (PFN_ALIGN(_end) - PFN_ALIGN(_text)) >> PAGE_SHIFT;
 	text = __pa(_text);
 
 	if (kernel_map_pages_in_pgd(pgd, text >> PAGE_SHIFT, text, npages, 0)) {
-- 
2.6.2

Re: [PATCH 1/6] x86/efi: PFN_ALIGN() _text and _end when calculating number of pages

[Ingo Molnar]

* Matt Fleming <matt@codeblueprint.co.uk> wrote:

> While _text is currently aligned to PAGE_SIZE in the vmlinux linker
> script because it's based on CONFIG_PHYSICAL_ALIGN, it's always better
> to be explicit about these things to be sure no alignment bugs are
> lurking. There's no analogous enforcement for _end.
> 
> Dave provided an example of why the 'npages' calculation is wrong,
> 
>  "Just for fun, imagine that _end=0xfff and _text=0x1001.  npages
>   would be 0."
> 
> Use PFN_ALIGN() to be sure the calculation is correctly aligned to
> PAGE_SIZE.
> 
> Reported-by: Dave Hansen <dave.hansen@intel.com>
> Reported-by: Thomas Gleixner <tglx@linutronix.de>
> Cc: Borislav Petkov <bp@alien8.de>
> Cc: Sai Praneeth Prakhya <sai.praneeth.prakhya@intel.com>
> Cc: Ingo Molnar <mingo@kernel.org>
> Cc: <stable@vger.kernel.org>
> Signed-off-by: Matt Fleming <matt@codeblueprint.co.uk>
> ---
>  arch/x86/platform/efi/efi_64.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/arch/x86/platform/efi/efi_64.c b/arch/x86/platform/efi/efi_64.c
> index a0ac0f9c307f..3a90eb72d153 100644
> --- a/arch/x86/platform/efi/efi_64.c
> +++ b/arch/x86/platform/efi/efi_64.c
> @@ -183,7 +183,7 @@ int __init efi_setup_page_tables(unsigned long pa_memmap, unsigned num_pages)
>  	efi_scratch.phys_stack = virt_to_phys(page_address(page));
>  	efi_scratch.phys_stack += PAGE_SIZE; /* stack grows down */
>  
> -	npages = (_end - _text) >> PAGE_SHIFT;
> +	npages = (PFN_ALIGN(_end) - PFN_ALIGN(_text)) >> PAGE_SHIFT;
>  	text = __pa(_text);

Didn't we want to do the _end alignment linker script fix instead?

Alignment assumptions are easy to make when symbols are well aligned typically (as 
in this case), so we should guarantee the alignment property instead of 
complicating the code.

Thanks,

	Ingo

Re: [PATCH 1/6] x86/efi: PFN_ALIGN() _text and _end when calculating number of pages

[Matt Fleming]

On Tue, 24 Nov, at 09:23:23AM, Ingo Molnar wrote:
> 
> Didn't we want to do the _end alignment linker script fix instead?
 
I think we should do both. This patch is tagged for stable because it
fixes a bug in the existing code. It's obvious and it's explicit and
it's much easier to know when someone might want to backport it.

Changing the linker script which indirectly fixes the above bug is a
much more subtle solution, with much larger potential for fallout
because it affects multiple chunks of kernel code.

> Alignment assumptions are easy to make when symbols are well aligned typically (as 
> in this case), so we should guarantee the alignment property instead of 
> complicating the code.

I don't agree that sprinkling PFN_ALIGN() complicates the code, it's a
minimal change with a well known kernel idiom. But yes, aligning these
symbols in the linker script is generally a good idea.

The two patches are worthwhile, for different reasons; let's do both.

Re: [PATCH 1/6] x86/efi: PFN_ALIGN() _text and _end when calculating number of pages

[Ingo Molnar]

* Matt Fleming <matt@codeblueprint.co.uk> wrote:

> On Tue, 24 Nov, at 09:23:23AM, Ingo Molnar wrote:
> > 
> > Didn't we want to do the _end alignment linker script fix instead?
>  
> I think we should do both. This patch is tagged for stable because it
> fixes a bug in the existing code. It's obvious and it's explicit and
> it's much easier to know when someone might want to backport it.
> 
> Changing the linker script which indirectly fixes the above bug is a
> much more subtle solution, with much larger potential for fallout
> because it affects multiple chunks of kernel code.
> 
> > Alignment assumptions are easy to make when symbols are well aligned typically (as 
> > in this case), so we should guarantee the alignment property instead of 
> > complicating the code.
> 
> I don't agree that sprinkling PFN_ALIGN() complicates the code, it's a
> minimal change with a well known kernel idiom. But yes, aligning these
> symbols in the linker script is generally a good idea.
> 
> The two patches are worthwhile, for different reasons; let's do both.

I disagree, this form:

        npages = (_end - _text) >> PAGE_SHIFT;

is a lot clearer to read than:

        npages = (PFN_ALIGN(_end) - PFN_ALIGN(_text)) >> PAGE_SHIFT;

especially once we ensure that _end and _text are page aligned. The latter form 
will only result in cargo-cult carrying over of unnecessary PFN_ALIGN() 
operations.

Section boundaries of the kernel should generally be page aligned, this is useful 
for a number of other reasons as well.

As far as backporting goes, it would generally be _safer_ to backport the linker 
script fix, in case there are other unrealized alignment bugs in the kernel. 
Especially if upstream does the same.

Thanks,

	Ingo

Re: [PATCH 1/6] x86/efi: PFN_ALIGN() _text and _end when calculating number of pages

[Matt Fleming]

On Thu, 26 Nov, at 12:13:23PM, Ingo Molnar wrote:
> 
> As far as backporting goes, it would generally be _safer_ to backport the linker 
> script fix, in case there are other unrealized alignment bugs in the kernel. 
> Especially if upstream does the same.

OK, I'll respin this series and replace this patch with the linker
script fix.