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

Add ImagePropertiesRecordLib and Fix MAT Bugs #4590

Closed
wants to merge 14 commits into from

Conversation

TaylorBeebe
Copy link
Contributor

@TaylorBeebe TaylorBeebe commented Jun 28, 2023

The UEFI and SMM MAT logic contains duplicate logic for manipulating image properties records which is used to track runtime images. This patch series adds a new library, ImagePropertiesRecordLib, which consolidates this logic and fixes the bugs which currently exist in the MAT logic.

The first patch adds the ImagePropertiesRecordLib implementation which is a copy of the UEFI MAT logic with minor modifications to remove the reliance on globabl variables and make the code unit testable.

The second patch adds a unit test for the ImagePropertiesRecordLib. The logic tests various potential layouts of the EFI memory map and runtime images. 3/4 of these tests will fail which demonstrates the MAT logic bugs.

The third patch fixes the logic in the ImagePropertiesRecordLib so that all of the unit tests pass and the MAT logic can be fixed by using the library.

The remaining patches add library instances to DSC files and remove the image properties record logic from the SMM and UEFI MAT logic.

A writeup of the bug which initiated this patch series is below.

Bugzilla: https://bugzilla.tianocore.org/show_bug.cgi?id=4492

To create the MAT, we call into a function CoreGetMemoryMapWithSeparatedImageSection() which will get the memory map (and thus its required size) but update that size to be the size of the map plus the amount of extra space necessary to break up the memory map. The extra space in this case is indicated by a field called CodeSegmentMax which is set to be the most number of code segments found in a single image loaded. Let’s say we found an image with 2 code sections, making CodeSegmentMax = 2. With two code segments, here are a couple of ways an image could be laid out:

Image 1: Data | Code | Data | Code | Data
Image 2: Data | Code | Code | Data

So, if a single memory map descriptor is the same address range as a singe image, that one map descriptor could become at most 5, leading to the calculation (2 * CodeSegmentMax + 1) * NumberOfImageRecords.

After returning the required pool size (MemoryMapSize) the next call will be with a pool of that size. When we call into get the memory map, MemoryMapSize will be updated to be the actual size of the memory map and we make sure that size is what we expect before calling into SplitTable(). Within SplitTable(), MemoryMapSize is equal to the size of the actual memory map fetched, but the buffer containing the memory map is much larger as described above. We set IndexOld to be the last descriptor in the memory map fetched from CoreGetMemoryMap() and IndexNew to the last descriptor in the entire buffer. We work our way backwards through the descriptors generated from CoreGetMemoryMap() and write new descriptors to the end of the entire buffer. We call into GetMaxSplitRecord() which calculates how many images might be contained in a single memory map descriptor. Let’s say one descriptor range maps to one loaded image which has 2 code sections, so GetMaxSplitRecord() will return 4 (we subtract 1 because IndexNew is already pointing to a valid descriptor).

Let’s zoom into those 5 currently empty descriptors:

| | | | | |
^
IndexNew

Let’s say the code/data sections are laid out like Image 2 shown above. We call into SplitRecord() which will get the image covering the original descriptor range then call into SetNewRecord() to start populating the descriptors. There’s a stray portion of SplitRecord() which makes sure there isn’t any lingering memory that hasn’t been covered by one of the new descriptors which we can ignore in this case. In the case of microsoft/mu_basecore#2 above, we will write 4 descriptors but only return the value 3.

When we get back to SplitTable(), MaxSplitRecordCount = 4 and RealSplitRecordCount = 3 and those 5 descriptors look like this:

| d1 | c1 | c2 | d2 | |
^
IndexNew

We then do a mem copy with a destination of IndexNew + MaxSplitRecordCount – RealSplitRecordCount = IndexNew + 1 and a source of IndexNew and a length of 3 which would result in the following:

| d1 | d1 | c1 | c2 | |
^
IndexNew

And this is clearly not right because we have a duplicated the first data section and overwritten the last data section. Index 0 will be overwritten, but Index 4 is now a garbage memory descriptor which will be captured when we copy all the new records to the front of the buffer and update memory map size to be the new number of entries.

