Re: [PATCH v3] x86, efi: Calling __pa() with an ioremap'd address is invalid

Re: [PATCH v3] x86, efi: Calling __pa() with an ioremap'd address is invalid

[Matthew Garrett]

On Mon, Nov 07, 2011 at 03:34:48PM +0000, Matt Fleming wrote:

> After the feedback from v1 I tried to unify the efi_ioremap()
> implementations but ran into the issue detailed in the RH bug report
> in the changelog. Unless we teach the x86 setup code that
> EFI_RUNTIME_SERVICES_DATA regions should be part of the direct kernel
> mapping table (even though they're marked as E820_RESERVED) I think
> this patch makes the most sense.

Honestly it seems like there may well be an argument for that. We're 
talking about executable code that the kernel will be calling - it seems 
theoretically neater for it to be added to the direct mapping. We're 
just heavily constrained by our collapsing of the EFI memory map onto 
the rather less fine-grained E820 one and the lack of any obvious way to 
extend that in an OS-specific manner. I guess we could expect the 
bootloader to conform to the standard and then re-walk the EFI memory 
map ourselves to fix things up, but eww...

-- 
Matthew Garrett | mjg59@srcf.ucam.org

Re: [PATCH v3] x86, efi: Calling __pa() with an ioremap'd address is invalid

[H. Peter Anvin]

On 11/07/2011 12:23 PM, Matthew Garrett wrote:
> On Mon, Nov 07, 2011 at 03:34:48PM +0000, Matt Fleming wrote:
> 
>> After the feedback from v1 I tried to unify the efi_ioremap()
>> implementations but ran into the issue detailed in the RH bug report
>> in the changelog. Unless we teach the x86 setup code that
>> EFI_RUNTIME_SERVICES_DATA regions should be part of the direct kernel
>> mapping table (even though they're marked as E820_RESERVED) I think
>> this patch makes the most sense.
> 
> Honestly it seems like there may well be an argument for that. We're 
> talking about executable code that the kernel will be calling - it seems 
> theoretically neater for it to be added to the direct mapping. We're 
> just heavily constrained by our collapsing of the EFI memory map onto 
> the rather less fine-grained E820 one and the lack of any obvious way to 
> extend that in an OS-specific manner. I guess we could expect the 
> bootloader to conform to the standard and then re-walk the EFI memory 
> map ourselves to fix things up, but eww...
> 

Yes, I think it makes a lot of sense.  If we need to introduce new
meta-types to deal with the fact that there are EFI types that don't map
to E820, then so be it... and this is *exactly* why we want the EFI
setup stub to be part of the kernel image and not off in a separate
bootloader, requiring a stable interface...

	-hpa

Re: [PATCH v3] x86, efi: Calling __pa() with an ioremap'd address is invalid

[Matthew Garrett]

On Mon, Nov 07, 2011 at 12:36:13PM -0800, H. Peter Anvin wrote:

> Yes, I think it makes a lot of sense.  If we need to introduce new
> meta-types to deal with the fact that there are EFI types that don't map
> to E820, then so be it... and this is *exactly* why we want the EFI
> setup stub to be part of the kernel image and not off in a separate
> bootloader, requiring a stable interface...

I don't disagree, it's just going to be an absolute pain to manage that 
in a secure boot world. Looking at the code, we actually already seem to 
have decided to start just making up E820 types, so maybe we should just 
do that...

-- 
Matthew Garrett | mjg59@srcf.ucam.org

Re: [PATCH v3] x86, efi: Calling __pa() with an ioremap'd address is invalid

[H. Peter Anvin]

On 11/07/2011 12:37 PM, Matthew Garrett wrote:
> On Mon, Nov 07, 2011 at 12:36:13PM -0800, H. Peter Anvin wrote:
> 
>> Yes, I think it makes a lot of sense.  If we need to introduce new
>> meta-types to deal with the fact that there are EFI types that don't map
>> to E820, then so be it... and this is *exactly* why we want the EFI
>> setup stub to be part of the kernel image and not off in a separate
>> bootloader, requiring a stable interface...
> 
> I don't disagree, it's just going to be an absolute pain to manage that 
> in a secure boot world.

Could you clarify, please?

> Looking at the code, we actually already seem to 
> have decided to start just making up E820 types, so maybe we should just 
> do that... 

That's what I just said, no?  I think we use negative numbers for those.

	-hpa

Re: [PATCH v3] x86, efi: Calling __pa() with an ioremap'd address is invalid

[Matthew Garrett]

On Mon, Nov 07, 2011 at 12:45:31PM -0800, H. Peter Anvin wrote:
> On 11/07/2011 12:37 PM, Matthew Garrett wrote:
> > On Mon, Nov 07, 2011 at 12:36:13PM -0800, H. Peter Anvin wrote:
> > 
> >> Yes, I think it makes a lot of sense.  If we need to introduce new
> >> meta-types to deal with the fact that there are EFI types that don't map
> >> to E820, then so be it... and this is *exactly* why we want the EFI
> >> setup stub to be part of the kernel image and not off in a separate
> >> bootloader, requiring a stable interface...
> > 
> > I don't disagree, it's just going to be an absolute pain to manage that 
> > in a secure boot world.
> 
> Could you clarify, please?

If the kernel is able to call boot services then the kernel needs to be 
signed. If it's all handled by the bootloader then the bootloader can be 
signed and the kernel doesn't have to be. Depends which one people 
update more, I guess.

-- 
Matthew Garrett | mjg59@srcf.ucam.org

Re: [PATCH v3] x86, efi: Calling __pa() with an ioremap'd address is invalid

[H. Peter Anvin]

On 11/07/2011 12:48 PM, Matthew Garrett wrote:
> 
> If the kernel is able to call boot services then the kernel needs to be 
> signed. If it's all handled by the bootloader then the bootloader can be 
> signed and the kernel doesn't have to be. Depends which one people 
> update more, I guess.
> 

... and what security attributes they are looking for.

However, "EFI stub in the kernel" doesn't mean "can't use an external
bootloader."

	-hpa

Re: [PATCH v3] x86, efi: Calling __pa() with an ioremap'd address is invalid

[Matthew Garrett]

On Mon, Nov 07, 2011 at 12:57:40PM -0800, H. Peter Anvin wrote:
> On 11/07/2011 12:48 PM, Matthew Garrett wrote:
> > 
> > If the kernel is able to call boot services then the kernel needs to be 
> > signed. If it's all handled by the bootloader then the bootloader can be 
> > signed and the kernel doesn't have to be. Depends which one people 
> > update more, I guess.
> > 
> 
> ... and what security attributes they are looking for.

Yup.

> However, "EFI stub in the kernel" doesn't mean "can't use an external
> bootloader."

Agreed. It just means that we're still plausibly going to need some 
handshaking between them. Alternatively, as long as the bootloader 
passes us the memory map, we can just ignore any E820 map it gives us 
anyway.

-- 
Matthew Garrett | mjg59@srcf.ucam.org

Re: [PATCH v3] x86, efi: Calling __pa() with an ioremap'd address is invalid

[H. Peter Anvin]

On 11/07/2011 12:58 PM, Matthew Garrett wrote:
> 
>> However, "EFI stub in the kernel" doesn't mean "can't use an external
>> bootloader."
> 
> Agreed. It just means that we're still plausibly going to need some 
> handshaking between them. Alternatively, as long as the bootloader 
> passes us the memory map, we can just ignore any E820 map it gives us 
> anyway.
> 

I know we need to be able to pass the initramfs in memory; anything else
we need other than the normal EFI executable entry conditions?

	-hpa

Re: [PATCH v3] x86, efi: Calling __pa() with an ioremap'd address is invalid

[Matthew Garrett]

On Mon, Nov 07, 2011 at 01:01:40PM -0800, H. Peter Anvin wrote:
> On 11/07/2011 12:58 PM, Matthew Garrett wrote:
> > 
> >> However, "EFI stub in the kernel" doesn't mean "can't use an external
> >> bootloader."
> > 
> > Agreed. It just means that we're still plausibly going to need some 
> > handshaking between them. Alternatively, as long as the bootloader 
> > passes us the memory map, we can just ignore any E820 map it gives us 
> > anyway.
> > 
> 
> I know we need to be able to pass the initramfs in memory; anything else
> we need other than the normal EFI executable entry conditions?

If we're called before ExitBootServices(), no. If we're called after, 
we'll need the map from GetMemoryMap(). There's some other things that 
we may want to pass, such as option ROMs that we can get from firmware 
but which may not otherwise be mapped - I guess those could arguably be 
passed in the initramfs.

-- 
Matthew Garrett | mjg59@srcf.ucam.org

Re: [PATCH v3] x86, efi: Calling __pa() with an ioremap'd address is invalid

[H. Peter Anvin]

On 11/07/2011 01:07 PM, Matthew Garrett wrote:
>>
>> I know we need to be able to pass the initramfs in memory; anything else
>> we need other than the normal EFI executable entry conditions?
> 
> If we're called before ExitBootServices(), no. If we're called after, 
> we'll need the map from GetMemoryMap(). There's some other things that 
> we may want to pass, such as option ROMs that we can get from firmware 
> but which may not otherwise be mapped - I guess those could arguably be 
> passed in the initramfs.
> 

No, we should be called before ExitBootServices(), obviously.

That was a significant part of the point.

Getting a list of the stuff we need would be useful, so we can come up
with a workable ABI for passing it.

	-hpa

Re: [PATCH v3] x86, efi: Calling __pa() with an ioremap'd address is invalid

[Matthew Garrett]

On Mon, Nov 07, 2011 at 01:20:45PM -0800, H. Peter Anvin wrote:
> On 11/07/2011 01:07 PM, Matthew Garrett wrote:
> >>
> >> I know we need to be able to pass the initramfs in memory; anything else
> >> we need other than the normal EFI executable entry conditions?
> > 
> > If we're called before ExitBootServices(), no. If we're called after, 
> > we'll need the map from GetMemoryMap(). There's some other things that 
> > we may want to pass, such as option ROMs that we can get from firmware 
> > but which may not otherwise be mapped - I guess those could arguably be 
> > passed in the initramfs.
> > 
> 
> No, we should be called before ExitBootServices(), obviously.
<
> That was a significant part of the point.

I know. But we also have to handle being called after 
ExitBootServices(). There are going to be people who don't want to deal 
with signing their kernel builds.
 
-- 
Matthew Garrett | mjg59@srcf.ucam.org

Re: [PATCH v3] x86, efi: Calling __pa() with an ioremap'd address is invalid

[H. Peter Anvin]

On 11/07/2011 01:23 PM, Matthew Garrett wrote:
> 
> I know. But we also have to handle being called after 
> ExitBootServices(). There are going to be people who don't want to deal 
> with signing their kernel builds.
>  

Could you clarify why these two are connected?

	-hpa

Re: [PATCH v3] x86, efi: Calling __pa() with an ioremap'd address is invalid

[Matthew Garrett]

On Mon, Nov 07, 2011 at 01:34:01PM -0800, H. Peter Anvin wrote:
> On 11/07/2011 01:23 PM, Matthew Garrett wrote:
> > 
> > I know. But we also have to handle being called after 
> > ExitBootServices(). There are going to be people who don't want to deal 
> > with signing their kernel builds.
> >  
> 
> Could you clarify why these two are connected?

You can't execute anything unsigned until after ExitBootServices() has 
been called. Otherwise your bootloader is also a malware loader, and 
your signature probably gets blacklisted.

-- 
Matthew Garrett | mjg59@srcf.ucam.org

Re: [PATCH v3] x86, efi: Calling __pa() with an ioremap'd address is invalid

[Alan Cox]

> I know. But we also have to handle being called after 
> ExitBootServices(). There are going to be people who don't want to deal 
> with signing their kernel builds.

So they can keep their system in setup mode, or we can add signing to the
distro install script so your make install still gets it right.

Alan

Re: [PATCH v3] x86, efi: Calling __pa() with an ioremap'd address is invalid

[huang ying]

On Tue, Nov 8, 2011 at 4:23 AM, Matthew Garrett <mjg59@srcf.ucam.org> wrote:
> On Mon, Nov 07, 2011 at 03:34:48PM +0000, Matt Fleming wrote:
>
>> After the feedback from v1 I tried to unify the efi_ioremap()
>> implementations but ran into the issue detailed in the RH bug report
>> in the changelog. Unless we teach the x86 setup code that
>> EFI_RUNTIME_SERVICES_DATA regions should be part of the direct kernel
>> mapping table (even though they're marked as E820_RESERVED) I think
>> this patch makes the most sense.
>
> Honestly it seems like there may well be an argument for that. We're
> talking about executable code that the kernel will be calling - it seems
> theoretically neater for it to be added to the direct mapping. We're
> just heavily constrained by our collapsing of the EFI memory map onto
> the rather less fine-grained E820 one and the lack of any obvious way to
> extend that in an OS-specific manner. I guess we could expect the
> bootloader to conform to the standard and then re-walk the EFI memory
> map ourselves to fix things up, but eww...

Sorry for late.

Why ioremap_cache() can not work for some EFI_RUNTIME_SERVICES_DATA region?

Best Regards,
Huang Ying

Re: [PATCH v3] x86, efi: Calling __pa() with an ioremap'd address is invalid

[Matt Fleming]

On Fri, 2011-11-11 at 09:02 +0800, huang ying wrote:
> On Tue, Nov 8, 2011 at 4:23 AM, Matthew Garrett <mjg59@srcf.ucam.org> wrote:
> > On Mon, Nov 07, 2011 at 03:34:48PM +0000, Matt Fleming wrote:
> >
> >> After the feedback from v1 I tried to unify the efi_ioremap()
> >> implementations but ran into the issue detailed in the RH bug report
> >> in the changelog. Unless we teach the x86 setup code that
> >> EFI_RUNTIME_SERVICES_DATA regions should be part of the direct kernel
> >> mapping table (even though they're marked as E820_RESERVED) I think
> >> this patch makes the most sense.
> >
> > Honestly it seems like there may well be an argument for that. We're
> > talking about executable code that the kernel will be calling - it seems
> > theoretically neater for it to be added to the direct mapping. We're
> > just heavily constrained by our collapsing of the EFI memory map onto
> > the rather less fine-grained E820 one and the lack of any obvious way to
> > extend that in an OS-specific manner. I guess we could expect the
> > bootloader to conform to the standard and then re-walk the EFI memory
> > map ourselves to fix things up, but eww...
> 
> Sorry for late.
> 
> Why ioremap_cache() can not work for some EFI_RUNTIME_SERVICES_DATA region?

It looks like ioremap_cache() maps the region with the NX bit set.

-- 
Matt Fleming, Intel Open Source Technology Center

Re: [PATCH v3] x86, efi: Calling __pa() with an ioremap'd address is invalid

[Matt Fleming]

On Fri, 2011-11-11 at 13:12 +0000, Matt Fleming wrote:
> On Fri, 2011-11-11 at 09:02 +0800, huang ying wrote:
> > On Tue, Nov 8, 2011 at 4:23 AM, Matthew Garrett <mjg59@srcf.ucam.org> wrote:
> > > On Mon, Nov 07, 2011 at 03:34:48PM +0000, Matt Fleming wrote:
> > >
> > >> After the feedback from v1 I tried to unify the efi_ioremap()
> > >> implementations but ran into the issue detailed in the RH bug report
> > >> in the changelog. Unless we teach the x86 setup code that
> > >> EFI_RUNTIME_SERVICES_DATA regions should be part of the direct kernel
> > >> mapping table (even though they're marked as E820_RESERVED) I think
> > >> this patch makes the most sense.
> > >
> > > Honestly it seems like there may well be an argument for that. We're
> > > talking about executable code that the kernel will be calling - it seems
> > > theoretically neater for it to be added to the direct mapping. We're
> > > just heavily constrained by our collapsing of the EFI memory map onto
> > > the rather less fine-grained E820 one and the lack of any obvious way to
> > > extend that in an OS-specific manner. I guess we could expect the
> > > bootloader to conform to the standard and then re-walk the EFI memory
> > > map ourselves to fix things up, but eww...
> > 
> > Sorry for late.
> > 
> > Why ioremap_cache() can not work for some EFI_RUNTIME_SERVICES_DATA region?
> 
> It looks like ioremap_cache() maps the region with the NX bit set.

Sorry, ignore this nonsensical answer, obviously we shouldn't be
executing data.

Honestly, I'm not sure why it doesn't work.

-- 
Matt Fleming, Intel Open Source Technology Center

[PATCH v3] x86, efi: Calling __pa() with an ioremap'd address is invalid

[Matt Fleming]

From: Matt Fleming <matt.fleming@intel.com>

(Sorry for people seeing this email twice)

If we encounter an efi_memory_desc_t without EFI_MEMORY_WB set in
->attribute we currently call set_memory_uc(), which in turn calls
__pa() on a potentially ioremap'd address. On CONFIG_X86_32 this is
invalid, resulting in the following oops,

  BUG: unable to handle kernel paging request at f7f22280
  IP: [<c10257b9>] reserve_ram_pages_type+0x89/0x210
  *pdpt = 0000000001978001 *pde = 0000000001ffb067 *pte = 0000000000000000
  Oops: 0000 [#1] PREEMPT SMP
  Modules linked in:

  Pid: 0, comm: swapper Not tainted 3.0.0-acpi-efi-0805 #3
   EIP: 0060:[<c10257b9>] EFLAGS: 00010202 CPU: 0
   EIP is at reserve_ram_pages_type+0x89/0x210
   EAX: 0070e280 EBX: 38714000 ECX: f7814000 EDX: 00000000
   ESI: 00000000 EDI: 38715000 EBP: c189fef0 ESP: c189fea8
   DS: 007b ES: 007b FS: 00d8 GS: 0000 SS: 0068
  Process swapper (pid: 0, ti=c189e000 task=c18bbe60 task.ti=c189e000)
  Stack:
   80000200 ff108000 00000000 c189ff00 00038714 00000000 00000000 c189fed0
   c104f8ca 00038714 00000000 00038715 00000000 00000000 00038715 00000000
   00000010 38715000 c189ff48 c1025aff 38715000 00000000 00000010 00000000
  Call Trace:
   [<c104f8ca>] ? page_is_ram+0x1a/0x40
   [<c1025aff>] reserve_memtype+0xdf/0x2f0
   [<c1024dc9>] set_memory_uc+0x49/0xa0
   [<c19334d0>] efi_enter_virtual_mode+0x1c2/0x3aa
   [<c19216d4>] start_kernel+0x291/0x2f2
   [<c19211c7>] ? loglevel+0x1b/0x1b
   [<c19210bf>] i386_start_kernel+0xbf/0xc8

So, if we're ioremap'ing an address range let's setup the mapping with
the correct caching attribute from the start instead of modifying it
after the fact. The uncached case can be handled by ioremap_nocache()
for both 32-bit and 64-bit. However, in the cached case we need to
handle the mapping differently. CONFIG_X86_64 can simply extend the
direct kernel mapping to include the address range, whereas on
CONFIG_X86_32 we need to use ioremap_cache().

Despite first impressions, it's not possible to use ioremap_cache()
for CONFIG_X86_64 because of the way that the memory map might be
configured as detailed in the following bug report,

	https://bugzilla.redhat.com/show_bug.cgi?id=748516

We need to be careful to ensure that any EFI_RUNTIME_SERVICES_DATA
regions are covered by the direct kernel mapping table on
CONFIG_X86_64, which unfortunately means we can't unify the 32-bit and
64-bit efi_ioremap() implementations.

Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Ingo Molnar <mingo@elte.hu>
Cc: H. Peter Anvin <hpa@zytor.com>
Cc: Matthew Garrett <mjg@redhat.com>
Cc: Zhang Rui <rui.zhang@intel.com>
Cc: Huang Ying <huang.ying.caritas@gmail.com>
Cc: stable@kernel.org
Signed-off-by: Matt Fleming <matt.fleming@intel.com>
---

The two previous version of this patch can be found here,

 1. https://lkml.org/lkml/2011/10/11/157
 2. http://lkml.org/lkml/2011/10/14/155

After the feedback from v1 I tried to unify the efi_ioremap()
implementations but ran into the issue detailed in the RH bug report
in the changelog. Unless we teach the x86 setup code that
EFI_RUNTIME_SERVICES_DATA regions should be part of the direct kernel
mapping table (even though they're marked as E820_RESERVED) I think
this patch makes the most sense.

 arch/x86/include/asm/efi.h     |    5 ++---
 arch/x86/platform/efi/efi.c    |   24 ++++++++++++++----------
 arch/x86/platform/efi/efi_64.c |    8 ++------
 3 files changed, 18 insertions(+), 19 deletions(-)

diff --git a/arch/x86/include/asm/efi.h b/arch/x86/include/asm/efi.h
index 7093e4a..940107d 100644
--- a/arch/x86/include/asm/efi.h
+++ b/arch/x86/include/asm/efi.h
@@ -33,7 +33,7 @@ extern unsigned long asmlinkage efi_call_phys(void *, ...);
 #define efi_call_virt6(f, a1, a2, a3, a4, a5, a6)	\
 	efi_call_virt(f, a1, a2, a3, a4, a5, a6)
 
-#define efi_ioremap(addr, size, type)		ioremap_cache(addr, size)
+#define efi_ioremap(addr, size)		ioremap_cache(addr, size)
 
 #else /* !CONFIG_X86_32 */
 
@@ -84,8 +84,7 @@ extern u64 efi_call6(void *fp, u64 arg1, u64 arg2, u64 arg3,
 	efi_call6((void *)(efi.systab->runtime->f), (u64)(a1), (u64)(a2), \
 		  (u64)(a3), (u64)(a4), (u64)(a5), (u64)(a6))
 
-extern void __iomem *efi_ioremap(unsigned long addr, unsigned long size,
-				 u32 type);
+extern void __iomem *efi_ioremap(unsigned long addr, unsigned long size);
 
 #endif /* CONFIG_X86_32 */
 
diff --git a/arch/x86/platform/efi/efi.c b/arch/x86/platform/efi/efi.c
index 3ae4128..8f2f7f6 100644
--- a/arch/x86/platform/efi/efi.c
+++ b/arch/x86/platform/efi/efi.c
@@ -670,10 +670,21 @@ void __init efi_enter_virtual_mode(void)
 		end_pfn = PFN_UP(end);
 		if (end_pfn <= max_low_pfn_mapped
 		    || (end_pfn > (1UL << (32 - PAGE_SHIFT))
-			&& end_pfn <= max_pfn_mapped))
+			&& end_pfn <= max_pfn_mapped)) {
 			va = __va(md->phys_addr);
-		else
-			va = efi_ioremap(md->phys_addr, size, md->type);
+
+			if (!(md->attribute & EFI_MEMORY_WB)) {
+				addr = (u64) (unsigned long)va;
+				npages = md->num_pages;
+				memrange_efi_to_native(&addr, &npages);
+				set_memory_uc(addr, npages);
+			}
+		} else {
+			if (!(md->attribute & EFI_MEMORY_WB))
+				va = ioremap_nocache(md->phys_addr, size);
+			else
+				va = efi_ioremap(md->phys_addr, size);
+		}
 
 		md->virt_addr = (u64) (unsigned long) va;
 
@@ -683,13 +694,6 @@ void __init efi_enter_virtual_mode(void)
 			continue;
 		}
 
-		if (!(md->attribute & EFI_MEMORY_WB)) {
-			addr = md->virt_addr;
-			npages = md->num_pages;
-			memrange_efi_to_native(&addr, &npages);
-			set_memory_uc(addr, npages);
-		}
-
 		systab = (u64) (unsigned long) efi_phys.systab;
 		if (md->phys_addr <= systab && systab < end) {
 			systab += md->virt_addr - md->phys_addr;
diff --git a/arch/x86/platform/efi/efi_64.c b/arch/x86/platform/efi/efi_64.c
index ac3aa54..1f4b86b 100644
--- a/arch/x86/platform/efi/efi_64.c
+++ b/arch/x86/platform/efi/efi_64.c
@@ -81,18 +81,14 @@ void __init efi_call_phys_epilog(void)
 	early_code_mapping_set_exec(0);
 }
 
-void __iomem *__init efi_ioremap(unsigned long phys_addr, unsigned long size,
-				 u32 type)
+void __iomem *__init efi_ioremap(unsigned long phys_addr, unsigned long size)
 {
 	unsigned long last_map_pfn;
 
-	if (type == EFI_MEMORY_MAPPED_IO)
-		return ioremap(phys_addr, size);
-
 	last_map_pfn = init_memory_mapping(phys_addr, phys_addr + size);
 	if ((last_map_pfn << PAGE_SHIFT) < phys_addr + size) {
 		unsigned long top = last_map_pfn << PAGE_SHIFT;
-		efi_ioremap(top, size - (top - phys_addr), type);
+		efi_ioremap(top, size - (top - phys_addr));
 	}
 
 	return (void __iomem *)__va(phys_addr);
-- 
1.7.4.4