Sync AARCH64 GCD Capabilities with Page Table #89

os-d · 2023-04-07T17:37:59Z

Preface

Please ensure you have read the contribution docs prior
to submitting the pull request. In particular,
pull request guidelines.

Description

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.

This patch goes one step further than the x86 GCD syncing by applying any existing non-cache attributes (which it is not supported to update) in the page table (today this is still the same set as in x86 but is more extensible for when new attributes are added) to the GCD capabilities and attributes. In this way, the GCD will actually be fully synced to the already existent state of the page table.

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:

  // 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.

For each item, place an "x" in between [ and ] if true. Example: [x].
(you can also check items in the GitHub UI)

Impacts functionality?
- Functionality - Does the change ultimately impact how firmware functions?
- Examples: Add a new library, publish a new PPI, update an algorithm, ...
Impacts security?
- Security - Does the change have a direct security impact on an application,
  flow, or firmware?
- Examples: Crypto algorithm change, buffer overflow fix, parameter
  validation improvement, ...
Breaking change?
- Breaking change - Will anyone consuming this change experience a break
  in build or boot behavior?
- Examples: Add a new library class, move a module to a different repo, call
  a function in a new library class in a pre-existing module, ...
Includes tests?
- Tests - Does the change include any explicit test code?
- Examples: Unit tests, integration tests, robot tests, ...
Includes documentation?
- Documentation - Does the change contain explicit documentation additions
  outside direct code modifications (and comments)?
- Examples: Update readme file, add feature readme file, link to documentation
  on an a separate Web page, ...

How This Was Tested

This was tested on QEMU and physical hardware that was experiencing the issue where EFI_MEMORY_XP was being removed when an additional attribute was set.

Integration Instructions

N/A.

ArmPkg/Drivers/CpuDxe/CpuMmuCommon.c

makubacki · 2023-04-11T19:16:05Z

Regarding this in the PR description:

"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."

Can you provide more context on the rationale behind the previous behavior? Was this an implementation error? A design gap? Etc.

ArmPkg/Drivers/CpuDxe/CpuMmuCommon.c

os-d · 2023-04-11T21:22:08Z

Regarding this in the PR description:

"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."

Can you provide more context on the rationale behind the previous behavior? Was this an implementation error? A design gap? Etc.

I updated the PR description, let me know if this answers your question.

makubacki · 2023-04-11T22:13:18Z

Can you provide more context on the rationale behind the previous behavior? Was this an implementation error? A design gap? Etc.

I updated the PR description, let me know if this answers your question.

Yes, thanks

# Preface Please ensure you have read the [contribution docs](https://github.com/microsoft/mu/blob/master/CONTRIBUTING.md) prior to submitting the pull request. In particular, [pull request guidelines](https://github.com/microsoft/mu/blob/master/CONTRIBUTING.md#pull-request-best-practices). ## Description 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. This patch goes one step further than the x86 GCD syncing by applying any existing non-cache attributes (which it is not supported to update) in the page table (today this is still the same set as in x86 but is more extensible for when new attributes are added) to the GCD capabilities and attributes. In this way, the GCD will actually be fully synced to the already existent state of the page table. 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. For each item, place an "x" in between `[` and `]` if true. Example: `[x]`. _(you can also check items in the GitHub UI)_ - [x] Impacts functionality? - **Functionality** - Does the change ultimately impact how firmware functions? - Examples: Add a new library, publish a new PPI, update an algorithm, ... - [x] Impacts security? - **Security** - Does the change have a direct security impact on an application, flow, or firmware? - Examples: Crypto algorithm change, buffer overflow fix, parameter validation improvement, ... - [ ] Breaking change? - **Breaking change** - Will anyone consuming this change experience a break in build or boot behavior? - Examples: Add a new library class, move a module to a different repo, call a function in a new library class in a pre-existing module, ... - [ ] Includes tests? - **Tests** - Does the change include any explicit test code? - Examples: Unit tests, integration tests, robot tests, ... - [ ] Includes documentation? - **Documentation** - Does the change contain explicit documentation additions outside direct code modifications (and comments)? - Examples: Update readme file, add feature readme file, link to documentation on an a separate Web page, ... ## How This Was Tested This was tested on QEMU and physical hardware that was experiencing the issue where `EFI_MEMORY_XP` was being removed when an additional attribute was set. ## Integration Instructions N/A.

