| Message ID | 20181129171230.18699-6-ard.biesheuvel@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | EFI updates | expand |
> 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
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.
> > 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
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.
> > > > 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
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?
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.
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.
> > > 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
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.
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.
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).
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