[05/11] x86/efi: Unmap EFI boot services code/data regions from efi_pgd

Message ID 20181129171230.18699-6-ard.biesheuvel@linaro.org
State New
Headers show
Series
  • EFI updates
Related show

Commit Message

Ard Biesheuvel Nov. 29, 2018, 5:12 p.m.
From: Sai Praneeth Prakhya <sai.praneeth.prakhya@intel.com>


efi_free_boot_services(), as the name suggests, frees EFI boot services
code/data regions but forgets to unmap these regions from efi_pgd. This
means that any code that's running in efi_pgd address space (e.g:
any EFI runtime service) would still be able to access these regions but
the contents of these regions would have long been over written by
someone else. So, it's important to unmap these regions. Hence,
introduce efi_unmap_pages() to unmap these regions from efi_pgd.

After unmapping EFI boot services code/data regions, any illegal access
by buggy firmware to these regions would result in page fault which will
be handled by EFI specific fault handler.

Signed-off-by: Sai Praneeth Prakhya <sai.praneeth.prakhya@intel.com>

Cc: Borislav Petkov <bp@alien8.de>
Cc: Ingo Molnar <mingo@kernel.org>
Cc: Andy Lutomirski <luto@kernel.org>
Cc: Dave Hansen <dave.hansen@intel.com>
Cc: Bhupesh Sharma <bhsharma@redhat.com>
Cc: Peter Zijlstra <peterz@infradead.org>
Acked-by: Thomas Gleixner <tglx@linutronix.de>

Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>

---
 arch/x86/platform/efi/quirks.c | 25 +++++++++++++++++++++++++
 1 file changed, 25 insertions(+)

-- 
2.19.1

Comments

Sai Praneeth Prakhya Dec. 17, 2018, 6:06 p.m. | #1
> Commit-ID:  08cfb38f3ef49cfd1bba11a00401451606477d80

> Gitweb:

> https://git.kernel.org/tip/08cfb38f3ef49cfd1bba11a00401451606477d80

> Author:     Sai Praneeth Prakhya <sai.praneeth.prakhya@intel.com>

> AuthorDate: Thu, 29 Nov 2018 18:12:24 +0100

> Committer:  Ingo Molnar <mingo@kernel.org>

> CommitDate: Fri, 30 Nov 2018 09:10:30 +0100

> 

> x86/efi: Unmap EFI boot services code/data regions from efi_pgd

> 

> efi_free_boot_services(), as the name suggests, frees EFI boot services

> code/data regions but forgets to unmap these regions from efi_pgd. This means

> that any code that's running in efi_pgd address space (e.g:

> any EFI runtime service) would still be able to access these regions but the

> contents of these regions would have long been over written by someone else.

> So, it's important to unmap these regions. Hence, introduce efi_unmap_pages()

> to unmap these regions from efi_pgd.

> 

> After unmapping EFI boot services code/data regions, any illegal access by

> buggy firmware to these regions would result in page fault which will be handled

> by EFI specific fault handler.


Hi Thomas and Ingo,

I recently noticed that the below commits [1] and [2] are broken when kernel command line 
argument "efi=old_map" is passed. Sorry! I missed to test this condition prior to sending 
these patches to mailing list. I am working on a fix and will send it to mailing list as 
soon as it's ready.

Meanwhile, could you please drop these patches before sending pull request to Linus?

[1] Commit 08cfb38f3ef4 ("x86/efi: Unmap EFI boot services code/data regions from efi_pgd")
[2] Commit 7e0dabd3010d ("x86/mm/pageattr: Introduce helper function to unmap EFI boot services")

Regards,
Sai
Ard Biesheuvel Dec. 17, 2018, 6:10 p.m. | #2
On Mon, 17 Dec 2018 at 19:06, Prakhya, Sai Praneeth
<sai.praneeth.prakhya@intel.com> wrote:
>

> > Commit-ID:  08cfb38f3ef49cfd1bba11a00401451606477d80

> > Gitweb:

> > https://git.kernel.org/tip/08cfb38f3ef49cfd1bba11a00401451606477d80

> > Author:     Sai Praneeth Prakhya <sai.praneeth.prakhya@intel.com>

> > AuthorDate: Thu, 29 Nov 2018 18:12:24 +0100

> > Committer:  Ingo Molnar <mingo@kernel.org>

> > CommitDate: Fri, 30 Nov 2018 09:10:30 +0100

> >

> > x86/efi: Unmap EFI boot services code/data regions from efi_pgd

