[CHERRY-PICK] [REBASE & FF] Revert Mu Commit in Favor of edk2 Commit #281
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
github-actions
bot
added
the
impact:non-functional
makubacki
approved these changes
Sep 4, 2024
apop5
approved these changes
Sep 4, 2024
On AARCH64 systems, the GCD is not fully synced with the page table. On x86 systems, the GCD is synced by adding `EFI_MEMORY_RO`, `EFI_MEMORY_RP`, and `EFI_MEMORY_XP` to the current capabilities of the GCD, then the page table attributes are set on the GCD attributes. However, on AARCH64, the GCD capabilities do not get updated, instead only the attributes from the page table are masked by the existing GCD capabilities, which means that any new page table attribute which are already set are dropped and the GCD does not reflect the state of the system. This has been seen to cause issues where memory in the page table that was marked `EFI_MEMORY_XP` had an additional attribute set using the GCD capabilities, which did not include `EFI_MEMORY_XP`, this caused the page table to be updated to lose `EFI_MEMORY_XP`, which is a potential security issue. The existing behavior on AARCH64 systems is an implementation error, it assumes one of two things: - The page table attributes must be a subset of the GCD capabilities - The GCD does not need to have its capabilities synced to what the page table attributes are The first is incorrect as important attributes such as `EFI_MEMORY_XP` do not get applied to the GCD capabilities by default and therefore must be synced back. This comment from ArmPkg's CpuDxe driver helps explain: ```c // The GCD implementation maintains its own copy of the state of memory // space attributes. GCD needs to know what the initial memory space // attributes are. The CPU Arch. Protocol does not provide a // GetMemoryAttributes function for GCD to get this so we must resort to // calling GCD (as if we were a client) to update its copy of the // attributes. This is bad architecture and should be replaced with a // way for GCD to query the CPU Arch. driver of the existing memory // space attributes instead. ``` However, this comment misses that updating the capabilities is critical to updating the attributes. The second is incorrect because significant pieces of core code reference the GCD attributes instead of the page table attributes. For example, NonDiscoverablePciDeviceDxe uses the GCD capabilities and attributes when interacting with a non-discoverable PCI device. When the GCD is not synced to the page table, we get the errors and security concerns listed above. Signed-off-by: Oliver Smith-Denny <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
This reverts a Mu commit that has been upstreamed in favor of the corresponding edk2 commit.
How This Was Tested
N/A.
Integration Instructions
N/A.