@TaylorBeebe TaylorBeebe force-pushed the fix_split_table branch 4 times, most recently from aebd282 to 31d3cb9 Compare July 18, 2023 15:22
@TaylorBeebe TaylorBeebe changed the title MdeModulePkg: Update SplitTable() Logic to Correctly Break Up Memory Map Update MAT Logic to Use ImagePropertiesRecordLib Jul 18, 2023
@TaylorBeebe TaylorBeebe force-pushed the fix_split_table branch 3 times, most recently from 630439f to b0a8af9 Compare July 18, 2023 18:01
@TaylorBeebe TaylorBeebe changed the title Update MAT Logic to Use ImagePropertiesRecordLib Add ImagePropertiesRecordLib and Fix MAT Bugs Jul 18, 2023
@TaylorBeebe TaylorBeebe force-pushed the fix_split_table branch 4 times, most recently from 5c0c434 to 4e7f6d3 Compare July 25, 2023 00:37
TaylorBeebe and others added 14 commits August 4, 2023 10:45
Create a library for manipulating image properties records. The
library is currently blank and will be filled in a future patch
to help with reviewer readability.

Signed-off-by: Taylor Beebe <t@taylorbeebe.com>
Add an instance of ImagePropertiesRecordLib which will be used by the
DXE Core.

Signed-off-by: Taylor Beebe <t@taylorbeebe.com>
Add an instance of ImagePropertiesRecordLib which will be used by the
DXE Core.

Signed-off-by: Taylor Beebe <t@taylorbeebe.com>
Add an instance of ImagePropertiesRecordLib which will be used by the
DXE Core.

Signed-off-by: Taylor Beebe <t@taylorbeebe.com>
Add an instance of ImagePropertiesRecordLib which will be used by the
DXE Core.

Signed-off-by: Taylor Beebe <t@taylorbeebe.com>
…e Use

This patch updates MemoryAttributesTable.c to reduce reliance on global
variables and allow some logic to move to a library.

Signed-off-by: Taylor Beebe <t@taylorbeebe.com>
Move some DXE MAT logic to ImagePropertiesRecordLib to consolidate
code and enable unit testability.

Signed-off-by: Taylor Beebe <t@taylorbeebe.com>
Create a host-based unit test for the ImagePropertiesRecordLib
SplitTable() logic. This test has 4 cases which tests different
potential image and memory map layouts. 3/4 of these tests fail
with the logic in its current state to provide proof of the bugs
in the current MAT logic.

Signed-off-by: Taylor Beebe <t@taylorbeebe.com>
Fix the bugs in the MAT logic before switching the
UEFI and SMM MAT logic to use the new library.

Signed-off-by: Taylor Beebe <t@taylorbeebe.com>
…ordLib

Update function headers to clarify the contract of each function and
improve readability. Add NULL checks to all functions that take a
pointer as an argument. Add return status to functions that
may need to return early due to invalid input.

Signed-off-by: Taylor Beebe <t@taylorbeebe.com>
The function EnforceMemoryMapAttribute() in the SMM MAT logic will
ensure that the CODE and DATA memory types have the desired attributes.
The consumer of the SMM MAT should only override the Attributes field
in the MAT if it is nonzero. This also allows the UEFI and SMM MAT
logic to use ImagePropertiesRecordLib instead of carrying two copies
of the image properties record manipulation.

Signed-off-by: Taylor Beebe <t@taylorbeebe.com>
Now that the bugs are fixed in the MAT logic, we can remove the
duplicate logic from PiSmmCore/MemoryAttributesTable.c and use
ImagePropertiesRecordLib instead.

Signed-off-by: Taylor Beebe <t@taylorbeebe.com>
Add logic to create and delete image properties records. Where
applicable, redirect existing code to use the new library.

Signed-off-by: Taylor Beebe <t@taylorbeebe.com>
Update DumpImageRecord() to be DumpImageRecords(), and improve
the debug output. The function will output at DEBUG_INFO instead,
and the function will be run in DXE and SMM
MAT logic when the MAT is installed at EndOfDxe on DEBUG builds.

Signed-off-by: Taylor Beebe <t@taylorbeebe.com>
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.

None yet

2 participants