Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

MdeModulePkg: Compatibility Mode: Only Remap System Memory Regions #1030

Merged
merged 1 commit into from
Jul 13, 2024

Conversation

os-d
Copy link
Contributor

@os-d os-d commented Jul 5, 2024

Description

When we enter memory protections compatibility mode, we attempt to disable null protection and remap 0 - 0xA0000 as RWX. This was done for x86 systems with broken shim/grubs on Linux that would attempt to use those regions. This resolved that issue and we could boot non-memory protection safe Linux images on x86 HW. However, this approach did not take into account systems that do not have that range marked as system memory, for example ARM64 systems do not have this requirement. As such, this would inappropriately map these regions as RWX when they were not system memory.

This patch updates the remapping to only remap and disable null protection if these ranges are marked as system memory, otherwise it will leave them alone.

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

Tested on an ARM64 platform that does not have 0 - 0xA0000 as system memory, as well as an X86 system that does have that range as system memory, booting a Linux image on both that forces us to enter compatibility mode.

Integration Instructions

N/A.

@github-actions github-actions bot added the impact:security Has a security impact label Jul 5, 2024
@codecov-commenter
Copy link

codecov-commenter commented Jul 5, 2024

Codecov Report

Attention: Patch coverage is 0% with 3 lines in your changes missing coverage. Please review.

Project coverage is 1.35%. Comparing base (90adf2b) to head (3f3adb8).

Files Patch % Lines
...eModulePkg/Core/Dxe/Misc/MemoryProtectionSupport.c 0.00% 3 Missing ⚠️
Additional details and impacted files
@@                Coverage Diff                 @@
##           release/202311    #1030      +/-   ##
==================================================
- Coverage            1.35%    1.35%   -0.01%     
==================================================
  Files                1304     1304              
  Lines              333790   333854      +64     
  Branches             5103     5103              
==================================================
  Hits                 4512     4512              
- Misses             329195   329259      +64     
  Partials               83       83              
Flag Coverage Δ
MdeModulePkg 0.68% <0.00%> (-0.01%) ⬇️

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.

Copy link
Member

@spbrogan spbrogan left a comment

Choose a reason for hiding this comment

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

reviewing but i have some concerns about under/overflow and checking consistency between different fields

@os-d
Copy link
Contributor Author

os-d commented Jul 11, 2024

@spbrogan, refactored to use SafeIntLib and to only use my own fields as the basis for further checks.

When we enter memory protections compatibility mode, we attempt to
disable null protection and remap 0 - 0xA0000 as RWX. This was done
for x86 systems with broken shim/grubs on Linux that would attempt
to use those regions. This resolved that issue and we could boot
non-memory protection safe Linux images on x86 HW. However, this
approach did not take into account systems that do not have that
range marked as system memory, for example ARM64 systems do not
have this requirement. As such, this would inappropriately map
these regions as RWX when they were not system memory.

This patch updates the remapping to only remap and disable null
protection if these ranges are marked as system memory, otherwise
it will leave them alone.
@os-d os-d dismissed spbrogan’s stale review July 12, 2024 23:37

Discussed in person and requested changes were made.

@os-d os-d enabled auto-merge (rebase) July 12, 2024 23:39
@os-d os-d merged commit 1cbfaf1 into microsoft:release/202311 Jul 13, 2024
31 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
impact:security Has a security impact
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants