arm*: efi: drop permanent mapping of the UEFI System table
PATCH 1/1 arm*: efi: drop permanent mapping of the UEFI System table
The permanent, writable mapping of the UEFI System table is only referenced during invocations of UEFI Runtime Services, at which time the UEFI virtual mapping is available, which also covers the system table (since the runtime services themselves need access to it)
So instead of creating this permanent mapping, record the virtual address of the system table inside the UEFI virtual mapping, and use that instead. This protects the contents of the system table from inadvertent (or deliberate) modification.
---
Something I spotted while working on the memory attribute table stuff. Since this is low hanging fruit and otherwise completely unrelated to it, I am posting it as a separate patch
drivers/firmware/efi/arm-init.c | 2 ++ drivers/firmware/efi/arm-runtime.c | 24 ++++++++++---------- 2 files changed, 14 insertions(+), 12 deletions(-)
drivers/firmware/efi/arm-init.c +2 -0
@@ -85,6 +85,8 @@ static int __init uefi_init(void) efi.systab->hdr.revision >>16 , efi.systab->hdr.revision & 0xffff); + efi.runtime_version = efi.systab->hdr.revision; +/* Show what we know for posterity */ c16 =early_memremap_ro (efi_to_phys (efi.systab->fw_vendor),sizeof (vendor) *sizeof (efi_char16_t));
drivers/firmware/efi/arm-runtime.c +12 -12
@@ -64,6 +64,16 @@ static bool __init efi_virtmap_init(void) &phys, ret);return false; } +/* +* If this entry covers the address of the UEFI system table, +* calculate and record its virtual address. +*/ +if (efi_system_table >= phys && + efi_system_table < phys + (md->num_pages *EFI_PAGE_SIZE )) { + efi.systab = (void *)(efi_system_table - phys + + md->virt_addr); +set_bit (EFI_SYSTEM_TABLES , &efi.flags); + } }if (efi_memattr_apply_permissions (&efi_mm, efi_set_mapping_permissions))
@@ -102,16 +112,8 @@ static int __init arm_enable_runtime_services(void) memmap.map_end = memmap.map + mapsize; efi.memmap = &memmap; - efi.systab = (__force void *)ioremap_cache (efi_system_table, -sizeof (efi_system_table_t)); -if (!efi.systab) { -pr_err ("Failed to remap EFI System Table\n" ); -return -ENOMEM ; - } -set_bit (EFI_SYSTEM_TABLES , &efi.flags); - -if (!efi_virtmap_init ()) { -pr_err ("No UEFI virtual mapping was installed -- runtime services will not be available\n" ); +if (!efi_virtmap_init () || !efi_enabled (EFI_SYSTEM_TABLES )) { +pr_err ("UEFI virtual mapping missing or invalid -- runtime services will not be available\n" );return -ENOMEM ; }
@@ -119,8 +121,6 @@ static int __init arm_enable_runtime_services(void)efi_native_runtime_setup ();set_bit (EFI_RUNTIME_SERVICES , &efi.flags); - efi.runtime_version = efi.systab->hdr.revision; -return 0 ; }early_initcall (arm_enable_runtime_services);
Signature · 1 line
2.5.0
PATCH 1/1 arm*: efi: drop permanent mapping of the UEFI System table
Quoted 4 lines · Ard Biesheuvel
The permanent, writable mapping of the UEFI System table is only referenced during invocations of UEFI Runtime Services, at which time the UEFI virtual mapping is available, which also covers the system table (since the runtime services themselves need access to it)
I'm not sure it's strictly true that the runtime services themselves need access to the system table. Why would that be necessary? Are runtime services mandated to indirect calls via the system table?
I would expect that the EFI system table and EFI runtime services table are not in EfiConventionalMemory, but I'm not sure that we have the guarnatee that they're both in EfiRuntimeServices* memory..
Quoted 1 line
From the spec, I couldn't find a mandate that the system table (nor the
runtime services table) were in a region of EfiRuntimeServicesData memory. I suspect I'm looking in the wrong place...
Quoted 37 lines
So instead of creating this permanent mapping, record the virtual address of the system table inside the UEFI virtual mapping, and use that instead. This protects the contents of the system table from inadvertent (or deliberate) modification.
Signed-off-by Ard Biesheuvel <[email protected]>---
Something I spotted while working on the memory attribute table stuff. Since this is low hanging fruit and otherwise completely unrelated to it, I am posting it as a separate patch
drivers/firmware/efi/arm-init.c | 2 ++ drivers/firmware/efi/arm-runtime.c | 24 ++++++++++---------- 2 files changed, 14 insertions(+), 12 deletions(-)drivers/firmware/efi/arm-init.c +2
-0@@ -85,6 +85,8 @@ static int __init uefi_init(void) efi.systab->hdr.revision >>16 , efi.systab->hdr.revision & 0xffff); + efi.runtime_version = efi.systab->hdr.revision; +/* Show what we know for posterity */ c16 =early_memremap_ro (efi_to_phys (efi.systab->fw_vendor),sizeof (vendor) *sizeof (efi_char16_t));drivers/firmware/efi/arm-runtime.c +0
-0@@ -64,6 +64,16 @@ static bool __init efi_virtmap_init(void) &phys, ret);return false; }
drivers/firmware/efi/arm-runtime.c static bool __init efi_virtmap_init(void) · line 76
Quoted 10 lines
drivers/firmware/efi/arm-runtime.c +10
-0+/* +* If this entry covers the address of the UEFI system table, +* calculate and record its virtual address. +*/ +if (efi_system_table >= phys && + efi_system_table < phys + (md->num_pages *EFI_PAGE_SIZE )) { + efi.systab = (void *)(efi_system_table - phys + + md->virt_addr); +set_bit (EFI_SYSTEM_TABLES , &efi.flags); + }
It seems very odd to me to set this given it's currently unused, and we don't have permanent access to the table. That sounds like we're just setting ourselves up for future fragility as users appear.
Thanks,
Mark.
PATCH 1/1 arm*: efi: drop permanent mapping of the UEFI System table
Quoted 10 lines · Mark Rutland
Quoted 4 lines · Ard Biesheuvel
The permanent, writable mapping of the UEFI System table is only referenced during invocations of UEFI Runtime Services, at which time the UEFI virtual mapping is available, which also covers the system table (since the runtime services themselves need access to it)
I'm not sure it's strictly true that the runtime services themselves need access to the system table. Why would that be necessary? Are runtime services mandated to indirect calls via the system table?
They don't need access per se, but they are allowed to reference it, and so the memory remapping layer must make it accessible after SetVirtualAddressMap(). The spec lists explicitly which fields are still valid after ExitBootServices()
Quoted 4 lines
I would expect that the EFI system table and EFI runtime services table are not in EfiConventionalMemory, but I'm not sure that we have the guarnatee that they're both in EfiRuntimeServices* memory..
SetVirtualAddressMap() is a runtime service, and one of the things it does is update the pointers in the system table, hence it must be located in RuntimeService memory, because anything else may be gone by this time.
Quoted 4 lines
From the spec, I couldn't find a mandate that the system table (nor the runtime services table) were in a region of EfiRuntimeServicesData memory. I suspect I'm looking in the wrong place...
We should clarify it if it is not clear (or if I turn out to be wrong)
Quoted 43 lines
Quoted 43 lines
So instead of creating this permanent mapping, record the virtual address of the system table inside the UEFI virtual mapping, and use that instead. This protects the contents of the system table from inadvertent (or deliberate) modification.
Signed-off-by Ard Biesheuvel <[email protected]>---
Something I spotted while working on the memory attribute table stuff. Since this is low hanging fruit and otherwise completely unrelated to it, I am posting it as a separate patch
drivers/firmware/efi/arm-init.c | 2 ++ drivers/firmware/efi/arm-runtime.c | 24 ++++++++++---------- 2 files changed, 14 insertions(+), 12 deletions(-)drivers/firmware/efi/arm-init.c +2
-0@@ -85,6 +85,8 @@ static int __init uefi_init(void) efi.systab->hdr.revision >>16 , efi.systab->hdr.revision & 0xffff); + efi.runtime_version = efi.systab->hdr.revision; +/* Show what we know for posterity */ c16 =early_memremap_ro (efi_to_phys (efi.systab->fw_vendor),sizeof (vendor) *sizeof (efi_char16_t));drivers/firmware/efi/arm-runtime.c +6
-0@@ -64,6 +64,16 @@ static bool __init efi_virtmap_init(void) &phys, ret);return false; } +/* +* If this entry covers the address of the UEFI system table, +* calculate and record its virtual address. +*/ +if (efi_system_table >= phys && + efi_system_table < phys + (md->num_pages *EFI_PAGE_SIZE )) {
Quoted 9 lines
drivers/firmware/efi/arm-runtime.c static bool __init efi_virtmap_init(void) · line 76
Quoted 4 lines
drivers/firmware/efi/arm-runtime.c +4
-0+ efi.systab = (void *)(efi_system_table - phys + + md->virt_addr); +set_bit (EFI_SYSTEM_TABLES , &efi.flags); + }It seems very odd to me to set this given it's currently unused, and we don't have permanent access to the table. That sounds like we're just setting ourselves up for future fragility as users appear.
I wondered about the purpose as well. It is only ever set, and never tested (until this patch)
@Matt: any thoughts?
PATCH 1/1 arm*: efi: drop permanent mapping of the UEFI System table
Quoted 12 lines · Ard Biesheuvel
Quoted 10 lines · Mark Rutland
Quoted 4 lines · Ard Biesheuvel
The permanent, writable mapping of the UEFI System table is only referenced during invocations of UEFI Runtime Services, at which time the UEFI virtual mapping is available, which also covers the system table (since the runtime services themselves need access to it)
I'm not sure it's strictly true that the runtime services themselves need access to the system table. Why would that be necessary? Are runtime services mandated to indirect calls via the system table?
Quoted 4 lines
They don't need access per se, but they are allowed to reference it, and so the memory remapping layer must make it accessible after
SetVirtualAddressMap(). The spec lists explicitly which fields are still valid afterExitBootServices()
I was language-lawyering ;)
I appreciate that they _can_, I just didn't think it was true that they _must_ (i.e. that they always "need to access it"). Per the below that's likely moot.
Quoted 4 lines
SetVirtualAddressMap()is a runtime service, and one of the things it does is update the pointers in the system table, hence it must be located in RuntimeService memory, because anything else may be gone by this time.
Good point. That does imply that it must be in EfiRuntimeServices* memory.
Quoted 5 lines
Quoted 3 lines
From the spec, I couldn't find a mandate that the system table (nor the runtime services table) were in a region of EfiRuntimeServicesData memory. I suspect I'm looking in the wrong place...
We should clarify it if it is not clear (or if I turn out to be wrong)
I'm hoping that I've simply missed something. Perhaps the implication above was intentional, albeit rather opaque.
Otherwise, I certainly agree a clarification would be a good thing!
Mark.
PATCH 1/1 arm*: efi: drop permanent mapping of the UEFI System table
Quoted 18 lines · Mark Rutland
Quoted 12 lines · Ard Biesheuvel
Quoted 10 lines · Mark Rutland
Quoted 4 lines · Ard Biesheuvel
The permanent, writable mapping of the UEFI System table is only referenced during invocations of UEFI Runtime Services, at which time the UEFI virtual mapping is available, which also covers the system table (since the runtime services themselves need access to it)
I'm not sure it's strictly true that the runtime services themselves need access to the system table. Why would that be necessary? Are runtime services mandated to indirect calls via the system table?
Quoted 4 lines
They don't need access per se, but they are allowed to reference it, and so the memory remapping layer must make it accessible after
SetVirtualAddressMap(). The spec lists explicitly which fields are still valid afterExitBootServices()
Quoted 6 lines
I was language-lawyering ;)
I appreciate that they _can_, I just didn't think it was true that they _must_ (i.e. that they always "need to access it"). Per the below that's likely moot.
Runtime services drivers must call ConvertPointer() to translate the pointer variables in their global state to their virtual equivalents. So unless a Runtime driver does not have such state at all, it needs access to the runtime services table, which is usually retrieved through the system table. So yes, you're right. And yes, it's moot :-)
Quoted 20 lines
Quoted 4 lines
SetVirtualAddressMap()is a runtime service, and one of the things it does is update the pointers in the system table, hence it must be located in RuntimeService memory, because anything else may be gone by this time.Good point. That does imply that it must be in EfiRuntimeServices* memory.
Quoted 5 lines
Quoted 3 lines
From the spec, I couldn't find a mandate that the system table (nor the runtime services table) were in a region of EfiRuntimeServicesData memory. I suspect I'm looking in the wrong place...
We should clarify it if it is not clear (or if I turn out to be wrong)
I'm hoping that I've simply missed something. Perhaps the implication above was intentional, albeit rather opaque.
Otherwise, I certainly agree a clarification would be a good thing!
Mark.
PATCH 1/1 arm*: efi: drop permanent mapping of the UEFI System table
In reply to Ard BiesheuvelQuoted 26 lines · Ard Biesheuvel
Quoted 15 lines · Mark Rutland
Quoted 14 lines · Ard Biesheuvel
drivers/firmware/efi/arm-runtime.c +6
-0@@ -64,6 +64,16 @@ static bool __init efi_virtmap_init(void) &phys, ret);return false; } +/* +* If this entry covers the address of the UEFI system table, +* calculate and record its virtual address. +*/ +if (efi_system_table >= phys && + efi_system_table < phys + (md->num_pages *EFI_PAGE_SIZE )) {Quoted 9 lines
drivers/firmware/efi/arm-runtime.c static bool __init efi_virtmap_init(void) · line 76
Quoted 4 lines
drivers/firmware/efi/arm-runtime.c +4
-0+ efi.systab = (void *)(efi_system_table - phys + + md->virt_addr); +set_bit (EFI_SYSTEM_TABLES , &efi.flags); + }It seems very odd to me to set this given it's currently unused, and we don't have permanent access to the table. That sounds like we're just setting ourselves up for future fragility as users appear.
Quoted 4 lines
I wondered about the purpose as well. It is only ever set, and never tested (until this patch)
@Matt: any thoughts?
Good question. EFI_SYSTEM_TABLES was introduced over 4 years ago, but it looks like it has never had any users. I don't remember why I added it, probably just be to be "complete" since we had bits for all the other tables.
If somebody wants to rip it out, I wouldn't object.
To be fair, we don't have permanent access to any of the tables - you always have to switch to the dedicated EFI page tables before accessing regions.