Re: [edk2-devel] [PATCH v3 1/1] MdeModulePkg: Add the EFI_RESOURCE_ATTRIBUTE_SPECIAL_PURPOSE attribute
On 5/30/2024 8:44 PM, Du Lin wrote: Is there any feedback from CXL stakeholders? I am meeting with some folks today and will follow up on this thread. It is OK if more time is needed to check with CXL stakeholders. But looks like we all agree that the GCD attribute conversion table shall be updated to convert EFI_RESOURCE_ATTRIBUTE_SPECIAL_PURPOSE to EFI_MEMORY_SP. So could we review and merge the conversion table update first, while we are waiting for the feedback from CXL stakeholders? If you agree, a pull request has been created for the conversion table update: https://github.com/tianocore/edk2/pull/5691. Yes, I agree with your PR and have already approved it. I think the clarifications we are looking for would be in the UEFI spec and around the memory type of EFI_MEMORY_SP, this PR is not controversial. Thanks, Oliver -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#119412): https://edk2.groups.io/g/devel/message/119412 Mute This Topic: https://groups.io/mt/106165072/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] [PATCH v3 1/1] MdeModulePkg: Add the EFI_RESOURCE_ATTRIBUTE_SPECIAL_PURPOSE attribute
On 5/23/2024 2:17 AM, Du Lin wrote: Thanks for the quick response. Agree that the PI and UEFI specs are vague on SP. That is also why I opted to minimize code changes to DXE core for SP support in patch https://edk2.groups.io/g/devel/message/118712. Would it make more sense to let the caller determine if SP memory is available for UEFI via EFI resource types (e.g., EFI_RESOURCE_SYSTEM_MEMORY vs EFI_RESOURCE_MEMORY_RESERVED)? CDAT can be read in PEI phase via DOE method and CDAT is important to support CXL 2.0. I believe CDAT spec is referencing EFI_MEMORY_TYPE and Memory Attributes defined in UEFI spec section 7.2. "EfiConventionalMemory Type with EFI_MEMORY_SP Attribute" may suggest that the memory type shall be EfiConventionalMemory and the attribute shall have SP set when reporting the memory to OS. And the concern is whether this combination can still be supported if we always mark resource HOBs with SP set as EfiGcdMemoryTypeReserved. Thanks for the clarification. I agree that it makes sense to let the resource HOB creator determine whether UEFI will put this in system memory or reserved memory. DxeCore at that point could decide to not allocate any memory with the EFI_MEMORY_SP attribute (or it could decide it doesn't care). We are meeting with some CXL stakeholders to make sure there is no concern with changing this patch and then we will respin this. Thanks, Oliver -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#119168): https://edk2.groups.io/g/devel/message/119168 Mute This Topic: https://groups.io/mt/106165072/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] [PATCH v3 1/1] MdeModulePkg: Add the EFI_RESOURCE_ATTRIBUTE_SPECIAL_PURPOSE attribute
On 5/21/2024 8:40 PM, Du Lin wrote: Coherent Device Attribute Table (CDAT) specification defines a EFI memory type and attribute "EfiConventionalMemory Type with EFI_MEMORY_SP Attribute". Can we still support this type if assigning the GCD memory type "EfiGcdMemoryTypeReserved" for resource HOBs with the SPECIAL_PURPOSE attribute set? CDAT 1.04 specification: https://uefi.org/sites/default/files/resources/Coherent%20Device%20Attribute%20Table_1.04%20published_0.pdf Thanks for pointing out the CDAT specification. I think one of the issues here is that the PI and UEFI specs are very vague on EFI_MEMORY_SP, to the point that there is no user story on it. This CDAT spec helps close some of the gap, but I think the PI and UEFI spec should be updated to be more verbose in the intended usage of this flag. Does any CDAT code exist in edk2? A naive search didn't turn up anything (and even looking at some of the CXL structures I didn't see any usage outside of the header). Is the intention that the CDAT would be read in PEI and a resource descriptor HOB created from it? If so, I agree this would prevent EfiConventionalMemory from being set. I think the CDAT spec should not be referencing EFI memory types if that is the case, because resource descriptor HOBs deal with resource types, not EFI types, so there is a mismatch. If the CDAT is read at a different time and a resource descriptor HOB is not created from it, then this should not affect it. The reason that GCD reserved type was chosen here was that the assumption for CXL or other remote memory is that UEFI would not want to use it (what if the bus went down and DXE core was allocated there). By marking it reserved with the EFI_MEMORY_SP bit set, we ensure that UEFI does not use it and that the OS knows that this is usable, but the kernel itself may not want to use it, for the same reason (or for performance). Again, the UEFI spec does not describe what memory type this should be, so if there is a different use case here, please do share. Thanks, Oliver -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#119115): https://edk2.groups.io/g/devel/message/119115 Mute This Topic: https://groups.io/mt/106165072/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] [PATCH v1 1/1] StandaloneMmPkg/StandaloneMmHobLib: Remove HOB creation
On 5/2/2024 7:28 PM, Nhi Pham via groups.io wrote: On 5/2/2024 4:31 AM, Oliver Smith-Denny via groups.io wrote: Hi folks, returning to this thread because I noticed that HOB creation still exists in StandaloneMmCore for ARM: https://github.com/tianocore/edk2/blob/5d4c5253e8bbc0baa8837fcd868925212df85201/StandaloneMmPkg/Library/StandaloneMmCoreEntryPoint/Arm/CreateHobList.c As far as I can tell, there is only this one file that creates 6 HOBs from StandaloneMmCore. Per our earlier discussion that led to disabling HOB creation in StandaloneMm, I think that this falls into the case where StandaloneMm is a HOB consumer phase, not a producer phase and so it should not be creating these HOBs. On the x64 side, all of the StandaloneMm HOB creation functions ASSERT with the comment that StandaloneMm is a HOB consumer phase and should not be creating HOBs. The HOB creation in StandaloneMmCoreEntryPoint is exclusively consumed by the MM_CORE_STANDALONE module (StandaloneMmPkg/Core/StandaloneMmCore.inf), not a MM_STANDALONE module. Per my understanding earlier, the MM_CORE_STANDALONE is a producer and other MM_STANDALONE modules are consumers. So we removed the HOB creation in the StandaloneMmHobLib which is consumed by MM_STANDALONE modules. Also, we still retain the HOB creation in the StandaloneMmPkg/Library/StandaloneMmCoreHobLib library instance. The PI spec is light on details on HOBs in Standalone MM, but it is clear that there are HOB producer phases and HOB consumer phases. Pre-Standalone MM, the HOB producer phase was PEI and the HOB consumer phase was DXE/SMM. In the Standalone MM world, it still follows that a boot phase is either a HOB consumer or a HOB producer. Currently, Standalone MM is both, regardless that it is the core that is producing the HOBs. I think that Levi's work to remove HOB creation in Standalone MM and move it to TF-A is the right move here. Thanks, Oliver -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#118556): https://edk2.groups.io/g/devel/message/118556 Mute This Topic: https://groups.io/mt/103018869/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] Call or topics for May TianoCore Community Meeting
Hi Mike, Perhaps having a discussion on the PR transition would be good here? It would be ideal if we can have a meeting time that is friendly to folks on the thread about that (obviously can't accommodate every time zone). Thanks, Oliver On 5/1/2024 9:45 AM, Michael D Kinney wrote: Please let me know if you have any topics for the TianoCore Community Meeting this month. Thanks, Mike -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#118535): https://edk2.groups.io/g/devel/message/118535 Mute This Topic: https://groups.io/mt/105846194/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] [PATCH v1 1/1] StandaloneMmPkg/StandaloneMmHobLib: Remove HOB creation
Hi Sami,
Great! It's always a good day when the thing you are looking at is
already being worked on :). Looking forward to seeing the patches.
Thanks,
Oliver
On 5/2/2024 7:52 AM, Sami Mujawar wrote:
Hi Oliver,
We are working on a solution to remove the HOB creation logic from StandaloneMm
for Arm, and this involves implementing the Firmware handoff specification
(https://github.com/FirmwareHandoff/firmware_handoff/releases/download/v0.9/firmware_handoff.pdf).
As you rightly mentioned this also requires changes in TF-A, and this work is
in progress.
Levi (Yeo) is currently working on this feature and will post the patches to
the mailing list once we have the necessary components ready.
Regards,
Sami Mujawar
On 01/05/2024, 22:32, "Oliver Smith-Denny" mailto:o...@linux.microsoft.com>> wrote:
Hi folks, returning to this thread because I noticed that HOB
creation still exists in StandaloneMmCore for ARM:
https://github.com/tianocore/edk2/blob/5d4c5253e8bbc0baa8837fcd868925212df85201/StandaloneMmPkg/Library/StandaloneMmCoreEntryPoint/Arm/CreateHobList.c
<https://github.com/tianocore/edk2/blob/5d4c5253e8bbc0baa8837fcd868925212df85201/StandaloneMmPkg/Library/StandaloneMmCoreEntryPoint/Arm/CreateHobList.c>
As far as I can tell, there is only this one file that creates 6
HOBs from StandaloneMmCore. Per our earlier discussion that led to
disabling HOB creation in StandaloneMm, I think that this falls into
the case where StandaloneMm is a HOB consumer phase, not a producer
phase and so it should not be creating these HOBs. On the x64 side,
all of the StandaloneMm HOB creation functions ASSERT with the
comment that StandaloneMm is a HOB consumer phase and should not
be creating HOBs.
On ARM this is more complicated, as all of this information would
seem to originate from TF-A and so we would need TF-A to produce
these HOBs to tell StandaloneMm the information it needs to
operate. Today TF-A already has to communicate this information, the
HOBs are just created in StandaloneMmCore instead of in TF-A.
Curious to get other folks thoughts here on this paradigm.
Thanks,
Oliver
On 12/5/2023 5:47 AM, Nhi Pham wrote:
According to the discussion in "StandaloneMmPkg: Fix HOB space and
heap space conflicted issue" [1], Standalone MM modules should be HOB
consumers where HOB is read-only. Therefore, this patch removes the
supported functions for HOB creation in the StandaloneMmHobLib.
[1] https://edk2.groups.io/g/devel/message/108333
<https://edk2.groups.io/g/devel/message/108333>
Cc: Ard Biesheuvel mailto:ardb+tianoc...@kernel.org>>
Cc: Ray Ni mailto:ray...@intel.com>>
Cc: Sami Mujawar mailto:sami.muja...@arm.com>>
Cc: Oliver Smith-Denny mailto:o...@linux.microsoft.com>>
Signed-off-by: Nhi Pham mailto:nhipham...@gmail.com>>
---
StandaloneMmPkg/Library/StandaloneMmHobLib/StandaloneMmHobLib.c | 171
++--
1 file changed, 51 insertions(+), 120 deletions(-)
diff --git a/StandaloneMmPkg/Library/StandaloneMmHobLib/StandaloneMmHobLib.c
b/StandaloneMmPkg/Library/StandaloneMmHobLib/StandaloneMmHobLib.c
index ee61bdd227d0..bef66d167494 100644
--- a/StandaloneMmPkg/Library/StandaloneMmHobLib/StandaloneMmHobLib.c
+++ b/StandaloneMmPkg/Library/StandaloneMmHobLib/StandaloneMmHobLib.c
@@ -1,5 +1,5 @@
/** @file
- HOB Library implementation for Standalone MM Core.
+ HOB Library implementation for Standalone MM modules.
Copyright (c) 2006 - 2014, Intel Corporation. All rights reserved.
Copyright (c) 2017 - 2018, ARM Limited. All rights reserved.
@@ -250,48 +250,13 @@ GetBootModeHob (
return HandOffHob->BootMode;
}
-VOID *
-CreateHob (
- IN UINT16 HobType,
- IN UINT16 HobLength
- )
-{
- EFI_HOB_HANDOFF_INFO_TABLE *HandOffHob;
- EFI_HOB_GENERIC_HEADER *HobEnd;
- EFI_PHYSICAL_ADDRESS FreeMemory;
- VOID *Hob;
-
- HandOffHob = GetHobList ();
-
- HobLength = (UINT16)((HobLength + 0x7) & (~0x7));
-
- FreeMemory = HandOffHob->EfiFreeMemoryTop - HandOffHob->EfiFreeMemoryBottom;
-
- if (FreeMemory < HobLength) {
- return NULL;
- }
-
- Hob = (VOID *)(UINTN)HandOffHob->EfiEndOfHobList;
- ((EFI_HOB_GENERIC_HEADER *)Hob)->HobType = HobType;
- ((EFI_HOB_GENERIC_HEADER *)Hob)->HobLength = HobLength;
- ((EFI_HOB_GENERIC_HEADER *)Hob)->Reserved = 0;
-
- HobEnd = (EFI_HOB_GENERIC_HEADER *)((UINTN)Hob + HobLength);
- HandOffHob->EfiEndOfHobList = (EFI_PHYSICAL_ADDRESS)(UINTN)HobEnd;
-
- HobEnd->HobType = EFI_HOB_TYPE_END_OF_HOB_LIST;
- HobEnd->HobLength = sizeof (EFI_HOB_GENERIC_HEADER);
- HobEnd->Reserved = 0;
- HobEnd++;
- HandOffHob->EfiFreeMemoryBottom = (EFI_PHYSICAL_ADDRESS)(UINTN)HobEnd;
-
- return Hob;
-}
-
/**
Builds a HOB for a loaded PE32 module.
This function builds a HOB for a loaded PE32 module.
+ It can only be invoked by Standalone MM Core.
+ For Standalone MM drivers, it will ASSERT() since HOB is read only for
Standalone MM dr
Re: [edk2-devel] [PATCH v1 1/1] StandaloneMmPkg/StandaloneMmHobLib: Remove HOB creation
Hi folks, returning to this thread because I noticed that HOB
creation still exists in StandaloneMmCore for ARM:
https://github.com/tianocore/edk2/blob/5d4c5253e8bbc0baa8837fcd868925212df85201/StandaloneMmPkg/Library/StandaloneMmCoreEntryPoint/Arm/CreateHobList.c
As far as I can tell, there is only this one file that creates 6
HOBs from StandaloneMmCore. Per our earlier discussion that led to
disabling HOB creation in StandaloneMm, I think that this falls into
the case where StandaloneMm is a HOB consumer phase, not a producer
phase and so it should not be creating these HOBs. On the x64 side,
all of the StandaloneMm HOB creation functions ASSERT with the
comment that StandaloneMm is a HOB consumer phase and should not
be creating HOBs.
On ARM this is more complicated, as all of this information would
seem to originate from TF-A and so we would need TF-A to produce
these HOBs to tell StandaloneMm the information it needs to
operate. Today TF-A already has to communicate this information, the
HOBs are just created in StandaloneMmCore instead of in TF-A.
Curious to get other folks thoughts here on this paradigm.
Thanks,
Oliver
On 12/5/2023 5:47 AM, Nhi Pham wrote:
According to the discussion in "StandaloneMmPkg: Fix HOB space and
heap space conflicted issue" [1], Standalone MM modules should be HOB
consumers where HOB is read-only. Therefore, this patch removes the
supported functions for HOB creation in the StandaloneMmHobLib.
[1] https://edk2.groups.io/g/devel/message/108333
Cc: Ard Biesheuvel
Cc: Ray Ni
Cc: Sami Mujawar
Cc: Oliver Smith-Denny
Signed-off-by: Nhi Pham
---
StandaloneMmPkg/Library/StandaloneMmHobLib/StandaloneMmHobLib.c | 171
++--
1 file changed, 51 insertions(+), 120 deletions(-)
diff --git a/StandaloneMmPkg/Library/StandaloneMmHobLib/StandaloneMmHobLib.c
b/StandaloneMmPkg/Library/StandaloneMmHobLib/StandaloneMmHobLib.c
index ee61bdd227d0..bef66d167494 100644
--- a/StandaloneMmPkg/Library/StandaloneMmHobLib/StandaloneMmHobLib.c
+++ b/StandaloneMmPkg/Library/StandaloneMmHobLib/StandaloneMmHobLib.c
@@ -1,5 +1,5 @@
/** @file
- HOB Library implementation for Standalone MM Core.
+ HOB Library implementation for Standalone MM modules.
Copyright (c) 2006 - 2014, Intel Corporation. All rights reserved.
Copyright (c) 2017 - 2018, ARM Limited. All rights reserved.
@@ -250,48 +250,13 @@ GetBootModeHob (
return HandOffHob->BootMode;
}
-VOID *
-CreateHob (
- IN UINT16 HobType,
- IN UINT16 HobLength
- )
-{
- EFI_HOB_HANDOFF_INFO_TABLE *HandOffHob;
- EFI_HOB_GENERIC_HEADER *HobEnd;
- EFI_PHYSICAL_ADDRESSFreeMemory;
- VOID*Hob;
-
- HandOffHob = GetHobList ();
-
- HobLength = (UINT16)((HobLength + 0x7) & (~0x7));
-
- FreeMemory = HandOffHob->EfiFreeMemoryTop - HandOffHob->EfiFreeMemoryBottom;
-
- if (FreeMemory < HobLength) {
-return NULL;
- }
-
- Hob= (VOID
*)(UINTN)HandOffHob->EfiEndOfHobList;
- ((EFI_HOB_GENERIC_HEADER *)Hob)->HobType = HobType;
- ((EFI_HOB_GENERIC_HEADER *)Hob)->HobLength = HobLength;
- ((EFI_HOB_GENERIC_HEADER *)Hob)->Reserved = 0;
-
- HobEnd = (EFI_HOB_GENERIC_HEADER *)((UINTN)Hob +
HobLength);
- HandOffHob->EfiEndOfHobList = (EFI_PHYSICAL_ADDRESS)(UINTN)HobEnd;
-
- HobEnd->HobType = EFI_HOB_TYPE_END_OF_HOB_LIST;
- HobEnd->HobLength = sizeof (EFI_HOB_GENERIC_HEADER);
- HobEnd->Reserved = 0;
- HobEnd++;
- HandOffHob->EfiFreeMemoryBottom = (EFI_PHYSICAL_ADDRESS)(UINTN)HobEnd;
-
- return Hob;
-}
-
/**
Builds a HOB for a loaded PE32 module.
This function builds a HOB for a loaded PE32 module.
+ It can only be invoked by Standalone MM Core.
+ For Standalone MM drivers, it will ASSERT() since HOB is read only for
Standalone MM drivers.
+
If ModuleName is NULL, then ASSERT().
If there is no additional space for HOB creation, then ASSERT().
@@ -310,33 +275,19 @@ BuildModuleHob (
IN EFI_PHYSICAL_ADDRESS EntryPoint
)
{
- EFI_HOB_MEMORY_ALLOCATION_MODULE *Hob;
-
- ASSERT (
-((MemoryAllocationModule & (EFI_PAGE_SIZE - 1)) == 0) &&
-((ModuleLength & (EFI_PAGE_SIZE - 1)) == 0)
-);
-
- Hob = CreateHob (EFI_HOB_TYPE_MEMORY_ALLOCATION, sizeof
(EFI_HOB_MEMORY_ALLOCATION_MODULE));
-
- CopyGuid (&(Hob->MemoryAllocationHeader.Name),
);
- Hob->MemoryAllocationHeader.MemoryBaseAddress = MemoryAllocationModule;
- Hob->MemoryAllocationHeader.MemoryLength = ModuleLength;
- Hob->MemoryAllocationHeader.MemoryType= EfiBootServicesCode;
-
//
- // Zero the reserved space to match HOB spec
+ // HOB is read only for Standalone MM drivers
//
- ZeroMem (Hob->MemoryAllocationHeader.Reserved, sizeof
(Hob->MemoryAllocationHeader.Reserved));
-
- CopyGuid (>ModuleName, M
Re: [edk2-devel] MdeModulePkg: Fix MAT SplitRecord() Logic introduce one bug and will cause SUT reset when boot to windows
On 4/18/2024 11:43 PM, Ni, Ray wrote: So this is just junk unallocated memory that we are reporting as a type it *could* be if an allocation occurs to minimize failures of ExitBootServices. Which is questionable. But in terms of attributes, I would expect we either have this unallocated memory marked the same as the bin type or better, mark it RP if we can (Taylor is making a change to set RP on free memory by default, so we would have this in the page table, but we would need to decide what we tell the OS). [Ray] When reviewing today's logic of memory protection through page table, I feel that it was designed improperly in the beginning. My rough thought is: * All memory is RP initially (as you said Taylor will do that) Correct, Taylor is working on a change here and actually taking this a step further, that all free memory will be RP, to catch any use after free and keep a safer environment. * Allocated memory is mapped as either RO or XD, depending on code/data. Or RP if it's a guard page. This is mostly true, of course it depends on how the memory protections are configured. I would like to see this go to not being an option but something that DxeCore enforces by default depending on memory type allocated. Maybe I am not aware of some limitations of the above idea. The limitations prevented the initial design be in this way. Or what Taylor will do aligns to the idea? The issue in this mailing thread is not what DXE's page tables are, but what get reported in the MAT to the OS. Before Taylor's change to improve the SplitTable logic the extra RuntimeServicesCode sections that get reported to the OS (these are the leftover sections in the memory bins to improve the chance for S4 resume) were getting reported as XP. Taylor is proposing these get marked RO instead as they are marked as Code sections (although they really hold junk in them). Another path would be can we mark them both RO and XP. These are junk sections, they should not be used and definitely not executed from. We only report them to the OS so that our memory map changes less between boots (I also wonder if there are better ways we can do this, but I'll have to think about this more). Thanks, Oliver -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#118143): https://edk2.groups.io/g/devel/message/118143 Mute This Topic: https://groups.io/mt/105477564/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] MdeModulePkg: Fix MAT SplitRecord() Logic introduce one bug and will cause SUT reset when boot to windows
On 4/18/2024 6:56 AM, Huang, Yanbo wrote: The PCD PcdPlatformEfiRtCodeMemorySize is used in https://github.com/tianocore/edk2-platforms/blob/master/Platform/Intel/MinPlatformPkg/PlatformInit/PlatformInitPei/PlatformInitPreMem.c This PCD seems defined the size allocated for run time services code, and the similar PCD is PcdPlatformEfiRtDataMemorySize, seems defined the size allocated for run time services data. I guess dandan means if the runtime services code size is small than the PCD defined, then the " Extra EfiRuntimeServicesCode regions which aren't part of loaded runtime images." size should be FixedPcdGet32 (PcdPlatformEfiRtCodeMemorySize) - the actual runtime image code size? Drawing the lines here, we will lie in the EFI_MEMORY_MAP to say that all bin memory is allocated, regardless of this PCD: https://github.com/tianocore/edk2/blob/0afb8743493853e30171f6000de51242e22a1eb8/MdeModulePkg/Core/Dxe/Mem/Page.c#L1973-L1989 So this is just junk unallocated memory that we are reporting as a type it *could* be if an allocation occurs to minimize failures of ExitBootServices. Which is questionable. But in terms of attributes, I would expect we either have this unallocated memory marked the same as the bin type or better, mark it RP if we can (Taylor is making a change to set RP on free memory by default, so we would have this in the page table, but we would need to decide what we tell the OS). Going back to the questionableness of this: can we not report the entire bin as allocated memory to the OS? I understand what the comment is saying; I don't like that we will lie to the OS and reserve larger chunks of runtime memory than we actually need and that will get the wrong permissions set on it (for example making this garbage data be executable). Thanks, Oliver -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#117994): https://edk2.groups.io/g/devel/message/117994 Mute This Topic: https://groups.io/mt/105477564/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] [PATCH v1] MdeModulePkg: Fixup MAT Attributes After Splitting EFI Memory Map
On 4/17/2024 2:41 PM, Oliver Smith-Denny wrote: Now, for the case of ARM64, where you have 64k runtime granularity and often will end up with the case of many extra pages in a code section, those pages will be marked as RO and executable, even though they contain garbage. I think it would be worthwhile to mark the excess garbage pages, if they exist for a given section, as RP. Nothing should be using them in any fashion, they are padding. I thought this through some more. We can't do this because of the UEFI spec provision that says no 64k region can have different attributes set. So, we leave open the possibility of executing garbage :) Oliver -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#117936): https://edk2.groups.io/g/devel/message/117936 Mute This Topic: https://groups.io/mt/105570114/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] [PATCH v1] MdeModulePkg: Fixup MAT Attributes After Splitting EFI Memory Map
On 4/17/2024 9:52 AM, Ard Biesheuvel wrote: So the purpose of the MAT is to describe RT code (and to a lesser extent, RT data) regions where we cannot apply either RO or XP to the whole thing. IIRC there was never an intent to exhaustively describe all memory runtime regions. Also note that RO was introduced at this point, because WP was already being used in the ordinary memory map in a deviating manner. RO is defined both for the memory map and the MAT, and so it can occur in either. At the principle level, I think we can say that we want all runtime code regions to RO and all runtime data regions to be XP. Regardless of whatever situation we have today, I think this is a reasonable principle to maintain. If you don't want those attributes, a different memory type should be allocated. If we agree on this principle, I think we should put it into practice. Again, the UEFI spec calls out that EfiRuntimeServicesCode is for image code. From a security and safety standpoint, we know we want image code to be RO. To help with any existing (mis)use of EfiRuntimeServicesCode, I do think we should put a big old assert in the MAT generation logic that says I found a EfiRuntimeServicesCode section that is not described in an image record, something is wrong with your configuration, you are not using EfiRuntimeServicesCode correctly. If I am missing a legitimate use of EfiRuntimeServicesCode, please help educate me. Also, I know that modern Windows security features rely on the MAT describing all EfiRuntimeServicesCode and EfiRuntimeServicesData regions. Here is an MSDN link that makes a statement towards that: https://learn.microsoft.com/en-us/windows-hardware/drivers/bringup/unified-extensible-firmware-interface In a more actionable way, the Windows testing infrastructure will test to ensure that there are no EfiRuntimeServices[Code|Data] sections in the EFI memory map that are not described in the MAT. Again, there are various security features that rely on this. Thanks, Oliver -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#117935): https://edk2.groups.io/g/devel/message/117935 Mute This Topic: https://groups.io/mt/105570114/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] [PATCH v1] MdeModulePkg: Fixup MAT Attributes After Splitting EFI Memory Map
On 4/17/2024 7:34 AM, Taylor Beebe wrote: On 4/17/2024 7:09 AM, Oliver Smith-Denny wrote: On 4/17/2024 7:05 AM, Taylor Beebe wrote: On 4/17/2024 6:40 AM, Oliver Smith-Denny wrote: Aside from this, I wonder if we can be more aspirational here. These EfiRuntimeServicesCode regions without attributes set are, if I am understanding correctly, from loaded images. These EfiRuntimeServicesCode regions without attributes set are not part of loaded image memory. I think that's what you meant but wanted to clarify. Are these regions without attributes from image sections that have been padded to RUNTIME_PAGE_ALLOCATION_GRANULARITY, i.e. they are the pads? Or are we saying we don't know what these regions are at this point? It is true in theory someone could just allocate an EfiRuntimeServicesCode section. Good question -- I had not considered the extra padding applied to these allocations. It could be either. The memory map returned via GetMemoryMap() will merge descriptors together based on type so it's possible to mistake an unrelated EfiRuntimeServicesCode allocation with padding applied to a runtime image memory allocation if they are contiguous. Taylor and I had an offline conversation and checked on this, my recent patch moving ImagePropertiesRecordLib to use VirtualSize instead of SizeOfRawData fixed this. So we should not see any padding sections as without attributes. Now, for the case of ARM64, where you have 64k runtime granularity and often will end up with the case of many extra pages in a code section, those pages will be marked as RO and executable, even though they contain garbage. I think it would be worthwhile to mark the excess garbage pages, if they exist for a given section, as RP. Nothing should be using them in any fashion, they are padding. Oliver -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#117934): https://edk2.groups.io/g/devel/message/117934 Mute This Topic: https://groups.io/mt/105570114/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] [PATCH v1] MdeModulePkg: Fixup MAT Attributes After Splitting EFI Memory Map
On 4/17/2024 7:05 AM, Taylor Beebe wrote: On 4/17/2024 6:40 AM, Oliver Smith-Denny wrote: Aside from this, I wonder if we can be more aspirational here. These EfiRuntimeServicesCode regions without attributes set are, if I am understanding correctly, from loaded images. These EfiRuntimeServicesCode regions without attributes set are not part of loaded image memory. I think that's what you meant but wanted to clarify. Are these regions without attributes from image sections that have been padded to RUNTIME_PAGE_ALLOCATION_GRANULARITY, i.e. they are the pads? Or are we saying we don't know what these regions are at this point? It is true in theory someone could just allocate an EfiRuntimeServicesCode section. -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#117923): https://edk2.groups.io/g/devel/message/117923 Mute This Topic: https://groups.io/mt/105570114/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] [PATCH v1] MdeModulePkg: Fixup MAT Attributes After Splitting EFI Memory Map
Hi Ard, On 4/16/2024 11:38 PM, Ard Biesheuvel wrote: For entries where we lack such additional metadata, I don't think we can make assumptions based on the type beyond mapping data and MMIO regions XP. We have no idea how those EfiRuntimeServicesCode regions may be used, and currently, the spec permits RWX semantics for those if no restrictions are specified. The spec suggests that omitting an entry from the MAT is the same as listing it with RO|XP cleared. How RO|XP from the original entry should be interpreted wrt to the MAT is unspecified. So I think the only thing we can do at this point is preserve the entry if it has RO|XP set, or just drop it otherwise. I do agree that it is probably safer to exclude the sub-region from the MAT entirely. However, from my reading of the spec, it is unclear to me whether it envisions that. From section 4.6.4 of UEFI spec 2.10: "The Memory Attributes Table may define multiple entries to describe sub-regions that comprise a single entry returned by GetMemoryMap() however the sub-regions must total to completely describe the larger region and may not cross boundaries between entries reported by GetMemoryMap() . If a run-time region returned in GetMemoryMap() entry is not described within the Memory Attributes Table, this region is assumed to not be compatible with any memory protections." The unclear part to me here is "the sub-regions must total to completely describe the larger region." To me that says that in Taylor's case, where we have a memory map descriptor with attributes set for part of the region but not the whole region, that the spec envisions the MAT having either the whole region split into sub-regions or not including the MAT entry for this region. In this reading the final sentence would say that if an entire memory map entry is not described in the MAT, then it is assumed to be incompatible. A different reading would say what you are saying, that a sub-region can be dropped from the MAT (although it is hard to justify that with the language that says the sub-regions must total to completely describe the larger region). What are your thoughts here? Aside from this, I wonder if we can be more aspirational here. These EfiRuntimeServicesCode regions without attributes set are, if I am understanding correctly, from loaded images. The spec does call out that EfiRuntimeServicesCode is explicitly for code sections of loaded images. Can we just say outright that any memory of this type should be RO? I understand that existing drivers may attempt to break this, but from the core, I think this is reasonable to say, but would love to hear thoughts. Note that the spec also mentions that the MAT must only contain EfiRuntimeServicesCode or EfiRuntimeServicesData entries, and it looks like this is not being enforced either. Agreed, this should be amended. Thanks, Oliver -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#117920): https://edk2.groups.io/g/devel/message/117920 Mute This Topic: https://groups.io/mt/105570114/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] Call or topics for April TianoCore Community Meeting
On 4/3/2024 4:28 PM, Kinney, Michael D wrote: MdeModulePkg has many different reviewers for different subsets. We should also consider seeing if any of those reviewers that are responsible for different subsets of MdeModulePkg would be willing to be a backup maintainer for the MdeModulePkg. Zhiguang Liu [LiuZhiguang001] Dandan Bi [dandanbi] Zhichao Gao [ZhichaoGao] Ray Ni [niruiyu] Jiaxin Wu [jiaxinwu] Star Zeng [lzeng14] Gua Guo [gguo11837463] Prakashan Krishnadas Veliyathuparambil [kprakas2] K N Karthik [karthikkabbigere1] Richard Ho [richardho] Rebecca Cran [bcran] Abdul Lateef Attar [abdattar] Nickle Wang [nicklela] That seems reasonable to reach out to these folks as well. We can take it as a data point on how long it takes one of the existing submaintainers to respond to your email :). Thanks, Oliver -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#117443): https://edk2.groups.io/g/devel/message/117443 Mute This Topic: https://groups.io/mt/105299780/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] Call or topics for April TianoCore Community Meeting
On 4/3/2024 1:38 PM, Michael D Kinney wrote: Hi Oliver, I missed this response. Did not show up in thread for some reason. No worries. But we can continue these topics on email. The TianoCore roles and responsibilities are documented here: https://github.com/tianocore/tianocore.github.io/wiki/TianoCore-Who-we-are If there are maintainers are not following their responsibilities, then please let us know and we can work together to find additional maintainers. Thanks, as I've mentioned in several of my last patches to MdeModulePkg, I think we need an additional top level maintainer there who is currently active. Liming has obviously been a huge part of edk2, but has many roles and has been quiet on MdeModulePkg patches for over a month. His input is greatly valued, but it seems he could use some help maintaining the package so we can keep up developer flow, I had multiple patches that contained items such as fixes for UEFI spec violations and boot breaks that were sitting with multiple reviews that were not getting the maintainer review and merging in. I think it would be very valuable to have a second MdeModulePkg top level maintainer in any case, this covers areas such as DxeCore that are some of the most central and complex parts of the code base and bugs there need immediate resolution. I don't want to put him on the spot, but I would be tempted to nominate Ard, if he wants the added responsibility, as he is very knowledgeable in UEFI and one of the few people who actively reviews patches closely and timely. If he does not want the additional maintenance burden, then I would ask the stewards to help find a community member to take on this role with Liming. There is not much progress on PR process. We need resources to work on the list of action items documented here: https://github.com/orgs/tianocore/projects/5 Thanks for this pointer, I know the majority of active community members report in many different forums that moving to PRs is critical and I am confident that as a community we can make this happen. Thanks, Oliver -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#117395): https://edk2.groups.io/g/devel/message/117395 Mute This Topic: https://groups.io/mt/105299780/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] Call or topics for April TianoCore Community Meeting
Hi Mike, My two topics would be PRs and lack of maintainer response, the proper process there, etc. Thanks, Oliver On 4/2/2024 5:12 PM, Michael D Kinney wrote: Please let me know if you have any topics for the TianoCore Community Meeting this month. Thanks, Mike -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#117376): https://edk2.groups.io/g/devel/message/117376 Mute This Topic: https://groups.io/mt/105299780/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] [PATCH v3 0/3] MdeModulePkg: ImagePropertiesRecordLib Fixes
Sorry Mike, I realized I missed you on the cover letter email. I have the CI PR up (the commit messages there don't have the RBs/AB) and I made sure it passed all gates. Thanks for getting this merged. Oliver On 3/29/2024 1:21 PM, Oliver Smith-Denny wrote: ImagePropertiesRecordLib is currently creating Image Records that are not accurate. It is setting the CodeSegmentSize to be the size of the raw data in the image file, however, when the image is loaded into memory, the raw data size is aligned to the section alignment. This caused the memory attributes table to have incorrect entries for systems, like ARM64, where the section alignment is not 4k for all modules. In fixing this, I noticed that MemoryProtection.c is using its own version of image record creation where this logic was actually correct. ImagePropertiesRecordLib was created to consolidate the logic around creating and managing image records, so this patchset also updates MemoryProtection.c to use ImagePropertiesRecordsLib after making a few small adjustments to ensure the same functionality is present. This patchset was tested on ArmVirtQemu to ensure that all image records were the same before and after this, other than fixing the CodeSegmentSize. v3: - Fix merge conflict in MemoryProtection.c v2: - Align VirtualSize instead of SizeOfRawData Github PR: https://github.com/tianocore/edk2/pull/5504 Cc: Liming Gao Cc: Leif Lindholm Cc: Ard Biesheuvel Cc: Sami Mujawar Cc: Taylor Beebe Oliver Smith-Denny (3): MdeModulePkg: ImagePropertiesRecordLib: Use SectionAlignment for CodeSize MdeModulePkg: ImagePropertiesRecordLib: Consolidate Usage MdeModulePkg: MemoryProtection: Use ImageRecordPropertiesLib Oliver Smith-Denny (3): MdeModulePkg: ImagePropertiesRecordLib: Use SectionAlignment for CodeSize MdeModulePkg: ImagePropertiesRecordLib: Consolidate Usage MdeModulePkg: MemoryProtection: Use ImageRecordPropertiesLib MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c| 241 +++- MdeModulePkg/Library/ImagePropertiesRecordLib/ImagePropertiesRecordLib.c | 86 +-- 2 files changed, 94 insertions(+), 233 deletions(-) -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#117244): https://edk2.groups.io/g/devel/message/117244 Mute This Topic: https://groups.io/mt/105223002/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] [PATCH v2 0/3] MdeModulePkg: ImagePropertiesRecordLib Fixes
I sent a v3 with the fixup (it was trivial, from my last patch, apologies for not fixing up before). I added the RBs/AB, hope that was alright since there was no substantial code change, just the merge fix. Thanks, Oliver On 3/29/2024 10:28 AM, Oliver Smith-Denny wrote: Will do! Sorry, it's sat for a bit, so not too surprising. I'll get that up soon. Thanks, Oliver On 3/29/2024 10:27 AM, Michael D Kinney wrote: Hi Oliver, I am seeing a merge conflict with the V2 patches from the emails. Can you please rebase, resolve conflicts, and resend? Thanks, Mike -Original Message- From: Oliver Smith-Denny Sent: Friday, March 29, 2024 10:14 AM To: devel@edk2.groups.io; Kinney, Michael D ; Ard Biesheuvel Cc: Liming Gao ; Ni, Ray ; Leif Lindholm ; Sami Mujawar ; Taylor Beebe Subject: Re: [edk2-devel] [PATCH v2 0/3] MdeModulePkg: ImagePropertiesRecordLib Fixes Thanks Mike! Oliver On 3/29/2024 10:12 AM, Michael D Kinney wrote: Hi Ard, I have reviewed the discussion on the V1 and V2 versions of the series. For the V2 Series: Acked-by: Michael D Kinney I will add the Rb/Ab tags and get this merged. Mike -Original Message- From: Ard Biesheuvel Sent: Friday, March 29, 2024 1:03 AM To: devel@edk2.groups.io; o...@linux.microsoft.com Cc: Liming Gao ; Kinney, Michael D ; Ni, Ray ; Leif Lindholm ; Sami Mujawar ; Taylor Beebe Subject: Re: [edk2-devel] [PATCH v2 0/3] MdeModulePkg: ImagePropertiesRecordLib Fixes On Wed, 27 Mar 2024 at 20:14, Oliver Smith-Denny wrote: Hi Mike and Ray, I'm here to bug you again :). I have another patchset that unfortunately Liming has not gotten to review in a month's time frame with weekly pings. When I bugged you last time, he reviewed the next day, so something must have worked out there. Can you please help get this merged? Liming, do you need the community to find another member to help as a second MdeModulePkg maintainer? It's obviously a large job and you have been less responsive the past few months, which has slowed down getting some really important fixes into MdeModulePkg. Yes, could we please get this merged? These are important fixes. In case it was missed, for the series: Reviewed-by: Ard Biesheuvel -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#117243): https://edk2.groups.io/g/devel/message/117243 Mute This Topic: https://groups.io/mt/104873191/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
[edk2-devel] [PATCH v3 0/3] MdeModulePkg: ImagePropertiesRecordLib Fixes
ImagePropertiesRecordLib is currently creating Image Records that are not accurate. It is setting the CodeSegmentSize to be the size of the raw data in the image file, however, when the image is loaded into memory, the raw data size is aligned to the section alignment. This caused the memory attributes table to have incorrect entries for systems, like ARM64, where the section alignment is not 4k for all modules. In fixing this, I noticed that MemoryProtection.c is using its own version of image record creation where this logic was actually correct. ImagePropertiesRecordLib was created to consolidate the logic around creating and managing image records, so this patchset also updates MemoryProtection.c to use ImagePropertiesRecordsLib after making a few small adjustments to ensure the same functionality is present. This patchset was tested on ArmVirtQemu to ensure that all image records were the same before and after this, other than fixing the CodeSegmentSize. v3: - Fix merge conflict in MemoryProtection.c v2: - Align VirtualSize instead of SizeOfRawData Github PR: https://github.com/tianocore/edk2/pull/5504 Cc: Liming Gao Cc: Leif Lindholm Cc: Ard Biesheuvel Cc: Sami Mujawar Cc: Taylor Beebe Oliver Smith-Denny (3): MdeModulePkg: ImagePropertiesRecordLib: Use SectionAlignment for CodeSize MdeModulePkg: ImagePropertiesRecordLib: Consolidate Usage MdeModulePkg: MemoryProtection: Use ImageRecordPropertiesLib Oliver Smith-Denny (3): MdeModulePkg: ImagePropertiesRecordLib: Use SectionAlignment for CodeSize MdeModulePkg: ImagePropertiesRecordLib: Consolidate Usage MdeModulePkg: MemoryProtection: Use ImageRecordPropertiesLib MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c| 241 +++- MdeModulePkg/Library/ImagePropertiesRecordLib/ImagePropertiesRecordLib.c | 86 +-- 2 files changed, 94 insertions(+), 233 deletions(-) -- 2.40.1 -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#117239): https://edk2.groups.io/g/devel/message/117239 Mute This Topic: https://groups.io/mt/105223002/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
[edk2-devel] [PATCH v3 3/3] MdeModulePkg: MemoryProtection: Use ImageRecordPropertiesLib
The functionality to create and delete Image Records has been
consolidated in a library and ensured that MemoryProtection.c's
usage is encapsulated there.
This patch moves MemoryProtection.c to reuse the code in the lib
and to prevent issues in the future where code is updated in one
place but not the other.
Cc: Liming Gao
Cc: Taylor Beebe
Acked-by: Michael D Kinney
Reviewed-by: Ard Biesheuvel
Signed-off-by: Oliver Smith-Denny
---
MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c | 241 +++-
1 file changed, 28 insertions(+), 213 deletions(-)
diff --git a/MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c
b/MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c
index eb243a0137b1..2c069cc12c63 100644
--- a/MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c
+++ b/MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c
@@ -281,80 +281,43 @@ SetUefiImageProtectionAttributes (
}
/**
- Return if the PE image section is aligned.
+ Return the section alignment requirement for the PE image section type.
- @param[in] SectionAlignmentPE/COFF section alignment
- @param[in] MemoryType PE/COFF image memory type
+ @param[in] MemoryType PE/COFF image memory type
+
+ @retval The required section alignment for this memory type
- @retval TRUE The PE image section is aligned.
- @retval FALSE The PE image section is not aligned.
**/
-BOOLEAN
-IsMemoryProtectionSectionAligned (
- IN UINT32 SectionAlignment,
+STATIC
+UINT32
+GetMemoryProtectionSectionAlignment (
IN EFI_MEMORY_TYPE MemoryType
)
{
- UINT32 PageAlignment;
+ UINT32 SectionAlignment;
switch (MemoryType) {
case EfiRuntimeServicesCode:
case EfiACPIMemoryNVS:
case EfiReservedMemoryType:
- PageAlignment = RUNTIME_PAGE_ALLOCATION_GRANULARITY;
+ SectionAlignment = RUNTIME_PAGE_ALLOCATION_GRANULARITY;
break;
case EfiRuntimeServicesData:
ASSERT (FALSE);
- PageAlignment = RUNTIME_PAGE_ALLOCATION_GRANULARITY;
+ SectionAlignment = RUNTIME_PAGE_ALLOCATION_GRANULARITY;
break;
case EfiBootServicesCode:
case EfiLoaderCode:
- PageAlignment = EFI_PAGE_SIZE;
+ SectionAlignment = EFI_PAGE_SIZE;
break;
case EfiACPIReclaimMemory:
default:
ASSERT (FALSE);
- PageAlignment = EFI_PAGE_SIZE;
+ SectionAlignment = EFI_PAGE_SIZE;
break;
}
- if ((SectionAlignment & (PageAlignment - 1)) != 0) {
-return FALSE;
- } else {
-return TRUE;
- }
-}
-
-/**
- Free Image record.
-
- @param[in] ImageRecordA UEFI image record
-**/
-VOID
-FreeImageRecord (
- IN IMAGE_PROPERTIES_RECORD *ImageRecord
- )
-{
- LIST_ENTRY*CodeSegmentListHead;
- IMAGE_PROPERTIES_RECORD_CODE_SECTION *ImageRecordCodeSection;
-
- CodeSegmentListHead = >CodeSegmentList;
- while (!IsListEmpty (CodeSegmentListHead)) {
-ImageRecordCodeSection = CR (
- CodeSegmentListHead->ForwardLink,
- IMAGE_PROPERTIES_RECORD_CODE_SECTION,
- Link,
- IMAGE_PROPERTIES_RECORD_CODE_SECTION_SIGNATURE
- );
-RemoveEntryList (>Link);
-FreePool (ImageRecordCodeSection);
- }
-
- if (ImageRecord->Link.ForwardLink != NULL) {
-RemoveEntryList (>Link);
- }
-
- FreePool (ImageRecord);
+ return SectionAlignment;
}
/**
@@ -369,19 +332,10 @@ ProtectUefiImage (
IN EFI_DEVICE_PATH_PROTOCOL *LoadedImageDevicePath
)
{
- VOID *ImageAddress;
- EFI_IMAGE_DOS_HEADER *DosHdr;
- UINT32PeCoffHeaderOffset;
- UINT32SectionAlignment;
- EFI_IMAGE_SECTION_HEADER *Section;
- EFI_IMAGE_OPTIONAL_HEADER_PTR_UNION Hdr;
- UINT8 *Name;
- UINTN Index;
- IMAGE_PROPERTIES_RECORD *ImageRecord;
- CHAR8 *PdbPointer;
- IMAGE_PROPERTIES_RECORD_CODE_SECTION *ImageRecordCodeSection;
- BOOLEAN IsAligned;
- UINT32ProtectionPolicy;
+ IMAGE_PROPERTIES_RECORD *ImageRecord;
+ UINT32 ProtectionPolicy;
+ EFI_STATUS Status;
+ UINT32 RequiredAlignment;
DEBUG ((DEBUG_INFO, "ProtectUefiImageCommon - 0x%x\n", LoadedImage));
DEBUG ((DEBUG_INFO, " - 0x%016lx - 0x%016lx\n",
(EFI_PHYSICAL_ADDRESS)(UINTN)LoadedImage->ImageBase, LoadedImage->ImageSize));
@@ -406,160 +360,21 @@ ProtectUefiImage (
return;
}
- ImageRecord->Signature = IMAGE_PROPERTIES_RECORD_SIGNATURE;
+ RequiredAlignment = GetMemoryProtectionSectionAlignment
(LoadedImage->ImageCodeType);
- //
- // Step 1: record whole region
- //
- ImageRecord->ImageBase = (EFI_PHYSICAL_ADDRESS)(UINTN)LoadedIma
Re: [edk2-devel] [PATCH v2 0/3] MdeModulePkg: ImagePropertiesRecordLib Fixes
Hi Mike and Ray, I'm here to bug you again :). I have another patchset that unfortunately Liming has not gotten to review in a month's time frame with weekly pings. When I bugged you last time, he reviewed the next day, so something must have worked out there. Can you please help get this merged? Liming, do you need the community to find another member to help as a second MdeModulePkg maintainer? It's obviously a large job and you have been less responsive the past few months, which has slowed down getting some really important fixes into MdeModulePkg. Thanks, Oliver On 3/20/2024 10:35 AM, Oliver Smith-Denny wrote: Hi Liming, Another friendly ping, can you review these patches? 2 RBs and conversation has died down. Thanks, Oliver On 3/13/2024 10:33 AM, Oliver Smith-Denny wrote: Hi Liming, Friendly ping, can you please review this patchset? Thanks, Oliver On 3/11/2024 2:29 PM, Oliver Smith-Denny wrote: ImagePropertiesRecordLib is currently creating Image Records that are not accurate. It is setting the CodeSegmentSize to be the size of the raw data in the image file, however, when the image is loaded into memory, the raw data size is aligned to the section alignment. This caused the memory attributes table to have incorrect entries for systems, like ARM64, where the section alignment is not 4k for all modules. In fixing this, I noticed that MemoryProtection.c is using its own version of image record creation where this logic was actually correct. ImagePropertiesRecordLib was created to consolidate the logic around creating and managing image records, so this patchset also updates MemoryProtection.c to use ImagePropertiesRecordsLib after making a few small adjustments to ensure the same functionality is present. This patchset was tested on ArmVirtQemu to ensure that all image records were the same before and after this, other than fixing the CodeSegmentSize. v2: - Align VirtualSize instead of SizeOfRawData Github PR: https://github.com/tianocore/edk2/pull/5402 Cc: Liming Gao Cc: Leif Lindholm Cc: Ard Biesheuvel Cc: Sami Mujawar Cc: Taylor Beebe Oliver Smith-Denny (3): MdeModulePkg: ImagePropertiesRecordLib: Use SectionAlignment for CodeSize MdeModulePkg: ImagePropertiesRecordLib: Consolidate Usage MdeModulePkg: MemoryProtection: Use ImageRecordPropertiesLib MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c | 241 +++- MdeModulePkg/Library/ImagePropertiesRecordLib/ImagePropertiesRecordLib.c | 86 +-- 2 files changed, 94 insertions(+), 233 deletions(-) -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#117178): https://edk2.groups.io/g/devel/message/117178 Mute This Topic: https://groups.io/mt/104873191/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] [PATCH v2 0/3] MdeModulePkg: ImagePropertiesRecordLib Fixes
Hi Liming, Another friendly ping, can you review these patches? 2 RBs and conversation has died down. Thanks, Oliver On 3/13/2024 10:33 AM, Oliver Smith-Denny wrote: Hi Liming, Friendly ping, can you please review this patchset? Thanks, Oliver On 3/11/2024 2:29 PM, Oliver Smith-Denny wrote: ImagePropertiesRecordLib is currently creating Image Records that are not accurate. It is setting the CodeSegmentSize to be the size of the raw data in the image file, however, when the image is loaded into memory, the raw data size is aligned to the section alignment. This caused the memory attributes table to have incorrect entries for systems, like ARM64, where the section alignment is not 4k for all modules. In fixing this, I noticed that MemoryProtection.c is using its own version of image record creation where this logic was actually correct. ImagePropertiesRecordLib was created to consolidate the logic around creating and managing image records, so this patchset also updates MemoryProtection.c to use ImagePropertiesRecordsLib after making a few small adjustments to ensure the same functionality is present. This patchset was tested on ArmVirtQemu to ensure that all image records were the same before and after this, other than fixing the CodeSegmentSize. v2: - Align VirtualSize instead of SizeOfRawData Github PR: https://github.com/tianocore/edk2/pull/5402 Cc: Liming Gao Cc: Leif Lindholm Cc: Ard Biesheuvel Cc: Sami Mujawar Cc: Taylor Beebe Oliver Smith-Denny (3): MdeModulePkg: ImagePropertiesRecordLib: Use SectionAlignment for CodeSize MdeModulePkg: ImagePropertiesRecordLib: Consolidate Usage MdeModulePkg: MemoryProtection: Use ImageRecordPropertiesLib MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c | 241 +++- MdeModulePkg/Library/ImagePropertiesRecordLib/ImagePropertiesRecordLib.c | 86 +-- 2 files changed, 94 insertions(+), 233 deletions(-) -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#116943): https://edk2.groups.io/g/devel/message/116943 Mute This Topic: https://groups.io/mt/104873191/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel][PATCH v1 1/3] MdeModulePkg: ImagePropertiesRecordLib: Use SectionAlignment for CodeSize
On 3/15/2024 4:45 PM, Marvin Häuser wrote: On 15. Mar 2024, at 23:57, Oliver Smith-Denny wrote: I don't think this is what I'm saying. What I am trying to say is that on MSVC, I see PE images getting created that have VirtualSize set to the actual number of initialized bytes in that section (not padded to the section alignment). On ElfConverted binaries, I see the VirtualSize is padded to the section alignment. I've dropped an example below Ah, mismatched terminology. Zero-initialized as Ard and I used it refers to implicitly or explicitly 0-initialized global variables and such, which is not stored in the file, not the padding. So when you mentioned “real data”, I assumed you meant strictly the non-0 data from the file. Same misunderstanding with SizeOfImage, so that’s all fine. Whew. :) Ah gotcha, thanks I was figuring I was using the wrong terminology :). What you said makes sense and I see your concern based on what I said. No, the specific case where I was researching this was explicitly setting /ALIGN:0x1 and /FILEALIGN:0x1000 for DXE_RUNTIME_DRIVERs on ARM64 (a UEFI spec requirement). So I would see the SizeOfRawData is aligned to the file alignment, as expected, but VirtualSize would be the actual size of the data. Again, the troubling thing here for me is that the same binary built with gcc has the VirtualSize aligned to the section alignment. And I have seen other code that loads PE images that relies on VirtualSize not including the padding. The spec is vague here, it says VirtualSize is the size of the section as loaded in memory (which would lead me to believe this should include padding) but it does not explicitly say it should be a multiple of the section alignment (as other fields do). But at a minimum I think we should have different toolchains doing the same behavior here. Well, not rounding to pad is somewhat superior in some scenarios. If you round, you lose the information on what is section data and what is padding, so you might end up treating padding as data for some reason (because it is indistinguishable from mentioned 0-initialized data). This shouldn’t matter too much for executables and libraries, but MSVC/PE have a lot less of a distinction between object file and executable/library concepts (e.g. no distinction between sections and segments). That might be why they do it this way. I agree, I've seen other environments that will use VirtualSize to set attributes for that section and then set stricter attributes, like RP on the padded section (if greater than a page). Point being that there are use cases and at least some folks relying on that definition of VirtualSize. See below for the VirtualSize examples, I'm confused on your comment on SizeOfImage. I agree that SizeOfImage covers everything as loaded into memory and I have not seen any issues there. See first comment. Do you mind adding your RB to v2? And certainly if you have any other comments that is greatly appreciated. Will try to remember tomorrow. :) Thanks! Examples of the differences I see between MSVC and gcc binaries: I originally noticed this on ARM64 on edk2, but wanted to make sure I saw it on x64 too, so this is with binaries from Project Mu's QemuQ35Pkg (edk2 doesn't have VS2022 support and I didn't feel like adding it or reverting back to VS2019). For reference, this is building the current top of tree at a4dd5d9785f48302c95fec852338d6bd26dd961a. I dumped ReportStatusCodeRouterRuntimeDxe.efi from both using dumpbin (from VS2022) to examine the PE headers. MSVC selected header values: Main header: 0x3200 size of code 0x2400 size of initialized data 0x0size of uninitialized data 0x1000 section alignment 0x200 file alignment 0xB000 size of image 6 sections: .data, .pdata, .rdata, .reloc, .text, .xdata .text section: 0x30DF virtual size 0x3200 size of raw data .data section: 0x130 virtual size 0x200 size of raw data GCC ElfConverted selected header values: Main header: 0x4000 size of code 0x1000 size of initialized data 0x0size of uninitialized data 0x1000 section alignment 0x1000 file alignment 0x7000 size of image 3 sections: .data, .text, .reloc .text section: 0x4000 virtual size 0x4000 size of raw data .data section: 0x1000 virtual size 0x1000 size of raw data So my concern here is that ElfConvert takes a different view of VirtualSize, that is should be section aligned, whereas MSVC binaries take VirtualSize to be the actual size without padding. I think the correct thing to do would be change ElfConvert to do what MSVC does (although the spec is vague and so I can understand the confusion). I don’t think it really matters, but it wouldn’t hurt either. Both kinds of binaries are in the wild, so you cannot really leverage any of the choices’ advantages either way. Adjusting to MSVC’s behaviour would be right though, as you can at least properly distinguish between padding and 0-data with new binaries. Yeah, I agree. I may take
Re: [edk2-devel][PATCH v1 1/3] MdeModulePkg: ImagePropertiesRecordLib: Use SectionAlignment for CodeSize
On 3/14/2024 2:57 PM, Marvin Häuser wrote: On 14. Mar 2024, at 15:45, Oliver Smith-Denny wrote: This does not appear to be the case with MSVC built binaries. I am seeing that VirtualSize is the size of the data-initialized part of the section. What? :( So you are saying modern MSVC generates PEs that either have non-contiguous sections, or they have sections that contain zero-initialized portions explicitly? Both variants sound badly borked and should be investigated, even if only for resource constraints. I don't think this is what I'm saying. What I am trying to say is that on MSVC, I see PE images getting created that have VirtualSize set to the actual number of initialized bytes in that section (not padded to the section alignment). On ElfConverted binaries, I see the VirtualSize is padded to the section alignment. I've dropped an example below Just thinking about it, is there any chance you set ALIGN = FILEALIGN? No, the specific case where I was researching this was explicitly setting /ALIGN:0x1 and /FILEALIGN:0x1000 for DXE_RUNTIME_DRIVERs on ARM64 (a UEFI spec requirement). So I would see the SizeOfRawData is aligned to the file alignment, as expected, but VirtualSize would be the actual size of the data. Again, the troubling thing here for me is that the same binary built with gcc has the VirtualSize aligned to the section alignment. And I have seen other code that loads PE images that relies on VirtualSize not including the padding. The spec is vague here, it says VirtualSize is the size of the section as loaded in memory (which would lead me to believe this should include padding) but it does not explicitly say it should be a multiple of the section alignment (as other fields do). But at a minimum I think we should have different toolchains doing the same behavior here. Because then this will be invoked and your file will be converted to XIP: https://github.com/tianocore/edk2/blob/5572b43c6767f7cc46b074ae1fc288f6eccdc65d/BaseTools/Source/C/GenFw/GenFw.c#L612 <https://github.com/tianocore/edk2/blob/5572b43c6767f7cc46b074ae1fc288f6eccdc65d/BaseTools/Source/C/GenFw/GenFw.c#L612> BaseTools has no concept of what should be a XIP and what should not, so if a file somewhat qualifies, it will just always emit a XIP: https://github.com/tianocore/edk2/blob/5572b43c6767f7cc46b074ae1fc288f6eccdc65d/BaseTools/Source/C/GenFw/GenFw.c#L2150 <https://github.com/tianocore/edk2/blob/5572b43c6767f7cc46b074ae1fc288f6eccdc65d/BaseTools/Source/C/GenFw/GenFw.c#L2150> In AUDK I addressed this by tying XIP to reloc stripping, but that is more of a convenient heuristic: https://github.com/acidanthera/audk/blob/13f99fa7cbde0829ba98e1e4f2a03891a0791ac6/BaseTools/Source/Python/GenFds/EfiSection.py#L293 <https://github.com/acidanthera/audk/blob/13f99fa7cbde0829ba98e1e4f2a03891a0791ac6/BaseTools/Source/Python/GenFds/EfiSection.py#L293> Whereas on our ElfConverted binaries, what you say is true. The difference concerns me. Yes, very concerning. Would you mind sharing dumps (section table, etc.) and/or binaries (unless you indeed hit what I outlined above)? I recall MSVC to rather be more aggressive with space-savings, whereas ElfConvert would include section-alignment padding within the file (but not zero-init data). Correct, ElfConvert sets the file alignment and the section alignment to the same value, based on common-page-size/max-page-size. I will show some examples at the bottom. Agreed. I have done further research and v2 uses VirtualSize. I was initially led astray here because the ElfConvert tool sets VirtualSize and SizeOfRawData to the SectionAlignment universally: https://github.com/tianocore/edk2/blob/5572b43c6767f7cc46b074ae1fc288f6eccdc65d/BaseTools/Source/C/GenFw/ElfConvert.c#L134-L136 <https://github.com/tianocore/edk2/blob/5572b43c6767f7cc46b074ae1fc288f6eccdc65d/BaseTools/Source/C/GenFw/ElfConvert.c#L134-L136> Yes… plenty of more things in the tool to lead people astray. :) Yep, the PE headers are definitely filled with information that can be deduced from other sources and the spec is not very verbose, which is why I definitely want to tread carefully here. For example, it says SizeOfImage is the size of the image loaded into memory, which current implementations seem to take to mean the sum of the headers plus each section's VirtualSize rounded to the SectionAlignment (which the spec does call out that SizeOfImage must be aligned to the SectionAlignment). VirtualSize it says is also the size of the section as loaded into memory, but current implementations (other than our ElfConvert script which I'm currently investigating if we should change that) seem to take this to mean the size of the initialized data in memory, i.e. the "real" data from the image, not the zeroed data to get to the SectionAlignment. Do you have an example for that? I’m quite certain SizeOfImage should always cover the whole thing. See below
Re: [edk2-devel][PATCH v1 1/3] MdeModulePkg: ImagePropertiesRecordLib: Use SectionAlignment for CodeSize
Hi Marvin, Thanks for looking at this. Since sending out the v1, I have done a lot more digging into our PE loader, others, the spec, and differences between MSVC and our ElfConvert tool. I agree that my understanding was flawed in my original comments. On 3/14/2024 2:57 AM, Marvin Häuser wrote: Good day everyone, Sorry for interjecting, but I’d like to avoid premature changes to PE code that would only make the current mess even worse. On 4. Mar 2024, at 20:24, Oliver Smith-Denny wrote: I relooked at the spec, you are correct, SizeOfRawData is aligned to the FileAlignment. So this is only a bug for a file with SectionAlignment != FileAlignment. Still no, for some reasons you already mentioned, but also reasons you didn’t: - VirtualSize often is not aligned, so they can still trivially mismatch. - VirtualSize includes zero-initialized data not part of the file. This does not appear to be the case with MSVC built binaries. I am seeing that VirtualSize is the size of the data-initialized part of the section. Whereas on our ElfConverted binaries, what you say is true. The difference concerns me. - In the old days, VirtualSize = 0 was used to disable loading of debug data, but SizeOfRawData remained intact, so it could be loaded on demand. - SizeOfRawData may be unaligned due to bugs. In fact, there is no reason to trust FileAlignment, it has no real meaning in an UEFI context (I am not even sure it does on Windows). I see, yes I do believe VirtualSize could be used here instead. *Must*. As Ard said, SizeOfRawData is only interesting to know how many bytes to copy from the file and for *nothing* else. Agreed. I have done further research and v2 uses VirtualSize. I was initially led astray here because the ElfConvert tool sets VirtualSize and SizeOfRawData to the SectionAlignment universally: https://github.com/tianocore/edk2/blob/5572b43c6767f7cc46b074ae1fc288f6eccdc65d/BaseTools/Source/C/GenFw/ElfConvert.c#L134-L136 Two things give me pause. One is that the PE spec states that SizeOfRawData is rounded and VirtualSize is not, so that SizeOfRawData may be greater than the VirtualSize in some cases (which seems incorrect). The other is that the image loader partially uses VirtualSize when loading: https://github.com/tianocore/edk2/blob/918288ab5a7c3abe9c58d576ccc0ae32e2c7dea0/MdePkg/Library/BasePeCoffLib/BasePeCoff.c#L1399-L1400 However, when determining the size of a loaded image (and therefore the number of pages to allocate) it will allocate an extra page: https://github.com/tianocore/edk2/blob/918288ab5a7c3abe9c58d576ccc0ae32e2c7dea0/MdeModulePkg/Core/Dxe/Image/Image.c#L646-L652 Sorry, I don’t understand the “however”, this is sound. as ImageSize here is: https://github.com/tianocore/edk2/blob/918288ab5a7c3abe9c58d576ccc0ae32e2c7dea0/MdePkg/Library/BasePeCoffLib/BasePeCoff.c#L312 Which according to the spec, SizeOfImage is the size of the image as loaded into memory and must be a multiple of section alignment. Side note, SizeOfImage may be borked, not only due to bugs, but also due to malice. There is literally no reason whatsoever to use it - to validate it, you need to calculate a sane value, and once you have that, you obviously do not need SizeOfImage anymore. You could reject images with malformed SizeOfImage, but there likely are various legitimate-bugged ones out there. This is just an ancient artifact from before memory safety was a thing. :) Yep, the PE headers are definitely filled with information that can be deduced from other sources and the spec is not very verbose, which is why I definitely want to tread carefully here. For example, it says SizeOfImage is the size of the image loaded into memory, which current implementations seem to take to mean the sum of the headers plus each section's VirtualSize rounded to the SectionAlignment (which the spec does call out that SizeOfImage must be aligned to the SectionAlignment). VirtualSize it says is also the size of the section as loaded into memory, but current implementations (other than our ElfConvert script which I'm currently investigating if we should change that) seem to take this to mean the size of the initialized data in memory, i.e. the "real" data from the image, not the zeroed data to get to the SectionAlignment. But the language in the spec is almost identical between the two. And again, we have a difference in our MSVC and gcc built binaries here. So, reflecting on this, let me test with VirtualSize here, I think that is the right value to use, the only time we would have a SizeOfRawData that is greater than the VirtualSize would be if our FileAlignment is greater than our SectionAlignment, which would be a misconfiguration. No, it can simply be that FileAlignment = SectionAlignment and SizeOfRawData is aligned and VirtualSize is not. Even when both are aligned, the former may carry extra optional data (like previously mentioned debug data), or it might be some
Re: [edk2-devel][PATCH v3 0/3] Fix Runtime Granularity Issues
Hi Ray and Mike, Can you help get this patchset reviewed and merged? It contains a fix for a UEFI spec violation and it has been sitting on the mailing list for a month. Unfortunately Liming has not reviewed after weekly friendly pings and he is the only main level MdeModulePkg maintainer. Liming, perhaps you could use a co-maintainer from the community to help balance the load? I totally understand that being a maintainer is a large burden, but I also have several patchsets up that need reviews and merging for over a month. Thanks, Oliver On 3/9/2024 11:06 AM, Oliver Smith-Denny wrote: This patch series is the third version of MdeModulePkg: DxeCore: Don't Guard Large Runtime Granularity Allocations. The subject line has been updated because this went from a one commit patch with no cover letter to a multi-commit patch. The commit messages cover the vast amount of detail here, but this patchset fixes three issues: - a UEFI spec violation for which memory types require runtime page allocation granularity alignment - An incompatibility of the heap guard system to guard these regions that require runtime page allocation granularities greater than the EFI_PAGE_SIZE. - A CodeQL error that fails CI when updating the Page.c code v3: - edit comments for readability v2: - Add commit to fix UEFI spec violation - Add commit to fix newly flagged CodeQL error - Update guard commit message, comments, and static assert to use the correct types Cc: Leif Lindholm Cc: Ard Biesheuvel Cc: Sami Mujawar Cc: Liming Gao Oliver Smith-Denny (3): MdeModulePkg: DxeCore: Fix CodeQL Error in FreePages MdeModulePkg: DxeCore: Correct Runtime Granularity Memory Type MdeModulePkg: DxeCore: Do Not Apply Guards to Unsupported Types MdeModulePkg/MdeModulePkg.dec | 10 + MdeModulePkg/Core/Dxe/Mem/HeapGuard.h | 14 + MdeModulePkg/Core/Dxe/Mem/Page.c | 22 +--- MdeModulePkg/Core/Dxe/Mem/Pool.c | 15 +++-- MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c | 4 ++-- MdeModulePkg/Core/Pei/Memory/MemoryServices.c | 2 +- 6 files changed, 59 insertions(+), 8 deletions(-) -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#116719): https://edk2.groups.io/g/devel/message/116719 Mute This Topic: https://groups.io/mt/104832605/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] [PATCH v2 0/3] MdeModulePkg: ImagePropertiesRecordLib Fixes
Hi Liming, Friendly ping, can you please review this patchset? Thanks, Oliver On 3/11/2024 2:29 PM, Oliver Smith-Denny wrote: ImagePropertiesRecordLib is currently creating Image Records that are not accurate. It is setting the CodeSegmentSize to be the size of the raw data in the image file, however, when the image is loaded into memory, the raw data size is aligned to the section alignment. This caused the memory attributes table to have incorrect entries for systems, like ARM64, where the section alignment is not 4k for all modules. In fixing this, I noticed that MemoryProtection.c is using its own version of image record creation where this logic was actually correct. ImagePropertiesRecordLib was created to consolidate the logic around creating and managing image records, so this patchset also updates MemoryProtection.c to use ImagePropertiesRecordsLib after making a few small adjustments to ensure the same functionality is present. This patchset was tested on ArmVirtQemu to ensure that all image records were the same before and after this, other than fixing the CodeSegmentSize. v2: - Align VirtualSize instead of SizeOfRawData Github PR: https://github.com/tianocore/edk2/pull/5402 Cc: Liming Gao Cc: Leif Lindholm Cc: Ard Biesheuvel Cc: Sami Mujawar Cc: Taylor Beebe Oliver Smith-Denny (3): MdeModulePkg: ImagePropertiesRecordLib: Use SectionAlignment for CodeSize MdeModulePkg: ImagePropertiesRecordLib: Consolidate Usage MdeModulePkg: MemoryProtection: Use ImageRecordPropertiesLib MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c| 241 +++- MdeModulePkg/Library/ImagePropertiesRecordLib/ImagePropertiesRecordLib.c | 86 +-- 2 files changed, 94 insertions(+), 233 deletions(-) -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#116718): https://edk2.groups.io/g/devel/message/116718 Mute This Topic: https://groups.io/mt/104873191/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel][PATCH v1 1/3] MdeModulePkg: ImagePropertiesRecordLib: Use SectionAlignment for CodeSize
On 3/12/2024 1:32 AM, Ard Biesheuvel wrote: On Mon, 11 Mar 2024 at 20:34, Oliver Smith-Denny wrote: On 3/4/2024 2:38 PM, Oliver Smith-Denny wrote: On 3/4/2024 11:24 AM, Oliver Smith-Denny wrote: On 3/4/2024 10:54 AM, Ard Biesheuvel wrote: On Mon, 4 Mar 2024 at 18:49, Oliver Smith-Denny wrote: Hi Ard, On 3/1/2024 3:58 AM, Ard Biesheuvel wrote: Hi Oliver, On Tue, 27 Feb 2024 at 21:27, Oliver Smith-Denny wrote: - ImageRecordCodeSection->CodeSegmentSize = Section[Index].SizeOfRawData; + ImageRecordCodeSection->CodeSegmentSize = ALIGN_VALUE (Section[Index].SizeOfRawData, SectionAlignment); This should be the virtual size, not the file size, right? Correct, SectionAlignment is the alignment of the image as loaded in memory, so in the case of a DXE runtime driver on ARM64, it will be 64k. No, I mean we should not be using SizeOfRawData here but VirtualSize. I understand this is unlikely to make a difference in practice, but VirtualSize may exceed SizeOfRawData by a wide margin, and we need the whole region to be covered. I see, yes I do believe VirtualSize could be used here instead. Two things give me pause. One is that the PE spec states that SizeOfRawData is rounded and VirtualSize is not, so that SizeOfRawData may be greater than the VirtualSize in some cases (which seems incorrect). The other is that the image loader partially uses VirtualSize when loading: https://github.com/tianocore/edk2/blob/918288ab5a7c3abe9c58d576ccc0ae32e2c7dea0/MdePkg/Library/BasePeCoffLib/BasePeCoff.c#L1399-L1400 However, when determining the size of a loaded image (and therefore the number of pages to allocate) it will allocate an extra page: https://github.com/tianocore/edk2/blob/918288ab5a7c3abe9c58d576ccc0ae32e2c7dea0/MdeModulePkg/Core/Dxe/Image/Image.c#L646-L652 as ImageSize here is: https://github.com/tianocore/edk2/blob/918288ab5a7c3abe9c58d576ccc0ae32e2c7dea0/MdePkg/Library/BasePeCoffLib/BasePeCoff.c#L312 Which according to the spec, SizeOfImage is the size of the image as loaded into memory and must be a multiple of section alignment. So, reflecting on this, let me test with VirtualSize here, I think that is the right value to use, the only time we would have a SizeOfRawData that is greater than the VirtualSize would be if our FileAlignment is greater than our SectionAlignment, which would be a misconfiguration. I do think the ImageLoader should also be fixed to only allocate ImageSize number of pages (which should be the sum of the section VirtualSizes + any headers that aren't included). Might as well not waste an extra page for each image and then our image record code is simpler as well (ensuring we are protecting the right pages). I think this patch series should go in, as it is fixing an active bug, but I will also take a look at the image loader creating the image records and having a protocol it produces to retrieve the list, to attempt to avoid issues like this in the future. Surprisingly, I am seeing that the VirtualSize is not 64k aligned there. I am looking deeper into GenFw to make sure it is correctly getting set to align to SectionAlignment in the section headers. When I use dumpbin to dump the headers, it shows each section having VirtualSize as 64k aligned for a runtime image, but the same image doesn't show that in FW. I'll do some digging here. Following up on this: Not surprisingly, different toolchains do different things here. gcc obviously creates ELF files and ElfConvert*.c converts these to PE images. However, when it does this, it always sets the section and file alignment to the same value (I'm not as familiar with ELF images, I'm not sure if there is the same concept as file vs section alignment). GenFw could probably update this information to shrink the file alignment, but that's a space optimization for gcc built binaries. In ElfConvert.c, the VirtualSize does get set to the section aligned value, which is what I would expect from the spec. In MSVC PE images, VirtualSize is not section aligned and the expectation appears to be that loaders will align the VirtualSize to the section alignment. I am planning on reaching out to the MSVC folks to learn why this is the case, is it intended, etc., as my understanding is that the VirtualSize in the section headers should be section aligned. We are stuck with this for existing MSVC toolchains, however, so we will need to align either the VirtualSize or SizeOfRawData to the section alignment when we create the image records. I don't have a preference between the two, they end up being the same when we align them, so I can send a v2 with aligning the VirtualSize. This will be a no-op on gcc built binaries and will set it to the correct value for MSVC. I don't think this is correct. The VirtualSize could be much larger than the SizeOfRawData (to the extent where the delta exceeds the alignment padding), in cases where some of the memory is data-initialized and some of it is zero-initi
[edk2-devel] [PATCH v2 0/3] MdeModulePkg: ImagePropertiesRecordLib Fixes
ImagePropertiesRecordLib is currently creating Image Records that are not accurate. It is setting the CodeSegmentSize to be the size of the raw data in the image file, however, when the image is loaded into memory, the raw data size is aligned to the section alignment. This caused the memory attributes table to have incorrect entries for systems, like ARM64, where the section alignment is not 4k for all modules. In fixing this, I noticed that MemoryProtection.c is using its own version of image record creation where this logic was actually correct. ImagePropertiesRecordLib was created to consolidate the logic around creating and managing image records, so this patchset also updates MemoryProtection.c to use ImagePropertiesRecordsLib after making a few small adjustments to ensure the same functionality is present. This patchset was tested on ArmVirtQemu to ensure that all image records were the same before and after this, other than fixing the CodeSegmentSize. v2: - Align VirtualSize instead of SizeOfRawData Github PR: https://github.com/tianocore/edk2/pull/5402 Cc: Liming Gao Cc: Leif Lindholm Cc: Ard Biesheuvel Cc: Sami Mujawar Cc: Taylor Beebe Oliver Smith-Denny (3): MdeModulePkg: ImagePropertiesRecordLib: Use SectionAlignment for CodeSize MdeModulePkg: ImagePropertiesRecordLib: Consolidate Usage MdeModulePkg: MemoryProtection: Use ImageRecordPropertiesLib MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c| 241 +++- MdeModulePkg/Library/ImagePropertiesRecordLib/ImagePropertiesRecordLib.c | 86 +-- 2 files changed, 94 insertions(+), 233 deletions(-) -- 2.40.1 -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#116657): https://edk2.groups.io/g/devel/message/116657 Mute This Topic: https://groups.io/mt/104873191/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel][PATCH v1 1/3] MdeModulePkg: ImagePropertiesRecordLib: Use SectionAlignment for CodeSize
On 3/4/2024 2:38 PM, Oliver Smith-Denny wrote: On 3/4/2024 11:24 AM, Oliver Smith-Denny wrote: On 3/4/2024 10:54 AM, Ard Biesheuvel wrote: On Mon, 4 Mar 2024 at 18:49, Oliver Smith-Denny wrote: Hi Ard, On 3/1/2024 3:58 AM, Ard Biesheuvel wrote: Hi Oliver, On Tue, 27 Feb 2024 at 21:27, Oliver Smith-Denny wrote: - ImageRecordCodeSection->CodeSegmentSize = Section[Index].SizeOfRawData; + ImageRecordCodeSection->CodeSegmentSize = ALIGN_VALUE (Section[Index].SizeOfRawData, SectionAlignment); This should be the virtual size, not the file size, right? Correct, SectionAlignment is the alignment of the image as loaded in memory, so in the case of a DXE runtime driver on ARM64, it will be 64k. No, I mean we should not be using SizeOfRawData here but VirtualSize. I understand this is unlikely to make a difference in practice, but VirtualSize may exceed SizeOfRawData by a wide margin, and we need the whole region to be covered. I see, yes I do believe VirtualSize could be used here instead. Two things give me pause. One is that the PE spec states that SizeOfRawData is rounded and VirtualSize is not, so that SizeOfRawData may be greater than the VirtualSize in some cases (which seems incorrect). The other is that the image loader partially uses VirtualSize when loading: https://github.com/tianocore/edk2/blob/918288ab5a7c3abe9c58d576ccc0ae32e2c7dea0/MdePkg/Library/BasePeCoffLib/BasePeCoff.c#L1399-L1400 However, when determining the size of a loaded image (and therefore the number of pages to allocate) it will allocate an extra page: https://github.com/tianocore/edk2/blob/918288ab5a7c3abe9c58d576ccc0ae32e2c7dea0/MdeModulePkg/Core/Dxe/Image/Image.c#L646-L652 as ImageSize here is: https://github.com/tianocore/edk2/blob/918288ab5a7c3abe9c58d576ccc0ae32e2c7dea0/MdePkg/Library/BasePeCoffLib/BasePeCoff.c#L312 Which according to the spec, SizeOfImage is the size of the image as loaded into memory and must be a multiple of section alignment. So, reflecting on this, let me test with VirtualSize here, I think that is the right value to use, the only time we would have a SizeOfRawData that is greater than the VirtualSize would be if our FileAlignment is greater than our SectionAlignment, which would be a misconfiguration. I do think the ImageLoader should also be fixed to only allocate ImageSize number of pages (which should be the sum of the section VirtualSizes + any headers that aren't included). Might as well not waste an extra page for each image and then our image record code is simpler as well (ensuring we are protecting the right pages). I think this patch series should go in, as it is fixing an active bug, but I will also take a look at the image loader creating the image records and having a protocol it produces to retrieve the list, to attempt to avoid issues like this in the future. Surprisingly, I am seeing that the VirtualSize is not 64k aligned there. I am looking deeper into GenFw to make sure it is correctly getting set to align to SectionAlignment in the section headers. When I use dumpbin to dump the headers, it shows each section having VirtualSize as 64k aligned for a runtime image, but the same image doesn't show that in FW. I'll do some digging here. Following up on this: Not surprisingly, different toolchains do different things here. gcc obviously creates ELF files and ElfConvert*.c converts these to PE images. However, when it does this, it always sets the section and file alignment to the same value (I'm not as familiar with ELF images, I'm not sure if there is the same concept as file vs section alignment). GenFw could probably update this information to shrink the file alignment, but that's a space optimization for gcc built binaries. In ElfConvert.c, the VirtualSize does get set to the section aligned value, which is what I would expect from the spec. In MSVC PE images, VirtualSize is not section aligned and the expectation appears to be that loaders will align the VirtualSize to the section alignment. I am planning on reaching out to the MSVC folks to learn why this is the case, is it intended, etc., as my understanding is that the VirtualSize in the section headers should be section aligned. We are stuck with this for existing MSVC toolchains, however, so we will need to align either the VirtualSize or SizeOfRawData to the section alignment when we create the image records. I don't have a preference between the two, they end up being the same when we align them, so I can send a v2 with aligning the VirtualSize. This will be a no-op on gcc built binaries and will set it to the correct value for MSVC. Thanks, Oliver -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#116653): https://edk2.groups.io/g/devel/message/116653 Mute This Topic: https://groups.io/mt/104610770/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch..
[edk2-devel][PATCH v3 0/3] Fix Runtime Granularity Issues
This patch series is the third version of MdeModulePkg: DxeCore: Don't Guard Large Runtime Granularity Allocations. The subject line has been updated because this went from a one commit patch with no cover letter to a multi-commit patch. The commit messages cover the vast amount of detail here, but this patchset fixes three issues: - a UEFI spec violation for which memory types require runtime page allocation granularity alignment - An incompatibility of the heap guard system to guard these regions that require runtime page allocation granularities greater than the EFI_PAGE_SIZE. - A CodeQL error that fails CI when updating the Page.c code v3: - edit comments for readability v2: - Add commit to fix UEFI spec violation - Add commit to fix newly flagged CodeQL error - Update guard commit message, comments, and static assert to use the correct types Cc: Leif Lindholm Cc: Ard Biesheuvel Cc: Sami Mujawar Cc: Liming Gao Oliver Smith-Denny (3): MdeModulePkg: DxeCore: Fix CodeQL Error in FreePages MdeModulePkg: DxeCore: Correct Runtime Granularity Memory Type MdeModulePkg: DxeCore: Do Not Apply Guards to Unsupported Types MdeModulePkg/MdeModulePkg.dec | 10 + MdeModulePkg/Core/Dxe/Mem/HeapGuard.h | 14 + MdeModulePkg/Core/Dxe/Mem/Page.c | 22 +--- MdeModulePkg/Core/Dxe/Mem/Pool.c | 15 +++-- MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c | 4 ++-- MdeModulePkg/Core/Pei/Memory/MemoryServices.c | 2 +- 6 files changed, 59 insertions(+), 8 deletions(-) -- 2.40.1 -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#116559): https://edk2.groups.io/g/devel/message/116559 Mute This Topic: https://groups.io/mt/104832605/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel][PATCH v2 0/3] Fix Runtime Granularity Issues
Hi Liming, Friendly ping again, can you please review this? It fixes a UEFI spec violation. Thanks, Oliver On 2/26/2024 8:46 AM, Oliver Smith-Denny wrote: Hi Liming, Now that the stable tag is finished, can you review this MdeModulePkg patch? Thanks, Oliver On 2/16/2024 5:27 PM, Oliver Smith-Denny wrote: This patch series is the second version of MdeModulePkg: DxeCore: Don't Guard Large Runtime Granularity Allocations. The subject line has been updated because this went from a one commit patch with no cover letter to a multi-commit patch. The commit messages cover the vast amount of detail here, but this patchset fixes three issues: - a UEFI spec violation for which memory types require runtime page allocation granularity alignment - An incompatibility of the heap guard system to guard these regions that require runtime page allocation granularities greater than the EFI_PAGE_SIZE. - A CodeQL error that fails CI when updating the Page.c code v2: - Add commit to fix UEFI spec violation - Add commit to fix newly flagged CodeQL error - Update guard commit message, comments, and static assert to use the correct types Cc: Leif Lindholm Cc: Ard Biesheuvel Cc: Sami Mujawar Cc: Liming Gao Oliver Smith-Denny (3): MdeModulePkg: DxeCore: Fix CodeQL Error in FreePages MdeModulePkg: DxeCore: Correct Runtime Granularity Memory Type MdeModulePkg: DxeCore: Do Not Apply Guards to Unsupported Types MdeModulePkg/MdeModulePkg.dec | 10 + MdeModulePkg/Core/Dxe/Mem/HeapGuard.h | 14 + MdeModulePkg/Core/Dxe/Mem/Page.c | 22 +--- MdeModulePkg/Core/Dxe/Mem/Pool.c | 15 +++-- MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c | 4 ++-- MdeModulePkg/Core/Pei/Memory/MemoryServices.c | 2 +- 6 files changed, 59 insertions(+), 8 deletions(-) -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#116452): https://edk2.groups.io/g/devel/message/116452 Mute This Topic: https://groups.io/mt/104405577/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel][PATCH v1 1/3] MdeModulePkg: ImagePropertiesRecordLib: Use SectionAlignment for CodeSize
On 3/4/2024 11:24 AM, Oliver Smith-Denny wrote: On 3/4/2024 10:54 AM, Ard Biesheuvel wrote: On Mon, 4 Mar 2024 at 18:49, Oliver Smith-Denny wrote: Hi Ard, On 3/1/2024 3:58 AM, Ard Biesheuvel wrote: Hi Oliver, On Tue, 27 Feb 2024 at 21:27, Oliver Smith-Denny wrote: - ImageRecordCodeSection->CodeSegmentSize = Section[Index].SizeOfRawData; + ImageRecordCodeSection->CodeSegmentSize = ALIGN_VALUE (Section[Index].SizeOfRawData, SectionAlignment); This should be the virtual size, not the file size, right? Correct, SectionAlignment is the alignment of the image as loaded in memory, so in the case of a DXE runtime driver on ARM64, it will be 64k. No, I mean we should not be using SizeOfRawData here but VirtualSize. I understand this is unlikely to make a difference in practice, but VirtualSize may exceed SizeOfRawData by a wide margin, and we need the whole region to be covered. I see, yes I do believe VirtualSize could be used here instead. Two things give me pause. One is that the PE spec states that SizeOfRawData is rounded and VirtualSize is not, so that SizeOfRawData may be greater than the VirtualSize in some cases (which seems incorrect). The other is that the image loader partially uses VirtualSize when loading: https://github.com/tianocore/edk2/blob/918288ab5a7c3abe9c58d576ccc0ae32e2c7dea0/MdePkg/Library/BasePeCoffLib/BasePeCoff.c#L1399-L1400 However, when determining the size of a loaded image (and therefore the number of pages to allocate) it will allocate an extra page: https://github.com/tianocore/edk2/blob/918288ab5a7c3abe9c58d576ccc0ae32e2c7dea0/MdeModulePkg/Core/Dxe/Image/Image.c#L646-L652 as ImageSize here is: https://github.com/tianocore/edk2/blob/918288ab5a7c3abe9c58d576ccc0ae32e2c7dea0/MdePkg/Library/BasePeCoffLib/BasePeCoff.c#L312 Which according to the spec, SizeOfImage is the size of the image as loaded into memory and must be a multiple of section alignment. So, reflecting on this, let me test with VirtualSize here, I think that is the right value to use, the only time we would have a SizeOfRawData that is greater than the VirtualSize would be if our FileAlignment is greater than our SectionAlignment, which would be a misconfiguration. I do think the ImageLoader should also be fixed to only allocate ImageSize number of pages (which should be the sum of the section VirtualSizes + any headers that aren't included). Might as well not waste an extra page for each image and then our image record code is simpler as well (ensuring we are protecting the right pages). I think this patch series should go in, as it is fixing an active bug, but I will also take a look at the image loader creating the image records and having a protocol it produces to retrieve the list, to attempt to avoid issues like this in the future. Surprisingly, I am seeing that the VirtualSize is not 64k aligned there. I am looking deeper into GenFw to make sure it is correctly getting set to align to SectionAlignment in the section headers. When I use dumpbin to dump the headers, it shows each section having VirtualSize as 64k aligned for a runtime image, but the same image doesn't show that in FW. I'll do some digging here. Thanks, Oliver -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#116343): https://edk2.groups.io/g/devel/message/116343 Mute This Topic: https://groups.io/mt/104610770/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel][PATCH v1 1/3] MdeModulePkg: ImagePropertiesRecordLib: Use SectionAlignment for CodeSize
On 3/4/2024 10:54 AM, Ard Biesheuvel wrote: On Mon, 4 Mar 2024 at 18:49, Oliver Smith-Denny wrote: Hi Ard, On 3/1/2024 3:58 AM, Ard Biesheuvel wrote: Hi Oliver, On Tue, 27 Feb 2024 at 21:27, Oliver Smith-Denny wrote: When an ImageRecord is stored by ImagePropertiesRecordLib, it reports the CodeSegmentSize as the SizeOfRawData from the image. However, the image as loaded into memory is aligned to the SectionAlignment, so SizeOfRawData is under the actual size in memory. This is important, because the memory attributes table uses these image records to create its entries and it will report that the alignment of an image is incorrect, even though the actual image is correct. This was discovered on ARM64, which has a 64k runtime page granularity alignment, which is backed by a 64k section alignment for DXE_RUNTIME_DRIVERs. The runtime code and data was correctly being loaded into memory, however the memory attribute table was incorrectly reporting misaligned ranges to the OS, causing attributes to be ignored for these sections for OSes using greater than 4k pages. This patch correctly aligns the CodeSegmentSize to the SectionAlignment and the corresponding memory attribute table entries are now correctly aligned and pointing to the right places in memory. Can you explain how these can differ in the first place? Our flaky ELF-to-PE/COFF converter should never generate such images to begin with (which is probably how we ended up with this problem in the first place), so I suppose this is native PE/COFF tooling emitting sections either using a non-1:1 file:memory mapping, or with unallocated holes in the file representation? This is an artifact of PE/COFF itself and is useful for having smaller FW images. In PE/COFF we have the section alignment (which is how we get loaded into memory) and the file alignment (how the actual file is aligned). It is valid for these two to be different. For example, these runtime DXE drivers, which are not XIP (which the case we do need the section and file alignment to be the same, as we are executing from the file image) can be a smaller size in the file, but when loaded into memory we will relocate them and do the proper rebasing to set these on 64k boundaries. Different toolchains have different ways of specifying the two things, on gcc I have seen common-page-size affect the section alignment and max-page-size affect both section and file alignment. For msvc there are /ALIGN and /FILEALIGN commands which directly manipulate these values. The issue here was not that we have different section and file alignment, in fact, the issue still exists if section alignement == file alignment. This is because SizeOfRawData in the PE/COFF header is the raw size of bytes used, not even page aligned. So no matter what, the image records we were creating here had bad sizes being set which do not match what the image loader was actually doing. IIUC the SizeOfRawData is the file view not the memory view, and must always be aligned to the FileAlignment. I relooked at the spec, you are correct, SizeOfRawData is aligned to the FileAlignment. So this is only a bug for a file with SectionAlignment != FileAlignment. I do think there is a fair argument that would say the image loader should create the image records when it loads images, since in the end we want the record to match exactly what the image in memory is, creating after the fact is a poor pattern. Yeah, no disagreement there. Cc: Liming Gao Cc: Leif Lindholm Cc: Ard Biesheuvel Cc: Sami Mujawar Cc: Taylor Beebe Signed-off-by: Oliver Smith-Denny --- MdeModulePkg/Library/ImagePropertiesRecordLib/ImagePropertiesRecordLib.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/MdeModulePkg/Library/ImagePropertiesRecordLib/ImagePropertiesRecordLib.c b/MdeModulePkg/Library/ImagePropertiesRecordLib/ImagePropertiesRecordLib.c index e53ce086c54c..07ced0e54e38 100644 --- a/MdeModulePkg/Library/ImagePropertiesRecordLib/ImagePropertiesRecordLib.c +++ b/MdeModulePkg/Library/ImagePropertiesRecordLib/ImagePropertiesRecordLib.c @@ -1090,7 +1090,7 @@ CreateImagePropertiesRecord ( ImageRecordCodeSection->Signature = IMAGE_PROPERTIES_RECORD_CODE_SECTION_SIGNATURE; ImageRecordCodeSection->CodeSegmentBase = (UINTN)ImageBase + Section[Index].VirtualAddress; - ImageRecordCodeSection->CodeSegmentSize = Section[Index].SizeOfRawData; + ImageRecordCodeSection->CodeSegmentSize = ALIGN_VALUE (Section[Index].SizeOfRawData, SectionAlignment); This should be the virtual size, not the file size, right? Correct, SectionAlignment is the alignment of the image as loaded in memory, so in the case of a DXE runtime driver on ARM64, it will be 64k. No, I mean we should not be using SizeOfRawData here but VirtualSize. I understand this is unlikely to make a difference in practice, but VirtualSize may exceed SizeOfRawData by a wide margin, and we need the
Re: [edk2-devel][PATCH v1 1/3] MdeModulePkg: ImagePropertiesRecordLib: Use SectionAlignment for CodeSize
Hi Ard, On 3/1/2024 3:58 AM, Ard Biesheuvel wrote: Hi Oliver, On Tue, 27 Feb 2024 at 21:27, Oliver Smith-Denny wrote: When an ImageRecord is stored by ImagePropertiesRecordLib, it reports the CodeSegmentSize as the SizeOfRawData from the image. However, the image as loaded into memory is aligned to the SectionAlignment, so SizeOfRawData is under the actual size in memory. This is important, because the memory attributes table uses these image records to create its entries and it will report that the alignment of an image is incorrect, even though the actual image is correct. This was discovered on ARM64, which has a 64k runtime page granularity alignment, which is backed by a 64k section alignment for DXE_RUNTIME_DRIVERs. The runtime code and data was correctly being loaded into memory, however the memory attribute table was incorrectly reporting misaligned ranges to the OS, causing attributes to be ignored for these sections for OSes using greater than 4k pages. This patch correctly aligns the CodeSegmentSize to the SectionAlignment and the corresponding memory attribute table entries are now correctly aligned and pointing to the right places in memory. Can you explain how these can differ in the first place? Our flaky ELF-to-PE/COFF converter should never generate such images to begin with (which is probably how we ended up with this problem in the first place), so I suppose this is native PE/COFF tooling emitting sections either using a non-1:1 file:memory mapping, or with unallocated holes in the file representation? This is an artifact of PE/COFF itself and is useful for having smaller FW images. In PE/COFF we have the section alignment (which is how we get loaded into memory) and the file alignment (how the actual file is aligned). It is valid for these two to be different. For example, these runtime DXE drivers, which are not XIP (which the case we do need the section and file alignment to be the same, as we are executing from the file image) can be a smaller size in the file, but when loaded into memory we will relocate them and do the proper rebasing to set these on 64k boundaries. Different toolchains have different ways of specifying the two things, on gcc I have seen common-page-size affect the section alignment and max-page-size affect both section and file alignment. For msvc there are /ALIGN and /FILEALIGN commands which directly manipulate these values. The issue here was not that we have different section and file alignment, in fact, the issue still exists if section alignement == file alignment. This is because SizeOfRawData in the PE/COFF header is the raw size of bytes used, not even page aligned. So no matter what, the image records we were creating here had bad sizes being set which do not match what the image loader was actually doing. I do think there is a fair argument that would say the image loader should create the image records when it loads images, since in the end we want the record to match exactly what the image in memory is, creating after the fact is a poor pattern. Cc: Liming Gao Cc: Leif Lindholm Cc: Ard Biesheuvel Cc: Sami Mujawar Cc: Taylor Beebe Signed-off-by: Oliver Smith-Denny --- MdeModulePkg/Library/ImagePropertiesRecordLib/ImagePropertiesRecordLib.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/MdeModulePkg/Library/ImagePropertiesRecordLib/ImagePropertiesRecordLib.c b/MdeModulePkg/Library/ImagePropertiesRecordLib/ImagePropertiesRecordLib.c index e53ce086c54c..07ced0e54e38 100644 --- a/MdeModulePkg/Library/ImagePropertiesRecordLib/ImagePropertiesRecordLib.c +++ b/MdeModulePkg/Library/ImagePropertiesRecordLib/ImagePropertiesRecordLib.c @@ -1090,7 +1090,7 @@ CreateImagePropertiesRecord ( ImageRecordCodeSection->Signature = IMAGE_PROPERTIES_RECORD_CODE_SECTION_SIGNATURE; ImageRecordCodeSection->CodeSegmentBase = (UINTN)ImageBase + Section[Index].VirtualAddress; - ImageRecordCodeSection->CodeSegmentSize = Section[Index].SizeOfRawData; + ImageRecordCodeSection->CodeSegmentSize = ALIGN_VALUE (Section[Index].SizeOfRawData, SectionAlignment); This should be the virtual size, not the file size, right? Correct, SectionAlignment is the alignment of the image as loaded in memory, so in the case of a DXE runtime driver on ARM64, it will be 64k. InsertTailList (>CodeSegmentList, >Link); ImageRecord->CodeSegmentCount++; -- 2.40.1 -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#116334): https://edk2.groups.io/g/devel/message/116334 Mute This Topic: https://groups.io/mt/104610770/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
[edk2-devel][PATCH v1 0/3] MdeModulePkg: ImagePropertiesRecordLib Fixes
ImagePropertiesRecordLib is currently creating Image Records that are not accurate. It is setting the CodeSegmentSize to be the size of the raw data in the image file, however, when the image is loaded into memory, the raw data size is aligned to the section alignment. This caused the memory attributes table to have incorrect entries for systems, like ARM64, where the section alignment is not 4k for all modules. In fixing this, I noticed that MemoryProtection.c is using its own version of image record creation where this logic was actually correct. ImagePropertiesRecordLib was created to consolidate the logic around creating and managing image records, so this patchset also updates MemoryProtection.c to use ImagePropertiesRecordsLib after making a few small adjustments to ensure the same functionality is present. This patchset was tested on ArmVirtQemu to ensure that all image records were the same before and after this, other than fixing the CodeSegmentSize. Github PR: https://github.com/tianocore/edk2/pull/5402 Cc: Liming Gao Cc: Leif Lindholm Cc: Ard Biesheuvel Cc: Sami Mujawar Cc: Taylor Beebe Oliver Smith-Denny (3): MdeModulePkg: ImagePropertiesRecordLib: Use SectionAlignment for CodeSize MdeModulePkg: ImagePropertiesRecordLib: Consolidate Usage MdeModulePkg: MemoryProtection: Use ImageRecordPropertiesLib MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c| 241 +++- MdeModulePkg/Library/ImagePropertiesRecordLib/ImagePropertiesRecordLib.c | 84 +-- 2 files changed, 92 insertions(+), 233 deletions(-) -- 2.40.1 -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#116059): https://edk2.groups.io/g/devel/message/116059 Mute This Topic: https://groups.io/mt/104610769/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel][PATCH v2 0/3] Fix Runtime Granularity Issues
Hi Liming, Now that the stable tag is finished, can you review this MdeModulePkg patch? Thanks, Oliver On 2/16/2024 5:27 PM, Oliver Smith-Denny wrote: This patch series is the second version of MdeModulePkg: DxeCore: Don't Guard Large Runtime Granularity Allocations. The subject line has been updated because this went from a one commit patch with no cover letter to a multi-commit patch. The commit messages cover the vast amount of detail here, but this patchset fixes three issues: - a UEFI spec violation for which memory types require runtime page allocation granularity alignment - An incompatibility of the heap guard system to guard these regions that require runtime page allocation granularities greater than the EFI_PAGE_SIZE. - A CodeQL error that fails CI when updating the Page.c code v2: - Add commit to fix UEFI spec violation - Add commit to fix newly flagged CodeQL error - Update guard commit message, comments, and static assert to use the correct types Cc: Leif Lindholm Cc: Ard Biesheuvel Cc: Sami Mujawar Cc: Liming Gao Oliver Smith-Denny (3): MdeModulePkg: DxeCore: Fix CodeQL Error in FreePages MdeModulePkg: DxeCore: Correct Runtime Granularity Memory Type MdeModulePkg: DxeCore: Do Not Apply Guards to Unsupported Types MdeModulePkg/MdeModulePkg.dec | 10 + MdeModulePkg/Core/Dxe/Mem/HeapGuard.h | 14 + MdeModulePkg/Core/Dxe/Mem/Page.c | 22 +--- MdeModulePkg/Core/Dxe/Mem/Pool.c | 15 +++-- MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c | 4 ++-- MdeModulePkg/Core/Pei/Memory/MemoryServices.c | 2 +- 6 files changed, 59 insertions(+), 8 deletions(-) -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#115975): https://edk2.groups.io/g/devel/message/115975 Mute This Topic: https://groups.io/mt/104405577/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
[edk2-devel][PATCH v2 0/3] Fix Runtime Granularity Issues
This patch series is the second version of MdeModulePkg: DxeCore: Don't Guard Large Runtime Granularity Allocations. The subject line has been updated because this went from a one commit patch with no cover letter to a multi-commit patch. The commit messages cover the vast amount of detail here, but this patchset fixes three issues: - a UEFI spec violation for which memory types require runtime page allocation granularity alignment - An incompatibility of the heap guard system to guard these regions that require runtime page allocation granularities greater than the EFI_PAGE_SIZE. - A CodeQL error that fails CI when updating the Page.c code v2: - Add commit to fix UEFI spec violation - Add commit to fix newly flagged CodeQL error - Update guard commit message, comments, and static assert to use the correct types Cc: Leif Lindholm Cc: Ard Biesheuvel Cc: Sami Mujawar Cc: Liming Gao Oliver Smith-Denny (3): MdeModulePkg: DxeCore: Fix CodeQL Error in FreePages MdeModulePkg: DxeCore: Correct Runtime Granularity Memory Type MdeModulePkg: DxeCore: Do Not Apply Guards to Unsupported Types MdeModulePkg/MdeModulePkg.dec | 10 + MdeModulePkg/Core/Dxe/Mem/HeapGuard.h | 14 + MdeModulePkg/Core/Dxe/Mem/Page.c | 22 +--- MdeModulePkg/Core/Dxe/Mem/Pool.c | 15 +++-- MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c | 4 ++-- MdeModulePkg/Core/Pei/Memory/MemoryServices.c | 2 +- 6 files changed, 59 insertions(+), 8 deletions(-) -- 2.40.1 -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#115560): https://edk2.groups.io/g/devel/message/115560 Mute This Topic: https://groups.io/mt/104405577/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel][PATCH v1 1/1] MdeModulePkg: DxeCore: Don't Guard Large Runtime Granularity Allocations
On 2/14/2024 11:50 PM, Ard Biesheuvel wrote: I looked at the EFI spec again, and EfiACPIReclaimMemory is not actually listed as a memory type that has this 64k alignment requirement. This makes sense, given that this memory type has no significance to the firmware itself, only to the OS. OTOH, reserved memory does appear there. So I suggest we fix that first, and then drop any mention of EfiACPIReclaimMemory from this patch. At least we'll have heap guard coverage for ACPI table allocations on arm64 going forward. The logic in question was added in 2007 in commit 28a00297189c, so this was probably the rule on Itanium, but that support is long gone. I wanted to understand this a little bit further. I do see that the spec does not call out EfiACPIReclaimMemory as needing 64k alignment. However, your statement that the memory type does not have have significance to the FW, only to the OS, so we don't need the 64k alignment is confusing to me. Isn't the 64k alignment needed only for regions that the OS does care about? In order to read this information at their 64k page granularity, doesn't this need to be aligned to 64k? Or are you saying the OS can just read the 64k page(s) containing these 4k aligned EfiACPIReclaimMemory pages and then start reading at the calculated offset from within the larger page, since these pages don't have any pointers to outside memory, unlike image sections? Thanks, Oliver -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#115533): https://edk2.groups.io/g/devel/message/115533 Mute This Topic: https://groups.io/mt/104364784/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel][PATCH v1 1/1] MdeModulePkg: DxeCore: Don't Guard Large Runtime Granularity Allocations
On 2/15/2024 9:21 AM, Ard Biesheuvel wrote: Of the two options you presented in this paragraph, I prefer the one where the allocation presented to the caller may not be aligned, but the region plus guards is. But disabling it entirely for these regions is still perfectly fine with me, especially if the remove ACPI reclaim memory from the set. Heap guard is a hardening feature, and if the implementation is too complex to reason about comfortably, I don't think we can confidently rely on it. And as far as the OS is concerned: with the MAT, the runtime DXEs are mapped in a way where the read-only regions are interleaved with the read-write regions, and the holes in between are not mapped at all (at least on Linux). IOW, there is some implicit guarding going on already. Looking back at the UEFI spec (section 2.3.6), I see this: "If a 64KiB physical page contains any 4KiB page with any of the following types listed below, then all 4KiB pages in the 64KiB page must use identical ARM Memory Page Attributes" where the following types are what you listed in the last email. Then there is a further statement: "Mixed attribute mappings within a larger page are not allowed." So this would seem to indicate that pushing the guard pages inside of the 64KiB would break the spec. Now, I think it could be hidden and still meet the intent of the spec, which would be that the OS gets consistent memory attributes reported, but still, the way the spec is written this would seem to be a violation. I'll send out a v2 with the type change. Thanks, Oliver -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#115532): https://edk2.groups.io/g/devel/message/115532 Mute This Topic: https://groups.io/mt/104364784/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] AArch64 with HeapGuard: page allocations wrongly aligned
On 1/22/2024 6:14 PM, Rebecca Cran wrote: Thanks! I wonder if the same problem occurs on LoongArch64, which also defines the runtime page allocation granularity to be 0x1? Yes, it should have exactly the same problem -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#114348): https://edk2.groups.io/g/devel/message/114348 Mute This Topic: https://groups.io/mt/103810212/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] AArch64 with HeapGuard: page allocations wrongly aligned
On 1/22/2024 2:06 PM, Rebecca Cran via groups.io wrote: On 1/19/2024 1:03 PM, Oliver Smith-Denny wrote: Thanks for trying. In lieu of being able to test myself, all I can offer is adding some more prints, when the memory gets allocated, making sure it is 64k aligned then. I'd be curious to see what the address is that is attempting to be freed. My guess (as it was earlier) is that it is going to be aligned to 64k but + 4k. I.e the guard page at the front is throwing it off. There may have just been an error in my attempt to fix the check for that. If however that address is not 64k + 4k aligned, then something else is afoot. Happy to look at some more data if you get it or can engineer an example on an open source system (can you force the system to call this function twice even without the extra SMBIOS entries, etc.). These are the addresses it's allocating with and without HeapGuard (with the original, upstream Page.c code). Without HeapGuard: SmbiosCreate64BitTable() re-allocate SMBIOS 64-bit table Allocated 0xEF11 with gBS->AllocatePages (AllocateAnyPages, EfiRuntimeServicesData, EFI_SIZE_TO_PAGES (55), 0xFB7C) ... SmbiosCreate64BitTable() re-allocate SMBIOS 64-bit table SmbiosCreate64BitTable: calling FreePages (0xEF11, 1) Allocated 0xEF11 with gBS->AllocatePages (AllocateAnyPages, EfiRuntimeServicesData, EFI_SIZE_TO_PAGES (4153), 0xFB8AEC8E) -- WITH HeapGuard: SmbiosCreate64BitTable() re-allocate SMBIOS 64-bit table Allocated 0xED36F000 with gBS->AllocatePages (AllocateAnyPages, EfiRuntimeServicesData, EFI_SIZE_TO_PAGES (55), 0xED38F000) ... SmbiosCreate64BitTable() re-allocate SMBIOS 64-bit table SmbiosCreate64BitTable: calling FreePages (0xED36F000, 1) I was able to repro your bug (by just turning on page guards on ArmVirtQemu, allocating runtime mem and freeing it). I think you are the first person to free runtime mem on ARM64 with page guards enabled (and to care when it failed :). The heap guard code is not written with ARM64 in mind (nor is much of the codebase, of course). Specifically in this case the heap guard code only wishes to preserve 4 KB alignment, it knows nothing of ARM64's runtime page granularity required. Let me take a look at this, I'm working on a solution here, but I want to test this out further. I'll try to send a patch later this week or next. Oliver -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#114162): https://edk2.groups.io/g/devel/message/114162 Mute This Topic: https://groups.io/mt/103810212/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] EFI_SYSTEM_TABLE allocated by AllocateRuntimeCopyPool isn't aligned to 4KB
On 1/22/2024 11:52 AM, Rebecca Cran wrote: I've been working on updating the T32 script EfiLoadDxe.cmm in EmbeddedPkg/Scripts/LauterbachT32 and one of the issues I've run into is that the EFI_SYSTEM_TABLE - pointed to by `gST` and `gDxeCoreST` - isn't aligned to 4KB like the script expects. Instead, I'm seeing AllocateRuntimeCopyPool return 0xFB7D0018 in MdeModulePkg/Core/Dxe/DxeMain/DxeMain.c: // // Allocate the EFI System Table and EFI Runtime Service Table from EfiRuntimeServicesData // Use the templates to initialize the contents of the EFI System Table and EFI Runtime Services Table // gDxeCoreST = AllocateRuntimeCopyPool (sizeof (EFI_SYSTEM_TABLE), ); ASSERT (gDxeCoreST != NULL); I'm wondering which is wrong: should AllocateRuntimeCopyPool be returning a 4KB-aligned pointer, or is the script's assumption wrong? Allocating pool memory will never be 4KB aligned because of the POOL_HEAD. See: https://github.com/tianocore/edk2/blob/master/MdeModulePkg/Core/Dxe/Mem/Pool.c#L463-L537. The allocated page is 4KB aligned, of course, but we store the POOL_HEAD at the beginning of that page and return Head->Data to the caller. In addition, this is only in the case where you allocate a new page for the pool. In the case where the pool has the pages it needs, you don't have any alignment guarantee, I believe, it is just offset further into one of the already allocated pages. Oliver -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#114147): https://edk2.groups.io/g/devel/message/114147 Mute This Topic: https://groups.io/mt/103894456/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] AArch64 with HeapGuard: page allocations wrongly aligned
On 1/19/2024 8:34 AM, Rebecca Cran wrote: On 1/18/2024 12:26 PM, Oliver Smith-Denny wrote: Does this solve your issue? I have to run to a meeting, but I can write this in actual patch form (and give it a quick test) later. Unfortunately that didn't work: I still get the assert. ... SmbiosCreate64BitTable() re-allocate SMBIOS 64-bit table ASSERT_EFI_ERROR (Status = Invalid Parameter) ASSERT [SmbiosDxe] /local-data/src/ampereone/edk2/MdePkg/Library/UefiMemoryAllocationLib/MemoryAllocationLib.c(145): !(((INTN)(RETURN_STATUS)(Status)) < 0) Thanks for trying. In lieu of being able to test myself, all I can offer is adding some more prints, when the memory gets allocated, making sure it is 64k aligned then. I'd be curious to see what the address is that is attempting to be freed. My guess (as it was earlier) is that it is going to be aligned to 64k but + 4k. I.e the guard page at the front is throwing it off. There may have just been an error in my attempt to fix the check for that. If however that address is not 64k + 4k aligned, then something else is afoot. Happy to look at some more data if you get it or can engineer an example on an open source system (can you force the system to call this function twice even without the extra SMBIOS entries, etc.). Thanks, Oliver -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#114098): https://edk2.groups.io/g/devel/message/114098 Mute This Topic: https://groups.io/mt/103810212/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] AArch64 with HeapGuard: page allocations wrongly aligned
On 1/18/2024 10:38 AM, Oliver Smith-Denny wrote:
I am suspicious that we are checking for the alignment before we
adjust the memory for the guard. I'm wondering if we actually
should do AdjustMemoryF (recalling function from memory) before
we check the alignment.
Following up on this, this is very suspicious to me. If you change
the CoreInternalFreePages alignment check to something like:
EFI_PHYSICAL_ADDRESS CheckMemory = Memory;
UINTNCheckPages = NumberOfPages;
AdjustMemoryF (, );
if ((CheckMemory & (Alignment - 1)) != 0) {
Status = EFI_INVALID_PARAMETER;
goto Done;
}
Does this solve your issue? I have to run to a meeting, but I can
write this in actual patch form (and give it a quick test) later.
Thanks,
Oliver
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#114008): https://edk2.groups.io/g/devel/message/114008
Mute This Topic: https://groups.io/mt/103810212/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] AArch64 with HeapGuard: page allocations wrongly aligned
On 1/18/2024 10:45 AM, Rebecca Cran via groups.io wrote: No, I mean SbsaQemu from edk2-platforms: https://github.com/tianocore/edk2-platforms/tree/master/Platform/Qemu/SbsaQemu Sure, if you can repro there that is helpful. Oliver -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#114006): https://edk2.groups.io/g/devel/message/114006 Mute This Topic: https://groups.io/mt/103810212/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] AArch64 with HeapGuard: page allocations wrongly aligned
On 1/18/2024 9:42 AM, Rebecca Cran via groups.io wrote: On 1/18/2024 9:48 AM, Oliver Smith-Denny via groups.io wrote: Are you including this commit: https://github.com/tianocore/edk2/commit/00b51e0d78a547dd78119ec44fcc74a01b6f79c8? Can you share some more details on where this is failing? I.e. what assert is getting tripped? Presumably without HeapGuard enabled, you aren't seeing the failure? Are you hitting this case: https://github.com/tianocore/edk2/blob/59f024c76ee57c2bec84794536302fc770cd6ec2/MdeModulePkg/Core/Dxe/Mem/Page.c#L1570-L1573? Does this repro on ArmVirtPkg? Yes, I have that commit in my tree. I'm hitting this assert in FreePages: https://github.com/tianocore/edk2/blob/59f024c76ee57c2bec84794536302fc770cd6ec2/MdeModulePkg/Library/DxeCoreMemoryAllocationLib/MemoryAllocationLib.c#L190 It's called by SmbiosCreate64BitTable: https://github.com/tianocore/edk2/blob/59f024c76ee57c2bec84794536302fc770cd6ec2/MdeModulePkg/Universal/SmbiosDxe/SmbiosDxe.c#L1342 And yes, that's the case I'm hitting. I'm having trouble getting ArmVirtPkg to run. Would it be useful testing using SbsaQemu instead? Yeah, if you can get it running there, that would be a good data point. I assume you mean the Project Mu QemuSbsaPkg? If so that is great, but you will need to update the RUNTIME_PAGE_ALLOCATION_GRANULARITY back to 0x1. It was set to 0x1000 for a historical issue that we are working on reconciling with what edk2 has. If you can get an open source repro, I'm happy to take a look at the failure. To clarify, if you turn off pool guard, does the assert go away? I am suspicious that we are checking for the alignment before we adjust the memory for the guard. I'm wondering if we actually should do AdjustMemoryF (recalling function from memory) before we check the alignment. Thanks, Oliver -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#114003): https://edk2.groups.io/g/devel/message/114003 Mute This Topic: https://groups.io/mt/103810212/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] AArch64 with HeapGuard: page allocations wrongly aligned
On 1/18/2024 7:27 AM, Rebecca Cran via groups.io wrote: I've been debugging an assert failure when using HeapGuard on AArch64. A call to FreePages in SmbiosDxe is failing because the memory is aligned to 0x1000 instead of 0x1 as defined by RUNTIME_PAGE_ALLOCATION_GRANULARITY. I'm enabling HeapGuard by setting the PCDs to the following values: gEfiMdeModulePkgTokenSpaceGuid.PcdHeapGuardPropertyMask|0x0F gEfiMdeModulePkgTokenSpaceGuid.PcdHeapGuardPageType|0xC000 gEfiMdeModulePkgTokenSpaceGuid.PcdHeapGuardPoolType|0xC000 Hi Rebecca, Are you including this commit: https://github.com/tianocore/edk2/commit/00b51e0d78a547dd78119ec44fcc74a01b6f79c8? Can you share some more details on where this is failing? I.e. what assert is getting tripped? Presumably without HeapGuard enabled, you aren't seeing the failure? Are you hitting this case: https://github.com/tianocore/edk2/blob/59f024c76ee57c2bec84794536302fc770cd6ec2/MdeModulePkg/Core/Dxe/Mem/Page.c#L1570-L1573? Does this repro on ArmVirtPkg? Thanks, Oliver -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#114001): https://edk2.groups.io/g/devel/message/114001 Mute This Topic: https://groups.io/mt/103810212/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] [PATCH v1 1/1] StandaloneMmPkg: Initialise serial port early in StandaloneMmEntryPoint
On 1/5/2024 9:22 AM, levi.yun wrote: Hi Ard :) So now we will always initialize the serial port in the entrypoint only because DebugLib might use it later with doing the initialization. That doesn't sound quite correct to me. Could you explain why we cannot rely on DebugLib to call the initializer / constructor at the right time? Because, DebugLib constructor which use serial port is called in StandAloneMmMain function. But, this constrcutor is in _ModuleEntryPoint in StandAloneMmCoreEntry. That means all DEBUG used in _ModuleEntryPoint can use uninitialized serial port. one of typical example is GetSpmVersion function. _ModuleEntryPoint (in StandAloneMmCoreEntry) // Hazard Area start GetSpmVersion DEBUG (DEBUG_INFO, xxx) // It could be use uninitalized Serial port. ... // Hazard Area end ProcessModuleEntryPointList (StandAloneMmMain) ProcessLibraryConstructorList // Here. call DebugLib constructor with SerialPortIntialize When you see above, I would be clear. between Hazard Area Start to Hazard Area End. DEBUG macro would use uninitailized Serial port if that's not initialized by TF-A. So, It should be call SerialPortInitialized at the _ModuleEntryPoint. + Laszlo This sounds very similar to our DxeCore early serial logging discussion a couple months ago :). Laszlo wrote up a good summary here: https://edk2.groups.io/g/devel/topic/101203427#109235. If I am understanding correctly, this would be the "lower left" in Laszlo's diagram. Standalone MM is likely smaller missing window than in DxeCore, but some important information could be lost there (like the SPM version called out, which could be very important for debugging early crashes). So this goes back to should be we have a more generic solution for the cores to use early logging, by fixing the SerialPortLibs? I'll parse this more and reread the old thread further, still paging the info back in. Oliver -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#113314): https://edk2.groups.io/g/devel/message/113314 Mute This Topic: https://groups.io/mt/103540969/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] [PATCH 3/3] StandaloneMmPkg:Remove MpInformation.h
On 1/3/2024 7:11 AM, Ard Biesheuvel wrote: On Thu, 9 Nov 2023 at 03:51, duntan wrote: Remove MpInformation.h in StandaloneMmPkg since it has been moved to UefiCpuPkg Signed-off-by: Dun Tan Cc: Ard Biesheuvel Cc: Sami Mujawar Cc: Ray Ni Doesn't this break the ARM build? Right, this would add a dependency on UefiCpuPkg in ArmPkg (via StandaloneMmPkg, which I don't believe is there today?). Don't we strongly not want that dependency since UefiCpuPkg is x86 specific? Thanks, Oliver -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#113103): https://edk2.groups.io/g/devel/message/113103 Mute This Topic: https://groups.io/mt/102479012/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] [PATCH] ArmVirtPkg: Allow EFI memory attributes protocol to be disabled
My first caveat here is I am writing this email from the depths of a parental leave, so my brain is only half here and I haven't been up to date for the past two months :). I'll return in Jan and hopefully be a bit more sensical. On 12/6/2023 5:23 AM, Ard Biesheuvel wrote: But what we might do is invent a way to avoid setting the XP attribute on the entire region based on some heuristic. Given that the main purpose of the EFI memory attribute protocol is to provide the ability to remove XP (and set RO instead), perhaps we can avoid the set entirely? Just brainstorming here. (cc'ing Taylor and Oliver given that this is related to the memory policy work as well) Perhaps we can use the fact that the active image is non-NX compat to make some tweaks? What I really want to avoid is derail our effort to tighten things down and comply with the NX compat related policies, by adding some build time control that the distros will enable now and never disable again, citing backward compat concerns. And the deafening silence from the shim developers is not an encouragement either. I completely agree here. My thoughts on this specific issue tend to be: - edk2 is reference FW and the mainline branch in particular. It should "do the right thing" not the hacky thing. - The bug here, as we all agree, is in shim. This is not a case where FW made a change for the better that broke something and so needs to fix the change it made, this is a case where FW offered a new option, which shim took advantage of and completely borked. edk2 can't guarantee that every OS will do the right thing and we should have guardrails that give us the best chance of booting, which is after all our top goal. However, we can't prevent an OS (or bootloader) from blowing its toes off. There's a part of me that thinks well, if your OS/bootloader sucks, get a better one...I understand this is complicated by the lack of release of a new shim (as this bug has been fixed upstream in shim for almost a year [1]). These two points being said, I tend to prefer Ard's approach, if I am reading it correctly in a quick skim, where this change is confined to QEMU. The platform FW (ArmVirtPkg in this instance) is the right place for the hacky stuff. The platform in the end is what has the requirement to boot certain versions of an OS/bootloader. edk2's requirement is to have sane and usable interfaces. So, I personally think that if we think edk2 has a sane memory attribute protocol (or at least as sane as we think it can be), then we should not change it. Now, it is totally valid to say edk2 does not have a sane memory attribute protocol and it should have better guardrails. However, I agree with Ard that if we add any generic edk2 level ability to disable the memory attribute protocol, it will never be used. I think the problem with the pattern Ard is describing (don't set XP based on some heuristic) is that in case it won't work. Because the XP comes from shim first and then they ask us to unset XP for an unaligned region. Aligning this call to a larger region seems fraught with peril. To Gerd's point, we could attempt to catch this in the fault handler, set some flag, not set XP in the next boot (different memory policy) and then everything would work (hmm, although in this case, the memory policy doesn't come into play right, because shim explicitly asks us to set XP, so either you'd have to disable the protocol or in the protocol check the memory policy, which feels wrong). If something is worked out here it feels...better than a boot time flag and I suppose the more reasonable way to go here. But, I think the platform is still the place to implement this. This feels much more like a specific workaround than a generic fault flow we are going to catch. If this version of shim had been tested on all the platforms it was going to support, this would have been caught immediately. So I go back to, the platform can work around this by enabling a custom compat memory policy (which I think is the benefit of that framework), but that edk2 shouldn't back bend here and compromise safety for a bad shim. Requiring a platform to fix this is more of a burden, but this is a burden the shim community created by not releasing a new shim and not testing the old one. We can't mitigate that. I think memory protections is an area where having the distros do this the right way is important. Sorry for the rambling and any context I'm missing, I'm typing this with a baby in arm :) Thanks, Oliver 1: https://github.com/rhboot/shim/commit/c7b305152802c8db688605654f75e1195def9fd6 -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#112137): https://edk2.groups.io/g/devel/message/112137 Mute This Topic: https://groups.io/mt/102967690/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] [PATCH v1 1/1] ArmVirtPkg: Enable Early Serial For DxeCore
On 9/7/2023 6:10 AM, Laszlo Ersek wrote: (replying on the webui... sorry!) The problem is actually embedded in MdePkg and MdeModulePkg. - In DxeMain() (and in functions called by DxeMain()), we call DebugLib APIs *before* reaching ProcessLibraryConstructorList(). - In ArmVirtQemu, we resolve the DXE Core's DebugLib dependency to BaseDebugLibSerialPort (from MdePkg). - BaseDebugLibSerialPort has a constructor function (BaseDebugLibSerialPortConstructor()). These already suffice for broken DebugLib behavior. APIs of a library class should not be called because the library instance has a chance to initialize. The rest is circumstantial. Like, BaseDebugLibSerialPortConstructor calls SerialPortInitialize, but our SerialPortInitialize (in FdtPL011SerialPortLib) does nothing. Well, the latter doesn't need to do anything, because FdtPL011SerialPortLib has its own constructor (FdtPL011SerialPortLibInitialize), thus, if constructors were called properly, then BaseDebugLibSerialPort + FdtPL011SerialPortLib would work properly together, regardless of SerialPortInitialize being empty in the latter. Basically the DXE Core has a hidden requirement -- it can only use such DebugLib instances that need no explicit initialization. The proposed patch works around the problem by satisfying that hidden requirement one level lower down: in the SerialPortLib instance. The initialization of BaseDebugLibSerialPort is still busted (its constructor is not called, so it cannot call SerialPortInitialize either), but now it is masked, because EarlyFdtPL011SerialPortLib works withouth *both* SerialPortInitialize and construction. The real fix would be to make the DXE Core requirement explicit, by introducing separate (dedicated) DebugLib and SerialPortLib *classes* (whose APIs are guaranteed to work without initialization). Laszlo Thanks for the comprehensive breakdown! :). I completely agree that fixing this at the upper level (and ideally documenting this requirement) is the better move. I can drop this patch and take a crack at that. I'm in the last few weeks leading up to an extended parental leave, so we'll see if I can squeeze it in prior to then :). Oliver -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#108402): https://edk2.groups.io/g/devel/message/108402 Mute This Topic: https://groups.io/mt/101203427/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] [PATCH v1 2/2] StandaloneMmPkg: Fix HOB space and heap space conflicted issue
On 9/6/2023 1:50 AM, Ard Biesheuvel wrote: On Wed, 6 Sept 2023 at 09:56, Nhi Pham wrote: On 9/6/2023 1:33 PM, Ni, Ray wrote: [EXTERNAL EMAIL NOTICE: This email originated from an external sender. Please be mindful of safe email handling and proprietary information protection practices.] I am a bit confused. The HOB list in standalone MM is read-only. Why could any module call BuildGuidHob() to modify the HOB. I saw Oliver mentioned something about StMM. I don't know what that is. But it seems that's ARM specific. Then, I don't think it's proper to modify code here for a specific arch ARM. The HOB creation is available in the StandaloneMmPkg/Library/StandaloneMmHobLib/StandaloneMmHobLib.inf. If other architectures also use that instance, I think the issue is not specific to ARM. The question here is whether the implementation follows the PI spec, and whether HOB creation should be supported to begin with. My reading of the PI spec is that this implementation does not follow it. However, the PI spec is not very explicit about Standalone MM in general, but particularly in relation to HOBs. However, in the generic HOB section of PI spec v1.7, Vol. 3, section 4 (entitled HOB Design Discussion) it explicitly lays out that there are HOB producer phases and HOB consumer phases. It uses PEI as a HOB producer phase and DXE as a HOB consumer phase and explicitly says that the HOB consumer phase must treat HOBs as read-only memory, per Ray's comment. In vol. 4, section 2.2, in discussing the Standalone MM entry point, the document talks about the HOB list being passed to Standalone MM to consume, which per the reading of the above section would classify Standalone MM as a HOB consumer phase, where HOBs should then be read-only. So, I believe that we should not support HOB creation in Standalone MM and instead rely on other mechanisms to pass information within the phase. Per Nhi's other email in this thread, we should have the discussion on how to solve that specific problem and that may well lead to a discussion on whether HOBs are in fact the right mechanism here, but I tend to lean towards leaving something as architectural as HOBs to what the PI spec defines and using different mechanisms to accomplish in-phase communication. Does this reading of the spec align with others' expectations? As I mentioned to Ray in another thread, Standalone MM feels like it could have extra clarification in a few areas in the PI spec. Thanks, Oliver -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#108333): https://edk2.groups.io/g/devel/message/108333 Mute This Topic: https://groups.io/mt/89020085/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] Regarding StandaloneMmPkg/Include/Guid/MpInformation.h
On 9/5/2023 11:39 PM, Ni, Ray wrote:
All,
StandaloneMmPkg/Include/Guid/MpInformation.h contains the
MP_INFORMATION_HOB_DATA HOB definition.
This HOB is used for transfering MP information collected in non-SMM
phase to standalone MM environment.
#pragmapack(1)
typedefstruct{
UINT64NumberOfProcessors;
UINT64NumberOfEnabledProcessors;
EFI_PROCESSOR_INFORMATIONProcessorInfoBuffer[];
}MP_INFORMATION_HOB_DATA;
#pragmapack()
externEFI_GUIDgMpInformationHobGuid;
Is there any reason that the MP_INFORMATION_HOB_DATA HOB definition not
documented in PI spec?
If no reason, I do feel that PI spec needs to carry the HOB definition.
Thoughts?
That seems reasonable to me. I was reviewing the PI spec recently to get
Standalone MM information, it is pretty sparse in general, in particular
where it relates to HOBs. I think more clarification for Standalone MM
in relation to HOBs would be helpful.
I see you are already on the other thread
(StandaloneMMPkg: Fix HOB Space and heap space conflicted issue), where
we are discussing just such a confusion in the PI spec.
Thanks,
Oliver
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#108332): https://edk2.groups.io/g/devel/message/108332
Mute This Topic: https://groups.io/mt/101187873/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] [PATCH v1 2/2] StandaloneMmPkg: Fix HOB space and heap space conflicted issue
On 9/4/2023 7:20 PM, Nhi Pham wrote: On 9/2/2023 3:43 AM, Oliver Smith-Denny wrote: On 8/31/2023 1:20 AM, Nhi Pham via groups.io wrote: If I am understanding this correctly, this is only an issue when HOBs are created in StMM, i.e. not from HOBs that are passed in. Is this correct? Yes, the issue only occurs when HOB are created in StandaloneMM by the HOB library instance StandaloneMmPkg/Library/StandaloneMmHobLib/StandaloneMmHobLib.inf If so, is HOB creation in StMM and supported use case? The only instance I think it is intended to work as the CreateHob() function is implemented. Well, that may just be a copy/paste sort of thing :). a quick search turns up is the ARM StMM Core entry, where some information from TF-A is converted to HOB format. Do we have any other use cases (and curious more on this use case). My thought process would be that StMM would not create any HOBs. Depending on FW configuration, it may receive HOBs from PEI. I have a use case when enabling the UEFI Variable driver running in StandaloneMM. Instead of using the PCDs, the in-memory NVRAM region is allocated **dynamically** at boot time in the StMM secure memory. Then, they will be passed into the gVariableFlashInfoHobGuid for being consumed by other variable MM drivers. I do believe that per the PI spec, we should have HOB producer and HOB consumer phases, where in this case PEI (if it was the launching entity for StMM) is the HOB producer and StMM is the HOB producer. This is the same pattern the PI spec details for PEI and DXE, where DXE is not intended to create new HOBs, but just to consume information from the previous phase. As I mentioned, there are other interfaces for passing information within a phase, such as protocols, dynamic PCDs, variables, etc. that are built for this application. I think it is useful to adhere to the model for HOBs (which are hand off blocks, one phase handing information to another phase) and that we will create more issues if we rely on HOB consumer phases producing HOBs. My proposal would be to remove the HOB creation code from StMM completely. I believe in your use case that you are describing a dynamic PCD or a protocol could work to pass the information. If we are saying that prior to your patch that HOB creation in StMM was completely broken, anyway, it seems that folks were not relying on this code? Thanks, Oliver -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#108293): https://edk2.groups.io/g/devel/message/108293 Mute This Topic: https://groups.io/mt/89020085/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] [PATCH v1 2/2] StandaloneMmPkg: Fix HOB space and heap space conflicted issue
On 8/31/2023 1:20 AM, Nhi Pham via groups.io wrote: Hi Ard, Thanks for your response on this patch. Please see my reply inline... On 8/30/2023 8:10 PM, Ard Biesheuvel wrote: [EXTERNAL EMAIL NOTICE: This email originated from an external sender. Please be mindful of safe email handling and proprietary information protection practices.] On Wed, 16 Aug 2023 at 10:56, Nhi Pham wrote: Hi Ard and Ming, I have been seeing an issue with StandaloneMM HobLib that can be fixed by this patch as well. The function CreateHob() in the HobLib instance StandaloneMmPkg/Library/StandaloneMmCoreHobLib/StandaloneMmCoreHobLib.inf does not work at all. The HobList is early created by the StandaloneMmCoreEntryPoint then it is relocated on the heap memory by StandaloneMmCore. But the FreeMemoryTop and FreeMemoryBottom are not updated accordingly and the HOB free memory top is overlapped with the heap space. This causes the CreateHob() function to not work as expected. Introducing the PcdMemoryHobSize is reasonable to fix this issue. I tested this patch in my end. Tested-by: Nhi Pham Thanks for reminding me. So if the HOB creation is completely broken, are we sure this is the correct fix? Wouldn't it be better to update FreeMemoryTop and FreeMemoryBottom to the correct values? Per your question, I had a deep dive into the HOB today. I think I need to clarify further the issue to make sure that we are on the same page. 1. When the base of the HOB list (HobStart) is reallocated in the MM heap memory, the Hob->EfiEndOfHobList must be adjusted accordingly compared to the new HOB base. Currently, we are missing that. That causes the CreateHob() function does not work. The GetNextHob() function always look up the old HOB list instead of the new HOB list. 2. The Memory Allocation StandaloneMmPkg/Core/Page.c does not update the Hob->EfiFreeMemoryTop, this may cause the MM heap space and HOB space to be overlapped at some points. It sounds like the issue on the memory allocation in StandaloneMM. For #1, I think we should write a new patch for it. For #2, could you advise? If I am understanding this correctly, this is only an issue when HOBs are created in StMM, i.e. not from HOBs that are passed in. Is this correct? If so, is HOB creation in StMM and supported use case? The only instance a quick search turns up is the ARM StMM Core entry, where some information from TF-A is converted to HOB format. Do we have any other use cases (and curious more on this use case). My thought process would be that StMM would not create any HOBs. Depending on FW configuration, it may receive HOBs from PEI. In PEI/DXE to StMM communication, we should be using the MM_Communicate interface. But in StMM to StMM communication, I would expect we would go through interfaces like protocols, PCDs, or UEFI variables. Any reason for HOBs that I'm not thinking of? I'm wondering if the answer here is to address what seemingly is one (or a very small set) of use cases and remove the HOB creation code in StMM altogether. Thanks, Oliver -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#108232): https://edk2.groups.io/g/devel/message/108232 Mute This Topic: https://groups.io/mt/89020085/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] another PR rejected by CI
Another option, when the main pipeline page says there is an uncrustify error is to take the cmdline it ran with and append UNCRUSTIFY_IN_PLACE=TRUE to it. It will then auto-fixup any uncrustify errors (which obviously should be audited afterwards). I do this to avoid having to wade through numerous errors, especially since I almost always take whatever the auto-format provides. For example, for your failure, if you ran: stuart_ci_build -c .pytool/CISettings.py -p OvmfPkg -t NO-TARGET,NOOPT -a IA32,X64,ARM,AARCH64,RISCV64,LOONGARCH64 TOOL_CHAIN_TAG=GCC5 UNCRUSTIFY_IN_PLACE=TRUE It will autofix the issue. Oliver On 8/28/2023 11:19 AM, Taylor Beebe wrote: Here's a git-patch so you can easily fix it: --- OvmfPkg/IoMmuDxe/IoMmuBuffer.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/OvmfPkg/IoMmuDxe/IoMmuBuffer.c b/OvmfPkg/IoMmuDxe/IoMmuBuffer.c index 2764c35044ac..d66763263784 100644 --- a/OvmfPkg/IoMmuDxe/IoMmuBuffer.c +++ b/OvmfPkg/IoMmuDxe/IoMmuBuffer.c @@ -434,7 +434,7 @@ IoMmuFreeBounceBuffer ( mReservedMemBitmap, mReservedMemBitmap & ((UINT32)(~MapInfo->ReservedMemBitmap)) )); - MapInfo->PlainTextAddress = 0; + MapInfo->PlainTextAddress = 0; ClearReservedMemBit (MapInfo->ReservedMemBitmap); MapInfo->ReservedMemBitmap = 0; } -- On 8/28/2023 11:16 AM, Mike Maslenkin wrote: Hello! https://dev.azure.com/tianocore/edk2-ci/_build/results?buildId=100301=ms.vss-test-web.build-test-results-tab=877528=16=attachments Uncrustify found formatting errors in IoMmuDxe/IoMmuBuffer.c See Standard_Error_Output.log in "attachments" It doesn't like two spaces at assignment MapInfo->PlainTextAddress_ _= 0; Best Regards, Mike On Mon, Aug 28, 2023 at 8:58 PM Ard Biesheuvel wrote: Could someone please explain to me how I can figure out why this PR was rejected by the CI? https://github.com/tianocore/edk2/pull/4763 -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#108064): https://edk2.groups.io/g/devel/message/108064 Mute This Topic: https://groups.io/mt/101015386/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
Re: 回复: edk2-stable202308 Re: [edk2-devel][PATCH v1 1/1] MdeModulePkg: HeapGuard: Don't Assume Pool Head Allocated In First Page
On 8/15/2023 6:40 PM, gaoliming wrote: Oliver: This change reverts the changes done in AdjustPoolHeadA(). It matches the allocation logic. I think this change is good. Reviewed-by: Liming Gao I am also OK to merge this change for the stable tag 202308. Besides, AdjustPoolHeadA() implementation has the extra logic " Size = ALIGN_VALUE (Size, 8);". Seemly, this logic is not required, because Size has aligned by ALIGN_VARIABLE before enter into AdjustPoolHeadA. Thanks Liming Thanks for the review, Liming. Looking at the alignment code, I agree, the ALIGN_VALUE doesn't seem to be needed. Do you want me to send a v2 with that dropped or take the patch as is? Looks like we have the required reviewers and probably no further folks reviewing. Thanks, Oliver -邮件原件- 发件人: devel@edk2.groups.io 代表 Leif Lindholm 发送时间: 2023年8月15日 18:58 收件人: Ard Biesheuvel ; Oliver Smith-Denny ; Liming Gao 抄送: devel@edk2.groups.io; Jian J Wang ; Dandan Bi ; Kinney, Michael D ; 'Andrew Fish' 主题: edk2-stable202308 Re: [edk2-devel][PATCH v1 1/1] MdeModulePkg: HeapGuard: Don't Assume Pool Head Allocated In First Page On 2023-08-09 22:51, Ard Biesheuvel wrote: On Wed, 9 Aug 2023 at 23:35, Oliver Smith-Denny wrote: Currently, HeapGuard, when in the GuardAlignedToTail mode, assumes that the pool head has been allocated in the first page of memory that was allocated. This is not the case for ARM64 platforms when allocating runtime pools, as RUNTIME_PAGE_ALLOCATION_GRANULARITY is 64k, unlike X64, which has RUNTIME_PAGE_ALLOCATION_GRANULARITY as 4k. When a runtime pool is allocated on ARM64, the minimum number of pages allocated is 16, to match the runtime granularity. When a small pool is allocated and GuardAlignedToTail is true, HeapGuard instructs the pool head to be placed as (MemoryAllocated + EFI_PAGES_TO_SIZE(Number of Pages) - SizeRequiredForPool). This gives this scenario: |Head Guard|Large Free Number of Pages|PoolHead|TailGuard| When this pool goes to be freed, HeapGuard instructs the pool code to free from (PoolHead & ~EFI_PAGE_MASK). However, this assumes that the PoolHead is in the first page allocated, which as shown above is not true in this case. For the 4k granularity case (i.e. where the correct number of pages are allocated for this pool), this logic does work. In this failing case, HeapGuard then instructs the pool code to free 16 (or more depending) pages from the page the pool head was allocated on, which as seen above means we overrun the pool and attempt to free memory far past the pool. We end up running into the tail guard and getting an access flag fault. This causes ArmVirtQemu to fail to boot with an access flag fault when GuardAlignedToTail is set to true (and pool guard enabled for runtime memory). It should also cause all ARM64 platforms to fail in this configuration, for exactly the same reason, as this is core code making the assumption. This patch removes HeapGuard's assumption that the pool head is allocated on the first page and instead undoes the same logic that HeapGuard did when allocating the pool head in the first place. With this patch in place, ArmVirtQemu boots with GuardAlignedToTail set to true (and when it is false, also). BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=4521 Github PR: https://github.com/tianocore/edk2/pull/4731 Cc: Leif Lindholm Cc: Ard Biesheuvel Cc: Jian J Wang Cc: Liming Gao Cc: Dandan Bi Signed-off-by: Oliver Smith-Denny Thanks a lot for fixing this - I ran into this a while ago but didn't quite figure out what exactly was going wrong, although the runtime allocation granularity was obviously a factor here. Reviewed-by: Ard Biesheuvel Can we include this in the stable tag please? Bugfix, submitted during soft freeze. I think it can go in. *but* this affects !AARCH64 also - need a reaction from MdeModulePkg maintainers. Acked-by: Leif Lindholm --- MdeModulePkg/Core/Dxe/Mem/HeapGuard.h | 7 ++- MdeModulePkg/Core/Dxe/Mem/HeapGuard.c | 14 +++--- MdeModulePkg/Core/Dxe/Mem/Pool.c | 2 +- 3 files changed, 18 insertions(+), 5 deletions(-) diff --git a/MdeModulePkg/Core/Dxe/Mem/HeapGuard.h b/MdeModulePkg/Core/Dxe/Mem/HeapGuard.h index 9a32b4dd51d5..24b4206c0e02 100644 --- a/MdeModulePkg/Core/Dxe/Mem/HeapGuard.h +++ b/MdeModulePkg/Core/Dxe/Mem/HeapGuard.h @@ -378,12 +378,17 @@ AdjustPoolHeadA ( Get the page base address according to pool head address. @param[in] MemoryHead address of pool to free. + @param[in] NoPages Number of pages actually allocated. + @param[in] Size Size of memory requested. +(plus pool head/tail overhead) @return Address of pool head. **/ VOID * AdjustPoolHeadF ( - IN EFI_PHYSICAL_ADDRESS Memory + IN EFI_PHYSICAL_ADDRESS Memory, + IN UINTN NoPages, + IN UINTN Size ); /** diff --git a/MdeModulePkg/Core/Dxe/Mem/HeapGuard.c b/Mde
Re: [edk2-devel] Runtime Page Granularity on ARM64
Thanks Ard and Andrew, I appreciate the info! That clears things up for me. Oliver On 8/9/2023 3:56 PM, Ard Biesheuvel wrote: Hi, On Wed, 9 Aug 2023 at 23:35, Oliver Smith-Denny wrote: Hi Ard, I just sent out a patch (MdeModulePkg: HeapGuard: Don't Assume Pool Head Allocated In First Page) to fix HeapGuard GuardAlignedToTail behavior on ARM64. However, this raised a question of why ARM64 sets RUNTIME_PAGE_ALLOCATION_GRANULARITY to 64k when X64 does not. You added this in ProcessorBind.h for ARM64, so I am hoping to get some additional context from you (or anyone on the mailing list who has insight). I understand that on ARM64 we can have 64k pages in the OS, but what I do not understand is why we need to map in 64k chunks in UEFI. I see the UEFI spec says that ARM allows for 64K pages and that if runtime code or data is within a 64KB page then all 4k pages within that 64K page need the same memory attributes, which makes sense. Is this runtime granularity just to satisfy that requirement that the memory attributes are the same or is there an additional reason that we need to use the 64k granularity on ARM64? In any case, I am confused why this is not an issue on X64 or if we have 2MB pages in the OS? I'm not as familiar with the mechanisms an OS will use to map runtime services within its space, but they will be virtualized and the OS will have its own page tables, so it doesn't quite follow to me why the OS cares all that much what UEFI has done. Any light you can shed here would be greatly appreciated. It is not about how UEFI maps them at boot time at all, and we happily use 1GB or 2MB mappings for runtime regions if the permission attributes allow it. It is simply about the granularity of regions that the OS needs to care about, i.e., those with the EFI_MEMORY_RUNTIME attribute set in the EFI memory map, as well as memory used for ACPI tables or mapped at runtime by the AML interpreter. There are two important differences with X64: - arm64 supports 16k and 64k pages - arm64 does not tolerate aliases with mismatched attributes If a EFI_MEMORY_RUNTIME region is not aligned to 64k, the OS is generally forced to round outwards if it is running with a page size > 4k.This means that some adjacent physical pages will be covered by [typically] a writeback cacheable mapping even though the memory map may describe them differently. For instance, a EFI reserved region used by the ACPI parking protocol requires non-cacheable mappings, and even if such a mapping is created elsewhere in the OS's virtual address space, if it overlaps with a cacheable mapping of the same physical 64k, the result is unpredictable. (The uncached accesses are likely to hit in the cache inadvertently if a [speculative] access via the cached alias pulled in the data) Whether or not the architecture or OS supports 2 MB pages is not really relevant, given that it will never be forced to use those for regions that are not aligned to 2 MB. A 64k pagesize OS simply has no smaller granule available, and so it has to round outwards (*) The least impactful way to achieve this in EDK2 was to increase the page allocation granularity for runtime memory types (and rework the pool allocator to make better use of memory that is allocated in larger chunks). I imagine there might be other ways to ensure that EFI_MEMORY_RUNTIME regions are aligned sufficiently, e.g., by reasoning about whether or not adjacent regions may require different attributes, and permitting misalignment if they don't. But this will be a lot more hassle, and a lot more room for error. -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#107661): https://edk2.groups.io/g/devel/message/107661 Mute This Topic: https://groups.io/mt/100652665/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
[edk2-devel] Runtime Page Granularity on ARM64
Hi Ard, I just sent out a patch (MdeModulePkg: HeapGuard: Don't Assume Pool Head Allocated In First Page) to fix HeapGuard GuardAlignedToTail behavior on ARM64. However, this raised a question of why ARM64 sets RUNTIME_PAGE_ALLOCATION_GRANULARITY to 64k when X64 does not. You added this in ProcessorBind.h for ARM64, so I am hoping to get some additional context from you (or anyone on the mailing list who has insight). I understand that on ARM64 we can have 64k pages in the OS, but what I do not understand is why we need to map in 64k chunks in UEFI. I see the UEFI spec says that ARM allows for 64K pages and that if runtime code or data is within a 64KB page then all 4k pages within that 64K page need the same memory attributes, which makes sense. Is this runtime granularity just to satisfy that requirement that the memory attributes are the same or is there an additional reason that we need to use the 64k granularity on ARM64? In any case, I am confused why this is not an issue on X64 or if we have 2MB pages in the OS? I'm not as familiar with the mechanisms an OS will use to map runtime services within its space, but they will be virtualized and the OS will have its own page tables, so it doesn't quite follow to me why the OS cares all that much what UEFI has done. Any light you can shed here would be greatly appreciated. Thanks, Oliver -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#107657): https://edk2.groups.io/g/devel/message/107657 Mute This Topic: https://groups.io/mt/100652665/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] ArmVirtPkg: non-executable EFI_LOADER_DATA breaks GRUB on Ubuntu 22.04
On 7/13/2023 10:41 AM, Pedro Falcato wrote:
On Thu, Jul 13, 2023 at 6:20 PM Ard Biesheuvel wrote:
On Thu, 13 Jul 2023 at 18:57, Pedro Falcato wrote:
On Tue, Jul 11, 2023 at 7:58 AM Gerd Hoffmann wrote:
On Mon, Jul 10, 2023 at 04:58:15PM +0100, Pedro Falcato wrote:
On Mon, Jul 10, 2023 at 2:28 PM wrote:
I have an existing install of Ubuntu 22.04 on a QEMU virtual machine which I've decided to update
the UEFI firmware. After doing so, GRUB no longer boots ("Synchronous Exception" message
seen). After a git bisect session, I found the problematic
2997ae38739756ecba9b0de19e86032ebc689ef9. The comment says GRUB should have been fixed in 2017, but
for one reason or another, my VM which was built in 2022 still had the issue. Regardless, I don't
think it's a good idea to break GRUB, even if it's fixed in 2017. In the very least, a better error
message would be preferable to crashing with an "Synchronous Exception." Googling this
error message shows that other people may be hitting this issue as well but the vague error symptom
means its impossible to know if it's the same issue or not.
+CC Some of the folks involved in the original discussion
In the original thread, people discussed some alternative behavior to
just crashing on a NX fault. Is this still an alternative?
The idea is: Improve page fault handler to (a) print a big'n'fat
warning, and (b) loosening up memory permissions for the faulting
page address.
No patch for that emerged (yet?).
Ack. I can work on that.
I'm kind of thinking this should be addressed by distros anyway
How is $CURRENT_YEAR Ubuntu still shipping bad GRUBs? I know the
situation around GRUB and distro patching is complicated but...
Do we have any idea of how many distros/GRUBs are affected by this?
Too many :(
Ugh, even the latest releases?
Personally, I would like to avoid loosening up memory permissions.
Well, you can't have both. You have to pick between strict nx handling
and grub bug compatibility ...
Yes. IMO it should be ok to add a hack around NX handling if there's a
solid plan for dealing with this from the distros' side (and phasing
this out). And I'm assuming upstream GRUB has this fixed.
This whole situation is kind of messy as firmware people add new
restrictions that weren't really there in the first place.
Also, what's the situation on this for x86? I assume it's a lot worse there?
To be honest, I have little sympathy for the gigantic mess that the
distros have created for themselves with GRUB, shim, etc. Mainline
GRUB works fine with mainline EDK2, and secure boot in a arm64 VM is
rather pointless, given that the [emulated] NOR flash is writable by
the guest OS. The breakage is in the downstream GRUB changes that make
it interoperate with shim, and its hacked up PE loader.
If we are going to accommodate every broken GRUB build that the
distros ever released, we won't be able to make any progress on this
front. I understand that the distros need to support their existing
user bases, so I am willing to consider facilities that make it easier
to create builds that work around such issues.
However, just turning off NX support is not one of the options.
Upstream is not what the distros are shipping, this applies to GRUB
and shim as well as EDK2: so if their downstream GRUB breaks EDK2,
they can fix it in their EDK2 builds, either by carrying a code
change, or by enabling an upstream build flag that is off by default.
I understand your sentiment, but I don't see how this can work. Let's
say Fedora has a fixed GRUB (I have no idea if this is the case), so
they have a fully-NX'd edk2. Then someone tries to boot Ubuntu 22.04 -
it crashes because Ubuntu's GRUB is borked; what now? I don't know if
this case is super prevalent in virtualization, but it's definitely a
problem (and it seems to have happened to osy here?).
I agree that turning off NX sucks, but so does not being able to boot
into distros as recent as Ubuntu 22.04. It might be that a single
Sleep(10); + a nice loud error message gets the idea across? maybe
over a stable tag or so, then we remove the hack and add full-NX. What
do you think?
I agree with Ard here. If other software is buggy and outdated, our
general approach should be fix your outdated and buggy software,
especially when there are security and safety implications to bending
backwards for broken code.
A downstream platform may well need to support buggy OSes, etc. for the
lifespan of the device, but upstream edk2 is not the right place to add
hacks for buggy external software. edk2 is our opportunity to do the
right thing and help encourage the community to the right place.
When distros have the maintenance burden of working with downstream
platforms to support their lack of updating grub etc., they may decide
it is worthwhile to actually update grub...
Thanks,
Oliver
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#106923):
Re: [edk2-devel] heap/page guard broken on aarch64
On 7/12/2023 6:26 AM, Gerd Hoffmann wrote: On Wed, Jul 12, 2023 at 02:03:37PM +0200, Ard Biesheuvel wrote: On Wed, 12 Jul 2023 at 10:41, Gerd Hoffmann wrote: PcdHeapGuardPageType=0x7e PcdHeapGuardPoolType=0x7e This looks like the debug 'poison' value is applied to the freed guard page before the EFI_MEMORY_RP permission is removed. I wonder if the 'IsGuarded' logic in CoreFreePoolI is wrong here: this is runtime memory, which is rounded up to 64k granularity on AArch64, and I would not be surprised if that code is buggy. Looks plausible to me. Tried fix AdjustPoolHeadF() to use granularity instead of efi page size, that alone didn't make the firmware boot though. Clearing the two runtime memory type bits (0x7e -> 0x1e) makes the firmware boot. I haven't had a chance to look at this further yet, but this seems related to an issue I saw, where legitimate EFI_MEMORY_RP pages will crash in CoreFreePoolI on release builds, because we don't clear EFI_MEMORY_RP and that function attempts to read the page (on debug builds, we do set the DEBUG_CLEAR_MEMORY bit and so clear EFI_MEMORY_RP before the pool mgmt code tries to read it). Likely, we should be clearing EFI_MEMORY_RP from pool managed memory in all cases. It is an odd use case and I was debugging a different bug that was causing memory to accidentally get marked EFI_MEMORY_RP, but still, it seems to be a latent bug. Thanks, Oliver -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#106886): https://edk2.groups.io/g/devel/message/106886 Mute This Topic: https://groups.io/mt/100096124/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] [edk2-platforms][PATCH V1 00/20] Add the support for ARM Firmware First Framework
Thanks for sending out the patchset! I may be missing it, where is the StMM FF-A partition manifest as part of this? I see where it gets parsed, but not the actual information. My understanding, which may be flawed, is that there should be a separate file defining the StMM manifest. Is that going in TF-A? This seems mostly complete, I agree with Kun that extending support to MmCommunicatePei would seem to be necessary. Thanks, Oliver On 7/11/2023 7:36 AM, Nishant Sharma wrote: V1 : Following patchset add the support of ARM Firmware Framework protocol to MM communication. Following chagnes are made to enable the support 1> Changed the FFA enable flag(PcdFfaEnable) to integer to enable it use in assembly. 2> Add the support to Reserved SP stack space and program in startup code. 3> Added ABI for enabling FFA protocol 4> Added conditional support in Standalone MM to parse DTB and populated configuration info required for FFA. 5> Added FFA support in MmCommunicationDxe module. Patches are pushed to https://github.com/nissha03/edk2/tree/ArmFirmwareFramework ARM Firmware Framework Protocol: https://developer.arm.com/documentation/den0077/latest/ Achin Gupta (19): StandaloneMmPkg: Allocate and initialise SP stack from internal memory StandaloneMmPkg: Include libfdt in the StMM ArmPkg: Add data structures to receive FF-A boot information ArmPkg/ArmFfaSvc: Add helper macros and fids ArmPkg: Add support for FFA_MEM_PERM_GET/SET ABIs StandaloneMmPkg: define new data structure to stage FF-A boot information StandaloneMmPkg: Add backwards compatible support to detect FF-A v1.1 StandaloneMmPkg: parse SP manifest and populate new boot information StandaloneMmPkg: Populate Hoblist for SP init from StMM boot information StandaloneMmPkg: Skip zero sized sections while tweaking page permissions StandaloneMmPkg: Add global check for FF-A abis ArmPkg: Bump the StMM SP FF-A minor version to 1 ArmPkg/MmCommunicationDxe: Introduce FF-A version check ArmPkg/MmCommunicationDxe: Add support for obtaining FF-A partition ID ArmPkg/MmCommunicationDxe: Register FF-A RX/TX buffers ArmPkg/MmCommunicationDxe: Unmap FF-A RX/TX buffers during ExitBootServices ArmPkg/MmCommunicationDxe: Discover the StMM SP ArmPkg/MmCommunicationDxe: Use the FF-A transport for MM requests StandaloneMmPkg: Add support for MM requests as FF-A direct messages Nishant Sharma (1): ArmPkg: Change PcdFfaEnable flag datatype ArmPkg/ArmPkg.dec | 14 +- StandaloneMmPkg/StandaloneMmPkg.dsc | 3 +- ArmPkg/Drivers/MmCommunicationDxe/MmCommunication.inf | 3 + ArmPkg/Library/StandaloneMmMmuLib/ArmMmuStandaloneMmLib.inf | 4 +- StandaloneMmPkg/Library/StandaloneMmCoreEntryPoint/StandaloneMmCoreEntryPoint.inf | 8 +- ArmPkg/Drivers/MmCommunicationDxe/MmCommunicate.h | 7 +- ArmPkg/Include/IndustryStandard/ArmFfaSvc.h | 133 - StandaloneMmPkg/Include/Library/Arm/StandaloneMmCoreEntryPoint.h | 39 +- ArmPkg/Drivers/MmCommunicationDxe/MmCommunication.c | 332 ++-- ArmPkg/Library/StandaloneMmMmuLib/ArmMmuStandaloneMmLib.c | 140 - StandaloneMmPkg/Library/StandaloneMmCoreEntryPoint/Arm/CreateHobList.c | 186 ++- StandaloneMmPkg/Library/StandaloneMmCoreEntryPoint/Arm/SetPermissions.c | 18 +- StandaloneMmPkg/Library/StandaloneMmCoreEntryPoint/Arm/StandaloneMmCoreEntryPoint.c | 561 +--- StandaloneMmPkg/Library/StandaloneMmCoreEntryPoint/Arm/AArch64/ModuleEntryPoint.S | 68 +++ 14 files changed, 1367 insertions(+), 149 deletions(-) create mode 100644 StandaloneMmPkg/Library/StandaloneMmCoreEntryPoint/Arm/AArch64/ModuleEntryPoint.S -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#106885): https://edk2.groups.io/g/devel/message/106885 Mute This Topic: https://groups.io/mt/100079869/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] [edk2-platforms][PATCH V1 18/20] ArmPkg/MmCommunicationDxe: Discover the StMM SP
On 7/11/2023 7:36 AM, Nishant Sharma wrote:
From: Achin Gupta
This patch adds support for discovering the presence of the SP using the
EFI_MM_COMMUNICATION_PROTOCOL GUID that implements Standalone MM
drivers. This is done by querying the framework through
FFA_PARTITION_INFO_GET whether any partition that implements the
EFI_MM_COMMUNICATION_PROTOCOL is present or not. The partition ID and
its properties are stashed for use in subsequent communication with the
StMM SP.
Signed-off-by: Achin Gupta
Signed-off-by: Nishant Sharma
---
ArmPkg/Include/IndustryStandard/ArmFfaSvc.h | 24 +
ArmPkg/Drivers/MmCommunicationDxe/MmCommunication.c | 93 +++-
2 files changed, 114 insertions(+), 3 deletions(-)
diff --git a/ArmPkg/Include/IndustryStandard/ArmFfaSvc.h
b/ArmPkg/Include/IndustryStandard/ArmFfaSvc.h
index f78442a465e1..530af8bd3c2e 100644
--- a/ArmPkg/Include/IndustryStandard/ArmFfaSvc.h
+++ b/ArmPkg/Include/IndustryStandard/ArmFfaSvc.h
@@ -19,6 +19,9 @@
#define ARM_SVC_ID_FFA_VERSION_AARCH32 0x8463
#define ARM_SVC_ID_FFA_RXTX_MAP_AARCH32 0x8466
#define ARM_SVC_ID_FFA_RXTX_MAP_AARCH64 0xC466
+#define ARM_SVC_ID_FFA_RX_RELEASE_AARCH320x8465
+#define ARM_SVC_ID_FFA_RXTX_UNMAP_AARCH320x8467
+#define ARM_SVC_ID_FFA_PARTITION_INFO_GET_AARCH320x8468
#define ARM_SVC_ID_FFA_ID_GET_AARCH320x8469
#define ARM_SVC_ID_FFA_MSG_SEND_DIRECT_REQ_AARCH32 0x846F
#define ARM_SVC_ID_FFA_MSG_SEND_DIRECT_RESP_AARCH32 0x8470
@@ -154,4 +157,25 @@ typedef struct {
UINT64 Reserved;
} EFI_FFA_BOOT_INFO_HEADER;
+// FF-A partition information descriptor
+typedef struct {
+ UINT16 PartId;
+ UINT16 EcCnt;
+ UINT32 PartProps;
+ UINT32 PartGuid[4];
+} EFI_FFA_PART_INFO_DESC;
+
+#define PART_INFO_PROP_MASK0x3f
+#define PART_INFO_PROP_SHIFT 0
+#define PART_INFO_PROP_DIR_MSG_RECV_BIT(1u << 0)
+#define PART_INFO_PROP_DIR_MSG_SEND_BIT(1u << 1)
+#define PART_INFO_PROP_INDIR_MSG_BIT (1u << 2)
+#define PART_INFO_PROP_NOTIFICATIONS_BIT (1u << 3)
+#define PART_INFO_PROP_EP_TYPE_MASK0x3
+#define PART_INFO_PROP_EP_TYPE_SHIFT 4
+#define PART_INFO_PROP_EP_PE 0
+#define PART_INFO_PROP_EP_SEPID_IND1
+#define PART_INFO_PROP_EP_SEPID_DEP2
+#define PART_INFO_PROP_EP_AUX 3
+
#endif // ARM_FFA_SVC_H_
diff --git a/ArmPkg/Drivers/MmCommunicationDxe/MmCommunication.c
b/ArmPkg/Drivers/MmCommunicationDxe/MmCommunication.c
index 39a1b329b9ea..94a5d96c051d 100644
--- a/ArmPkg/Drivers/MmCommunicationDxe/MmCommunication.c
+++ b/ArmPkg/Drivers/MmCommunicationDxe/MmCommunication.c
@@ -8,6 +8,7 @@
#include
#include
+#include
#include
#include
#include
@@ -28,6 +29,11 @@
//
STATIC UINT16 mFfaPartId;
+// Partition information of the StMM SP if FF-A support is enabled
+// TODO: Revisit assumption that there is only a single StMM SP
+//
+STATIC EFI_FFA_PART_INFO_DESC mStmmPartInfo;
+
//
// RX/TX pair if FF-A support is enabled
//
@@ -298,7 +304,9 @@ GetMmCompatibility (
{
EFI_STATUSStatus;
UINT32MmVersion;
+ UINT32SmccUuid[4];
ARM_SMC_ARGS SmcArgs = {0};
+ EFI_GUID MmCommProtGuid = EFI_MM_COMMUNICATION_PROTOCOL_GUID;
if (FixedPcdGet32 (PcdFfaEnable) != 0) {
SmcArgs.Arg0 = ARM_SVC_ID_FFA_VERSION_AARCH32;
@@ -335,14 +343,21 @@ GetMmCompatibility (
Status = EFI_UNSUPPORTED;
}
- // If FF-A is supported then discover our ID and register our RX/TX buffers.
+ // If FF-A is supported then discover the StMM SP's presence, ID, our ID and
+ // register our RX/TX buffers.
if (FixedPcdGet32 (PcdFfaEnable) != 0) {
+EFI_FFA_PART_INFO_DESC *StmmPartInfo;
+
// Get our ID
ZeroMem(, sizeof(SmcArgs));
SmcArgs.Arg0 = ARM_SVC_ID_FFA_ID_GET_AARCH32;
ArmCallSmc ();
if (SmcArgs.Arg0 == ARM_SVC_ID_FFA_ERROR_AARCH32) {
- DEBUG ((DEBUG_ERROR, "Unable to retrieve FF-A partition ID (%d).\n",
SmcArgs.Arg2));
+ DEBUG ((
+DEBUG_ERROR,
+"Unable to retrieve FF-A partition ID (%d).\n",
+SmcArgs.Arg2
+));
return EFI_UNSUPPORTED;
}
DEBUG ((DEBUG_INFO, "FF-A partition ID = 0x%lx.\n", SmcArgs.Arg2));
@@ -355,11 +370,83 @@ GetMmCompatibility (
SmcArgs.Arg3 = EFI_PAGE_SIZE / SIZE_4KB;
ArmCallSmc ();
if (SmcArgs.Arg0 == ARM_SVC_ID_FFA_ERROR_AARCH32) {
- DEBUG ((DEBUG_ERROR, "Unable to register FF-A RX/TX buffers (%d).\n",
SmcArgs.Arg2));
+ DEBUG ((
+DEBUG_ERROR,
+"Unable to register FF-A RX/TX buffers (%d).\n",
+SmcArgs.Arg2
+));
return EFI_UNSUPPORTED;
}
+// Discover the StMM SP after converting the EFI_GUID to a format TF-A will
+// understand.
+SmcArgs.Arg0 =
Re: [edk2-devel] [edk2-platforms][PATCH V1 17/20] ArmPkg/MmCommunicationDxe: Unmap FF-A RX/TX buffers during ExitBootServices
On 7/11/2023 7:36 AM, Nishant Sharma wrote:
From: Achin Gupta
An FF-A partition can map only a single RX/TX buffer pair with the
framework. The DXE MM communication driver maps its pair before
ExitBootServices is called. The OS cannot re-use this pair once it boots
subsequently and loads its own FF-A driver. This patch ensures that the
DXE MM communication driver unmaps its buffer pair when ExitBootServices
is called so that the OS can register its own pair if required.
Signed-off-by: Achin Gupta
Signed-off-by: Nishant Sharma
---
ArmPkg/Include/IndustryStandard/ArmFfaSvc.h | 10
ArmPkg/Drivers/MmCommunicationDxe/MmCommunication.c | 49
2 files changed, 59 insertions(+)
diff --git a/ArmPkg/Include/IndustryStandard/ArmFfaSvc.h
b/ArmPkg/Include/IndustryStandard/ArmFfaSvc.h
index ebdf29e8d69a..f78442a465e1 100644
--- a/ArmPkg/Include/IndustryStandard/ArmFfaSvc.h
+++ b/ArmPkg/Include/IndustryStandard/ArmFfaSvc.h
@@ -122,6 +122,16 @@
(((content) & FFA_BOOT_INFO_FLAG_CONTENT_MASK) \
<< FFA_BOOT_INFO_FLAG_CONTENT_SHIFT)
+/* Fromat SP ID info. */
+#define FFA_PARTITION_ID_SHIFT 16
+#define FFA_PARTITION_ID_WIDTH 16
+#define FFA_PARTITION_ID_MASK \
+ (((1U << FFA_PARTITION_ID_WIDTH) - 1) \
+ << FFA_PARTITION_ID_SHIFT)
+#define FFA_PARTITION_ID(partid)\
+ ((partid << FFA_PARTITION_ID_SHIFT) & \
+ FFA_PARTITION_ID_MASK)
+
// Descriptor to pass boot information as per the FF-A v1.1 spec.
typedef struct {
UINT32 Name[4];
diff --git a/ArmPkg/Drivers/MmCommunicationDxe/MmCommunication.c
b/ArmPkg/Drivers/MmCommunicationDxe/MmCommunication.c
index 8a4d46e4f80a..39a1b329b9ea 100644
--- a/ArmPkg/Drivers/MmCommunicationDxe/MmCommunication.c
+++ b/ArmPkg/Drivers/MmCommunicationDxe/MmCommunication.c
@@ -43,6 +43,9 @@ STATIC ARM_MEMORY_REGION_DESCRIPTOR mNsCommBuffMemRegion;
// Notification event when virtual address map is set.
STATIC EFI_EVENT mSetVirtualAddressMapEvent;
+// Notification event when exit boot services is called.
+STATIC EFI_EVENT mExitBootServicesEvent;
+
//
// Handle to install the MM Communication Protocol
//
@@ -255,6 +258,39 @@ NotifySetVirtualAddressMap (
}
}
+/**
+ Notification callback on ExitBootServices event.
+
+ This function notifies the MM communication protocol interface on
+ ExitBootServices event and releases the FF-A RX/TX buffer.
+
+ @param Event ExitBootServices event.
+ @param ContextA context when the ExitBootServices triggered.
+
+ @retval EFI_SUCCESSThe function executed successfully.
+ @retval Other Some error occurred when executing this function.
+
+**/
+STATIC
+VOID
+EFIAPI
+NotifyExitBootServices (
+ IN EFI_EVENT Event,
+ IN VOID *Context
+ )
+{
+ ARM_SMC_ARGS SmcArgs = {0};
+
+ SmcArgs.Arg0 = ARM_SVC_ID_FFA_RXTX_UNMAP_AARCH32;
If I am reading this correctly, this SVC macro does not
get added until patch 18. We need it here, otherwise
bisecting to this commit will have a build break, right?
Thanks,
Oliver
+ SmcArgs.Arg1 = FFA_PARTITION_ID(mFfaPartId);
+ ArmCallSmc ();
+
+ // We do not bother checking the error code of the RXTX_UNMAP invocation
+ // since we did map the buffers and this call must succeed.
+ return;
+
+}
+
STATIC
EFI_STATUS
GetMmCompatibility (
@@ -452,6 +488,19 @@ MmCommunication2Initialize (
goto CleanAddedMemorySpace;
}
+ // Register notification callback when ExitBootservices is called to
+ // unregister the FF-A RX/TX buffer pair. This allows the OS to register its
+ // own buffer pair.
+ if (FixedPcdGet32 (PcdFfaEnable) != 0) {
+Status = gBS->CreateEvent (
+EVT_SIGNAL_EXIT_BOOT_SERVICES,
+TPL_NOTIFY,
+NotifyExitBootServices,
+NULL,
+
+);
+ASSERT_EFI_ERROR (Status);
+ }
// Register notification callback when virtual address is associated
// with the physical address.
// Create a Set Virtual Address Map event.
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#106883): https://edk2.groups.io/g/devel/message/106883
Mute This Topic: https://groups.io/mt/100079891/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] [edk2-platforms][PATCH V1 10/20] StandaloneMmPkg: Populate Hoblist for SP init from StMM boot information
On 7/11/2023 7:36 AM, Nishant Sharma wrote:
From: Achin Gupta
This patch adds support for creating a hoblist from the reduced boot
information retrieved from the SP manifest.
Signed-off-by: Achin Gupta
Signed-off-by: Nishant Sharma
---
StandaloneMmPkg/Include/Library/Arm/StandaloneMmCoreEntryPoint.h
| 16 ++
StandaloneMmPkg/Library/StandaloneMmCoreEntryPoint/Arm/CreateHobList.c
| 186 +++-
StandaloneMmPkg/Library/StandaloneMmCoreEntryPoint/Arm/StandaloneMmCoreEntryPoint.c
| 6 +-
3 files changed, 206 insertions(+), 2 deletions(-)
diff --git a/StandaloneMmPkg/Include/Library/Arm/StandaloneMmCoreEntryPoint.h
b/StandaloneMmPkg/Include/Library/Arm/StandaloneMmCoreEntryPoint.h
index 90d67a2f25b5..9daa76324221 100644
--- a/StandaloneMmPkg/Include/Library/Arm/StandaloneMmCoreEntryPoint.h
+++ b/StandaloneMmPkg/Include/Library/Arm/StandaloneMmCoreEntryPoint.h
@@ -170,6 +170,22 @@ CreateHobListFromBootInfo (
IN EFI_SECURE_PARTITION_BOOT_INFO *PayloadBootInfo
);
+/**
+ Use the boot information passed by the SPMC to populate a HOB list
+ suitable for consumption by the MM Core and drivers.
+
+ @param [in, out] CpuDriverEntryPoint Address of MM CPU driver entrypoint
+ @param [in] StmmBootInfo Boot information passed by privileged
+ firmware
+
+**/
+VOID *
+EFIAPI
+CreateHobListFromStmmBootInfo (
+ IN OUT PI_MM_ARM_TF_CPU_DRIVER_ENTRYPOINT *CpuDriverEntryPoint,
+ IN EFI_STMM_BOOT_INFO *StmmBootInfo
+ );
+
/**
The entry point of Standalone MM Foundation.
diff --git a/StandaloneMmPkg/Library/StandaloneMmCoreEntryPoint/Arm/CreateHobList.c b/StandaloneMmPkg/Library/StandaloneMmCoreEntryPoint/Arm/CreateHobList.c
index 2ac2d354f06a..4592089a6020 100644
--- a/StandaloneMmPkg/Library/StandaloneMmCoreEntryPoint/Arm/CreateHobList.c
+++ b/StandaloneMmPkg/Library/StandaloneMmCoreEntryPoint/Arm/CreateHobList.c
@@ -2,7 +2,7 @@
Creates HOB during Standalone MM Foundation entry point
on ARM platforms.
-Copyright (c) 2017 - 2021, Arm Ltd. All rights reserved.
+Copyright (c) 2017 - 2023, Arm Ltd. All rights reserved.
SPDX-License-Identifier: BSD-2-Clause-Patent
**/
@@ -203,3 +203,187 @@ CreateHobListFromBootInfo (
return HobStart;
}
+
+STATIC
+VOID
+CreateMmramInformationHobFromImageLayout (
+ IN EFI_STMM_BOOT_INFO *StmmBootInfo,
+ IN EFI_HOB_HANDOFF_INFO_TABLE *HobStart
+)
+{
+ UINT32 *Idx;
+ UINT32 BufferSize;
+ EFI_MMRAM_HOB_DESCRIPTOR_BLOCK *MmramRangesHob;
+ EFI_MMRAM_DESCRIPTOR*MmramRanges;
+
+ // Find the size of the GUIDed HOB with SRAM ranges. This excludes any memory
+ // shared with the normal world or the SPMC. It includes the memory allocated
+ // to the SP image, used and unused heap.
+ BufferSize = sizeof (EFI_MMRAM_HOB_DESCRIPTOR_BLOCK);
+ BufferSize += 4 * sizeof (EFI_MMRAM_DESCRIPTOR);
+
+ // Create a GUIDed HOB with SRAM ranges
+ MmramRangesHob = BuildGuidHob (, BufferSize);
+
BuildGuidHob can return NULL, we should check MmramRangesHob before
using it. Same for the other cases below.
+ // Initialise the number of MMRAM memory regions
+ MmramRangesHob->NumberOfMmReservedRegions = 0;
+ Idx = >NumberOfMmReservedRegions ;
+
+ // Fill up the MMRAM ranges
+ MmramRanges = >Descriptor[0];
+
+ // Base and size of memory occupied by the Standalone MM image
+ MmramRanges[*Idx].PhysicalStart = StmmBootInfo->SpMemBase;
+ MmramRanges[*Idx].CpuStart = StmmBootInfo->SpMemBase;
+ MmramRanges[*Idx].PhysicalSize = StmmBootInfo->SpMemSize;
+ MmramRanges[*Idx].RegionState = EFI_CACHEABLE | EFI_ALLOCATED;
+ (*Idx)++;
+
+ // Base and size of memory occupied by the Standalone MM image
+ MmramRanges[*Idx].PhysicalStart = StmmBootInfo->SpSharedBufBase;
+ MmramRanges[*Idx].CpuStart = StmmBootInfo->SpSharedBufBase;
+ MmramRanges[*Idx].PhysicalSize = StmmBootInfo->SpSharedBufSize;
+ MmramRanges[*Idx].RegionState = EFI_CACHEABLE | EFI_ALLOCATED;
+ (*Idx)++;
+
+ // Base and size of memory occupied by the hoblist
+ MmramRanges[*Idx].PhysicalStart = (EFI_PHYSICAL_ADDRESS) (UINTN) HobStart;
+ MmramRanges[*Idx].CpuStart = (EFI_PHYSICAL_ADDRESS) (UINTN) HobStart;
+ MmramRanges[*Idx].PhysicalSize = HobStart->EfiFreeMemoryBottom -
(EFI_PHYSICAL_ADDRESS) (UINTN) HobStart;
+ MmramRanges[*Idx].RegionState = EFI_CACHEABLE | EFI_ALLOCATED;
+ (*Idx)++;
+
+ // Base and size of heap memory shared by all cpus
+ MmramRanges[*Idx].PhysicalStart = HobStart->EfiFreeMemoryBottom;
+ MmramRanges[*Idx].CpuStart = HobStart->EfiFreeMemoryBottom;
+ MmramRanges[*Idx].PhysicalSize = HobStart->EfiFreeMemoryTop -
HobStart->EfiFreeMemoryBottom;
+ MmramRanges[*Idx].RegionState = EFI_CACHEABLE;
+ (*Idx)++;
+
+ // Sanity check number of MMRAM regions
+ ASSERT (MmramRangesHob->NumberOfMmReservedRegions ==
Re: [edk2-devel] [edk2-platforms][PATCH V1 08/20] StandaloneMmPkg: Add backwards compatible support to detect FF-A v1.1
On 7/11/2023 7:36 AM, Nishant Sharma wrote:
From: Achin Gupta
For better or worse, an StMM SP can communicate with the SPM through one
of these interfaces.
1. SPM_MM interface
2. FF-A v1.0 interface
3. FF-A v1.1 interface
2) implements only minimal FF-A support. It reuses the initialisation ABI
defined by 1) and wraps the remaining communicaton in
FFA_MSG_SEND_DIRECT_REQ/RESP ABIs.
3) uses FF-A ABIs from the spec for both initialisation and communication.
Detecting these variations in the GetSpmVersion() function is tedious. This
patch restores the original function that discovered configuration
1). It defines a new function to discover presence of FF-A and
differentiate between v1.0 and v1.1.
Signed-off-by: Achin Gupta
Signed-off-by: Nishant Sharma
---
StandaloneMmPkg/Library/StandaloneMmCoreEntryPoint/Arm/StandaloneMmCoreEntryPoint.c
| 132 +---
1 file changed, 87 insertions(+), 45 deletions(-)
diff --git
a/StandaloneMmPkg/Library/StandaloneMmCoreEntryPoint/Arm/StandaloneMmCoreEntryPoint.c
b/StandaloneMmPkg/Library/StandaloneMmCoreEntryPoint/Arm/StandaloneMmCoreEntryPoint.c
index 682b55b5478a..9f6af55c86c4 100644
---
a/StandaloneMmPkg/Library/StandaloneMmCoreEntryPoint/Arm/StandaloneMmCoreEntryPoint.c
+++
b/StandaloneMmPkg/Library/StandaloneMmCoreEntryPoint/Arm/StandaloneMmCoreEntryPoint.c
@@ -32,13 +32,9 @@ SPDX-License-Identifier: BSD-2-Clause-Patent
#define SPM_MAJOR_VER_MASK 0x
#define SPM_MINOR_VER_MASK 0x
#define SPM_MAJOR_VER_SHIFT 16
-#define FFA_NOT_SUPPORTED-1
-STATIC CONST UINT32 mSpmMajorVer = SPM_MAJOR_VERSION;
-STATIC CONST UINT32 mSpmMinorVer = SPM_MINOR_VERSION;
-
-STATIC CONST UINT32 mSpmMajorVerFfa = SPM_MAJOR_VERSION_FFA;
-STATIC CONST UINT32 mSpmMinorVerFfa = SPM_MINOR_VERSION_FFA;
+#define SPM_MAJOR_VER 0
+#define SPM_MINOR_VER 1
#define BOOT_PAYLOAD_VERSION 1
@@ -110,6 +106,70 @@ GetAndPrintBootinformation (
}
return PayloadBootInfo;
+
+/**
+ An StMM SP implements partial support for FF-A v1.0. The FF-A ABIs are used
to
+ get and set permissions of memory pages in collaboration with the SPMC and
+ signalling completion of initialisation. The original Arm MM communication
+ interface is used for communication with the Normal world. A TF-A specific
+ interface is used for initialising the SP.
+
+ With FF-A v1.1, the StMM SP uses only FF-A ABIs for initialisation and
+ communication. This is subject to support for FF-A v1.1 in the SPMC. If this
+ is not the case, the StMM implementation reverts to the FF-A v1.0
+ behaviour. Any of this is applicable only if the feature flag PcdFfaEnable is
+ TRUE.
This should now be if the FixedAtBuild PcdFfaEnable is 1.
Thanks,
Oliver
+
+ This function helps the caller determine whether FF-A v1.1 or v1.0 are
+ available and if only FF-A ABIs can be used at runtime.
+**/
+STATIC
+EFI_STATUS
+CheckFfaCompatibility (BOOLEAN *UseOnlyFfaAbis)
+{
+ UINT16 SpmcMajorVer;
+ UINT16 SpmcMinorVer;
+ UINT32 SpmcVersion;
+ ARM_SVC_ARGS SpmcVersionArgs = {0};
+
+ // Sanity check in case of a spurious call.
+ if (FixedPcdGet32 (PcdFfaEnable) == 0) {
+return EFI_UNSUPPORTED;
+ }
+
+ // Send the SPMC our version to see whether it supports the same or not.
+ SpmcVersionArgs.Arg0 = ARM_SVC_ID_FFA_VERSION_AARCH32;
+ SpmcVersionArgs.Arg1 = FFA_VERSION_COMPILED;
+
+ ArmCallSvc ();
+ SpmcVersion = SpmcVersionArgs.Arg0;
+
+ // If the SPMC barfs then bail.
+ if (SpmcVersion == ARM_FFA_SPM_RET_NOT_SUPPORTED) {
+return EFI_UNSUPPORTED;
+ }
+
+ // Extract the SPMC version
+ SpmcMajorVer = (SpmcVersion >> FFA_VERSION_MAJOR_SHIFT) &
FFA_VERSION_MAJOR_MASK;
+ SpmcMinorVer = (SpmcVersion >> FFA_VERSION_MINOR_SHIFT) &
FFA_VERSION_MINOR_MASK;
+
+ // If the major versions differ then all bets are off.
+ if (SpmcMajorVer != SPM_MAJOR_VERSION_FFA) {
+return EFI_UNSUPPORTED;
+ }
+
+ // We advertised v1.1 as our version. If the SPMC supports it, it must return
+ // the same or a compatible version. If it does not then FF-A ABIs cannot be
+ // used for all communication.
+ if (SpmcMinorVer >= SPM_MINOR_VERSION_FFA) {
+*UseOnlyFfaAbis = TRUE;
+ } else {
+*UseOnlyFfaAbis = FALSE;
+ }
+
+ // We have validated that there is a compatible FF-A
+ // implementation. So. return success.
+ return EFI_SUCCESS;
}
/**
@@ -219,34 +279,19 @@ GetSpmVersion (
)
{
EFI_STATUSStatus;
- UINT16CalleeSpmMajorVer;
- UINT16CallerSpmMajorVer;
- UINT16CalleeSpmMinorVer;
- UINT16CallerSpmMinorVer;
+ UINT16SpmMajorVersion;
+ UINT16SpmMinorVersion;
UINT32SpmVersion;
ARM_SVC_ARGS SpmVersionArgs;
- if (FixedPcdGet32 (PcdFfaEnable) != 0) {
-SpmVersionArgs.Arg0 = ARM_SVC_ID_FFA_VERSION_AARCH32;
-SpmVersionArgs.Arg1 = mSpmMajorVerFfa << SPM_MAJOR_VER_SHIFT;
-SpmVersionArgs.Arg1 |= mSpmMinorVerFfa;
-CallerSpmMajorVer=
Re: [edk2-devel] [edk2-platforms][PATCH V1 06/20] ArmPkg: Add support for FFA_MEM_PERM_GET/SET ABIs
On 7/11/2023 7:36 AM, Nishant Sharma wrote:
From: Achin Gupta
This patch uses the FFA_MEM_PERM_GET/SET ABIs to tweak the permissions of a
set of pages if FF-A v1.1 and above is supported by the SPMC. For FF-A v1.0
the previous method through FFA_MSG_SEND_DIRECT_REQ/RESP is used.
Signed-off-by: Achin Gupta
Signed-off-by: Nishant Sharma
---
ArmPkg/Include/IndustryStandard/ArmFfaSvc.h | 2 +
ArmPkg/Library/StandaloneMmMmuLib/ArmMmuStandaloneMmLib.c | 132
+---
2 files changed, 120 insertions(+), 14 deletions(-)
diff --git a/ArmPkg/Include/IndustryStandard/ArmFfaSvc.h
b/ArmPkg/Include/IndustryStandard/ArmFfaSvc.h
index c80d783fad3f..7987678c996e 100644
--- a/ArmPkg/Include/IndustryStandard/ArmFfaSvc.h
+++ b/ArmPkg/Include/IndustryStandard/ArmFfaSvc.h
@@ -23,6 +23,8 @@
#define ARM_SVC_ID_FFA_MSG_SEND_DIRECT_RESP_AARCH64 0xC470
#define ARM_SVC_ID_FFA_SUCCESS_AARCH32 0x8461
#define ARM_SVC_ID_FFA_SUCCESS_AARCH64 0xC461
+#define ARM_SVC_ID_FFA_MEM_PERM_SET_AARCH32 0x8489
+#define ARM_SVC_ID_FFA_MEM_PERM_GET_AARCH32 0x8488
#define ARM_SVC_ID_FFA_ERROR_AARCH32 0x8460
#define ARM_SVC_ID_FFA_ERROR_AARCH64 0xC460
#define ARM_SVC_ID_FFA_MSG_WAIT_AARCH32 0x846B
diff --git a/ArmPkg/Library/StandaloneMmMmuLib/ArmMmuStandaloneMmLib.c
b/ArmPkg/Library/StandaloneMmMmuLib/ArmMmuStandaloneMmLib.c
index 1a41a289ef17..76ef214bcb85 100644
--- a/ArmPkg/Library/StandaloneMmMmuLib/ArmMmuStandaloneMmLib.c
+++ b/ArmPkg/Library/StandaloneMmMmuLib/ArmMmuStandaloneMmLib.c
@@ -25,6 +25,66 @@
#include
#include
+
+/**
+ Utility function to determine whether ABIs in FF-A v1.1 to set and get
+ memory permissions can be used. Ideally, this should be invoked once in the
+ library constructor and set a flag that can be used at runtime. However, the
+ StMM Core invokes this library before constructors are called and before the
+ StMM image itself is relocated.
+
+ @retval EFI_SUCCESS The availability of ABIs was correctly determined.
+ @retval Other value Software stack is misconfigured.
+
+**/
+STATIC
+BOOLEAN
+UseFfaMemPermAbis (
+ VOID
+ )
+{
+ ARM_SVC_ARGSSvcArgs;
+ UINT32 SpmcFfaVersion;
+ STATIC UINT16 SpmcMajorVer = 0;
+ STATIC UINT16 SpmcMinorVer = 0;
+
+ // Use prefetched version info. if either is not 0, then the version is
+ // already fetched.
+ if ((SpmcMajorVer | SpmcMinorVer) != 0) {
+return (SpmcMajorVer == SPM_MAJOR_VERSION_FFA) && (SpmcMinorVer >=
SPM_MINOR_VERSION_FFA);
+ }
+
+ // Nothing to do if FF-A has not be enabled
nit: "been enabled"
+ if (FixedPcdGet32 (PcdFfaEnable) == 0) {
+return FALSE;
+ }
+
+ // Prepare the message parameters.
+ ZeroMem (, sizeof (ARM_SVC_ARGS));
+ SvcArgs.Arg0 = ARM_SVC_ID_FFA_VERSION_AARCH32;
+ SvcArgs.Arg1 = FFA_VERSION_COMPILED;
+
+ // Invoke the ABI
+ ArmCallSvc ();
+
+ // Check if FF-A is supported and what version
+ SpmcFfaVersion = SvcArgs.Arg0;
+
+ // Damn! FF-A is not supported at all even though we specified v1.0 as our
+ // version. However, the feature flag has been turned on. This is a > + // misconfigured software stack. So, return an error and assert in
a debug build.
+ if (SpmcFfaVersion == ARM_FFA_SPM_RET_NOT_SUPPORTED) {
+ASSERT (0);
It would be nice to either have the assert be self documenting
(ASSERT (SpmcFfaVersion == ARM_FFA_SPM_RET_NOT_SUPPORTED)) or
to add a print here.
+return FALSE;
+ }
+
+ SpmcMajorVer = (SpmcFfaVersion >> FFA_VERSION_MAJOR_SHIFT) &
FFA_VERSION_MAJOR_MASK;
+ SpmcMinorVer = (SpmcFfaVersion >> FFA_VERSION_MINOR_SHIFT) &
FFA_VERSION_MINOR_MASK;
+
+ return (SpmcMajorVer == SPM_MAJOR_VERSION_FFA) && (SpmcMinorVer >=
SPM_MINOR_VERSION_FFA);
+}
+
+
/** Send memory permission request to target.
@param [in, out] SvcArgs Pointer to SVC arguments to send. On
@@ -55,6 +115,36 @@ SendMemoryPermissionRequest (
ArmCallSvc (SvcArgs);
if (FixedPcdGet32 (PcdFfaEnable) != 0) {
+
+// Check if FF-A memory permission ABIs were used.
+if (UseFfaMemPermAbis()) {
+ switch (SvcArgs->Arg0) {
+
+case ARM_SVC_ID_FFA_ERROR_AARCH32:
+case ARM_SVC_ID_FFA_ERROR_AARCH64:
+ switch (SvcArgs->Arg2) {
+ case ARM_FFA_SPM_RET_INVALID_PARAMETERS:
+return EFI_INVALID_PARAMETER;
+ case ARM_FFA_SPM_RET_NOT_SUPPORTED:
+return EFI_UNSUPPORTED;
+ default:
+// Undefined error code received.
+ASSERT (0);
For these two default cases, can we print out what
error code we received?
Thanks,
Oliver
+return EFI_INVALID_PARAMETER;
+ }
+
+case ARM_SVC_ID_FFA_SUCCESS_AARCH32:
+case ARM_SVC_ID_FFA_SUCCESS_AARCH64:
+ *RetVal = SvcArgs->Arg2;
+ return EFI_SUCCESS;
+
+default:
+ // Undefined error code received.
+
Re: [edk2-devel] [edk2-platforms][PATCH V1 04/20] ArmPkg: Add data structures to receive FF-A boot information
On 7/11/2023 7:36 AM, Nishant Sharma wrote:
From: Achin Gupta
The SPMC will pass the manifest to the StMM SP which contains the boot
information required for SP initialisation. This patch defines the data
structures defined in Section 5.4 of the FF-A v1.1 BETA0 spec to enable
this support. The manifest is identified by the TF-A UUID_TOS_FW_CONFIG.
Signed-off-by: Achin Gupta
Signed-off-by: Nishant Sharma
---
ArmPkg/Include/IndustryStandard/ArmFfaSvc.h | 69 +++-
1 file changed, 68 insertions(+), 1 deletion(-)
diff --git a/ArmPkg/Include/IndustryStandard/ArmFfaSvc.h
b/ArmPkg/Include/IndustryStandard/ArmFfaSvc.h
index 4126a4985bb2..54cc96598032 100644
--- a/ArmPkg/Include/IndustryStandard/ArmFfaSvc.h
+++ b/ArmPkg/Include/IndustryStandard/ArmFfaSvc.h
@@ -3,7 +3,7 @@
communication between S-EL0 and the Secure Partition
Manager(SPM)
- Copyright (c) 2020, ARM Limited. All rights reserved.
+ Copyright (c) 2020 - 2023, ARM Limited. All rights reserved.
SPDX-License-Identifier: BSD-2-Clause-Patent
@@ -53,4 +53,71 @@
//
https://github.com/OP-TEE/optee_os/blob/master/core/arch/arm/kernel/stmm_sp.c#L66
#define ARM_FFA_DESTINATION_ENDPOINT_ID 3
+/**
+ * Boot information protocol as per the FF-A v1.1 spec.
+ */
+#define FFA_INIT_DESC_SIGNATURE 0x0FFA
+
+/* Boot information type. */
+#define FFA_BOOT_INFO_TYPE_STD 0x0U
+#define FFA_BOOT_INFO_TYPE_IMPL 0x1U
+
+#define FFA_BOOT_INFO_TYPE_MASK 0x1U
+#define FFA_BOOT_INFO_TYPE_SHIFT0x7U
+#define FFA_BOOT_INFO_TYPE(type)\
+ (((type) & FFA_BOOT_INFO_TYPE_MASK) \
+ << FFA_BOOT_INFO_TYPE_SHIFT)
+
+/* Boot information identifier. */
+#define FFA_BOOT_INFO_TYPE_ID_FDT 0x0U
+#define FFA_BOOT_INFO_TYPE_ID_HOB 0x1U
+
+#define FFA_BOOT_INFO_TYPE_ID_MASK0x3FU
+#define FFA_BOOT_INFO_TYPE_ID_SHIFT 0x0U
+#define FFA_BOOT_INFO_TYPE_ID(type) \
+ (((type) & FFA_BOOT_INFO_TYPE_ID_MASK) \
+ << FFA_BOOT_INFO_TYPE_ID_SHIFT)
+
+/* Format of Flags Name field. */
+#define FFA_BOOT_INFO_FLAG_NAME_STRING 0x0U
+#define FFA_BOOT_INFO_FLAG_NAME_UUID0x1U
+
+#define FFA_BOOT_INFO_FLAG_NAME_MASK0x3U
+#define FFA_BOOT_INFO_FLAG_NAME_SHIFT 0x0U
+#define FFA_BOOT_INFO_FLAG_NAME(type) \
+ (((type) & FFA_BOOT_INFO_FLAG_NAME_MASK) \
+ << FFA_BOOT_INFO_FLAG_NAME_SHIFT)
+
+/* Format of Flags Contents field. */
+#define FFA_BOOT_INFO_FLAG_CONTENT_ADR0x0U
+#define FFA_BOOT_INFO_FLAG_CONTENT_VAL0x1U
+
+#define FFA_BOOT_INFO_FLAG_CONTENT_MASK 0x1U
+#define FFA_BOOT_INFO_FLAG_CONTENT_SHIFT 0x2U
+#define FFA_BOOT_INFO_FLAG_CONTENT(content) \
+ (((content) & FFA_BOOT_INFO_FLAG_CONTENT_MASK) \
+ << FFA_BOOT_INFO_FLAG_CONTENT_SHIFT)
+
+// Descriptor to pass boot information as per the FF-A v1.1 spec.
+typedef struct {
+ UINT32 Name[4];
+ UINT8 Type;
+ UINT8 Reserved;
+ UINT16 Flags;
+ UINT32 SizeBotInfo;
+ UINT64 Content;
+} EFI_FFA_BOOT_INFO_DESC;
+
+// Descriptor that contains boot info blobs size, number of desc it cointains
typo, should be "contains"
+// size of each descriptor and offset to the first descriptor.
+typedef struct {
+ UINT32 Magic; // 0xFFA^M
caught a copy paste "^M" here.
Thanks,
Oliver
+ UINT32 Version;
+ UINT32 SizeBootInfoBlob;
+ UINT32 SizeBootInfoDesc;
+ UINT32 CountBootInfoDesc;
+ UINT32 OffsetBootInfoDesc;
+ UINT64 Reserved;
+} EFI_FFA_BOOT_INFO_HEADER;
+
#endif // ARM_FFA_SVC_H_
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#106878): https://edk2.groups.io/g/devel/message/106878
Mute This Topic: https://groups.io/mt/100079874/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] [edk2-platforms][PATCH V1 01/20] ArmPkg: Change PcdFfaEnable flag datatype
On 7/11/2023 7:36 AM, Nishant Sharma wrote:
FeatureFlag type PCD flags are declared by typecasting an integer value
to BOOLEAN. These flags cannot be use in assembly code as assembler does
not recognise C primitive types. Change the flag data type from BOOLEAN
to UINT32.
Signed-off-by: Nishant Sharma
---
ArmPkg/ArmPkg.dec
| 14 +++---
ArmPkg/Library/StandaloneMmMmuLib/ArmMmuStandaloneMmLib.inf
| 4 ++--
StandaloneMmPkg/Library/StandaloneMmCoreEntryPoint/StandaloneMmCoreEntryPoint.inf
| 4 ++--
ArmPkg/Library/StandaloneMmMmuLib/ArmMmuStandaloneMmLib.c
| 8
StandaloneMmPkg/Library/StandaloneMmCoreEntryPoint/Arm/StandaloneMmCoreEntryPoint.c
| 8
5 files changed, 19 insertions(+), 19 deletions(-)
diff --git a/ArmPkg/ArmPkg.dec b/ArmPkg/ArmPkg.dec
index 1a16d044c94b..c36c23e2e059 100644
--- a/ArmPkg/ArmPkg.dec
+++ b/ArmPkg/ArmPkg.dec
@@ -155,13 +155,6 @@
# hardware coherency (i.e., no virtualization or cache coherent DMA)
gArmTokenSpaceGuid.PcdNormalMemoryNonshareableOverride|FALSE|BOOLEAN|0x0043
-[PcdsFeatureFlag.AARCH64, PcdsFeatureFlag.ARM]
- ## Used to select method for requesting services from S-EL1.
- # TRUE - Selects FF-A calls for communication between S-EL0 and SPMC.
- # FALSE - Selects SVC calls for communication between S-EL0 and SPMC.
- # @Prompt Enable FF-A support.
- gArmTokenSpaceGuid.PcdFfaEnable|FALSE|BOOLEAN|0x005B
-
[PcdsFixedAtBuild.common]
gArmTokenSpaceGuid.PcdTrustzoneSupport|FALSE|BOOLEAN|0x0006
@@ -298,6 +291,13 @@
# not currently supported.
gArmTokenSpaceGuid.PcdArmNonSecModeTransition|0x3c9|UINT32|0x003E
+ ## Used to select method for requesting services from S-EL1.
+ # 1 - Selects FF-A calls for communication between S-EL0 and SPMC.
+ # 0 - Selects SVC calls for communication between S-EL0 and SPMC.
+ # Using unsigned integer as boolean does not work on assembler.
+ # @Prompt Enable FF-A support.
+ gArmTokenSpaceGuid.PcdFfaEnable|0|UINT32|0x005B
+
A small nit: any reason not to go to UINT8 from BOOLEAN? It would seem
the logical conversion, unless you envision it extending greatly in the
future.
Thanks,
Oliver
#
# These PCDs are also defined as 'PcdsDynamic' or 'PcdsPatchableInModule' to
be
diff --git a/ArmPkg/Library/StandaloneMmMmuLib/ArmMmuStandaloneMmLib.inf
b/ArmPkg/Library/StandaloneMmMmuLib/ArmMmuStandaloneMmLib.inf
index ff20e5898051..3c733585f573 100644
--- a/ArmPkg/Library/StandaloneMmMmuLib/ArmMmuStandaloneMmLib.inf
+++ b/ArmPkg/Library/StandaloneMmMmuLib/ArmMmuStandaloneMmLib.inf
@@ -1,6 +1,6 @@
#/** @file
#
-# Copyright (c) 2017 - 2021, Arm Limited. All rights reserved.
+# Copyright (c) 2017 - 2023, Arm Limited. All rights reserved.
#
# SPDX-License-Identifier: BSD-2-Clause-Patent
#
@@ -23,7 +23,7 @@
ArmPkg/ArmPkg.dec
MdePkg/MdePkg.dec
-[FeaturePcd.ARM, FeaturePcd.AARCH64]
+[FixedPcd]
gArmTokenSpaceGuid.PcdFfaEnable
[LibraryClasses]
diff --git
a/StandaloneMmPkg/Library/StandaloneMmCoreEntryPoint/StandaloneMmCoreEntryPoint.inf
b/StandaloneMmPkg/Library/StandaloneMmCoreEntryPoint/StandaloneMmCoreEntryPoint.inf
index 75cfb98c0e75..dc6d3d859911 100644
---
a/StandaloneMmPkg/Library/StandaloneMmCoreEntryPoint/StandaloneMmCoreEntryPoint.inf
+++
b/StandaloneMmPkg/Library/StandaloneMmCoreEntryPoint/StandaloneMmCoreEntryPoint.inf
@@ -1,7 +1,7 @@
## @file
# Module entry point library for DXE core.
#
-# Copyright (c) 2017 - 2021, Arm Ltd. All rights reserved.
+# Copyright (c) 2017 - 2023, Arm Ltd. All rights reserved.
#
# SPDX-License-Identifier: BSD-2-Clause-Patent
#
@@ -51,7 +51,7 @@
gEfiStandaloneMmNonSecureBufferGuid
gEfiArmTfCpuDriverEpDescriptorGuid
-[FeaturePcd.ARM, FeaturePcd.AARCH64]
+[FixedPcd]
gArmTokenSpaceGuid.PcdFfaEnable
#
diff --git a/ArmPkg/Library/StandaloneMmMmuLib/ArmMmuStandaloneMmLib.c
b/ArmPkg/Library/StandaloneMmMmuLib/ArmMmuStandaloneMmLib.c
index d55aff76201e..1a41a289ef17 100644
--- a/ArmPkg/Library/StandaloneMmMmuLib/ArmMmuStandaloneMmLib.c
+++ b/ArmPkg/Library/StandaloneMmMmuLib/ArmMmuStandaloneMmLib.c
@@ -1,7 +1,7 @@
/** @file
File managing the MMU for ARMv8 architecture in S-EL0
- Copyright (c) 2017 - 2021, Arm Limited. All rights reserved.
+ Copyright (c) 2017 - 2023, Arm Limited. All rights reserved.
Copyright (c) 2021, Linaro Limited
SPDX-License-Identifier: BSD-2-Clause-Patent
@@ -54,7 +54,7 @@ SendMemoryPermissionRequest (
}
ArmCallSvc (SvcArgs);
- if (FeaturePcdGet (PcdFfaEnable)) {
+ if (FixedPcdGet32 (PcdFfaEnable) != 0) {
// Get/Set memory attributes is an atomic call, with
// StandaloneMm at S-EL0 being the caller and the SPM
// core being the callee. Thus there won't be a
@@ -163,7 +163,7 @@ GetMemoryPermissions (
// Prepare the message parameters.
Re: [edk2-devel][PATCH v1 1/1] ArmPkg: CpuDxe: Sync GCD Capabilities With Page Table Attributes
On 6/7/2023 11:03 AM, Oliver Smith-Denny wrote: On 6/7/2023 10:31 AM, Ard Biesheuvel wrote: On Wed, 7 Jun 2023 at 18:10, Oliver Smith-Denny wrote: Per the discussion in the memory protections design meeting this morning, I am kicking this patch back to the top of the inbox for review. If folks would like me to resend this patchset since the thread got bogged down with scheduling meetings, just let me know. I'll also pull up the BZ link for when the equivalent change went into the x86 CpuDxe driver in 2017: https://bugzilla.tianocore.org/show_bug.cgi?id=753 This contains lots of information about why the change went in on the x86 side (some dead mail links, but they can be retrieved through some digging). AFAICT, this change wasn't applied to ARM at the time due to an oversight, not a general design decision. Thanks for the background, this is useful. So I agree that for all system memory regions, we should be setting the RP, RO and XP capabilities. But what I don't understand is why these are not set to begin with. IOW, the resource descriptor HOBs that the initial regions are based on should have these capabilities set already, and then, we wouldn't have to do anything to at this point. If there is anything missing from the generic plumbing to make sure this transformation happens correctly, we should fix that first, and fix the existing ARM platforms to set the correct resource attributes. For example, ArmVirtQemu uses ResourceAttributes = ( EFI_RESOURCE_ATTRIBUTE_PRESENT | EFI_RESOURCE_ATTRIBUTE_INITIALIZED | EFI_RESOURCE_ATTRIBUTE_WRITE_BACK_CACHEABLE | EFI_RESOURCE_ATTRIBUTE_TESTED ); for the resource descriptor HOBs, and afaict, this should include EFI_RESOURCE_ATTRIBUTE_WRITE_PROTECTABLE EFI_RESOURCE_ATTRIBUTE_EXECUTION_PROTECTABLE EFI_RESOURCE_ATTRIBUTE_READ_ONLY_PROTECTABLE to accurately describe the region's capabilities. WIth that out of the way, I wonder if we still need this patch at all. Thanks, Ard. I definitely agree this should be set at GCD initialization. Looking at the code path you present, I think this could work. My initial concern was that the resource descriptor HOB was passing attributes not capabilities, but I see that it actually passes both in a field called attributes :). On ARM, as you point out, this looks like we would intercept at MemoryInitPeiLib when it builds the HOBs. This would be a separate method from what x86 does, but I can take a look at aligning x86 to initializing the capabilities through the resource descriptor HOBs. I'll take a crack at this and see how it shapes up. Seems reasonable to me, though. I'll need to look into adding memory ranges, we would want new ranges to have the capabilities, too. Hi Ard, I'm returning to this after a couple of weeks of vacation, hopefully with fresh eyes :). The path you describe of resource descriptor HOBs including the RO/RP/XP capabilities does looks like it would work and I do like that it moves these capabilities earlier. That being said, the downside of this approach to me is that it has the core code rely on the platform doing the right thing, which is far from guaranteed. I would prefer to have the core code control the memory protections as much as possible with platform configurability. Platforms have already had the opportunity to set these capabilities in resource descriptor HOBs, but are not. This leads into another related concept, what is the default state of free memory (is it XP, RO, RP, none of them, some combination, etc.). This is also something that should be enforced at the core level, I believe. I will explore this further in a different patch set. For this patchset, I think the right approach would be to set the RP/RO/XP capabilities by default when constructing the GCD. This would also allow the Intel side to not have the workaround they have, where they apply these capabilities when syncing the page table and the GCD after CpuDxe comes up (as I did in this patchset for ARM64). I can spin off a new version with that once I do so some testing. Thanks, Oliver On 4/25/2023 5:09 PM, Oliver Smith-Denny wrote: When ArmPkg's CpuDxe driver initializes, it attempts to sync the GCD with the page table. However, unlike when the UefiCpuPkg's CpuDxe initializes, the Arm version does not update the GCD capabilities with EFI_MEMORY_[RO|RP|XP] (this could also set the capabilities to be the existing page table attributes for this range, but the UefiCpuPkg CpuDxe sets the above attributes as they are software constructs, possible to set for any memory hardware). As a result, when the GCD attributes are attempted to be set against the old GCD capabilities, attributes that are set in the page table can get lost because the new attributes are not in the old GCD capabilities (and yet they are already set in the page table) meaning
Re: [edk2-devel] [PATCH 2/2] UefiCpuPkg/CpuMpPei X64: Reallocate page tables in permanent DRAM
On 6/8/2023 10:23 AM, Ard Biesheuvel wrote: Currently, we rely on the logic in DXE IPL to create new page tables from scratch when executing in X64 mode, which means that we run with the initial page tables all throughout PEI, and never enable protections such as the CPU stack guard, even though the logic is already in place for IA32. So let's enable the existing logic for X64 as well. This will permit us to apply stricter memory permissions to code and data allocations, as well as the stack, when executing in PEI. It also makes the DxeIpl logic redundant, and should allow us to make the PcdDxeIplBuildPageTables feature PCD limited to IA32 DxeIpl loading the x64 DXE core. When running in long mode, use the same logic that DxeIpl uses to determine the size of the address space, whether or not to use 1 GB leaf entries and whether or not to use 5 level paging. Note that in long mode, PEI is entered with paging enabled, and given that switching between 4 and 5 levels of paging is not currently supported without dropping out of 64-bit mode temporarily, all we can do is carry on without changing the number of levels. I certainly agree with extending the ability to have memory protections in PEI (and trying to unify across x86 and ARM (and beyond :)). A few things I am trying to understand: Does ARM today rebuild the page table in DxeIpl? Or is it using an earlier built page table? If I understand your proposal correctly, with the addition of this patch, you are suggesting we can drop creating new page tables in DxeIpl and use only one page table throughout. Again, I like the idea of having mapped memory protections that continue through, but do you have concerns that we may end up with garbage from PEI in DXE in the page table? For OEMs, they may not control PEI and therefore be at the whim of another's PEI page table. Would you envision the GCD gets built from the existing page table or that the GCD gets built according to resource descriptor HOBs and DxeCore ensures that the page table reflects what the HOBs indicated? Thanks, Oliver -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#105944): https://edk2.groups.io/g/devel/message/105944 Mute This Topic: https://groups.io/mt/99411875/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel][PATCH v1 1/1] ArmPkg: CpuDxe: Sync GCD Capabilities With Page Table Attributes
On 6/7/2023 10:31 AM, Ard Biesheuvel wrote: On Wed, 7 Jun 2023 at 18:10, Oliver Smith-Denny wrote: Per the discussion in the memory protections design meeting this morning, I am kicking this patch back to the top of the inbox for review. If folks would like me to resend this patchset since the thread got bogged down with scheduling meetings, just let me know. I'll also pull up the BZ link for when the equivalent change went into the x86 CpuDxe driver in 2017: https://bugzilla.tianocore.org/show_bug.cgi?id=753 This contains lots of information about why the change went in on the x86 side (some dead mail links, but they can be retrieved through some digging). AFAICT, this change wasn't applied to ARM at the time due to an oversight, not a general design decision. Thanks for the background, this is useful. So I agree that for all system memory regions, we should be setting the RP, RO and XP capabilities. But what I don't understand is why these are not set to begin with. IOW, the resource descriptor HOBs that the initial regions are based on should have these capabilities set already, and then, we wouldn't have to do anything to at this point. If there is anything missing from the generic plumbing to make sure this transformation happens correctly, we should fix that first, and fix the existing ARM platforms to set the correct resource attributes. For example, ArmVirtQemu uses ResourceAttributes = ( EFI_RESOURCE_ATTRIBUTE_PRESENT | EFI_RESOURCE_ATTRIBUTE_INITIALIZED | EFI_RESOURCE_ATTRIBUTE_WRITE_BACK_CACHEABLE | EFI_RESOURCE_ATTRIBUTE_TESTED ); for the resource descriptor HOBs, and afaict, this should include EFI_RESOURCE_ATTRIBUTE_WRITE_PROTECTABLE EFI_RESOURCE_ATTRIBUTE_EXECUTION_PROTECTABLE EFI_RESOURCE_ATTRIBUTE_READ_ONLY_PROTECTABLE to accurately describe the region's capabilities. WIth that out of the way, I wonder if we still need this patch at all. Thanks, Ard. I definitely agree this should be set at GCD initialization. Looking at the code path you present, I think this could work. My initial concern was that the resource descriptor HOB was passing attributes not capabilities, but I see that it actually passes both in a field called attributes :). On ARM, as you point out, this looks like we would intercept at MemoryInitPeiLib when it builds the HOBs. This would be a separate method from what x86 does, but I can take a look at aligning x86 to initializing the capabilities through the resource descriptor HOBs. I'll take a crack at this and see how it shapes up. Seems reasonable to me, though. I'll need to look into adding memory ranges, we would want new ranges to have the capabilities, too. Thanks, Oliver On 4/25/2023 5:09 PM, Oliver Smith-Denny wrote: When ArmPkg's CpuDxe driver initializes, it attempts to sync the GCD with the page table. However, unlike when the UefiCpuPkg's CpuDxe initializes, the Arm version does not update the GCD capabilities with EFI_MEMORY_[RO|RP|XP] (this could also set the capabilities to be the existing page table attributes for this range, but the UefiCpuPkg CpuDxe sets the above attributes as they are software constructs, possible to set for any memory hardware). As a result, when the GCD attributes are attempted to be set against the old GCD capabilities, attributes that are set in the page table can get lost because the new attributes are not in the old GCD capabilities (and yet they are already set in the page table) meaning that the attempted sync between the GCD and the page table was a failure and drivers querying one vs the other will see different state. This can lead to RWX memory regions even with the no-execute policy set, because core drivers (such as NonDiscoverablePciDeviceIo, to be fixed up in a later patchset) allocate pages, query the GCD attributes, attempt to set a new cache attribute and end up clearing the XP bit in the page table because the GCD attributes do not have XP set. This patch follows the UefiCpuPkg pattern and adds EFI_MEMORY_[RO|RP|XP] to the GCD capabilities during CpuDxe initialization. This ensures that memory regions which already have these attributes set get them set in the GCD attributes, properly syncing between the GCD and the page table. This mitigates the issue seen, however, additional investigations into setting the GCD attributes earlier and maintaining a better sync between the GCD and the page table are being done. Feedback on this proposal is greatly appreciated, particularly any pitfalls or more architectural solutions to issues seen with syncing the GCD and the page table. PR: https://github.com/tianocore/edk2/pull/4311 Personal branch: https://github.com/os-d/edk2/tree/osde/sync_aarch64_gcd_capabilities_v1 Cc: Leif Lindholm Cc: Ard Biesheuvel Cc: Sami Mujawar Cc: Michael Kubacki Cc: Sean Brogan
Re: [edk2-devel][PATCH v1 1/1] ArmPkg: CpuDxe: Sync GCD Capabilities With Page Table Attributes
Per the discussion in the memory protections design meeting
this morning, I am kicking this patch back to the top of
the inbox for review. If folks would like me to resend this
patchset since the thread got bogged down with scheduling
meetings, just let me know.
I'll also pull up the BZ link for when the equivalent
change went into the x86 CpuDxe driver in 2017:
https://bugzilla.tianocore.org/show_bug.cgi?id=753
This contains lots of information about why the change went
in on the x86 side (some dead mail links, but they can be
retrieved through some digging). AFAICT, this change wasn't
applied to ARM at the time due to an oversight, not a general
design decision.
Thanks,
Oliver
On 4/25/2023 5:09 PM, Oliver Smith-Denny wrote:
When ArmPkg's CpuDxe driver initializes, it attempts to sync the
GCD with the page table. However, unlike when the UefiCpuPkg's
CpuDxe initializes, the Arm version does not update the GCD
capabilities with EFI_MEMORY_[RO|RP|XP] (this could also set
the capabilities to be the existing page table attributes for
this range, but the UefiCpuPkg CpuDxe sets the above attributes
as they are software constructs, possible to set for any memory
hardware).
As a result, when the GCD attributes are attempted to be set
against the old GCD capabilities, attributes that are set in the
page table can get lost because the new attributes are not in the
old GCD capabilities (and yet they are already set in the page
table) meaning that the attempted sync between the GCD and the
page table was a failure and drivers querying one vs the other
will see different state. This can lead to RWX memory regions
even with the no-execute policy set, because core drivers (such
as NonDiscoverablePciDeviceIo, to be fixed up in a later patchset)
allocate pages, query the GCD attributes, attempt to set a new
cache attribute and end up clearing the XP bit in the page table
because the GCD attributes do not have XP set.
This patch follows the UefiCpuPkg pattern and adds
EFI_MEMORY_[RO|RP|XP] to the GCD capabilities during CpuDxe
initialization. This ensures that memory regions which already have
these attributes set get them set in the GCD attributes, properly
syncing between the GCD and the page table.
This mitigates the issue seen, however, additional investigations
into setting the GCD attributes earlier and maintaining a better
sync between the GCD and the page table are being done.
Feedback on this proposal is greatly appreciated, particularly
any pitfalls or more architectural solutions to issues seen
with syncing the GCD and the page table.
PR: https://github.com/tianocore/edk2/pull/4311
Personal branch:
https://github.com/os-d/edk2/tree/osde/sync_aarch64_gcd_capabilities_v1
Cc: Leif Lindholm
Cc: Ard Biesheuvel
Cc: Sami Mujawar
Cc: Michael Kubacki
Cc: Sean Brogan
Signed-off-by: Oliver Smith-Denny
---
ArmPkg/Drivers/CpuDxe/CpuMmuCommon.c | 55 +---
1 file changed, 49 insertions(+), 6 deletions(-)
diff --git a/ArmPkg/Drivers/CpuDxe/CpuMmuCommon.c
b/ArmPkg/Drivers/CpuDxe/CpuMmuCommon.c
index 2e73719dce04..3ef0380e084f 100644
--- a/ArmPkg/Drivers/CpuDxe/CpuMmuCommon.c
+++ b/ArmPkg/Drivers/CpuDxe/CpuMmuCommon.c
@@ -90,6 +90,7 @@ SetGcdMemorySpaceAttributes (
UINTN EndIndex;
EFI_PHYSICAL_ADDRESS RegionStart;
UINT64RegionLength;
+ UINT64Capabilities;
DEBUG ((
DEBUG_GCD,
@@ -146,14 +147,56 @@ SetGcdMemorySpaceAttributes (
RegionLength = MemorySpaceMap[Index].BaseAddress +
MemorySpaceMap[Index].Length - RegionStart;
}
+// Always add RO, RP, and XP as all memory is capable of supporting these
types (they are software
+// constructs, not hardware features) and they are critical to maintaining
a security boundary
+Capabilities = MemorySpaceMap[Index].Capabilities | EFI_MEMORY_RO |
EFI_MEMORY_RP | EFI_MEMORY_XP;
+
//
-// Set memory attributes according to MTRR attribute and the original
attribute of descriptor
+// Update GCD capabilities as these may have changed in the page table
since the GCD was created
+// this follows the same pattern as x86 GCD and Page Table syncing
//
-gDS->SetMemorySpaceAttributes (
- RegionStart,
- RegionLength,
- (MemorySpaceMap[Index].Attributes & ~EFI_MEMORY_CACHETYPE_MASK) |
(MemorySpaceMap[Index].Capabilities & Attributes)
- );
+Status = gDS->SetMemorySpaceCapabilities (
+RegionStart,
+RegionLength,
+Capabilities
+);
+
+if (EFI_ERROR (Status)) {
+ DEBUG ((
+DEBUG_ERROR,
+"%a - failed to update GCD capabilities: 0x%llx on memory region: 0x%llx
length: 0x%llx Status: %r\n",
+__func__,
+Capabilities,
+RegionStart,
+Region
Re: [edk2-devel] [RFC PATCH 00/11] Permit DXE drivers to execute in place
On 5/29/2023 3:16 AM, Ard Biesheuvel wrote: TL;DR - allow DXE drivers to execute in place from the decompressed FV loaded into memory by DxeIpl so we can apply strict permissions before dispatching DXE core. Currently, executable images loaded from firmware volumes are copied at least three times: once in the firmware volume driver, once in the DXE core load image code, and finally when the PE sections are populated in memory based on the section descriptions in the file. At least two of these copies serve little purpose, given that most drivers are typically dispatched from a memory-mapped firmware volume that is loaded into DRAM by DxeIpl from a compressed image in the boot FV, and so we can take a short-cut in the DXE image loader so that the PE/COFF library that performs the load uses the image in the memory mapped FV as its source directly. This is implemented by the first 6 patches (where the first 3 are just cleanups) With this logic in place, we can go one step further, and actually dispatch the image in place (similar to how XIP PEIMs are dispatched), without over moving it out of the decompressed firmware volume. This requires the image to be aligned sufficiently inside the FV, but this is also the same logic that applies to XIP PEIMs, and this can be achieved trivially by tweaking some FDF image generation rules. (Note that this adds padding to the FV, but this generally compresses well, and we ultimately uses less memory at runtime by not making a copy of the image). This requires the DXE IPL (which is the component that decompresses the firmware volumes to memory) to iterate over the contents and relocate these drivers in place. Given that DXE IPL is already in charge of applying NX permissions to the stack and to other memory regions, we can trivially extend it to apply restricted permissions to the XIP DXE drivers after relocation. This means we enter DXE core with those DXE drivers ready to be dispatched, removing the need to perform manipulation of memory attributes before the CPU arch protocol is dispatched, which is a bit of a catch-22 otherwise. With these changes in place, the platform no longer needs to map memory writable and executable by default, and all DRAM can be mapped non-executable right out of reset. Hi Ard, Thanks for sending out this RFC, great to see more work on the memory protections front. A few questions and thoughts: This seems a good effort (in conjunction with your last RFC) to close the protection gap between DxeCore launch and CpuDxe launch for marking non-code regions NX. Do you see other protections (guard pages for example) fitting into this method? I believe for any dynamic protections during this timeframe we would need the ability to manipulate the page tables directly from DxeCore. Similarly, in order to lessen the complexity of the DXE driver usage of memory resources and avoiding sync issues (e.g. a driver allocates pages that are mapped NX by default, then it sets a cacheability attribute and accidentally clears NX), I think further work would be valuable to reduce that complexity. I think your new PPI that allows setting and preserving bits independently of what is passed in is a very good step towards reducing this complexity. This patchset would move all properly aligned DXE drivers to be XIP, correct? Because we are XIP in DRAM, this should not have any performance implications (other than a benefit from reducing the extra copies in your first few patches), aside from potential space differences, which as you note compression will likely do away with, right? Thanks, Oliver -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#105567): https://edk2.groups.io/g/devel/message/105567 Mute This Topic: https://groups.io/mt/99197132/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] GoogleTest Compatibility with MdePkg's IndustyStandard header files
Hi Mike,
Thanks for looking for solutions here. This one feels like
quite a back bend, I'm imagining reading code and coming
across TpmStruct.CPLUSPLUS_OPERATOR_KEYWORD and having to
dig around quite a lot to see what goodness is going
on. Because we would have to update the C files, too, right,
depending on the test (there exist tests that want to test
static functions and so include the C file in the unit test
file). Perhaps that is an anti-pattern and googletest has
other ways of testing static functions, but in general it
feels odd to change our functional C code for a C++ unit
test framework.
But, that being said, this is an issue we face, so perhaps
it would be simpler to just rename the members to not conflict
with the C++ keywords, as previously suggested, even though
this may differ from the spec, but it would more align with
EDKII's conventions (shouty case) where the C++ keywords seem
to be lowercase. With the below patch, we would already be
changing the header file to not match with the spec, so from
a readability perspective of the header file, I think
Operator is much more readable than CPLUSPLUS_OPERATOR_KEYWORD.
Thanks,
Oliver
On 5/25/2023 10:06 AM, Michael D Kinney wrote:
Hi Pedro,
Thanks for the feedback!
Applying your pattern to edk2, we find all the usage of c++
reserved keywords and replace with a macro. We then define
that macro to either be the actual c++ reserved keyword if
build with a C compiler and rename the keyword if building
with a c++ compiler. The following patches build with VS
and GCC and should be no changes from any FW consumers of
Tpm12.h or Tpm20.h files and should support GoogleTest builds
of unit test case and code under test that will consistently
rename the reserved keyword.
We will see if this resolves Aaron specific use case and if
it does we can move this to a code review.
We can always expand the keyword set as needed in the future
if industry standard specs use C definitions that collide
with additional c++ reserved keywords.
diff --git a/MdePkg/Include/Base.h b/MdePkg/Include/Base.h
index 6597e441a6e2..b98ff33002b8 100644
--- a/MdePkg/Include/Base.h
+++ b/MdePkg/Include/Base.h
@@ -15,6 +15,17 @@ SPDX-License-Identifier: BSD-2-Clause-Patent
#ifndef __BASE_H__
#define __BASE_H__
+//
+// C++ reserved keyword defines
+//
+#if defined (__cplusplus)
+#define CPLUSPLUS_OPERATOR_KEYWORD operator_
+#define CPLUSPLUS_XOR_KEYWORD xor_
+#else
+#define CPLUSPLUS_OPERATOR_KEYWORD operator
+#define CPLUSPLUS_XOR_KEYWORD xor
+#endif
+
//
// Include processor specific binding
//
diff --git a/MdePkg/Include/IndustryStandard/Tpm12.h
b/MdePkg/Include/IndustryStandard/Tpm12.h
index 155dcc9f5f99..8551dd7c5f42 100644
--- a/MdePkg/Include/IndustryStandard/Tpm12.h
+++ b/MdePkg/Include/IndustryStandard/Tpm12.h
@@ -744,7 +744,7 @@ typedef struct tdTPM_PERMANENT_FLAGS {
BOOLEAN TPMpost;
BOOLEAN TPMpostLock;
BOOLEAN FIPS;
- BOOLEAN operator;
+ BOOLEAN CPLUSPLUS_OPERATOR_KEYWORD;
BOOLEAN enableRevokeEK;
BOOLEAN nvLocked;
BOOLEAN readSRKPub;
diff --git a/MdePkg/Include/IndustryStandard/Tpm20.h
b/MdePkg/Include/IndustryStandard/Tpm20.h
index 4440f3769f26..a96af9cfed63 100644
--- a/MdePkg/Include/IndustryStandard/Tpm20.h
+++ b/MdePkg/Include/IndustryStandard/Tpm20.h
@@ -1247,7 +1247,7 @@ typedef union {
TPMI_AES_KEY_BITSaes;
TPMI_SM4_KEY_BITSSM4;
TPM_KEY_BITS sym;
- TPMI_ALG_HASH xor;
+ TPMI_ALG_HASH CPLUSPLUS_XOR_KEYWORD;
} TPMU_SYM_KEY_BITS;
// Table 123 - TPMU_SYM_MODE Union
@@ -1320,7 +1320,7 @@ typedef struct {
// Table 136 - TPMU_SCHEME_KEYEDHASH Union
typedef union {
TPMS_SCHEME_HMAChmac;
- TPMS_SCHEME_XOR xor;
+ TPMS_SCHEME_XOR CPLUSPLUS_XOR_KEYWORD;
} TPMU_SCHEME_KEYEDHASH;
// Table 137 - TPMT_KEYEDHASH_SCHEME Structure
Mike
-Original Message-
From: Pedro Falcato
Sent: Thursday, May 25, 2023 2:44 AM
To: devel@edk2.groups.io; Kinney, Michael D
Cc: Pop, Aaron
Subject: Re: [edk2-devel] GoogleTest Compatibility with MdePkg's
IndustyStandard header files
On Thu, May 25, 2023 at 1:24 AM Michael D Kinney
wrote:
That is exactly what I did. Along with pragma to disable error on macros
redefining operators.
With that change use of "operator" did not generate a warning or error and
the build completed.
Did not work for "xor", and can not find any additional pragma to suppress
that error.
FWIW, it does work here: https://godbolt.org/z/EEdh9oh53
--
Pedro
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#105321): https://edk2.groups.io/g/devel/message/105321
Mute This Topic: https://groups.io/mt/99079638/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
Re: [edk2-devel] [RFC PATCH 07/10] MdeModulePkg/PeiCore: Apply restricted permissions in image loader
On 5/25/2023 7:30 AM, Ard Biesheuvel wrote:
Add a notification callback to the PEI core to grab a reference to the
memory attributes PPI as soon as it is registered, and use it in the
image loader to set restricted memory permissions after loading the
image if the image was loaded into memory.
There are two use cases for this:
- when the DXE IPL loads the DXE core using the PEI image services, its
mappings will be set according to the PE section permission attributes
if the image was built with 4k section alignment; this means DXE core
will never run with mappings that are both writable and executable.
- when PEIMs are shadowed to memory, they will not only be mapped
read-only, but any non-exec permissions will also be removed. (Note
that this requires the component that installs PEI permanent memory to
depex on the memory attributes PPI, to ensure that it is available to
manage permissions on permanent memory before it is used to load
images)
With this logic in place *, there is no longer a need for system memory
to be mapped with both write and execute permissions out of reset.
Instead, all memory can be mapped with non-executable permissions by
default, which means that the stack and other allocations used in PEI or
early in DXE will no longer need to be mapped non-exec explicitly.
* the DXE core will also need its own method for clearing non-exec
permissions on memory ranges, but this is being addressed in a
separate series.
Signed-off-by: Ard Biesheuvel
---
MdeModulePkg/Core/Pei/Image/Image.c | 160
MdeModulePkg/Core/Pei/PeiMain.h | 6 +
MdeModulePkg/Core/Pei/PeiMain.inf | 1 +
3 files changed, 167 insertions(+)
diff --git a/MdeModulePkg/Core/Pei/Image/Image.c
b/MdeModulePkg/Core/Pei/Image/Image.c
index cee9f09c6ea61e31..3a7de45014b8f772 100644
--- a/MdeModulePkg/Core/Pei/Image/Image.c
+++ b/MdeModulePkg/Core/Pei/Image/Image.c
@@ -18,6 +18,50 @@ EFI_PEI_PPI_DESCRIPTOR gPpiLoadFilePpiList = {
};
+/**
+ Provide a callback for when the memory attributes PPI is installed.
+
+ @param PeiServicesAn indirect pointer to the EFI_PEI_SERVICES table
+published by the PEI Foundation.
+ @param NotifyDescriptor The descriptor for the notification event.
+ @param PpiPointer to the PPI in question.
+
+ @return Always success
+
+**/
+STATIC
+EFI_STATUS
+EFIAPI
+MemoryAttributePpiNotifyCallback (
+ IN EFI_PEI_SERVICES **PeiServices,
+ IN EFI_PEI_NOTIFY_DESCRIPTOR *NotifyDescriptor,
+ IN VOID *Ppi
+ )
+{
+ PEI_CORE_INSTANCE *PrivateData;
+
+ //
+ // Get PEI Core private data
+ //
+ PrivateData = PEI_CORE_INSTANCE_FROM_PS_THIS (PeiServices);
+
+ //
+ // If there isn't a memory attribute PPI installed, use the one from
+ // notification
+ //
+ if (PrivateData->MemoryAttributePpi == NULL) {
+PrivateData->MemoryAttributePpi = (EDKII_MEMORY_ATTRIBUTE_PPI *)Ppi;
+ }
+
+ return EFI_SUCCESS;
+}
+
+STATIC CONST EFI_PEI_NOTIFY_DESCRIPTOR mNotifyList = {
+ EFI_PEI_PPI_DESCRIPTOR_NOTIFY_DISPATCH |
EFI_PEI_PPI_DESCRIPTOR_TERMINATE_LIST,
+ ,
+ MemoryAttributePpiNotifyCallback
+};
+
/**
Support routine for the PE/COFF Loader that reads a buffer from a PE/COFF
file.
@@ -243,6 +287,106 @@ GetPeCoffImageFixLoadingAssignedAddress (
return Status;
}
+/**
+ Remap the memory region covering a loaded image so it can be executed.
+
+ @param ImageContextPointer to the image context structure that describes
the
+ PE/COFF image that needs to be examined by this
function.
+ @param FileTypeThe FFS file type of the image
+ @param ImageAddressThe start of the memory region covering the image
+ @param ImageSize The size of the memory region covering the image
+
+ @retval EFI_SUCCESS The image is ready to be executed
+ @retval EFI_OUT_OF_RESOURCES Not enough memory available to update the
memory
+mapping
+
+**/
+STATIC
+EFI_STATUS
+RemapLoadedImageForExecution (
+ IN CONST PE_COFF_LOADER_IMAGE_CONTEXT *ImageContext,
+ IN EFI_FV_FILETYPE FileType,
+ IN PHYSICAL_ADDRESSImageAddress,
+ IN UINT64 ImageSize
+ )
+{
+ PEI_CORE_INSTANCE*Private;
+ EFI_IMAGE_OPTIONAL_HEADER_PTR_UNION Hdr;
+ EFI_IMAGE_SECTION_HEADER *Section;
+ PHYSICAL_ADDRESS SectionAddress;
+ EFI_STATUS Status;
+ UINT64 Permissions;
+ UINTNIndex;
+
+ Private = PEI_CORE_INSTANCE_FROM_PS_THIS (GetPeiServicesTablePointer ());
+
+ if (Private->MemoryAttributePpi == NULL) {
+return EFI_SUCCESS;
We will have a gap here before the MemoryAttributePpi is installed,
Re: [edk2-devel] [RFC PATCH 00/10] Add PPI to manage PEI phase memory attributes
On 5/25/2023 7:30 AM, Ard Biesheuvel wrote: REF: https://bugzilla.tianocore.org/show_bug.cgi?id=4468 This is a proof-of-concept RFC that implements a PEI phase PPI to manage memory permission attributes, and wires it up to the PEI image loader so that shadowed PEIMs as well as the DXE core are remapped with the appropriate, restricted memory permission attributes before execution. This means that neither shadowed PEIMs nor the DXE core will ever execute with writable code regions. It also removes the need on the part of PEI for memory to be mapped with both writable and executable permissions by default out of reset. Similar work still needs to be done to address the early DXE phase (before the CPU arch protocol becomes available), but once that is out of the way as well, platforms should be able to map all memory non-executable from the beginning. This by itself is a major improvement in terms of robustness. It is also a prerequisite for enabling the WXN MMU control on AArch64, which makes all writable memory mappings non-executable regardless of the non-exec page table attribute. Patches #1 to #4 are prepatory work. Patch #5 proposes the memory attribute PPI protocol interface. Patch #6 implements it for ARM and AARCH64. Patch #7 wires it up into the PEI image loader. Patches #8 to #10 update the DxeIpl to use this PPI on ARM/AARCH64 for mapping the stack NX. instead of an explicit reference to ArmMmuLib. Other architectures (except IA32/X64) will seamlessly inherit this once they implement the PPI as well. Hi Ard, Thanks for the RFC. This same issue exists for X64, I saw your mailing list question about IA32 PEI, do you have plans for extending this to X64 PEI (I'm not sure its state of readiness)? If so, do you think it is feasible to merge the X64 DxeLoadFunc with the generic one you have created? Overall, this seems a reasonable approach to me towards making DXE more protected with the additional benefit of protecting shadowed PEIMs. I did wonder if the same discussion we are having on the DXE side about moving these MMU manipulations to the core instead of the CPU driver make sense in PEI, too, but with the greatly reduced complexity of the environment (no GCD, etc.), I don't think it makes sense now and that focusing on the DXE reorganization and expansion is going to deliver a much greater impact. Thanks, Oliver -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#105319): https://edk2.groups.io/g/devel/message/105319 Mute This Topic: https://groups.io/mt/99131172/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] [PATCH v1 1/1] ArmPkg: CpuDxe: Sync GCD Capabilities With Page Table Attributes
On 5/8/2023 11:59 PM, Ard Biesheuvel wrote: On Tue, 9 May 2023 at 04:04, Kinney, Michael D wrote: I would prefer next week as well. Mike Next week, i can only do Wednesday. The week after (22-26), the time slot works for me on any day of the week. Weds works from our side, the week after also works perfectly well any day. Thanks for the flexibility and willingness to meet. For reference for this specific patch, this bz may help cache in some info: https://bugzilla.tianocore.org/show_bug.cgi?id=753. The mail links are largely dead, of course, but can be found on other mailing list retention sites (maybe in the future we will have PRs and discussions to reference :). Thanks, Oliver -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#104385): https://edk2.groups.io/g/devel/message/104385 Mute This Topic: https://groups.io/mt/98505340/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] [PATCH v1 1/1] ArmPkg: CpuDxe: Sync GCD Capabilities With Page Table Attributes
On 5/1/2023 10:50 AM, Ard Biesheuvel wrote: On Mon, 1 May 2023 at 19:49, Michael D Kinney wrote: Hi, These UEFI Memory Map, GCD, and Page Table interactions can be complex and I agree there are some UEFI/PI spec clarifications that may help. Ray hosts a TianoCore design meeting when needed. Do you think a meeting with an open discussion on these topics would help, or do we prefer to continue with email discussions? I'll gladly join a call to discuss this if we can find a timeslot that works for everyone in terms of time zone. (I'm on Paris time) I also think a call would be great, I certainly would benefit from learning more here :). I'm sure various members of my team would be interested in joining, happy to be flexible on timeslot (we are generally in PST). Thanks, Oliver -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#103821): https://edk2.groups.io/g/devel/message/103821 Mute This Topic: https://groups.io/mt/98505340/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] [PATCH v1 1/1] ArmPkg: CpuDxe: Sync GCD Capabilities With Page Table Attributes
On 5/1/2023 6:02 AM, Ard Biesheuvel wrote:
On Wed, 26 Apr 2023 at 02:09, Oliver Smith-Denny
wrote:
When ArmPkg's CpuDxe driver initializes, it attempts to sync the
GCD with the page table. However, unlike when the UefiCpuPkg's
CpuDxe initializes, the Arm version does not update the GCD
capabilities with EFI_MEMORY_[RO|RP|XP] (this could also set
the capabilities to be the existing page table attributes for
this range, but the UefiCpuPkg CpuDxe sets the above attributes
as they are software constructs, possible to set for any memory
hardware).
As a result, when the GCD attributes are attempted to be set
against the old GCD capabilities, attributes that are set in the
page table can get lost because the new attributes are not in the
old GCD capabilities (and yet they are already set in the page
table) meaning that the attempted sync between the GCD and the
page table was a failure and drivers querying one vs the other
will see different state. This can lead to RWX memory regions
even with the no-execute policy set, because core drivers (such
as NonDiscoverablePciDeviceIo, to be fixed up in a later patchset)
allocate pages, query the GCD attributes, attempt to set a new
cache attribute and end up clearing the XP bit in the page table
because the GCD attributes do not have XP set.
This patch follows the UefiCpuPkg pattern and adds
EFI_MEMORY_[RO|RP|XP] to the GCD capabilities during CpuDxe
initialization. This ensures that memory regions which already have
these attributes set get them set in the GCD attributes, properly
syncing between the GCD and the page table.
This mitigates the issue seen, however, additional investigations
into setting the GCD attributes earlier and maintaining a better
sync between the GCD and the page table are being done.
Feedback on this proposal is greatly appreciated, particularly
any pitfalls or more architectural solutions to issues seen
with syncing the GCD and the page table.
I think this is conceptually correct. However, I've played with this
in the past, and IIRC, these attributes are inherited by the EFI
memory map too, and not updated when allocations are made. This means
that all code and data regions will be listed as RP, RO and XP in the
EFI memory map, which is going to confuse Linux at the very least, and
potentially other OSes as well.
Thanks for the detailed response, I appreciate it!
A clarification here: my patch follows what the x86 code does (see
https://github.com/tianocore/edk2/blob/56e9828380b7425678a080bd3a08e7c741af67ba/UefiCpuPkg/CpuDxe/CpuPageTable.c#LL994C1-L1077C27),
where the capabilities of each region are updated to include RP, RO, and
XP. However, the attributes of each region are only updated to include
these flags if the page table has the attributes set for this region.
I.e. we are only allowing for the possibility of these bits to be
set, but not actually setting them in the attributes unless the page
table already has them set. So, at the very least, we should be
matching what x86 does (so OS's should not be broken), unless
the other discrepancies in the x86 and Arm memory attribute handling
cause issue.
The main thing my change is intended to address is the case where
code that uses the GCD instead of the page table (such as the
NonDiscoverablePciDeviceIo driver I mentioned) attempts to set
a CPU memory attribute (such as UC) and ends up clearing the XP bit
that was set in the page table, but wasn't in the GCD (because it was
never properly synced with the page table). This could also be solved
by going to each caller and updating it to set the GCD capabilities
before it sets the GCD attributes (and perhaps they should be, already).
However, this puts the burden of maintaining memory protections such
as XP on each caller, instead of handling it at a core level (now,
there are of course complete design changes that could make this more
effective, but this was an attempt at a simple first step).
My understanding is that the EFI memory map is built on demand by calls
to GetMemoryMap from the page descriptor list, which is updated
thoughout bootservices, i.e. that it isn't pulling from the GCD. I will
go back and trace through this logic again.
Generally, the EFI and PI specs are very vague when it comes to
defining whether memory region attributes apply to the memory region
itself ("this memory can be mapped as read-only or non-exec") or its
contents ("these pages can be mapped read-only because the code does
not expect to be able to write to it").
Before we have some clarification and some rigid wording in the spec
as to what these attributes actually mean for non-allocated regions vs
allocated ones, I don't think we can use them like this.
Alternatively, we could update the GCD core so these attributes are
not inherited by the EFI memory map.
Another potential concern is fragmentation: the GCD memory map has
very few entries usually under the current usage model, but keeping
track of all memory p
Re: [edk2-devel] [Patch v2 00/12] Add gmock support for host-based unit testing
Thanks for sending this out, looks to be a pretty clean unit testing interface. We are testing this now with some of our unit tests and will update with any feedback. In the meantime, for the patchset: Reviewed-by: Oliver Smith-Denny On 4/4/2023 11:22 AM, Michael D Kinney wrote: REF: https://bugzilla.tianocore.org/show_bug.cgi?id=4389 PR: https://github.com/tianocore/edk2/pull/4180 Branch: https://github.com/mdkinney/edk2/tree/Bug_4389_UnitTestFrameworkPkg_AddGoogleMockSupport New in v2 == * SecurityPkg: Add unit test descriptions to SecureBootVariableLibGoogleTest * MdeModulePkg: Add unit test descriptions to UefiSortLibGoogleTest * UnitTestFrameworkPkg: Update ReadMe.md based on review findings * Fix FunctionMockLib example of the MockUefiLib declaration to align with the design code. * Fix FunctionMockLib Mocks header file location paths in the tables. * Fix FunctionMockLib Mocks example (and description) of the MockUefiLib declaration to align with the design code. * Fix FunctionMockLib Mocks example (and description) of the MockUefiLib INF file to align with the design file. * Fix typos in new Code Coverage section. V1 === * Add subhook submodule that is required to hook internal functions when using gmock. * Add gmock support to GoogleTestLib * Add FunctionMockLib library class and library instance * Add GoogleTest extension to GoogleTestLib.h for CHAR16 type * Add GoogleTest extension to GoogleTestLib.h for buffer types * Add gmock documentation * Add gmock examples * Fix VS20xx 4122 errors in SecurityPkg unit test * HOST_APPLICATION only supports IA32/X64 Cc: Michael Kubacki Cc: Nate DeSimone Cc: Jian J Wang Cc: Liming Gao Cc: Zhiguang Liu Cc: Jiewen Yao Cc: Michael D Kinney Cc: Sean Brogan Cc: Michael Kubacki Cc: Andrew Fish Cc: Leif Lindholm Signed-off-by: Chris Johnson Chris Johnson (7): UnitTestFrameworkPkg: Add subhook submodule required for gmock .pytool/CISettings.py: Add subhook submodule UnitTestFrameworkPkg: Add gmock support to GoogleTestLib UnitTestFrameworkPkg/ReadMe.md: Add gmock documentation MdePkg: Add gmock examples MdeModulePkg/Library/UefiSortLib: Add GoogleTestLib example SecurityPkg: Add gmock example Michael D Kinney (5): SecurityPkg/Library/SecureBootVariableLib: Fix VS20xx 4122 errors SecurityPkg/Library/SecureBootVariableLib: HOST_APPLICATION IA32/X64 only MdePkg/Library/BaseLib: HOST_APPLICATION IA32/X64 only MdeModulePkg: HOST_APPLICATION IA32/X64 only PrmPkg/Library: HOST_APPLICATION IA32/X64 only .gitmodules |3 + .pytool/CISettings.py |2 + .../MockUefiRuntimeServicesTableLib.inf |6 +- .../GoogleTest/UefiSortLibGoogleTest.cpp | 61 + .../GoogleTest/UefiSortLibGoogleTest.inf | 31 + MdeModulePkg/Test/MdeModulePkgHostTest.dsc|6 + .../VariableLockRequestToLockUnitTest.inf |2 +- .../Library/BaseLib/UnitTestHostBaseLib.inf |2 +- MdePkg/MdePkg.dec |1 + MdePkg/Test/MdePkgHostTest.dsc|2 + .../Include/GoogleTest/Library/MockUefiLib.h | 39 + .../Library/MockUefiRuntimeServicesTableLib.h | 42 + .../GoogleTest/MockUefiLib/MockUefiLib.cpp| 12 + .../GoogleTest/MockUefiLib/MockUefiLib.inf| 33 + .../MockUefiRuntimeServicesTableLib.cpp | 40 + .../MockUefiRuntimeServicesTableLib.inf | 33 + .../DxePrmContextBufferLibUnitTestHost.inf|2 +- .../DxePrmModuleDiscoveryLibUnitTestHost.inf |2 +- ReadMe.rst|1 + .../SecureBootVariableLibGoogleTest.cpp | 174 +++ .../SecureBootVariableLibGoogleTest.inf | 32 + .../UnitTest/MockPlatformPKProtectionLib.inf |6 +- .../UnitTest/MockUefiLib.inf |6 +- .../MockUefiRuntimeServicesTableLib.inf |6 +- .../UnitTest/SecureBootVariableLibUnitTest.c | 172 ++- SecurityPkg/SecurityPkg.dec |1 + .../Library/MockPlatformPKProtectionLib.h | 28 + .../MockPlatformPKProtectionLib.cpp | 11 + .../MockPlatformPKProtectionLib.inf | 34 + SecurityPkg/Test/SecurityPkgHostTest.dsc |8 + .../Include/Library/FunctionMockLib.h | 131 +++ .../Include/Library/GoogleTestLib.h | 96 ++ .../Include/Library/SubhookLib.h | 15 + .../Library/CmockaLib/CmockaLib.inf |2 +- .../Library/FunctionMockLib/FunctionMockLib.c |7 + .../FunctionMockLib/FunctionMockLib.inf | 31 + .../FunctionMockLib/FunctionMockLib.uni | 11 + .../Library/GoogleTestLib/GoogleTestLib.inf |6 +- .../Library/GoogleTestLib/GoogleTestLib.uni |3 - .../SubhookLib.inf} | 16 +- .../Library/SubhookLib/SubhookLib.uni | 11 + .../Library/SubhookLib/subhook|1
Re: [edk2-devel] [PATCH v2 17/17] MdeModulePkg: Enable forward edge CFI in mem attributes table
Turns out my old email was getting sent to a lot of folks spam, so
resending with hopefully a better email...
On 3/27/2023 4:01 AM, Ard Biesheuvel wrote:
The memory attributes table has been extended with a flag that indicates
whether or not the OS is permitted to map the EFI runtime code regions
with strict enforcement for IBT/BTI landing pad instructions.
Given that the PE/COFF spec now defines a DllCharacteristicsEx flag that
indicates whether or not a loaded image is compatible with this, we can
wire this up to the flag in the memory attributes table, and set it if
all loaded runtime image are compatible with it.
Signed-off-by: Ard Biesheuvel
---
MdeModulePkg/Core/Dxe/DxeMain.h| 2 ++
MdeModulePkg/Core/Dxe/Image/Image.c| 10 ++
MdeModulePkg/Core/Dxe/Misc/MemoryAttributesTable.c | 8 +++-
3 files changed, 19 insertions(+), 1 deletion(-)
diff --git a/MdeModulePkg/Core/Dxe/DxeMain.h b/MdeModulePkg/Core/Dxe/DxeMain.h
index 815a6b4bd844a452..43daa037be441150 100644
--- a/MdeModulePkg/Core/Dxe/DxeMain.h
+++ b/MdeModulePkg/Core/Dxe/DxeMain.h
@@ -280,6 +280,8 @@ extern EFI_MEMORY_TYPE_INFORMATION
gMemoryTypeInformation[EfiMaxMemoryType + 1]
extern BOOLEANgDispatcherRunning;
extern EFI_RUNTIME_ARCH_PROTOCOL gRuntimeTemplate;
+extern BOOLEAN gMemoryAttributesTableForwardCfi;
+
extern EFI_LOAD_FIXED_ADDRESS_CONFIGURATION_TABLE
gLoadModuleAtFixAddressConfigurationTable;
extern BOOLEAN
gLoadFixedAddressCodeMemoryReady;
//
diff --git a/MdeModulePkg/Core/Dxe/Image/Image.c
b/MdeModulePkg/Core/Dxe/Image/Image.c
index 8704ebea9a7c88c0..9dbfb2a1fad22ced 100644
--- a/MdeModulePkg/Core/Dxe/Image/Image.c
+++ b/MdeModulePkg/Core/Dxe/Image/Image.c
@@ -1399,6 +1399,16 @@ CoreLoadImageCommon (
CoreNewDebugImageInfoEntry (EFI_DEBUG_IMAGE_INFO_TYPE_NORMAL, >Info,
Image->Handle);
}
+ //
+ // Check whether we are loading a runtime image that lacks support for
+ // IBT/BTI landing pads.
+ //
+ if ((Image->ImageContext.ImageCodeMemoryType == EfiRuntimeServicesCode) &&
+ ((Image->ImageContext.DllCharacteristicsEx &
EFI_IMAGE_DLLCHARACTERISTICS_EX_FORWARD_CFI_COMPAT) == 0))
+ {
+gMemoryAttributesTableForwardCfi = FALSE;
+ }
If I understand this correctly, we are disabling Forward CFI if we
attempt to load any runtime images that don't support it. Would it make
sense to have a PCD to determine whether we strictly enforce
Forward CFI (i.e. don't load this incompatible image) in such a case? We
have a similar option for non-NX_COMPAT images.
Thanks,
Oliver
+
//
// Reinstall loaded image protocol to fire any notifications
//
diff --git a/MdeModulePkg/Core/Dxe/Misc/MemoryAttributesTable.c
b/MdeModulePkg/Core/Dxe/Misc/MemoryAttributesTable.c
index e079213711875f89..fd127ee167e1ac9a 100644
--- a/MdeModulePkg/Core/Dxe/Misc/MemoryAttributesTable.c
+++ b/MdeModulePkg/Core/Dxe/Misc/MemoryAttributesTable.c
@@ -89,6 +89,7 @@ BOOLEAN mMemoryAttributesTableEnable
= TRUE;
BOOLEAN mMemoryAttributesTableEndOfDxe= FALSE;
EFI_MEMORY_ATTRIBUTES_TABLE *mMemoryAttributesTable = NULL;
BOOLEAN mMemoryAttributesTableReadyToBoot = FALSE;
+BOOLEAN gMemoryAttributesTableForwardCfi = TRUE;
/**
Install MemoryAttributesTable.
@@ -182,7 +183,12 @@ InstallMemoryAttributesTable (
MemoryAttributesTable->Version =
EFI_MEMORY_ATTRIBUTES_TABLE_VERSION;
MemoryAttributesTable->NumberOfEntries = RuntimeEntryCount;
MemoryAttributesTable->DescriptorSize = (UINT32)DescriptorSize;
- MemoryAttributesTable->Reserved= 0;
+ if (gMemoryAttributesTableForwardCfi) {
+MemoryAttributesTable->Flags =
EFI_MEMORY_ATTRIBUTES_FLAGS_RT_FORWARD_CONTROL_FLOW_GUARD;
+ } else {
+MemoryAttributesTable->Flags = 0;
+ }
+
DEBUG ((DEBUG_VERBOSE, "MemoryAttributesTable:\n"));
DEBUG ((DEBUG_VERBOSE, " Version - 0x%08x\n",
MemoryAttributesTable->Version));
DEBUG ((DEBUG_VERBOSE, " NumberOfEntries - 0x%08x\n",
MemoryAttributesTable->NumberOfEntries));
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#102433): https://edk2.groups.io/g/devel/message/102433
Mute This Topic: https://groups.io/mt/97879305/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] [PATCH v7 09/12] ShellPkg: Fix conditionally uninitialized variables
On 3/28/2023 12:24 PM, Michael Kubacki wrote:
Hi Oliver,
Thanks for taking time to look through the series. I responded inline.
Thanks,
Michael
On 3/28/2023 2:49 PM, Oliver Smith-Denny wrote:
A couple comments below, thanks!
On 3/24/2023 3:30 PM, Michael Kubacki wrote:
From: Michael Kubacki
Fixes CodeQL alerts for CWE-457:
https://cwe.mitre.org/data/definitions/457.html
Cc: Erich McMillan
Cc: Michael D Kinney
Cc: Michael Kubacki
Cc: Ray Ni
Cc: Zhichao Gao
Co-authored-by: Erich McMillan
Signed-off-by: Michael Kubacki
Reviewed-by: Zhichao Gao
---
ShellPkg/Application/Shell/Shell.c | 1 +
ShellPkg/Application/Shell/ShellProtocol.c | 60
++--
ShellPkg/Library/UefiShellCommandLib/UefiShellCommandLib.c | 56
+-
ShellPkg/Library/UefiShellDebug1CommandsLib/Dblk.c | 18
+++---
ShellPkg/Library/UefiShellDebug1CommandsLib/EfiDecompress.c | 9 ++-
ShellPkg/Library/UefiShellDriver1CommandsLib/Connect.c | 14 +++--
ShellPkg/Library/UefiShellDriver1CommandsLib/Disconnect.c | 17
--
ShellPkg/Library/UefiShellDriver1CommandsLib/DrvDiag.c | 21
+++
8 files changed, 107 insertions(+), 89 deletions(-)
diff --git a/ShellPkg/Application/Shell/Shell.c
b/ShellPkg/Application/Shell/Shell.c
index 0ae6e14a34bf..f95c799bb2a4 100644
--- a/ShellPkg/Application/Shell/Shell.c
+++ b/ShellPkg/Application/Shell/Shell.c
@@ -1300,6 +1300,7 @@ DoStartupScript (
CHAR16 *FullFileStringPath;
UINTN NewSize;
+ CalleeStatus = EFI_SUCCESS;
Key.UnicodeChar = CHAR_NULL;
Key.ScanCode = 0;
diff --git a/ShellPkg/Application/Shell/ShellProtocol.c
b/ShellPkg/Application/Shell/ShellProtocol.c
index e6d20ab16479..da8c31cb038a 100644
--- a/ShellPkg/Application/Shell/ShellProtocol.c
+++ b/ShellPkg/Application/Shell/ShellProtocol.c
@@ -735,50 +735,52 @@ EfiShellGetDeviceName (
//
// Now check the parent controller using this as the child.
//
- if (DeviceNameToReturn == NULL) {
- PARSE_HANDLE_DATABASE_PARENTS (DeviceHandle,
, );
+ Status = PARSE_HANDLE_DATABASE_PARENTS (DeviceHandle,
, );
+ if ((DeviceNameToReturn == NULL) && !EFI_ERROR (Status)) {
for (LoopVar = 0; LoopVar < ParentControllerCount; LoopVar++) {
- PARSE_HANDLE_DATABASE_UEFI_DRIVERS
(ParentControllerBuffer[LoopVar], ,
);
- for (HandleCount = 0; HandleCount < ParentDriverCount;
HandleCount++) {
- //
- // try using that driver's component name with controller
and our driver as the child.
- //
- Status = gBS->OpenProtocol (
- ParentDriverBuffer[HandleCount],
- ,
- (VOID **),
- gImageHandle,
- NULL,
- EFI_OPEN_PROTOCOL_GET_PROTOCOL
- );
- if (EFI_ERROR (Status)) {
+ Status = PARSE_HANDLE_DATABASE_UEFI_DRIVERS
(ParentControllerBuffer[LoopVar], ,
);
+ if (!EFI_ERROR (Status)) {
+ for (HandleCount = 0; HandleCount < ParentDriverCount;
HandleCount++) {
+ //
+ // try using that driver's component name with
controller and our driver as the child.
+ //
Status = gBS->OpenProtocol (
ParentDriverBuffer[HandleCount],
- ,
+ ,
(VOID **),
gImageHandle,
NULL,
EFI_OPEN_PROTOCOL_GET_PROTOCOL
);
- }
+ if (EFI_ERROR (Status)) {
+ Status = gBS->OpenProtocol (
+ ParentDriverBuffer[HandleCount],
+ ,
+ (VOID **),
+ gImageHandle,
+ NULL,
+ EFI_OPEN_PROTOCOL_GET_PROTOCOL
+ );
+ }
+
+ if (EFI_ERROR (Status)) {
+ continue;
+ }
- if (EFI_ERROR (Status)) {
- continue;
+ Lang = GetBestLanguageForDriver
(CompName2->SupportedLanguages, Language, FALSE);
+ Status = CompName2->GetControllerName (CompName2,
ParentControllerBuffer[LoopVar], DeviceHandle, Lang,
);
+ FreePool (Lang);
+ Lang = NULL;
+ if (!EFI_ERROR (Status) && (DeviceNameToReturn != NULL)) {
+ break;
+ }
}
- Lang = GetBestLanguageForDriver
(CompName2->SupportedLanguages, Language, FALSE);
- Status = CompName2->GetControllerName (CompName2,
ParentControllerBuffer[LoopVar], DeviceHandle, Lang,
);
- FreePool (Lang);
-
Re: [edk2-devel] [PATCH v2 00/17] Enable BTI support in memory attributes table
For the patchset: Reviewed-by: Oliver Smith-Denny Thanks! On 3/27/2023 4:00 AM, Ard Biesheuvel wrote: Implement version 2 of the memory attributes table, which now contains a flag informing the OS whether or not code regions may be mapped with CFI mitigations such as IBT or BTI enabled. This series covers roughly the following parts: - (AARCH64) Annotate ELF objects generated from asm as BTI compatible when BTI codegen is enabled - Update the BaseTools to emit the appropriate PE/COFF annotation when a BTI/IBT compatible ELF executable is converted to PE/COFF - Take this PE/COFF annotation into account when populating the memory attributes table in the DXE core TODO: - X64 changes to make the code IBT compatible and emit the ELF note - Figure out how to generate such executables with native PE toolchains - Implement BTI/IBT enforcement at boot time - this is something I intend to look into next. Can be tested with the CLANG38 toolchain (both Clang compiler and LLD linker, version 3.8 or newer) with the following build options. [BuildOptions] GCC:*_*_AARCH64_PP_FLAGS = -mbranch-protection=bti GCC:*_*_AARCH64_CC_FLAGS = -mbranch-protection=bti GCC:*_*_AARCH64_DLINK_FLAGS = -fuse-ld=lld -Wl,--no-relax,--no-pie,-z,bti-report=error Cc: Michael Kinney Cc: Liming Gao Cc: Jiewen Yao Cc: Michael Kubacki Cc: Sean Brogan Cc: Rebecca Cran Cc: Leif Lindholm Cc: Sami Mujawar Cc: Taylor Beebe Cc: Marvin Häuser Cc: Bob Feng Ard Biesheuvel (17): MdePkg/ProcessorBind AARCH64: Add asm macro to emit GNU BTI note MdePkg/BaseCpuLib AARCH64: Make asm files BTI compatible MdePkg/BaseIoLibIntrinsic AARCH64: Make asm files BTI compatible MdePkg/BaseLib AARCH64: Make LongJump() BTI compatible MdePkg/BaseLib AARCH64: Make asm files BTI compatible MdePkg/BaseMemoryLibOptDxe AARCH64: Make asm files BTI compatible MdePkg/BaseSynchronizationLib AARCH64: Make asm files BTI compatible MdePkg/BaseRngLib AARCH64: Make asm files BTI compatible ArmPkg: Emit BTI opcodes when BTI codegen is enabled ArmPkg/GccLto AARCH64: Add BTI note to LTO helper library ArmPkg, BaseTools AARCH64: Add BTI ELF note to .hii objects ArmPlatformPkg/PrePeiCore: Make vector table object BTI compatible BaseTools/GenFw: Parse IBT/BTI support status from ELF note BaseTools/GenFw: Add DllCharacteristicsEx field to debug data MdePkg: Update MemoryAttributesTable to v2.10 MdePkg/PeCoffLib: Capture DLL characteristics fieldis in image context MdeModulePkg: Enable forward edge CFI in mem attributes table ArmPkg/Include/AsmMacroIoLibV8.h| 3 +- ArmPkg/Library/ArmExceptionLib/AArch64/ExceptionSupport.S | 3 +- ArmPkg/Library/ArmSvcLib/AArch64/ArmSvc.S | 4 +- ArmPkg/Library/GccLto/liblto-aarch64.a | Bin 1016 -> 1128 bytes ArmPkg/Library/GnuNoteBti.bin | Bin 0 -> 32 bytes ArmPlatformPkg/PrePeiCore/AArch64/Exception.S | 2 + ArmVirtPkg/Library/ArmPlatformLibQemu/IdMap.S | 2 + BaseTools/Conf/tools_def.template | 4 +- BaseTools/Source/C/GenFw/Elf64Convert.c | 104 +--- BaseTools/Source/C/GenFw/GenFw.c| 3 +- BaseTools/Source/C/GenFw/elf_common.h | 9 ++ BaseTools/Source/C/Include/IndustryStandard/PeImage.h | 13 ++- MdeModulePkg/Core/Dxe/DxeMain.h | 2 + MdeModulePkg/Core/Dxe/Image/Image.c | 10 ++ MdeModulePkg/Core/Dxe/Misc/MemoryAttributesTable.c | 8 +- MdePkg/Include/AArch64/ProcessorBind.h | 31 ++ MdePkg/Include/Guid/MemoryAttributesTable.h | 8 +- MdePkg/Include/IndustryStandard/PeImage.h | 13 ++- MdePkg/Include/Library/PeCoffLib.h | 6 ++ MdePkg/Library/BaseCpuLib/AArch64/CpuFlushTlb.S | 1 + MdePkg/Library/BaseCpuLib/AArch64/CpuSleep.S| 1 + MdePkg/Library/BaseIoLibIntrinsic/AArch64/ArmVirtMmio.S | 8 ++ MdePkg/Library/BaseLib/AArch64/CpuBreakpoint.S | 1 + MdePkg/Library/BaseLib/AArch64/DisableInterrupts.S | 1 + MdePkg/Library/BaseLib/AArch64/EnableInterrupts.S | 1 + MdePkg/Library/BaseLib/AArch64/GetInterruptsState.S | 1 + MdePkg/Library/BaseLib/AArch64/MemoryFence.S| 1 + MdePkg/Library/BaseLib/AArch64/SetJumpLongJump.S| 5 +- MdePkg/Library/BaseLib/AArch64/SpeculationBarrier.S | 1 + MdePkg/Library/BaseLib/AArch64/SwitchStack.S| 2 + MdePkg/Library/BaseMemoryLib
Re: [edk2-devel] [PATCH v2 17/17] MdeModulePkg: Enable forward edge CFI in mem attributes table
On 3/27/2023 4:01 AM, Ard Biesheuvel wrote:
The memory attributes table has been extended with a flag that indicates
whether or not the OS is permitted to map the EFI runtime code regions
with strict enforcement for IBT/BTI landing pad instructions.
Given that the PE/COFF spec now defines a DllCharacteristicsEx flag that
indicates whether or not a loaded image is compatible with this, we can
wire this up to the flag in the memory attributes table, and set it if
all loaded runtime image are compatible with it.
Signed-off-by: Ard Biesheuvel
---
MdeModulePkg/Core/Dxe/DxeMain.h| 2 ++
MdeModulePkg/Core/Dxe/Image/Image.c| 10 ++
MdeModulePkg/Core/Dxe/Misc/MemoryAttributesTable.c | 8 +++-
3 files changed, 19 insertions(+), 1 deletion(-)
diff --git a/MdeModulePkg/Core/Dxe/DxeMain.h b/MdeModulePkg/Core/Dxe/DxeMain.h
index 815a6b4bd844a452..43daa037be441150 100644
--- a/MdeModulePkg/Core/Dxe/DxeMain.h
+++ b/MdeModulePkg/Core/Dxe/DxeMain.h
@@ -280,6 +280,8 @@ extern EFI_MEMORY_TYPE_INFORMATION
gMemoryTypeInformation[EfiMaxMemoryType + 1]
extern BOOLEANgDispatcherRunning;
extern EFI_RUNTIME_ARCH_PROTOCOL gRuntimeTemplate;
+extern BOOLEAN gMemoryAttributesTableForwardCfi;
+
extern EFI_LOAD_FIXED_ADDRESS_CONFIGURATION_TABLE
gLoadModuleAtFixAddressConfigurationTable;
extern BOOLEAN
gLoadFixedAddressCodeMemoryReady;
//
diff --git a/MdeModulePkg/Core/Dxe/Image/Image.c
b/MdeModulePkg/Core/Dxe/Image/Image.c
index 8704ebea9a7c88c0..9dbfb2a1fad22ced 100644
--- a/MdeModulePkg/Core/Dxe/Image/Image.c
+++ b/MdeModulePkg/Core/Dxe/Image/Image.c
@@ -1399,6 +1399,16 @@ CoreLoadImageCommon (
CoreNewDebugImageInfoEntry (EFI_DEBUG_IMAGE_INFO_TYPE_NORMAL, >Info,
Image->Handle);
}
+ //
+ // Check whether we are loading a runtime image that lacks support for
+ // IBT/BTI landing pads.
+ //
+ if ((Image->ImageContext.ImageCodeMemoryType == EfiRuntimeServicesCode) &&
+ ((Image->ImageContext.DllCharacteristicsEx &
EFI_IMAGE_DLLCHARACTERISTICS_EX_FORWARD_CFI_COMPAT) == 0))
+ {
+gMemoryAttributesTableForwardCfi = FALSE;
+ }
If I understand this correctly, we are disabling Forward CFI if we attempt
to load any runtime images that don't support it. Would it make
sense to have a PCD to determine whether we strictly enforce
Forward CFI (i.e. don't load this incompatible image) in such a case?
Thanks,
Oliver
+
//
// Reinstall loaded image protocol to fire any notifications
//
diff --git a/MdeModulePkg/Core/Dxe/Misc/MemoryAttributesTable.c
b/MdeModulePkg/Core/Dxe/Misc/MemoryAttributesTable.c
index e079213711875f89..fd127ee167e1ac9a 100644
--- a/MdeModulePkg/Core/Dxe/Misc/MemoryAttributesTable.c
+++ b/MdeModulePkg/Core/Dxe/Misc/MemoryAttributesTable.c
@@ -89,6 +89,7 @@ BOOLEAN mMemoryAttributesTableEnable
= TRUE;
BOOLEAN mMemoryAttributesTableEndOfDxe= FALSE;
EFI_MEMORY_ATTRIBUTES_TABLE *mMemoryAttributesTable = NULL;
BOOLEAN mMemoryAttributesTableReadyToBoot = FALSE;
+BOOLEAN gMemoryAttributesTableForwardCfi = TRUE;
/**
Install MemoryAttributesTable.
@@ -182,7 +183,12 @@ InstallMemoryAttributesTable (
MemoryAttributesTable->Version =
EFI_MEMORY_ATTRIBUTES_TABLE_VERSION;
MemoryAttributesTable->NumberOfEntries = RuntimeEntryCount;
MemoryAttributesTable->DescriptorSize = (UINT32)DescriptorSize;
- MemoryAttributesTable->Reserved= 0;
+ if (gMemoryAttributesTableForwardCfi) {
+MemoryAttributesTable->Flags =
EFI_MEMORY_ATTRIBUTES_FLAGS_RT_FORWARD_CONTROL_FLOW_GUARD;
+ } else {
+MemoryAttributesTable->Flags = 0;
+ }
+
DEBUG ((DEBUG_VERBOSE, "MemoryAttributesTable:\n"));
DEBUG ((DEBUG_VERBOSE, " Version - 0x%08x\n",
MemoryAttributesTable->Version));
DEBUG ((DEBUG_VERBOSE, " NumberOfEntries - 0x%08x\n",
MemoryAttributesTable->NumberOfEntries));
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#102072): https://edk2.groups.io/g/devel/message/102072
Mute This Topic: https://groups.io/mt/97879305/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] [PATCH v7 00/12] Enable New CodeQL Queries
With comments and for the patchset:
Reviewed-by: Oliver Smith-Denny
Thanks!
On 3/24/2023 3:30 PM, Michael Kubacki wrote:
From: Michael Kubacki
Adds queries for the following:
1. cpp/conditionallyuninitializedvariable
2. cpp/pointer-overflow-check
3. cpp/overrunning-write
4. cpp/overrunning-write-with-float
5. cpp/very-likely-overrunning-write
These check for vulnerabilities with the following CWEs:
- https://cwe.mitre.org/data/definitions/120.html
- https://cwe.mitre.org/data/definitions/457.html
- https://cwe.mitre.org/data/definitions/676.html
- https://cwe.mitre.org/data/definitions/758.html
- https://cwe.mitre.org/data/definitions/787.html
- https://cwe.mitre.org/data/definitions/805.html
The first part of this patch series contains fixes for CodeQL alerts
across various packages that are produced by the new queries being
enabled.
The second part updates the CodeQL queries.
Note: The changes are currently in the following pull request
https://github.com/tianocore/edk2/pull/4133
v7 series changes:
1. Added R-b tag to UefiCpuPkg patch
2. Merged Rebecca's patch https://edk2.groups.io/g/devel/message/101819
into [PATCH v7 02/12]
v6 series changes:
1. Also change "1u" to "1" in:
- UefiCpuPkg/CpuMpPei/CpuPaging.c
- UefiCpuPkg/CpuMpPei/CpuMpPei.c
v5 series changes:
1. Changed "1u" to "1" in UefiCpuPkg/CpuMpPei/CpuBist.c
2. Added Rb tags from v4 series
v4 series changes:
1. Simplify conditional logic in Patch 1 per Michael Brown's
suggestion.
v3 series changes:
1. Rebased series onto 93a21b4 (current edk2/master)
2. Added v2 Rb tags
V2 series changes:
1. MdeModulePkg/Universal/SmbiosDxe/SmbiosDxe.c
- Applied SafeUintnAdd() to both variables in the comparison
in ParseAndAddExistingSmbiosTable()
Addresses feedback from: Mike Kinney
2. CryptoPkg/Library/BaseCryptLib/Pk/CryptX509.c
- Changes:
if (!(Inf & 0x80) && (Asn1Tag != V_ASN1_SEQUENCE)) {
To:
if (((Inf & 0x80) == 0x00) && (Asn1Tag != V_ASN1_SEQUENCE)) {
Addresses feedback from: Mike Kinney
3. MdePkg/Library/BaseLib/String.c
- Removes: #include
- Changes conditional style in changes to if statement from
ternary for changes made throughout the file
- Updates commit message to describe change in return value
Addresses feedback from: Mike Kinney
4. NetworkPkg/Library/DxeHttpLib/DxeHttpLib.c
- Changes:
if (!EFI_ERROR (Status) && (Data > HTTP_URI_PORT_MAX_NUM)) {
Status = EFI_INVALID_PARAMETER;
goto ON_EXIT;
}
To:
if (EFI_ERROR (Status) || (Data > HTTP_URI_PORT_MAX_NUM)) {
Status = EFI_INVALID_PARAMETER;
goto ON_EXIT;
}
Addresses feedback from: Mike Kinney
5. ShellPkg/Application/Shell/Shell.c
- Initializes CalleeStatus to EFI_SUCCESS in DoStartupScript()
- Restores original if statement logic in DoStartupScript()
Addresses feedback from: Zhichao Gao
6. ShellPkg/Application/Shell/ShellProtocol.c
- Adds additional check for return value from
PARSE_HANDLE_DATABASE_UEFI_DRIVERS() in EfiShellGetDeviceName()
Addresses feedback from: Zhichao Gao
7. Includes up-to-date R-b tags
---
Cc: Bob Feng
Cc: Dandan Bi
Cc: Eric Dong
Cc: Erich McMillan
Cc: Guomin Jiang
Cc: Jian J Wang
Cc: Jiaxin Wu
Cc: Jiewen Yao
Cc: Liming Gao
Cc: Maciej Rabeda
Cc: Michael Brown
Cc: Michael D Kinney
Cc: Michael Kubacki
Cc: Rahul Kumar
Cc: Ray Ni
Cc: Sean Brogan
Cc: Siyuan Fu
Cc: Star Zeng
Cc: Xiaoyu Lu
Cc: Yuwei Chen
Cc: Zhichao Gao
Cc: Zhiguang Liu
Signed-off-by: Michael Kubacki
Erich McMillan (1):
MdeModulePkg/SmbiosDxe: Fix pointer and buffer overflow CodeQL alerts
Michael Kubacki (11):
BaseTools/PatchCheck.py: Add PCCTS to tab exemption list
BaseTools/VfrCompile: Fix potential buffer overwrites
CryptoPkg: Fix conditionally uninitialized variable
MdeModulePkg: Fix conditionally uninitialized variables
MdePkg: Fix conditionally uninitialized variables
NetworkPkg: Fix conditionally uninitialized variables
PcAtChipsetPkg: Fix conditionally uninitialized variables
ShellPkg: Fix conditionally uninitialized variables
UefiCpuPkg: Fix conditionally uninitialized variables
.github/codeql/edk2.qls: Enable CWE 457, 676, and 758 queries
.github/codeql/edk2.qls: Enable CWE 120, 787, and 805 queries
BaseTools/Source/C/VfrCompile/Pccts/antlr/gen.c | 10 ++--
BaseTools/Source/C/VfrCompile/Pccts/antlr/main.c | 4 +-
CryptoPkg/Library/BaseCryptLib/Pk/CryptX509.c | 21 ---
MdeModulePkg/Bus/Pci/PciBusDxe/PciIo.c| 5 +-
MdeModulePkg/Bus/Pci/UhciDxe/Uhci.c | 24 +---
MdeModulePkg/Core/Dxe/Mem/Page.c | 17 +++---
MdeModulePkg/Library/BootMaintenanceManagerUiLib/BootOption.c | 25
MdeMod
Re: [edk2-devel] [PATCH v7 09/12] ShellPkg: Fix conditionally uninitialized variables
A couple comments below, thanks!
On 3/24/2023 3:30 PM, Michael Kubacki wrote:
From: Michael Kubacki
Fixes CodeQL alerts for CWE-457:
https://cwe.mitre.org/data/definitions/457.html
Cc: Erich McMillan
Cc: Michael D Kinney
Cc: Michael Kubacki
Cc: Ray Ni
Cc: Zhichao Gao
Co-authored-by: Erich McMillan
Signed-off-by: Michael Kubacki
Reviewed-by: Zhichao Gao
---
ShellPkg/Application/Shell/Shell.c | 1 +
ShellPkg/Application/Shell/ShellProtocol.c | 60
++--
ShellPkg/Library/UefiShellCommandLib/UefiShellCommandLib.c | 56
+-
ShellPkg/Library/UefiShellDebug1CommandsLib/Dblk.c | 18 +++---
ShellPkg/Library/UefiShellDebug1CommandsLib/EfiDecompress.c | 9 ++-
ShellPkg/Library/UefiShellDriver1CommandsLib/Connect.c | 14 +++--
ShellPkg/Library/UefiShellDriver1CommandsLib/Disconnect.c | 17 --
ShellPkg/Library/UefiShellDriver1CommandsLib/DrvDiag.c | 21 +++
8 files changed, 107 insertions(+), 89 deletions(-)
diff --git a/ShellPkg/Application/Shell/Shell.c
b/ShellPkg/Application/Shell/Shell.c
index 0ae6e14a34bf..f95c799bb2a4 100644
--- a/ShellPkg/Application/Shell/Shell.c
+++ b/ShellPkg/Application/Shell/Shell.c
@@ -1300,6 +1300,7 @@ DoStartupScript (
CHAR16 *FullFileStringPath;
UINTN NewSize;
+ CalleeStatus= EFI_SUCCESS;
Key.UnicodeChar = CHAR_NULL;
Key.ScanCode= 0;
diff --git a/ShellPkg/Application/Shell/ShellProtocol.c b/ShellPkg/Application/Shell/ShellProtocol.c
index e6d20ab16479..da8c31cb038a 100644
--- a/ShellPkg/Application/Shell/ShellProtocol.c
+++ b/ShellPkg/Application/Shell/ShellProtocol.c
@@ -735,50 +735,52 @@ EfiShellGetDeviceName (
//
// Now check the parent controller using this as the child.
//
-if (DeviceNameToReturn == NULL) {
- PARSE_HANDLE_DATABASE_PARENTS (DeviceHandle, ,
);
+Status = PARSE_HANDLE_DATABASE_PARENTS (DeviceHandle, ,
);
+if ((DeviceNameToReturn == NULL) && !EFI_ERROR (Status)) {
for (LoopVar = 0; LoopVar < ParentControllerCount; LoopVar++) {
-PARSE_HANDLE_DATABASE_UEFI_DRIVERS (ParentControllerBuffer[LoopVar],
, );
-for (HandleCount = 0; HandleCount < ParentDriverCount; HandleCount++) {
- //
- // try using that driver's component name with controller and our
driver as the child.
- //
- Status = gBS->OpenProtocol (
- ParentDriverBuffer[HandleCount],
- ,
- (VOID **),
- gImageHandle,
- NULL,
- EFI_OPEN_PROTOCOL_GET_PROTOCOL
- );
- if (EFI_ERROR (Status)) {
+Status = PARSE_HANDLE_DATABASE_UEFI_DRIVERS (ParentControllerBuffer[LoopVar],
, );
+if (!EFI_ERROR (Status)) {
+ for (HandleCount = 0; HandleCount < ParentDriverCount;
HandleCount++) {
+//
+// try using that driver's component name with controller and our
driver as the child.
+//
Status = gBS->OpenProtocol (
ParentDriverBuffer[HandleCount],
-,
+,
(VOID **),
gImageHandle,
NULL,
EFI_OPEN_PROTOCOL_GET_PROTOCOL
);
- }
+if (EFI_ERROR (Status)) {
+ Status = gBS->OpenProtocol (
+ ParentDriverBuffer[HandleCount],
+ ,
+ (VOID **),
+ gImageHandle,
+ NULL,
+ EFI_OPEN_PROTOCOL_GET_PROTOCOL
+ );
+}
+
+if (EFI_ERROR (Status)) {
+ continue;
+}
- if (EFI_ERROR (Status)) {
-continue;
+Lang = GetBestLanguageForDriver (CompName2->SupportedLanguages,
Language, FALSE);
+Status = CompName2->GetControllerName (CompName2,
ParentControllerBuffer[LoopVar], DeviceHandle, Lang, );
+FreePool (Lang);
+Lang = NULL;
+if (!EFI_ERROR (Status) && (DeviceNameToReturn != NULL)) {
+ break;
+}
}
- Lang = GetBestLanguageForDriver (CompName2->SupportedLanguages, Language, FALSE);
- Status = CompName2->GetControllerName (CompName2,
ParentControllerBuffer[LoopVar], DeviceHandle, Lang, );
- FreePool (Lang);
- Lang = NULL;
+ SHELL_FREE_NON_NULL (ParentDriverBuffer);
if (!EFI_ERROR (Status) && (DeviceNameToReturn != NULL)) {
break;
}
}
-
-SHELL_FREE_NON_NULL (ParentDriverBuffer);
-if (!EFI_ERROR
Re: [edk2-devel] [PATCH 0/9] BaseTools,CryptoPkg,MdePkg,OvmfPkg: Delete CLANG35,CLANG38 and update CLANGDWARF, delete VS 2008-2013, EBC
Thanks for the cleanup! Reviewed-by: Oliver Smith-Denny On 3/21/2023 6:30 PM, Rebecca Cran wrote: Update the toolchain definitions: - Delete the CLANG35 and CLANG38 toolchains, and replace CLANG38 with CLANGDWARF, updating it to support ARM and AARCH64 in addition to X64 and IA32. - Remove VS2008, VS2010, VS2012 and VS2013. - Remove EBC compiler definitions. Full removal of EBC support from the various packages etc. will be done in a follow-up patch series. - Remove unused IPHONE_TOOLS and SOURCERY_CYGWIN_TOOLS definitions. Personal GitHub PR: https://github.com/tianocore/edk2/pull/4158 GitHub branch: https://github.com/bcran/edk2/tree/clangdwarf Rebecca Cran (9): OvmfPkg: Replace static struct initialization with ZeroMem call CryptoPkg: Add CLANGDWARF and remove CLANG35 and CLANG38 compiler flags BaseTools: Update CLANGDWARF toolchain and remove CLANG35 and CLANG38 BaseTools: Remove VS2008, 2010, 2012 and 2013 toolchain definitions BaseTools: Remove VS2008-VS2013 remnants MdePkg: Remove VS2008-VS2013 remnants edksetup.bat: Remove VS2008-VS2013 remnants BaseTools: Remove unused IPHONE_TOOLS and SOURCERY_CYGWIN_TOOLS defs BaseTools: Remove EBC (EFI Byte Code) compiler definitions CryptoPkg/Library/BaseCryptLib/BaseCryptLib.inf |3 +- CryptoPkg/Library/BaseCryptLib/PeiCryptLib.inf |3 +- CryptoPkg/Library/BaseCryptLib/RuntimeCryptLib.inf |3 +- CryptoPkg/Library/BaseCryptLib/SecCryptLib.inf |3 +- CryptoPkg/Library/BaseCryptLib/SmmCryptLib.inf |3 +- CryptoPkg/Library/BaseCryptLib/UnitTestHostBaseCryptLib.inf |3 +- CryptoPkg/Library/OpensslLib/OpensslLib.inf |3 +- CryptoPkg/Library/OpensslLib/OpensslLibAccel.inf|3 +- CryptoPkg/Library/OpensslLib/OpensslLibCrypto.inf |3 +- CryptoPkg/Library/OpensslLib/OpensslLibFull.inf |3 +- CryptoPkg/Library/OpensslLib/OpensslLibFullAccel.inf|3 +- MdePkg/Include/Ia32/ProcessorBind.h |8 +- MdePkg/Include/X64/ProcessorBind.h |8 +- OvmfPkg/Library/PlatformInitLib/MemDetect.c |4 +- BaseTools/Conf/tools_def.template | 1572 +++- BaseTools/Scripts/SetVisualStudio.bat | 22 +- BaseTools/Scripts/ShowEnvironment.bat | 44 - BaseTools/get_vsvars.bat| 13 - BaseTools/set_vsprefix_envs.bat | 64 - BaseTools/toolsetup.bat | 24 +- edksetup.bat|6 +- 21 files changed, 192 insertions(+), 1606 deletions(-) -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#101590): https://edk2.groups.io/g/devel/message/101590 Mute This Topic: https://groups.io/mt/97769541/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] [Patch 0/2] Add and use FirmwareVolumeShadowPpi
For the patchset: Reviewed-by: Oliver Smith-Denny Thanks! On 3/21/2023 7:06 PM, Michael D Kinney wrote: Add FirmwareVolumeShadow PPI to shadow an FV to memory. and use it to shadow FVs to memory. Cc: Jiewen Yao Cc: Jian J Wang Cc: Liming Gao Signed-off-by: Patel Umang Umang Patel (2): MdeModulePkg/Include/Ppi: Add FirmwareVolumeShadowPpi SecurityPkg/FvReportPei: Use FirmwareVolumeShadowPpi .../Include/Ppi/FirmwareVolumeShadowPpi.h | 61 +++ MdeModulePkg/MdeModulePkg.dec | 3 + SecurityPkg/FvReportPei/FvReportPei.c | 37 --- SecurityPkg/FvReportPei/FvReportPei.h | 1 + SecurityPkg/FvReportPei/FvReportPei.inf | 1 + 5 files changed, 96 insertions(+), 7 deletions(-) create mode 100644 MdeModulePkg/Include/Ppi/FirmwareVolumeShadowPpi.h -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#101587): https://edk2.groups.io/g/devel/message/101587 Mute This Topic: https://groups.io/mt/97770066/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] [Patch 2/2] SecurityPkg/FvReportPei: Use FirmwareVolumeShadowPpi
One comment below, thanks!
On 3/21/2023 7:06 PM, Michael D Kinney wrote:
From: Umang Patel
If FirmwareVolumeShadow PPI is available, then use it to
shadow FVs to memory. Otherwise fallback to CopyMem().
Cc: Jiewen Yao
Cc: Jian J Wang
Signed-off-by: Patel Umang
---
SecurityPkg/FvReportPei/FvReportPei.c | 37 -
SecurityPkg/FvReportPei/FvReportPei.h | 1 +
SecurityPkg/FvReportPei/FvReportPei.inf | 1 +
3 files changed, 32 insertions(+), 7 deletions(-)
diff --git a/SecurityPkg/FvReportPei/FvReportPei.c
b/SecurityPkg/FvReportPei/FvReportPei.c
index 846605cda1e4..6288dde16b2a 100644
--- a/SecurityPkg/FvReportPei/FvReportPei.c
+++ b/SecurityPkg/FvReportPei/FvReportPei.c
@@ -114,12 +114,13 @@ VerifyHashedFv (
IN EFI_BOOT_MODE BootMode
)
{
- UINTNFvIndex;
- CONST HASH_ALG_INFO *AlgInfo;
- UINT8*HashValue;
- UINT8*FvHashValue;
- VOID *FvBuffer;
- EFI_STATUS Status;
+ UINTN FvIndex;
+ CONST HASH_ALG_INFO *AlgInfo;
+ UINT8 *HashValue;
+ UINT8 *FvHashValue;
+ VOID *FvBuffer;
+ EDKII_PEI_FIRMWARE_VOLUME_SHADOW_PPI *FvShadowPpi;
+ EFI_STATUSStatus;
if ((HashInfo == NULL) ||
(HashInfo->HashSize == 0) ||
@@ -191,8 +192,30 @@ VerifyHashedFv (
// Copy FV to permanent memory to avoid potential TOC/TOU.
//
FvBuffer = AllocatePages (EFI_SIZE_TO_PAGES
((UINTN)FvInfo[FvIndex].Length));
+
ASSERT (FvBuffer != NULL);
While we are here, should we make this more robust (and amenable to
static analysis) and add error handling if FvBuffer is NULL, not just
assert?
-CopyMem (FvBuffer, (CONST VOID *)(UINTN)FvInfo[FvIndex].Base,
(UINTN)FvInfo[FvIndex].Length);
+Status = PeiServicesLocatePpi (
+ ,
+ 0,
+ NULL,
+ (VOID **)
+ );
+
+if (!EFI_ERROR (Status)) {
+ Status = FvShadowPpi->FirmwareVolumeShadow (
+ (EFI_PHYSICAL_ADDRESS)FvInfo[FvIndex].Base,
+ FvBuffer,
+ (UINTN)FvInfo[FvIndex].Length
+ );
+}
+
+if (EFI_ERROR (Status)) {
+ CopyMem (
+FvBuffer,
+(CONST VOID *)(UINTN)FvInfo[FvIndex].Base,
+(UINTN)FvInfo[FvIndex].Length
+);
+}
if (!AlgInfo->HashAll (FvBuffer, (UINTN)FvInfo[FvIndex].Length, FvHashValue)) {
Status = EFI_ABORTED;
diff --git a/SecurityPkg/FvReportPei/FvReportPei.h
b/SecurityPkg/FvReportPei/FvReportPei.h
index 92504a3c51e1..07ffb2f5768c 100644
--- a/SecurityPkg/FvReportPei/FvReportPei.h
+++ b/SecurityPkg/FvReportPei/FvReportPei.h
@@ -14,6 +14,7 @@ SPDX-License-Identifier: BSD-2-Clause-Patent
#include
#include
+#include
#include
#include
diff --git a/SecurityPkg/FvReportPei/FvReportPei.inf
b/SecurityPkg/FvReportPei/FvReportPei.inf
index 408406889765..4246fb75ebaa 100644
--- a/SecurityPkg/FvReportPei/FvReportPei.inf
+++ b/SecurityPkg/FvReportPei/FvReportPei.inf
@@ -46,6 +46,7 @@ [LibraryClasses]
[Ppis]
gEdkiiPeiFirmwareVolumeInfoPrehashedFvPpiGuid ## PRODUCES
gEdkiiPeiFirmwareVolumeInfoStoredHashFvPpiGuid ## CONSUMES
+ gEdkiiPeiFirmwareVolumeShadowPpiGuid## CONSUMES
[Pcd]
gEfiSecurityPkgTokenSpaceGuid.PcdStatusCodeFvVerificationPass
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#101586): https://edk2.groups.io/g/devel/message/101586
Mute This Topic: https://groups.io/mt/97770069/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] [PATCH v3 1/1] ArmPkg/SmbiosMiscDxe: Adjust the priority of getting firmware version
One nit below, otherwise:
Reviewed-by: Oliver Smith-Denny
Thanks!
On 3/20/2023 8:16 PM, Tinh Nguyen via groups.io wrote:
The BIOS Firmware Version in the SMBIOS Type 0 can be fetched from
the fixed PcdFirmwareVersionString or platform specific OemMiscLib.
In fact, the support from OemMiscLib comes into play when the firmware
version may be modified at boot time for extended information.
Therefore, the priority of getting the version from OemMiscLib should
be higher.
In case there is no modification in the OemMiscLib, we have to keep
HII string STR_MISC_BIOS_VERSION empty or 'Not Specified'
to indicate that the firmware version should be fetched from
the PcdFirmwareVersionString.
Signed-off-by: Tinh Nguyen
Reviewed-by: Rebecca Cran
---
Changes since v1:
+ Change GetBiosVersion () to SetBiosVersion () and move the selection logic
fully into SetBiosVersion ().
Changes since v2:
+ Add Reviewed-by: Rebecca Cran and remove @retval
VOID
ArmPkg/Universal/Smbios/SmbiosMiscDxe/Type00/MiscBiosVendorFunction.c | 57
1 file changed, 35 insertions(+), 22 deletions(-)
diff --git
a/ArmPkg/Universal/Smbios/SmbiosMiscDxe/Type00/MiscBiosVendorFunction.c
b/ArmPkg/Universal/Smbios/SmbiosMiscDxe/Type00/MiscBiosVendorFunction.c
index 66ead22a6e2c..876a74614285 100644
--- a/ArmPkg/Universal/Smbios/SmbiosMiscDxe/Type00/MiscBiosVendorFunction.c
+++ b/ArmPkg/Universal/Smbios/SmbiosMiscDxe/Type00/MiscBiosVendorFunction.c
@@ -1,6 +1,6 @@
/** @file
- Copyright (c) 2022, Ampere Computing LLC. All rights reserved.
+ Copyright (c) 2022 - 2023, Ampere Computing LLC. All rights reserved.
Copyright (c) 2021, NUVIA Inc. All rights reserved.
Copyright (c) 2009, Intel Corporation. All rights reserved.
Copyright (c) 2015, Hisilicon Limited. All rights reserved.
@@ -124,22 +124,46 @@ GetBiosReleaseDate (
return ReleaseDate;
}
-/**
- Fetches the firmware ('BIOS') version from the
- FirmwareVersionInfo HOB.
+/** Fetches the Firmware version string for SMBIOS type 0
+
+ This function get the Firmware version string from OemMiscLib first,
+ if it is invalid then PcdFirmwareVersionString is used as a fallback.
nit: this function comment seems a bit at odds with the name of the
function (i.e. the comment says it gets the FW version, but the name of
the function is SetBiosVersion, which I see was changed in this patch).
Perhaps updating the comment to indicate it retrieves the BIOS version
from OemMiscLib or PcdFirmwareVersionString and then sets it in SMBIOS.
- @return The version as a UTF-16 string
**/
-CHAR16 *
-GetBiosVersion (
+VOID
+SetBiosVersion (
VOID
)
{
- CHAR16 *ReleaseString;
+ CHAR16 *DefaultVersionString;
+ CHAR16 *Version;
+ EFI_STRING_ID TokenToUpdate;
- ReleaseString = (CHAR16 *)FixedPcdGetPtr (PcdFirmwareVersionString);
+ DefaultVersionString = HiiGetString (
+ mSmbiosMiscHiiHandle,
+ STRING_TOKEN (STR_MISC_BIOS_VERSION),
+ NULL
+ );
- return ReleaseString;
+ OemUpdateSmbiosInfo (
+mSmbiosMiscHiiHandle,
+STRING_TOKEN (STR_MISC_BIOS_VERSION),
+BiosVersionType00
+);
+
+ Version = HiiGetString (
+ mSmbiosMiscHiiHandle,
+ STRING_TOKEN (STR_MISC_BIOS_VERSION),
+ NULL
+ );
+
+ if (((StrCmp (Version, DefaultVersionString) == 0) || (StrLen (Version) ==
0))) {
+Version = (CHAR16 *)FixedPcdGetPtr (PcdFirmwareVersionString);
+if (StrLen (Version) > 0) {
+ TokenToUpdate = STRING_TOKEN (STR_MISC_BIOS_VERSION);
+ HiiSetString (mSmbiosMiscHiiHandle, TokenToUpdate, Version, NULL);
+}
+ }
}
/**
@@ -187,18 +211,7 @@ SMBIOS_MISC_TABLE_FUNCTION (MiscBiosVendor) {
HiiSetString (mSmbiosMiscHiiHandle, TokenToUpdate, Vendor, NULL);
}
- Version = GetBiosVersion ();
-
- if (StrLen (Version) > 0) {
-TokenToUpdate = STRING_TOKEN (STR_MISC_BIOS_VERSION);
-HiiSetString (mSmbiosMiscHiiHandle, TokenToUpdate, Version, NULL);
- } else {
-OemUpdateSmbiosInfo (
- mSmbiosMiscHiiHandle,
- STRING_TOKEN (STR_MISC_BIOS_VERSION),
- BiosVersionType00
- );
- }
+ SetBiosVersion ();
Char16String = GetBiosReleaseDate ();
if (StrLen (Char16String) > 0) {
--
2.39.2
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#101497): https://edk2.groups.io/g/devel/message/101497
Mute This Topic: https://groups.io/mt/97748102/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] [PATCH v5 35/38] MdeModulePkg/DxeCore: Clear NX permissions on non-protected images
On 3/13/2023 10:17 AM, Ard Biesheuvel wrote:
Currently, we rely on the memory type for loading images being
executable by default, and only restrict the permissions if the policy
says so, and the image sections are suitably aligned. This requires that
the various 'code' memory types are executable by default, which is
unfortunate.
In order to be able to tighten this, let's update the image protection
policy handling so that images that should not be mapped with strict
separation of RW- and R-X are remapped RWX explicitly if the memory type
used for loading the images is marked as NX by default.
Signed-off-by: Ard Biesheuvel
---
MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c | 98 +++-
1 file changed, 54 insertions(+), 44 deletions(-)
diff --git a/MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c
b/MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c
index 301ddd6eb053..7c7a946c1b48 100644
--- a/MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c
+++ b/MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c
@@ -373,11 +373,62 @@ ProtectUefiImage (
}
ProtectionPolicy = GetUefiImageProtectionPolicy (LoadedImage,
LoadedImageDevicePath);
+
+ ImageAddress = LoadedImage->ImageBase;
+
+ PdbPointer = PeCoffLoaderGetPdbPointer ((VOID *)(UINTN)ImageAddress);
+ if (PdbPointer != NULL) {
+DEBUG ((DEBUG_VERBOSE, " Image - %a\n", PdbPointer));
+ }
+
switch (ProtectionPolicy) {
-case DO_NOT_PROTECT:
- return EFI_SUCCESS;
case PROTECT_IF_ALIGNED_ELSE_ALLOW:
- break;
+ //
+ // Check PE/COFF image
+ //
+ DosHdr = (EFI_IMAGE_DOS_HEADER *)(UINTN)ImageAddress;
+ PeCoffHeaderOffset = 0;
+ if (DosHdr->e_magic == EFI_IMAGE_DOS_SIGNATURE) {
+PeCoffHeaderOffset = DosHdr->e_lfanew;
+ }
+
+ Hdr.Pe32 = (EFI_IMAGE_NT_HEADERS32 *)((UINT8 *)(UINTN)ImageAddress +
PeCoffHeaderOffset);
+ if (Hdr.Pe32->Signature != EFI_IMAGE_NT_SIGNATURE) {
+DEBUG ((DEBUG_INFO, "Hdr.Pe32->Signature invalid - 0x%x\n",
Hdr.Pe32->Signature));
+// It might be image in SMM.
In this case we now fall through to explicitly unprotect the image that
does not have the EFI_IMAGE_NT_SIGNATURE. Is it expected that we will
always run these images (or that if we shouldn't run it that that will
be caught elsewhere) and that giving it full permissions is permissible?
+ } else {
+//
+// Get SectionAlignment
+//
+if (Hdr.Pe32->OptionalHeader.Magic ==
EFI_IMAGE_NT_OPTIONAL_HDR32_MAGIC) {
+ SectionAlignment = Hdr.Pe32->OptionalHeader.SectionAlignment;
+} else {
+ SectionAlignment = Hdr.Pe32Plus->OptionalHeader.SectionAlignment;
+}
+
+if (SectionAlignment >= EFI_PAGE_SIZE) {
+ break;
+}
+
+DEBUG ((
+ DEBUG_VERBOSE,
+ " ProtectUefiImageCommon - Section Alignment(0x%x) is incorrect
\n",
+ SectionAlignment
+ ));
+ }
+ // fall through to unprotect image if needed
+case DO_NOT_PROTECT:
+ if ((PcdGet64 (PcdDxeNxMemoryProtectionPolicy) &
+ LShiftU64 (1, LoadedImage->ImageCodeType)) != 0)
+ {
+SetUefiImageMemoryAttributes (
+ (UINTN)LoadedImage->ImageBase,
+ (LoadedImage->ImageSize + EFI_PAGE_MASK) & ~(UINT64)EFI_PAGE_MASK,
+ 0
+ );
+ }
+
+ return EFI_SUCCESS;
default:
ASSERT (FALSE);
return EFI_SUCCESS;
@@ -396,47 +447,6 @@ ProtectUefiImage (
ImageRecord->ImageBase =
(EFI_PHYSICAL_ADDRESS)(UINTN)LoadedImage->ImageBase;
ImageRecord->ImageSize = LoadedImage->ImageSize;
- ImageAddress = LoadedImage->ImageBase;
-
- PdbPointer = PeCoffLoaderGetPdbPointer ((VOID *)(UINTN)ImageAddress);
- if (PdbPointer != NULL) {
-DEBUG ((DEBUG_VERBOSE, " Image - %a\n", PdbPointer));
- }
-
- //
- // Check PE/COFF image
- //
- DosHdr = (EFI_IMAGE_DOS_HEADER *)(UINTN)ImageAddress;
- PeCoffHeaderOffset = 0;
- if (DosHdr->e_magic == EFI_IMAGE_DOS_SIGNATURE) {
-PeCoffHeaderOffset = DosHdr->e_lfanew;
- }
-
- Hdr.Pe32 = (EFI_IMAGE_NT_HEADERS32 *)((UINT8 *)(UINTN)ImageAddress +
PeCoffHeaderOffset);
- if (Hdr.Pe32->Signature != EFI_IMAGE_NT_SIGNATURE) {
-DEBUG ((DEBUG_VERBOSE, "Hdr.Pe32->Signature invalid - 0x%x\n",
Hdr.Pe32->Signature));
-// It might be image in SMM.
-goto Finish;
- }
-
- //
- // Get SectionAlignment
- //
- if (Hdr.Pe32->OptionalHeader.Magic == EFI_IMAGE_NT_OPTIONAL_HDR32_MAGIC) {
-SectionAlignment = Hdr.Pe32->OptionalHeader.SectionAlignment;
- } else {
-SectionAlignment = Hdr.Pe32Plus->OptionalHeader.SectionAlignment;
- }
-
- if (SectionAlignment >= EFI_PAGE_SIZE) {
-DEBUG ((
- DEBUG_VERBOSE,
- " ProtectUefiImageCommon - Section Alignment(0x%x) is incorrect
\n",
- SectionAlignment
-
Re: [edk2-devel] [PATCH v5 18/38] MdeModulePkg/DxeIpl AARCH64: Remap DXE core code section before launch
On 3/13/2023 10:16 AM, Ard Biesheuvel wrote:
One nitpick below.
To permit the platform to adopt a stricter policy when it comes to
memory protections, and map all memory XP by default, add the necessary
handling to the DXE IPL PEIM to ensure that the DXE core code section is
mapped executable before invoking the DXE core.
It is up to the DXE core itself to manage the executable permissions on
other DXE and UEFI drivers and applications that it dispatches.
Note that this requires that the DXE IPL executes non-shadowed from a FV
that is mapped executable.
Signed-off-by: Ard Biesheuvel
---
MdeModulePkg/Core/DxeIplPeim/Arm/DxeLoadFunc.c | 73
MdeModulePkg/Core/DxeIplPeim/DxeIpl.inf| 1 +
2 files changed, 74 insertions(+)
diff --git a/MdeModulePkg/Core/DxeIplPeim/Arm/DxeLoadFunc.c
b/MdeModulePkg/Core/DxeIplPeim/Arm/DxeLoadFunc.c
index f62b6dcb38a7..c57ffa87e30f 100644
--- a/MdeModulePkg/Core/DxeIplPeim/Arm/DxeLoadFunc.c
+++ b/MdeModulePkg/Core/DxeIplPeim/Arm/DxeLoadFunc.c
@@ -11,6 +11,73 @@ SPDX-License-Identifier: BSD-2-Clause-Patent
#include "DxeIpl.h"
#include
+#include
+
+/**
+ Discover the code sections of the DXE core, and remap them read-only
+ and executable.
+
+ @param DxeCoreEntryPoint The entrypoint of the DXE core executable.
+ @param HobListThe list of HOBs passed to the DXE core from PEI.
+**/
+STATIC
+VOID
+RemapDxeCoreCodeReadOnly (
nit: should this be called RemapDxeCoreCodeReadExecute or something? The
naming seems confusing here to say map read only when we are mapping it
read and execute.
+ IN EFI_PHYSICAL_ADDRESS DxeCoreEntryPoint,
+ IN EFI_PEI_HOB_POINTERS HobList
+ )
+{
+ EFI_PEI_HOB_POINTERS Hob;
+ PE_COFF_LOADER_IMAGE_CONTEXT ImageContext;
+ RETURN_STATUSStatus;
+ EFI_IMAGE_OPTIONAL_HEADER_PTR_UNION Hdr;
+ EFI_IMAGE_SECTION_HEADER *Section;
+ UINTNIndex;
+
+ ImageContext.ImageRead = PeCoffLoaderImageReadFromMemory;
+ ImageContext.Handle= NULL;
+
+ //
+ // Find the module HOB for the DXE core
+ //
+ for (Hob.Raw = HobList.Raw; !END_OF_HOB_LIST (Hob); Hob.Raw = GET_NEXT_HOB
(Hob)) {
+if ((GET_HOB_TYPE (Hob) == EFI_HOB_TYPE_MEMORY_ALLOCATION) &&
+(CompareGuid (>AllocDescriptor.Name,
)) &&
+(Hob.MemoryAllocationModule->EntryPoint == DxeCoreEntryPoint))
+{
+ ImageContext.Handle = (VOID
*)(UINTN)Hob.MemoryAllocation->AllocDescriptor.MemoryBaseAddress;
+ break;
+}
+ }
+
+ ASSERT (ImageContext.Handle != NULL);
+
+ Status = PeCoffLoaderGetImageInfo ();
+ ASSERT_RETURN_ERROR (Status);
+
+ Hdr.Union = (EFI_IMAGE_OPTIONAL_HEADER_UNION *)((UINT8 *)ImageContext.Handle
+
+
ImageContext.PeCoffHeaderOffset);
+ ASSERT (Hdr.Pe32->Signature == EFI_IMAGE_NT_SIGNATURE);
+
+ Section = (EFI_IMAGE_SECTION_HEADER *)((UINT8 *)Hdr.Union + sizeof (UINT32) +
+ sizeof (EFI_IMAGE_FILE_HEADER) +
+
Hdr.Pe32->FileHeader.SizeOfOptionalHeader
+ );
+
+ for (Index = 0; Index < Hdr.Pe32->FileHeader.NumberOfSections; Index++) {
+if ((Section[Index].Characteristics & EFI_IMAGE_SCN_CNT_CODE) != 0) {
+ ArmSetMemoryRegionReadOnly (
+(UINTN)((UINT8 *)ImageContext.Handle + Section[Index].VirtualAddress),
+Section[Index].Misc.VirtualSize
+);
+
+ ArmClearMemoryRegionNoExec (
+(UINTN)((UINT8 *)ImageContext.Handle + Section[Index].VirtualAddress),
+Section[Index].Misc.VirtualSize
+);
+}
+ }
+}
/**
Transfers control to DxeCore.
@@ -33,6 +100,12 @@ HandOffToDxeCore (
VOID*TopOfStack;
EFI_STATUS Status;
+ //
+ // DRAM may be mapped with non-executable permissions by default, so
+ // we'll need to map the DXE core code region executable explicitly.
+ //
+ RemapDxeCoreCodeReadOnly (DxeCoreEntryPoint, HobList);
+
//
// Allocate 128KB for the Stack
//
diff --git a/MdeModulePkg/Core/DxeIplPeim/DxeIpl.inf
b/MdeModulePkg/Core/DxeIplPeim/DxeIpl.inf
index 62821477d012..d85ca79dc0c3 100644
--- a/MdeModulePkg/Core/DxeIplPeim/DxeIpl.inf
+++ b/MdeModulePkg/Core/DxeIplPeim/DxeIpl.inf
@@ -82,6 +82,7 @@ [LibraryClasses]
[LibraryClasses.ARM, LibraryClasses.AARCH64]
ArmMmuLib
+ PeCoffLib
[Ppis]
gEfiDxeIplPpiGuid ## PRODUCES
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#101370): https://edk2.groups.io/g/devel/message/101370
Mute This Topic: https://groups.io/mt/97586019/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] [PATCH v5 00/38] Implement strict memory permissions throughout
Hi Ard, For the patchset: Reviewed- and Tested-by: Oliver Smith-Denny Thanks for sending this out! I tested with some integrations to Project Mu on both a virtual and physical platform. Oliver On 3/13/2023 10:16 AM, Ard Biesheuvel wrote: Link: https://bugzilla.tianocore.org/show_bug.cgi?id=4369 This v5 now covers a lot more ground, and has ballooned quite substantially as a result. The series is essentially a proof of concept of a way to implement rigorous W^X memory protections from SEC all the way to booting the OS. In particular: - the AArch64 WXN control is enabled so that NX is implied for all writable memory regions, which is rather helpful when testing changes such as these; - avoid PEIM shadowing where possible, as that would involve managing the executable permissions of the shadowed code - remap the DXE core code section read-only explicitly from IPL - equip the DXE core with a way to manage memory permissions before the CPU arch protocol driver is dispatched; - permit the NX memory protection policy to apply to code memory type regions as well - check the NX compat DLL flag and section alignment to decide whether an image can be loaded when the NX policy is applied to such a code region - implement the EFI memory attributes protocol (for ARM and AArch64 only) so that such NX compat compliant images have a way to create executable mappings v4: - major cleanup of the 32-bit ARM code - add support for EFI_MEMORY_RP using the access flag - enable stack guard in ArmVirtPkg (which uses EFI_MEMORY_RP) - incorporate optimization from other series [0] to avoid splitting block entries unnecessarily v3: - fix ARM32 bug in attribute conversion - add Liming's ack to patch #1 - include draft patch (NOT FOR MERGE) used to test the changes v2: - drop patch to bump exposed UEFI revision to v2.10 - add missing permitted return values to protocol definition [0] https://edk2.groups.io/g/devel/message/99801 Cc: Michael Kinney Cc: Liming Gao Cc: Jiewen Yao Cc: Michael Kubacki Cc: Sean Brogan Cc: Rebecca Cran Cc: Leif Lindholm Cc: Sami Mujawar Cc: Taylor Beebe Ard Biesheuvel (38): ArmPkg/ArmMmuLib ARM: Remove half baked large page support ArmPkg/ArmMmuLib ARM: Split off XN page descriptor bit from type field ArmPkg/CpuDxe ARM: Fix page-to-section attribute conversion ArmPkg/ArmMmuLib ARM: Isolate the access flag from AP mask ArmPkg/ArmMmuLib ARM: Clear individual permission bits ArmPkg/ArmMmuLib: Implement EFI_MEMORY_RP using access flag ArmVirtPkg: Enable stack guard ArmPkg/ArmMmuLib: Avoid splitting block entries if possible ArmPkg/CpuDxe: Expose unified region-to-EFI attribute conversion MdePkg: Add Memory Attribute Protocol definition ArmPkg/CpuDxe: Implement EFI memory attributes protocol ArmPkg/CpuDxe: Perform preliminary NX remap of free memory MdeModulePkg/DxeCore: Unconditionally set memory protections ArmPkg/Mmu: Remove handling of NONSECURE memory regions ArmPkg/ArmMmuLib: Introduce region types for RO/XP WB cached memory MdePkg/BasePeCoffLib: Add API to keep track of relocation range MdeModulePkg/DxeIpl: Avoid shadowing IPL PEIM by default MdeModulePkg/DxeIpl AARCH64: Remap DXE core code section before launch MdeModulePkg/DxeCore: Reduce range of W+X remaps at EBS time MdeModulePkg/DxeCore: Permit preliminary CPU arch fallback ArmPkg: Implement ArmSetMemoryOverrideLib MdeModulePkg/PcdPeim: Permit unshadowed execution EmbeddedPkg/PrePiLib AARCH64: Remap DXE core before execution ArmVirtPkg/ArmVirtQemu: Use XP memory mappings by default ArmVirtPkg/ArmVirtQemu: Use PEI flavor of ArmMmuLib for all PEIMs ArmVirtPkg/ArmVirtQemu: Use read-only memory region type for code flash BaseTools/GccBase AARCH64: Avoid page sharing between code and data ArmVirtPkg/ArmVirtQemu: Enable hardware enforced W^X memory permissions MdePkg/PeCoffLib: Capture DLL characteristics field in image context MdePkg/IndustryStandard: PeImage.h: Import DLL characteristics MdeModulePkg/DxeCore: Remove redundant DEBUG statements MdeModulePkg/DxeCore: Update memory protections before freeing a region MdeModulePkg/DxeCore: Disregard runtime alignment for image protection MdeModulePkg/DxeCore: Deal with failure in UefiProtectImage() MdeModulePkg/DxeCore: Clear NX permissions on non-protected images MdeModulePkg/DxeCore: Permit NX protection for code regions MdeModulePkg/DxeCore: Check NX compat when using restricted code regions MdeModulePkg DEC: Remove inaccurate comment ArmPkg/ArmPkg.dec | 5 + ArmPkg/ArmPkg.dsc | 1 + ArmPkg/Drivers/CpuDxe/AArch64/Mmu.c| 25 +- ArmPkg/Drivers/CpuDxe/Arm/Mmu.c
