Re: [tip:efi/core] x86/efi: Unmap EFI boot services code/data regions from efi_pgd
On Sat, 22 Dec, at 12:07:48PM, Ard Biesheuvel wrote: > On Fri, 21 Dec 2018 at 20:29, Borislav Petkov wrote: > > > > On Fri, Dec 21, 2018 at 06:26:23PM +0100, Ard Biesheuvel wrote: > > > On Fri, 21 Dec 2018 at 18:13, Borislav Petkov 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).
Re: [tip:efi/core] x86/efi: Unmap EFI boot services code/data regions from efi_pgd
On Fri, 21 Dec 2018 at 20:29, Borislav Petkov wrote: > > On Fri, Dec 21, 2018 at 06:26:23PM +0100, Ard Biesheuvel wrote: > > On Fri, 21 Dec 2018 at 18:13, Borislav Petkov 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.
Re: [tip:efi/core] x86/efi: Unmap EFI boot services code/data regions from efi_pgd
On Fri, Dec 21, 2018 at 06:26:23PM +0100, Ard Biesheuvel wrote: > On Fri, 21 Dec 2018 at 18:13, Borislav Petkov 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.
RE: [tip:efi/core] x86/efi: Unmap EFI boot services code/data regions from efi_pgd
> > > 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
Re: [tip:efi/core] x86/efi: Unmap EFI boot services code/data regions from efi_pgd
On Fri, 21 Dec 2018 at 18:13, Borislav Petkov 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.
Re: [tip:efi/core] x86/efi: Unmap EFI boot services code/data regions from efi_pgd
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.
Re: [tip:efi/core] x86/efi: Unmap EFI boot services code/data regions from efi_pgd
On Mon, 17 Dec 2018 at 20:48, Prakhya, Sai Praneeth 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?
RE: [tip:efi/core] x86/efi: Unmap EFI boot services code/data regions from efi_pgd
> > > > 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
Re: [tip:efi/core] x86/efi: Unmap EFI boot services code/data regions from efi_pgd
On Mon, 17 Dec 2018 at 19:42, Prakhya, Sai Praneeth 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.
RE: [tip:efi/core] x86/efi: Unmap EFI boot services code/data regions from efi_pgd
> > 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
Re: [tip:efi/core] x86/efi: Unmap EFI boot services code/data regions from efi_pgd
On Mon, 17 Dec 2018 at 19:06, Prakhya, Sai Praneeth wrote: > > > Commit-ID: 08cfb38f3ef49cfd1bba11a00401451606477d80 > > Gitweb: > > https://git.kernel.org/tip/08cfb38f3ef49cfd1bba11a00401451606477d80 > > Author: Sai Praneeth Prakhya > > AuthorDate: Thu, 29 Nov 2018 18:12:24 +0100 > > Committer: Ingo Molnar > > 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.
RE: [tip:efi/core] x86/efi: Unmap EFI boot services code/data regions from efi_pgd
> Commit-ID: 08cfb38f3ef49cfd1bba11a00401451606477d80 > Gitweb: > https://git.kernel.org/tip/08cfb38f3ef49cfd1bba11a00401451606477d80 > Author: Sai Praneeth Prakhya > AuthorDate: Thu, 29 Nov 2018 18:12:24 +0100 > Committer: Ingo Molnar > 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
[tip:efi/core] x86/efi: Unmap EFI boot services code/data regions from efi_pgd
Commit-ID: 08cfb38f3ef49cfd1bba11a00401451606477d80 Gitweb: https://git.kernel.org/tip/08cfb38f3ef49cfd1bba11a00401451606477d80 Author: Sai Praneeth Prakhya AuthorDate: Thu, 29 Nov 2018 18:12:24 +0100 Committer: Ingo Molnar 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. Signed-off-by: Sai Praneeth Prakhya Signed-off-by: Ard Biesheuvel Acked-by: Thomas Gleixner Cc: Andy Lutomirski Cc: Arend van Spriel Cc: Bhupesh Sharma Cc: Borislav Petkov Cc: Dave Hansen Cc: Eric Snowberg Cc: Hans de Goede Cc: Joe Perches Cc: Jon Hunter Cc: Julien Thierry Cc: Linus Torvalds Cc: Marc Zyngier Cc: Matt Fleming Cc: Nathan Chancellor Cc: Peter Zijlstra Cc: Sedat Dilek Cc: YiFei Zhu Cc: linux-...@vger.kernel.org Link: http://lkml.kernel.org/r/20181129171230.18699-6-ard.biesheu...@linaro.org Signed-off-by: Ingo Molnar --- arch/x86/platform/efi/quirks.c | 25 + 1 file changed, 25 insertions(+) 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
[tip:efi/core] x86/efi: Unmap EFI boot services code/data regions from efi_pgd
Commit-ID: 08cfb38f3ef49cfd1bba11a00401451606477d80 Gitweb: https://git.kernel.org/tip/08cfb38f3ef49cfd1bba11a00401451606477d80 Author: Sai Praneeth Prakhya AuthorDate: Thu, 29 Nov 2018 18:12:24 +0100 Committer: Ingo Molnar 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. Signed-off-by: Sai Praneeth Prakhya Signed-off-by: Ard Biesheuvel Acked-by: Thomas Gleixner Cc: Andy Lutomirski Cc: Arend van Spriel Cc: Bhupesh Sharma Cc: Borislav Petkov Cc: Dave Hansen Cc: Eric Snowberg Cc: Hans de Goede Cc: Joe Perches Cc: Jon Hunter Cc: Julien Thierry Cc: Linus Torvalds Cc: Marc Zyngier Cc: Matt Fleming Cc: Nathan Chancellor Cc: Peter Zijlstra Cc: Sedat Dilek Cc: YiFei Zhu Cc: linux-...@vger.kernel.org Link: http://lkml.kernel.org/r/20181129171230.18699-6-ard.biesheu...@linaro.org Signed-off-by: Ingo Molnar --- arch/x86/platform/efi/quirks.c | 25 + 1 file changed, 25 insertions(+) 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