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

UefiCpuPkg/PiSmmCpuDxeSmm:Check resource HOB range before mapping #6431

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

td36
Copy link
Contributor

@td36 td36 commented Nov 12, 2024

Description

This commit is to check if the resource HOB range does not exceed the max supported physical address.
The function BuildMemoryMapFromResDescHobs is to build Memory Region from resource HOBs. Then the memory maps will be used during creating or modifying SMM page table. If the resource HOB range exceeds the max supported physical address, then subsequent calling of PageTableMap() will fail.

How This Was Tested

Tested in Intel internal server platform with x86 standalone MM enabled and succussfully booted into shell

Count++;
ResourceHobEnd = Hob.ResourceDescriptor->PhysicalStart + Hob.ResourceDescriptor->ResourceLength;

if (ResourceHobEnd <= MaxPhysicalAddress) {
Copy link
Member

@jiaxinwu jiaxinwu Nov 19, 2024

Choose a reason for hiding this comment

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

Just need assert:
ASSERT ((Hob.ResourceDescriptor->PhysicalStart < MaxPhysicalAddress) && (ResourceHobEnd < MaxPhysicalAddress))

Copy link
Contributor Author

Choose a reason for hiding this comment

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

what if the unexpected behavior happens in release build?

Copy link
Member

Choose a reason for hiding this comment

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

for any issue check, we will use the debug version. Here, it's just help us quick find the problem. debug log will also not output in release version.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Per offline talk with Jiaxin, will add CpuDeadLoop here.
It maybe a better choice here to expose the invalid resource HOB range(MMIO range from platform) here early

(*MemoryRegion)[Index].Attribute = EFI_MEMORY_XP;
if (Hob.ResourceDescriptor->ResourceAttribute == EFI_RESOURCE_ATTRIBUTE_READ_ONLY_PROTECTED) {
(*MemoryRegion)[Index].Attribute |= EFI_MEMORY_RO;
ResourceHobEnd = Hob.ResourceDescriptor->PhysicalStart + Hob.ResourceDescriptor->ResourceLength;
Copy link
Member

Choose a reason for hiding this comment

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

don't need change here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You mean remove line 175 directly use the variable in line 177? I'm fine with it

Copy link
Member

Choose a reason for hiding this comment

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

i mean once we assert, those code are not needed.

This commit is to check if the resource HOB range does not
exceed the max supported physical address.
The function BuildMemoryMapFromResDescHobs is to build Memory
Region from resource HOBs. Then the memory maps will be used
during creating or modifying SMM page table. If the resource
HOB range exceeds the max supported physical address, then
subsequent calling of PageTableMap() will fail.

Signed-off-by: Dun Tan <dun.tan@intel.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.

3 participants