2013-06-02 22:12:53

by Matthew Garrett

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

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 <[email protected]>
---
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

2013-06-04 06:58:43

by Matt Fleming

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

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 <[email protected]>
> ---
> 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;

2013-06-04 12:27:46

by Matthew Garrett

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

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 | [email protected]
????{.n?+???????+%?????ݶ??w??{.n?+????{??G?????{ay?ʇڙ?,j??f???h?????????z_??(?階?ݢj"???m??????G????????????&???~???iO???z??v?^?m????????????I?

2013-06-06 13:23:31

by Matt Fleming

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

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 <[email protected]>
> ---
> 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.