> >

> > efi_free_boot_services(), as the name suggests, frees EFI boot services

> > code/data regions but forgets to unmap these regions from efi_pgd. This means

> > that any code that's running in efi_pgd address space (e.g:

> > any EFI runtime service) would still be able to access these regions but the

> > contents of these regions would have long been over written by someone else.

> > So, it's important to unmap these regions. Hence, introduce efi_unmap_pages()

> > to unmap these regions from efi_pgd.

> >

> > After unmapping EFI boot services code/data regions, any illegal access by

> > buggy firmware to these regions would result in page fault which will be handled

> > by EFI specific fault handler.

>

> Hi Thomas and Ingo,

>

> I recently noticed that the below commits [1] and [2] are broken when kernel command line

> argument "efi=old_map" is passed. Sorry! I missed to test this condition prior to sending

> these patches to mailing list. I am working on a fix and will send it to mailing list as

> soon as it's ready.

>


Could you elaborate on the problem please?

> Meanwhile, could you please drop these patches before sending pull request to Linus?

>

> [1] Commit 08cfb38f3ef4 ("x86/efi: Unmap EFI boot services code/data regions from efi_pgd")

> [2] Commit 7e0dabd3010d ("x86/mm/pageattr: Introduce helper function to unmap EFI boot services")

>


I'd like to understand what the issue is before we drop anything.
Sai Praneeth Prakhya Dec. 17, 2018, 6:42 p.m. | #3
> > Hi Thomas and Ingo,

> >

> > I recently noticed that the below commits [1] and [2] are broken when

> > kernel command line argument "efi=old_map" is passed. Sorry! I missed

> > to test this condition prior to sending these patches to mailing list.

> > I am working on a fix and will send it to mailing list as soon as it's ready.

> >

> 

> Could you elaborate on the problem please?


Sure! My bad..

Little bit of history here:
Boris with this patch set [1] introduced statically mapping EFI Runtime Services at -4G 
and also introduced "efi=old_map" to return to previous EFI functionality which used 
ioremap and __va(pa).

[3] and [4] are links to old_map_region()

The commit 08cfb38f3ef4 ("x86/efi: Unmap EFI boot services code/data regions from efi_pgd"), 
unmaps EFI boot services code/data regions *only* from efi_pgd but efi=old_map maps 
EFI boot services code/data regions into swapper_pgd. Also, efi=old_map  uses either 
ioremap() or __va(md->phys_addr) to map EFI runtime/boot time services and doesn't use kernel_map_pages_in_pgd().

So, we need a different unmapping routine to unmap EFI boot services code/data 
regions from swapper_pgd if they were mapped using efi=old_map.

[1] https://lkml.org/lkml/2013/9/19/235 - Cover letter for EFI runtime services virtual mapping
[2] https://lkml.org/lkml/2013/10/8/777 - Talks about efi=old_map
[3] https://elixir.bootlin.com/linux/v4.20-rc7/source/arch/x86/platform/efi/efi_64.c#L429
[4] https://elixir.bootlin.com/linux/v4.20-rc7/source/arch/x86/platform/efi/efi.c#L584

> 

> > Meanwhile, could you please drop these patches before sending pull request

> to Linus?

> >