# Preface Please ensure you have read the [contribution docs](https://github.com/microsoft/mu/blob/master/CONTRIBUTING.md) prior to submitting the pull request. In particular, [pull request guidelines](https://github.com/microsoft/mu/blob/master/CONTRIBUTING.md#pull-request-best-practices). ## Description 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. This patch goes one step further than the x86 GCD syncing by applying any existing non-cache attributes (which it is not supported to update) in the page table (today this is still the same set as in x86 but is more extensible for when new attributes are added) to the GCD capabilities and attributes. In this way, the GCD will actually be fully synced to the already existent state of the page table. 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. For each item, place an "x" in between `[` and `]` if true. Example: `[x]`. _(you can also check items in the GitHub UI)_ - [x] Impacts functionality? - **Functionality** - Does the change ultimately impact how firmware functions? - Examples: Add a new library, publish a new PPI, update an algorithm, ... - [x] Impacts security? - **Security** - Does the change have a direct security impact on an application, flow, or firmware? - Examples: Crypto algorithm change, buffer overflow fix, parameter validation improvement, ... - [ ] Breaking change? - **Breaking change** - Will anyone consuming this change experience a break in build or boot behavior? - Examples: Add a new library class, move a module to a different repo, call a function in a new library class in a pre-existing module, ... - [ ] Includes tests? - **Tests** - Does the change include any explicit test code? - Examples: Unit tests, integration tests, robot tests, ... - [ ] Includes documentation? - **Documentation** - Does the change contain explicit documentation additions outside direct code modifications (and comments)? - Examples: Update readme file, add feature readme file, link to documentation on an a separate Web page, ... ## How This Was Tested This was tested on QEMU and physical hardware that was experiencing the issue where `EFI_MEMORY_XP` was being removed when an additional attribute was set. ## Integration Instructions N/A.

