Skip to content

[Rebase&FF] Apply memory protections#1660

Open
apop5 wants to merge 2 commits intomicrosoft:release/202511from
apop5:personal/apop5/missedmemoryprotections
Open

[Rebase&FF] Apply memory protections#1660
apop5 wants to merge 2 commits intomicrosoft:release/202511from
apop5:personal/apop5/missedmemoryprotections

Conversation

@apop5
Copy link
Collaborator

@apop5 apop5 commented Feb 26, 2026

Description

During removal of Rp on Freed memory, some corner cases were also removed.

Apply the corner cases to allow for clean dxe paging audit.

  • Impacts functionality?
  • Impacts security?
  • Breaking change?
  • Includes tests?
  • Includes documentation?

How This Was Tested

Q35 DxePagingAuditTestApp was reporting missing memory protection regions.
After fixes, DxePagingAuditTestApp reports passing.

Integration Instructions

No integration necessary.

When CpuDxe runs, signal gEdkiiGcdSyncCompleteProtocolGuid to the Core to allow
memory protections to be applied correctly for existing allocations.

Missed when removing RP on free during integration.
Ensure that memory protections are applied to conventional memory.
Apply memory protections allocations which would bypass traditional
memory protections policy.

Missed while removing RP on Freed memory during integration.
@apop5 apop5 requested review from makubacki and os-d February 26, 2026 21:57
}

if (MemoryType == EfiConventionalMemory) {
Attributes |= EFI_MEMORY_RP;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This part we shouldn't have, right? This will add RP on free memory which we dropped

// MU_CHANGE START: Install blank protocol to signal the end of the GCD sync
gBS->InstallMultipleProtocolInterfaces (
&ImageHandle,
&gEdkiiGcdSyncCompleteProtocolGuid,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought this wasn't required without RP on free memory? My recollection was that this was added for that case. I see we still have the core doing it, but I think we need to revert that commit. Checking.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, it was: 144e3fe. I think we need to strip this out, it should have been removed when the RP on free memory commit was dropped. I can help if needed.

@codecov-commenter
Copy link

Codecov Report

❌ Patch coverage is 0% with 17 lines in your changes missing coverage. Please review.
⚠️ Please upload report for BASE (release/202511@2085528). Learn more about missing BASE report.

Files with missing lines Patch % Lines
MdeModulePkg/Core/Dxe/Mem/Page.c 0.00% 9 Missing ⚠️
MdeModulePkg/Core/Dxe/Mem/HeapGuard.c 0.00% 8 Missing ⚠️
Additional details and impacted files
@@                Coverage Diff                @@
##             release/202511    #1660   +/-   ##
=================================================
  Coverage                  ?    1.52%           
=================================================
  Files                     ?     1150           
  Lines                     ?   374200           
  Branches                  ?     3196           
=================================================
  Hits                      ?     5703           
  Misses                    ?   368072           
  Partials                  ?      425           
Flag Coverage Δ
FmpDevicePkg 5.06% <ø> (?)
MdeModulePkg 1.58% <0.00%> (?)
NetworkPkg 0.36% <ø> (?)
PolicyServicePkg 18.62% <ø> (?)
SecurityPkg 0.53% <ø> (?)
UefiCpuPkg 4.78% <ø> (?)
UnitTestFrameworkPkg 0.00% <ø> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants