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
