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

[REBASE && FF] FlatPageTableLib and DxePagingAuditTestApp Updates #395

Merged

Conversation

TaylorBeebe
Copy link
Contributor

@TaylorBeebe TaylorBeebe commented Dec 18, 2023

Description

This change set was primarily created to improve the accuracy of the DXE paging audit shell test. The problem was two-fold:

  1. The layout of memory would change as data was collected. This is solved by pre-allocating space for two of the memory maps (flat page table and EFI memory map). Those two maps will have their size requirement checked at the start of each function to ensure enough heap space has been allocated for them prior to populating them with memory information.
  2. The shell test was imprecise when reporting which regions failed the various tests. I'll use the UnallocatedMemoryIsRP() test as an example. The test parses the EFI memory map for EfiConventionalMemory regions which indicate unallocated memory. If any subsection of the region was not read protected, then the entire region would be reported as failing. With this update, every subsection which does not comply will be individually reported giving platform developers more precise information on where the problem areas are.

Patch breakdown:

  1. Updates the DXE paging audit to always write out all .dat files even if the buffers are empty. The presence of the file acts as a receipt that the audit was run successfully and helps with automated unit testing.
  2. Adds a function to FlatPageTableLib to dump the contents of a flat page table to the console. This is useful for debugging.
  3. Updates GetRegionAccessAttributes() in FlatPageTableLib to return the attributes of the first attribute-contiguous range found in the region and report the actual size of that range. This allows the caller to collect the attributes of the region by calling the function repeatedly instead of needing to guess which subsection of the region has contiguous attributes.
  4. Updates the DXE paging audit shell tests to pre-allocate memory for memory maps. The page table map, EFI memory map, and EFI memory space map all describe the layout of the system address space. Because of this, if allocations are performed while these maps are being generated, then the maps generated the earliest will be inaccurate.
  5. Adds ValidatePageTableAttributes() to the DXE paging audit shell test logic. This function validates the attributes of the input memory region. It uses GetRegionAccessAttributes() to get the attributes of the region and compares them to the attributes passed in. This function will be used by each test case to check that the page/translation table attributes match the memory protection requirements and enables more precise reporting of regions which do not match the requirements.
  6. Updates the DXE paging audit shell tests to use ValidatePageTableAttributes() to determine policy compliance.

How This Was Tested

This was tested on Q35 and an ARM platform. Some changes in test caused the UnallocatedMemoryIsRP() test to erroneously fail due to reason 1 above which has been fixed with this patch set. Additionally, this patch set was checked by comparing the shell test output to the full HTML paging audit to verify that errant regions are exact and consistent.

Integration Instructions

N/A

@TaylorBeebe TaylorBeebe force-pushed the update_flatpagetablelib branch 2 times, most recently from 66ab40d to a97f36f Compare December 19, 2023 02:10
@makubacki
Copy link
Member

I think it's useful to add back to the testing and integration sections to the PR description and summarize that information there. Gets rolled up into release notes for integrators to quickly review.

@TaylorBeebe TaylorBeebe added the impact:testing Affects testing label Dec 19, 2023
@TaylorBeebe TaylorBeebe force-pushed the update_flatpagetablelib branch from a97f36f to 926ba47 Compare December 19, 2023 20:30
Description

This updates the paging audit to always write out all .dat files
even if the buffers are empty. The presence of the file acts
as a receipt that the audit was run successfully and helps with
automated unit testing.

- [x] 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 Q35 by creating the paging audit.

Integration Instructions

N/A
Description

This patch adds a function to dump the contents of a flat page
table to the console. This is useful for debugging.

- [x] 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 Q35 by running the function.

Integration Instructions

N/A
Description

GetRegionAccessAttributes() takes an input region description and page
table map and outputs the attributes of that region. Because it's
common for multi-page regions to have varying attributes, the patch
updates the function to return the attributes of the first
attribute-contiguous range found in the region and report the actual
size of that range. This allows the caller to collect the attributes
of the region by calling the function repeatedly instead of needing
to guess which subsection of the region has contiguous attributes.

- [x] 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 Q35 by running the function.

Integration Instructions

N/A
Description

The page table map, EFI memory map, and EFI memory space map all
describe the layout of the system address space. Because of this,
if allocations are performed while these maps are being generated,
then the maps generated the earliest will be inaccurate. This
change pre-allocates the memory for these maps before they are
generated to ensure that the maps are consistent.

- [x] 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 Q35 by running the app with the following patch.

Integration Instructions

N/A
…sAttributes()

Description

ValidatePageTableAttributes() is a function that validates the attributes
of the input memory region. It uses GetRegionAccessAttributes() to get
the attributes of the region and compares them to the
attributes passed in. This function will be used by each test case
to check that the page/translation table attributes match the memory
protection requirements.

- [x] 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 Q35 by running the app with the following patch.

Integration Instructions

N/A
Description

This patch updates the shell tests to use the validate function from
the previous patch. This allows the tests to be more exact in reporting
regions which do not meet the memory protection security bar.

- [x] 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 Q35 by running the app.

Integration Instructions

N/A
@TaylorBeebe TaylorBeebe force-pushed the update_flatpagetablelib branch from 926ba47 to 83cc28d Compare January 3, 2024 19:13
@TaylorBeebe TaylorBeebe merged commit f79fe54 into microsoft:release/202302 Jan 3, 2024
29 checks passed
@TaylorBeebe TaylorBeebe deleted the update_flatpagetablelib branch January 3, 2024 20:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
impact:testing Affects testing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants