[PATCH] UEFI: Don't pass boot services regions to SetVirtualAddressMap()

[PATCH] UEFI: Don't pass boot services regions to SetVirtualAddressMap()

[Matthew Garrett]

We need to map boot services regions during startup in order to avoid
firmware bugs, but we shouldn't be passing those regions to
SetVirtualAddressMap(). Ensure that we're only passing regions that are
marked as being mapped at runtime.

Signed-off-by: Matthew Garrett <matthew.garrett@nebula.com>
---
 arch/x86/platform/efi/efi.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/arch/x86/platform/efi/efi.c b/arch/x86/platform/efi/efi.c
index 63e167a..add0e37 100644
--- a/arch/x86/platform/efi/efi.c
+++ b/arch/x86/platform/efi/efi.c
@@ -922,6 +922,13 @@ void __init efi_enter_virtual_mode(void)
 			va = efi_ioremap(md->phys_addr, size,
 					 md->type, md->attribute);
 
+		if (!(md->attribute & EFI_MEMORY_RUNTIME)) {
+			if (!va)
+				pr_err("ioremap of 0x%llX failed!\n",
+				       (unsigned long long)md->phys_addr);
+			continue;
+		}
+
 		md->virt_addr = (u64) (unsigned long) va;
 
 		if (!va) {
-- 
1.8.1.4

Re: [PATCH] UEFI: Don't pass boot services regions to SetVirtualAddressMap()

[Matt Fleming]

On 06/02/2013 11:12 PM, Matthew Garrett wrote:
> We need to map boot services regions during startup in order to avoid
> firmware bugs, but we shouldn't be passing those regions to
> SetVirtualAddressMap(). Ensure that we're only passing regions that are
> marked as being mapped at runtime.
> 
> Signed-off-by: Matthew Garrett <matthew.garrett@nebula.com>
> ---
>  arch/x86/platform/efi/efi.c | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/arch/x86/platform/efi/efi.c b/arch/x86/platform/efi/efi.c
> index 63e167a..add0e37 100644
> --- a/arch/x86/platform/efi/efi.c
> +++ b/arch/x86/platform/efi/efi.c
> @@ -922,6 +922,13 @@ void __init efi_enter_virtual_mode(void)
>  			va = efi_ioremap(md->phys_addr, size,
>  					 md->type, md->attribute);
>  
> +		if (!(md->attribute & EFI_MEMORY_RUNTIME)) {
> +			if (!va)
> +				pr_err("ioremap of 0x%llX failed!\n",
> +				       (unsigned long long)md->phys_addr);
> +			continue;
> +		}
> +
>  		md->virt_addr = (u64) (unsigned long) va;
>  
>  		if (!va) {
> 

Is there any point in calling efi_ioremap() for non-runtime regions in
this case? In other words, why not the following patch?

diff --git a/arch/x86/platform/efi/efi.c b/arch/x86/platform/efi/efi.c
index 63e167a..a438ed3 100644
--- a/arch/x86/platform/efi/efi.c
+++ b/arch/x86/platform/efi/efi.c
@@ -903,9 +903,7 @@ void __init efi_enter_virtual_mode(void)
 
 	for (p = memmap.map; p < memmap.map_end; p += memmap.desc_size) {
 		md = p;
-		if (!(md->attribute & EFI_MEMORY_RUNTIME) &&
-		    md->type != EFI_BOOT_SERVICES_CODE &&
-		    md->type != EFI_BOOT_SERVICES_DATA)
+		if (!(md->attribute & EFI_MEMORY_RUNTIME))
 			continue;
 
 		size = md->num_pages << EFI_PAGE_SHIFT;

Re: [PATCH] UEFI: Don't pass boot services regions to SetVirtualAddressMap()

[Matthew Garrett]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 472 bytes --]

On Tue, 2013-06-04 at 07:58 +0100, Matt Fleming wrote:

> Is there any point in calling efi_ioremap() for non-runtime regions in
> this case? In other words, why not the following patch?

Yeah, we need the boottime services regions to be mapped while we make
this call.
-- 
Matthew Garrett | mjg59@srcf.ucam.org
ÿôèº{.nÇ+‰·Ÿ®‰­†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a­¢f£¢·hšïêÿ‘êçz_è®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨è­Ú&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥

Re: [PATCH] UEFI: Don't pass boot services regions to SetVirtualAddressMap()

[Matt Fleming]

On 02/06/13 23:12, Matthew Garrett wrote:
> We need to map boot services regions during startup in order to avoid
> firmware bugs, but we shouldn't be passing those regions to
> SetVirtualAddressMap(). Ensure that we're only passing regions that are
> marked as being mapped at runtime.
> 
> Signed-off-by: Matthew Garrett <matthew.garrett@nebula.com>
> ---
>  arch/x86/platform/efi/efi.c | 7 +++++++
>  1 file changed, 7 insertions(+)

This doesn't fix a crash or regression, right? If it's a "this just
makes more sense" patch, I'll queue it up in the 'next' branch for
v3.11.