Please ensure you have read the [contribution docs](https://github.com/microsoft/mu/blob/master/CONTRIBUTING.md) prior to submitting the pull request. In particular, [pull request guidelines](https://github.com/microsoft/mu/blob/master/CONTRIBUTING.md#pull-request-best-practices). 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. This patch goes one step further than the x86 GCD syncing by applying any existing non-cache attributes (which it is not supported to update) in the page table (today this is still the same set as in x86 but is more extensible for when new attributes are added) to the GCD capabilities and attributes. In this way, the GCD will actually be fully synced to the already existent state of the page table. 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. For each item, place an "x" in between `[` and `]` if true. Example: `[x]`. _(you can also check items in the GitHub UI)_ - [x] Impacts functionality? - **Functionality** - Does the change ultimately impact how firmware functions? - Examples: Add a new library, publish a new PPI, update an algorithm, ... - [x] Impacts security? - **Security** - Does the change have a direct security impact on an application, flow, or firmware? - Examples: Crypto algorithm change, buffer overflow fix, parameter validation improvement, ... - [ ] Breaking change? - **Breaking change** - Will anyone consuming this change experience a break in build or boot behavior? - Examples: Add a new library class, move a module to a different repo, call a function in a new library class in a pre-existing module, ... - [ ] Includes tests? - **Tests** - Does the change include any explicit test code? - Examples: Unit tests, integration tests, robot tests, ... - [ ] Includes documentation? - **Documentation** - Does the change contain explicit documentation additions outside direct code modifications (and comments)? - Examples: Update readme file, add feature readme file, link to documentation on an a separate Web page, ... This was tested on QEMU and physical hardware that was experiencing the issue where `EFI_MEMORY_XP` was being removed when an additional attribute was set. N/A.

This reverts commit fb49fe6, as it has been upstreamed to edk2 as 662272e.

Please ensure you have read the [contribution docs](https://github.com/microsoft/mu/blob/master/CONTRIBUTING.md) prior to submitting the pull request. In particular, [pull request guidelines](https://github.com/microsoft/mu/blob/master/CONTRIBUTING.md#pull-request-best-practices). 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. This patch goes one step further than the x86 GCD syncing by applying any existing non-cache attributes (which it is not supported to update) in the page table (today this is still the same set as in x86 but is more extensible for when new attributes are added) to the GCD capabilities and attributes. In this way, the GCD will actually be fully synced to the already existent state of the page table. 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. For each item, place an "x" in between `[` and `]` if true. Example: `[x]`. _(you can also check items in the GitHub UI)_ - [x] Impacts functionality? - **Functionality** - Does the change ultimately impact how firmware functions? - Examples: Add a new library, publish a new PPI, update an algorithm, ... - [x] Impacts security? - **Security** - Does the change have a direct security impact on an application, flow, or firmware? - Examples: Crypto algorithm change, buffer overflow fix, parameter validation improvement, ... - [ ] Breaking change? - **Breaking change** - Will anyone consuming this change experience a break in build or boot behavior? - Examples: Add a new library class, move a module to a different repo, call a function in a new library class in a pre-existing module, ... - [ ] Includes tests? - **Tests** - Does the change include any explicit test code? - Examples: Unit tests, integration tests, robot tests, ... - [ ] Includes documentation? - **Documentation** - Does the change contain explicit documentation additions outside direct code modifications (and comments)? - Examples: Update readme file, add feature readme file, link to documentation on an a separate Web page, ... This was tested on QEMU and physical hardware that was experiencing the issue where `EFI_MEMORY_XP` was being removed when an additional attribute was set. N/A.

os-d requested review from spbrogan and TaylorBeebe April 7, 2023 17:37

github-actions bot added the impact:security Has a security impact label Apr 7, 2023

kenlautner approved these changes Apr 7, 2023

View reviewed changes

os-d commented Apr 7, 2023

View reviewed changes

ArmPkg/Drivers/CpuDxe/CpuMmuCommon.c Show resolved Hide resolved

TaylorBeebe approved these changes Apr 7, 2023

View reviewed changes

makubacki reviewed Apr 11, 2023

View reviewed changes

ArmPkg/Drivers/CpuDxe/CpuMmuCommon.c Outdated Show resolved Hide resolved

makubacki approved these changes Apr 12, 2023

View reviewed changes

os-d force-pushed the osde/sync_aarch64_gcd_capabilities branch from ea31c95 to c74464c Compare April 21, 2023 16:36

os-d added 4 commits April 21, 2023 10:46

Sync AARCH64 GCD Capabilities with Page Table

9d9d73a

Don't automatically add passed in attributes to capabilities

ab4281d

Update comment per PR feedback

43b214d

Uncrustify

b003a6b

os-d force-pushed the osde/sync_aarch64_gcd_capabilities branch from c74464c to b003a6b Compare April 21, 2023 17:46

os-d merged commit b133a3b into microsoft:release/202208 Apr 21, 2023

os-d deleted the osde/sync_aarch64_gcd_capabilities branch April 21, 2023 18:06

ProjectMuBot mentioned this pull request Feb 6, 2024

Bump Silicon/Arm/MU_TIANO from 2023020000.1.4 to 2023110000.0.0 microsoft/mu_tiano_platforms#845

Closed

os-d added a commit that referenced this pull request Aug 31, 2024

Revert "Sync AARCH64 GCD Capabilities with Page Table (#89)"

a41e5ed

This reverts commit fb49fe6, as it has been upstreamed to edk2 as 662272e.

os-d added a commit that referenced this pull request Oct 1, 2024

Revert "Sync AARCH64 GCD Capabilities with Page Table (#89)"

2849203

This reverts commit fb49fe6, as it has been upstreamed to edk2 as 662272e.

os-d added a commit that referenced this pull request Oct 1, 2024

Revert "Sync AARCH64 GCD Capabilities with Page Table (#89)"

d67abd4

This reverts commit fb49fe6, as it has been upstreamed to edk2 as 662272e.

This was referenced Nov 12, 2024

Bump Silicon/Arm/MU_TIANO from 2024050001.0.0 to 2024050001.0.1 microsoft/mu_tiano_platforms#1066

Closed

Bump Silicon/Arm/MU_TIANO from 2024050001.0.0 to 2024050001.0.2 microsoft/mu_tiano_platforms#1071

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Sync AARCH64 GCD Capabilities with Page Table #89

Sync AARCH64 GCD Capabilities with Page Table #89

os-d commented Apr 7, 2023 •

edited

Loading

makubacki commented Apr 11, 2023

os-d commented Apr 11, 2023

makubacki commented Apr 11, 2023

Sync AARCH64 GCD Capabilities with Page Table #89

Sync AARCH64 GCD Capabilities with Page Table #89

Conversation

os-d commented Apr 7, 2023 • edited Loading

Preface

Description

How This Was Tested

Integration Instructions

makubacki commented Apr 11, 2023

os-d commented Apr 11, 2023

makubacki commented Apr 11, 2023

os-d commented Apr 7, 2023 •

edited

Loading