> > [1] Commit 08cfb38f3ef4 ("x86/efi: Unmap EFI boot services code/data

> > regions from efi_pgd") [2] Commit 7e0dabd3010d ("x86/mm/pageattr:

> > Introduce helper function to unmap EFI boot services")

> >

> 

> I'd like to understand what the issue is before we drop anything.


Regards,
Sai
Ard Biesheuvel Dec. 17, 2018, 7:35 p.m. | #4
On Mon, 17 Dec 2018 at 19:42, Prakhya, Sai Praneeth
<sai.praneeth.prakhya@intel.com> wrote:
>

> > > Hi Thomas and Ingo,

> > >

> > > I recently noticed that the below commits [1] and [2] are broken when

> > > kernel command line argument "efi=old_map" is passed. Sorry! I missed

> > > to test this condition prior to sending these patches to mailing list.

> > > I am working on a fix and will send it to mailing list as soon as it's ready.

> > >

> >

> > Could you elaborate on the problem please?

>

> Sure! My bad..

>

> Little bit of history here:

> Boris with this patch set [1] introduced statically mapping EFI Runtime Services at -4G

> and also introduced "efi=old_map" to return to previous EFI functionality which used

> ioremap and __va(pa).

>

> [3] and [4] are links to old_map_region()

>

> The commit 08cfb38f3ef4 ("x86/efi: Unmap EFI boot services code/data regions from efi_pgd"),

> unmaps EFI boot services code/data regions *only* from efi_pgd but efi=old_map maps

> EFI boot services code/data regions into swapper_pgd. Also, efi=old_map  uses either

> ioremap() or __va(md->phys_addr) to map EFI runtime/boot time services and doesn't use kernel_map_pages_in_pgd().

>

> So, we need a different unmapping routine to unmap EFI boot services code/data

> regions from swapper_pgd if they were mapped using efi=old_map.

>


For the short term, could we simply make your changes dependent on efi
!= old_map? That gives us some time to fix the old_map case properly.
Sai Praneeth Prakhya Dec. 17, 2018, 7:48 p.m. | #5
> > > > Hi Thomas and Ingo,

> > > >

> > > > I recently noticed that the below commits [1] and [2] are broken

> > > > when kernel command line argument "efi=old_map" is passed. Sorry!

> > > > I missed to test this condition prior to sending these patches to mailing list.

> > > > I am working on a fix and will send it to mailing list as soon as it's ready.

> > > >

> > >

> > > Could you elaborate on the problem please?

> >

> > Sure! My bad..

> >

> > Little bit of history here:

> > Boris with this patch set [1] introduced statically mapping EFI

> > Runtime Services at -4G and also introduced "efi=old_map" to return to

> > previous EFI functionality which used ioremap and __va(pa).

> >

> > [3] and [4] are links to old_map_region()

> >

> > The commit 08cfb38f3ef4 ("x86/efi: Unmap EFI boot services code/data

> > regions from efi_pgd"), unmaps EFI boot services code/data regions

> > *only* from efi_pgd but efi=old_map maps EFI boot services code/data

> > regions into swapper_pgd. Also, efi=old_map  uses either

> > ioremap() or __va(md->phys_addr) to map EFI runtime/boot time services and

> doesn't use kernel_map_pages_in_pgd().

> >

> > So, we need a different unmapping routine to unmap EFI boot services

> > code/data regions from swapper_pgd if they were mapped using efi=old_map.

> >

> 

> For the short term, could we simply make your changes dependent on efi !=

> old_map? That gives us some time to fix the old_map case properly.


Yes, I think we could and it should work but I hesitated to propose it because 
(as you already noted) it's a short term fix and not the right fix.

Alternatively, we could also evaluate if we need to support efi=old_map case going further.
I thought dropping it would be a bad idea because it changes kernel user visible interface 
(because it's a kernel command line argument) and never brought it up.
Not sure what Thomas, Ingo or Linus might think about dropping a kernel command line 
argument.

Regards,
Sai
Ard Biesheuvel Dec. 21, 2018, 5:02 p.m. | #6
On Mon, 17 Dec 2018 at 20:48, Prakhya, Sai Praneeth
<sai.praneeth.prakhya@intel.com> wrote:
>

> > > > > Hi Thomas and Ingo,

> > > > >

> > > > > I recently noticed that the below commits [1] and [2] are broken

> > > > > when kernel command line argument "efi=old_map" is passed. Sorry!

> > > > > I missed to test this condition prior to sending these patches to mailing list.

> > > > > I am working on a fix and will send it to mailing list as soon as it's ready.

> > > > >

> > > >

> > > > Could you elaborate on the problem please?

> > >

> > > Sure! My bad..

> > >

> > > Little bit of history here:

> > > Boris with this patch set [1] introduced statically mapping EFI

> > > Runtime Services at -4G and also introduced "efi=old_map" to return to

> > > previous EFI functionality which used ioremap and __va(pa).

> > >

> > > [3] and [4] are links to old_map_region()

> > >

> > > The commit 08cfb38f3ef4 ("x86/efi: Unmap EFI boot services code/data

> > > regions from efi_pgd"), unmaps EFI boot services code/data regions

> > > *only* from efi_pgd but efi=old_map maps EFI boot services code/data

> > > regions into swapper_pgd. Also, efi=old_map  uses either

> > > ioremap() or __va(md->phys_addr) to map EFI runtime/boot time services and

> > doesn't use kernel_map_pages_in_pgd().

> > >

> > > So, we need a different unmapping routine to unmap EFI boot services

> > > code/data regions from swapper_pgd if they were mapped using efi=old_map.

> > >

> >

> > For the short term, could we simply make your changes dependent on efi !=

> > old_map? That gives us some time to fix the old_map case properly.

>

> Yes, I think we could and it should work but I hesitated to propose it because

> (as you already noted) it's a short term fix and not the right fix.

>


What is the status here?

> Alternatively, we could also evaluate if we need to support efi=old_map case going further.

> I thought dropping it would be a bad idea because it changes kernel user visible interface

> (because it's a kernel command line argument) and never brought it up.

> Not sure what Thomas, Ingo or Linus might think about dropping a kernel command line

> argument.

>


Dropping a command line argument is not a problem in itself, unless
anyone is actively using it :-)

As far as I can tell, the SGI x86 UV platforms still rely on this, so
we're stuck with it for the foreseeable future.

This means we need a fixes that makes your unmapping code conditional
on !old_memmap. Do you have an ETA for that?
Borislav Petkov Dec. 21, 2018, 5:13 p.m. | #7
On Fri, Dec 21, 2018 at 06:02:29PM +0100, Ard Biesheuvel wrote:
> As far as I can tell, the SGI x86 UV platforms still rely on this, so

> we're stuck with it for the foreseeable future.


What happened with the old apple laptops which couldn't handle high
virtual mappings and needed 1:1? We don't care anymore?

-- 
Regards/Gruss,
    Boris.

Good mailing practices for 400: avoid top-posting and trim the reply.
Ard Biesheuvel Dec. 21, 2018, 5:26 p.m. | #8
On Fri, 21 Dec 2018 at 18:13, Borislav Petkov <bp@alien8.de> wrote:
>

> On Fri, Dec 21, 2018 at 06:02:29PM +0100, Ard Biesheuvel wrote:

> > As far as I can tell, the SGI x86 UV platforms still rely on this, so

> > we're stuck with it for the foreseeable future.

>

> What happened with the old apple laptops which couldn't handle high

> virtual mappings and needed 1:1? We don't care anymore?

>


If that is the case (I wouldn't know) then yes, there is a second
reason why we need to keep this code.
Sai Praneeth Prakhya Dec. 21, 2018, 5:52 p.m. | #9
> > > For the short term, could we simply make your changes dependent on

> > > efi != old_map? That gives us some time to fix the old_map case properly.

> >

> > Yes, I think we could and it should work but I hesitated to propose it

> > because (as you already noted) it's a short term fix and not the right fix.

> >

> 

> What is the status here?


Making the unmapping code conditional on !old_map is ready and I will send it out.
I am working on unmapping boot services code/data when old_map is enabled 
and ran into issues with memblock and direct mapping in kernel. Will post those details 
in a separate thread.

> 

> > Alternatively, we could also evaluate if we need to support efi=old_map case

> going further.

> > I thought dropping it would be a bad idea because it changes kernel

> > user visible interface (because it's a kernel command line argument) and never

> brought it up.

> > Not sure what Thomas, Ingo or Linus might think about dropping a

> > kernel command line argument.

> >

> 

> Dropping a command line argument is not a problem in itself, unless anyone is

> actively using it :-)

> 

> As far as I can tell, the SGI x86 UV platforms still rely on this, so we're stuck with

> it for the foreseeable future.


Thanks (also Boris) for the info. Makes sense why we need efi=old_map.

> 

> This means we need a fixes that makes your unmapping code conditional on

> !old_memmap. Do you have an ETA for that?


Sure! I will do some more testing and if it works as expected, will send it before this Sunday.

Regards,
Sai
Borislav Petkov Dec. 21, 2018, 7:29 p.m. | #10
On Fri, Dec 21, 2018 at 06:26:23PM +0100, Ard Biesheuvel wrote:
> On Fri, 21 Dec 2018 at 18:13, Borislav Petkov <bp@alien8.de> wrote:

> >

> > On Fri, Dec 21, 2018 at 06:02:29PM +0100, Ard Biesheuvel wrote:

> > > As far as I can tell, the SGI x86 UV platforms still rely on this, so

> > > we're stuck with it for the foreseeable future.

> >

> > What happened with the old apple laptops which couldn't handle high

> > virtual mappings and needed 1:1? We don't care anymore?

> >

> 

> If that is the case (I wouldn't know) then yes, there is a second

> reason why we need to keep this code.


Fleming knows details and he's on CC, lemme "pull" him up into To: :-)

-- 
Regards/Gruss,
    Boris.

Good mailing practices for 400: avoid top-posting and trim the reply.
Ard Biesheuvel Dec. 22, 2018, 11:07 a.m. | #11
On Fri, 21 Dec 2018 at 20:29, Borislav Petkov <bp@alien8.de> wrote:
>

> On Fri, Dec 21, 2018 at 06:26:23PM +0100, Ard Biesheuvel wrote:

> > On Fri, 21 Dec 2018 at 18:13, Borislav Petkov <bp@alien8.de> wrote:

> > >

> > > On Fri, Dec 21, 2018 at 06:02:29PM +0100, Ard Biesheuvel wrote:

> > > > As far as I can tell, the SGI x86 UV platforms still rely on this, so

> > > > we're stuck with it for the foreseeable future.

> > >

> > > What happened with the old apple laptops which couldn't handle high

> > > virtual mappings and needed 1:1? We don't care anymore?

> > >

> >

> > If that is the case (I wouldn't know) then yes, there is a second

> > reason why we need to keep this code.

>

> Fleming knows details and he's on CC, lemme "pull" him up into To: :-)

>


IIUC the 1:1 mapping and the 'old' mapping are two different things,
and the new mapping also contains a 1:1 mapping of the boot services
regions, at least until SetVirtualAddressMap() returns.
Matt Fleming Jan. 7, 2019, 3:57 p.m. | #12
On Sat, 22 Dec, at 12:07:48PM, Ard Biesheuvel wrote:
> On Fri, 21 Dec 2018 at 20:29, Borislav Petkov <bp@alien8.de> wrote:

> >

> > On Fri, Dec 21, 2018 at 06:26:23PM +0100, Ard Biesheuvel wrote:

> > > On Fri, 21 Dec 2018 at 18:13, Borislav Petkov <bp@alien8.de> wrote:

> > > >

> > > > On Fri, Dec 21, 2018 at 06:02:29PM +0100, Ard Biesheuvel wrote:

> > > > > As far as I can tell, the SGI x86 UV platforms still rely on this, so

> > > > > we're stuck with it for the foreseeable future.

> > > >

> > > > What happened with the old apple laptops which couldn't handle high

> > > > virtual mappings and needed 1:1? We don't care anymore?

> > > >

> > >

> > > If that is the case (I wouldn't know) then yes, there is a second

> > > reason why we need to keep this code.

> >

> > Fleming knows details and he's on CC, lemme "pull" him up into To: :-)

> >

> 

> IIUC the 1:1 mapping and the 'old' mapping are two different things,

> and the new mapping also contains a 1:1 mapping of the boot services

> regions, at least until SetVirtualAddressMap() returns.


Yep, they're different. And yes the 1:1 mapping should stick around
with the new scheme IIRC (it's been a while).

Patch

diff --git a/arch/x86/platform/efi/quirks.c b/arch/x86/platform/efi/quirks.c
index 95e77a667ba5..09e811b9da26 100644
--- a/arch/x86/platform/efi/quirks.c
+++ b/arch/x86/platform/efi/quirks.c
@@ -369,6 +369,24 @@  void __init efi_reserve_boot_services(void)
 	}
 }
 
+/*
+ * Apart from having VA mappings for EFI boot services code/data regions,
+ * (duplicate) 1:1 mappings were also created as a quirk for buggy firmware. So,
+ * unmap both 1:1 and VA mappings.
+ */
+static void __init efi_unmap_pages(efi_memory_desc_t *md)
+{
+	pgd_t *pgd = efi_mm.pgd;
+	u64 pa = md->phys_addr;
+	u64 va = md->virt_addr;
+
+	if (kernel_unmap_pages_in_pgd(pgd, pa, md->num_pages))
+		pr_err("Failed to unmap 1:1 mapping for 0x%llx\n", pa);
+
+	if (kernel_unmap_pages_in_pgd(pgd, va, md->num_pages))
+		pr_err("Failed to unmap VA mapping for 0x%llx\n", va);
+}
+
 void __init efi_free_boot_services(void)
 {
 	phys_addr_t new_phys, new_size;
@@ -393,6 +411,13 @@  void __init efi_free_boot_services(void)
 			continue;
 		}
 
+		/*
+		 * Before calling set_virtual_address_map(), EFI boot services
+		 * code/data regions were mapped as a quirk for buggy firmware.
+		 * Unmap them from efi_pgd before freeing them up.
+		 */
+		efi_unmap_pages(md);
+
 		/*
 		 * Nasty quirk: if all sub-1MB memory is used for boot
 		 * services, we can get here without having allocated the