ArmPkg: ArmMmuLib: Pre-Allocate Page Table Memory

[os-d]

Description

Allocating memory when memory protection is active can cause the below infinite loop:

1. gCpu->SetMemoryAttributes(EFI_MEMORY_RO)
2. ArmSetMemoryAttributes ()
3. SetMemoryRegionAttribute()
4. UpdateRegionMapping()
5. UpdateRegionMappingRecursive()
6. AllocatePages() -> Need memory for a translation table entry
7. CoreAllocatePages()
8. ApplyMemoryProtectionPolicy() -> Policy says new page should be XN
9. gCpu->SetMemoryAttributes()
10. Back to 3

To fix this previously, CpuDxe would update conventional memory to be XN prior to installing the CpuArch protocol. However, when we transition to setting EFI_MEMORY_RP on free memory, this will no longer work. This a prerequisite to bring in the EFI_MEMORY_RP on free memory feature that has been discussed on the mailing list.

This PR updates ArmMmuLib to reserve page table memory for allocation during table spits to prevent the infinite loop. This follows the same pattern that the x86 CpuDxe does to preallocate page table memory:

edk2/UefiCpuPkg/CpuDxe/CpuPageTable.c

Line 1117 in f962adc

InitializePageTablePool (

.

It also updates the consumers of ArmMmuLib to consume the PEI version of the lib in the same commit so as to not break them.

Continuous-integration-options: PatchCheck.ignore-multi-package

• Breaking change?
  • Breaking change - Does this PR cause a break in build or boot behavior?
  • Examples: Does it add a new library class or move a module to a different repo.
• Impacts security?
  • Security - Does this PR have a direct security impact?
  • Examples: Crypto algorithm change or buffer overflow fix.
• Includes tests?
  • Tests - Does this PR include any explicit test code?
  • Examples: Unit tests or integration tests.

How This Was Tested

Tested on ArmVirtQemu by creating the scenario where the infinite loop (without the XN remap routine in place) and booting successfully. This was also tested using the EFI_MEMORY_RP on free memory feature that is pending upstreaming. This has been shipping in platforms.

Integration Instructions

Platforms which are using ArmMmuBaseLib for PEIM, PEI_CORE, and SEC
modules will need to switch those module types to use ArmMmuPeiLib.

[LibraryClasses.common.SEC]
  ArmMmuLib|ArmPkg/Library/ArmMmuLib/ArmMmuSecLib.inf

[LibraryClasses.common.PEIM, LibraryClasses.common.PEI_CORE]
  ArmMmuLib|ArmPkg/Library/ArmMmuLib/ArmMmuPeiLib.inf

Platforms will also need to remove gArmTokenSpaceGuid.PcdRemapUnusedMemoryNx as it has been removed.

[LeviYeoReum]

I think this patch overall good to me about the table entry pool.
But I have a question for

When we transition to setting EFI_MEMORY_RP on free memory, this will no longer work.

I don’t catch this sentence.
Because, the I don't know why EFI_MEMORY_RP affects for
step 8. ApplyMemoryProtectionPolicy() -> Policy says new page should be XN gCpu->SetMemoryAttributes()
The allocation of table-entry former is always type change from EfiConventional to EfiBootServiceData.
So if PcdDxeNxMemoryProtectionPolicy of EfiConventional and EfiBootServiceData are the same, it doesn't happen.

Also,

when we transition to setting EFI_MEMORY_RP on free memory

If setting EFI_MEMORY_RP on free memory (X address)
and supposes it requires to allocate table entry (In assumption that former X address are block-entry and EFI_MEMORY_RO)
While trying to allocate table entry, pool doesn't have enough space so try to expand table-entry pool
But, it returns the X address. Then what happen?
It seems altough it was free memory, but it would be changed with "Allocated" used by table-entry pool with "EFI_MEMORY_RP".

Because I couldn't find out discussion on maling list, above quetions seems silly.
but, Would you correct me if i miss something?

Thanks.

[os-d]

I think this patch overall good to me about the table entry pool. But I have a question for

When we transition to setting EFI_MEMORY_RP on free memory, this will no longer work.

I don’t catch this sentence. Because, the I don't know why EFI_MEMORY_RP affects for step 8. ApplyMemoryProtectionPolicy() -> Policy says new page should be XN gCpu->SetMemoryAttributes() The allocation of table-entry former is always type change from EfiConventional to EfiBootServiceData. So if PcdDxeNxMemoryProtectionPolicy of EfiConventional and EfiBootServiceData are the same, it doesn't happen.

Also,

when we transition to setting EFI_MEMORY_RP on free memory

If setting EFI_MEMORY_RP on free memory (X address) and supposes it requires to allocate table entry (In assumption that former X address are block-entry and EFI_MEMORY_RO) While trying to allocate table entry, pool doesn't have enough space so try to expand table-entry pool But, it returns the X address. Then what happen? It seems altough it was free memory, but it would be changed with "Allocated" used by table-entry pool with "EFI_MEMORY_RP".

Because I couldn't find out discussion on maling list, above quetions seems silly. but, Would you correct me if i miss something?

Thanks.

@LeviYeoReum thanks for reviewing this PR! I apologize, I should have been clearer on the EFI_MEMORY_RP on free memory comments. This was discussed on the mailing list with Ray some time back and I don't quite recall where, but we have a change we are planning on upstreaming which removes FreedMemoryGuard and by default puts all memory as EFI_MEMORY_RP, i.e. performing that same functionality but always and in an easier to maintain way (last time I tried FreedMemoryGuard was broken on both OVMF and ArmVirtPkg). So, this PR is a prerequisite to that work being upstreamed. Here is the bulk of the change we will be upstreaming: microsoft/mu_basecore@e20bd71. There are other pieces that need to go in first and there will be discussion on how it looks in the end, of course.

In the EFI_MEMORY_RP on free memory case, Page.c sets the attributes to EFI_MEMORY_RP when memory is freed and then to whatever appropriate type when the memory is allocated (in your example above it would set it to EFI_MEMORY_XP for EfiBootServicesData. In order for these attribute changes to work though, we need preallocated memory for the page table pool, like x86 does.

Please let me know if you have further questions.

[LeviYeoReum]

Hi @os-d.
Thanks for detail explaination.

In the EFI_MEMORY_RP on free memory case, Page.c sets the attributes to EFI_MEMORY_RP when memory is freed and then to whatever appropriate type when the memory is allocated (in your example above it would set it to EFI_MEMORY_XP for EfiBootServicesData. In order for these attribute changes to work though, we need preallocated memory for the page table pool, like x86 does.

Yes. but in ApplyMemoryProtectionPolicy(), If EfiBootServiceData and EfiConventional are set with "1" both by PcdDxeNxMemoryProtectionPolicy,

  1098   if (OldType != EfiMaxMemoryType) {
  1099     OldAttributes = GetPermissionAttributeForMemoryType (OldType);
  1100     if (OldAttributes == NewAttributes) {
  1101       // policy is the same between OldType and NewType
  1102       return EFI_SUCCESS;
  1103     }
  1104   }

It seems to return EFI_SUCCESS without calling gCpu->SetAttributes() by line 1100.
GuardPage is type of change from/to EfiConventaional/EfiBootServiceData and vise versa.
So if PcdDxeNxMemoryProtectionPolicy's EfiConventaional and EfiBootServiceData are set 1 together,
OldAttrivutes == NewAttributes == EFI_MEMORY_XP, It wouldn't call recursively in steps 8 & 9.

Am I missing?

Thanks

[os-d]

Hi @os-d. Thanks for detail explaination.

In the EFI_MEMORY_RP on free memory case, Page.c sets the attributes to EFI_MEMORY_RP when memory is freed and then to whatever appropriate type when the memory is allocated (in your example above it would set it to EFI_MEMORY_XP for EfiBootServicesData. In order for these attribute changes to work though, we need preallocated memory for the page table pool, like x86 does.

Yes. but in ApplyMemoryProtectionPolicy(), If EfiBootServiceData and EfiConventional are set with "1" both by PcdDxeNxMemoryProtectionPolicy,

  1098   if (OldType != EfiMaxMemoryType) {
  1099     OldAttributes = GetPermissionAttributeForMemoryType (OldType);
  1100     if (OldAttributes == NewAttributes) {
  1101       // policy is the same between OldType and NewType
  1102       return EFI_SUCCESS;
  1103     }
  1104   }

It seems to return EFI_SUCCESS without calling gCpu->SetAttributes() by line 1100. GuardPage is type of change from/to EfiConventaional/EfiBootServiceData and vise versa. So if PcdDxeNxMemoryProtectionPolicy's EfiConventaional and EfiBootServiceData are set 1 together, OldAttrivutes == NewAttributes == EFI_MEMORY_XP, It wouldn't call recursively in steps 8 & 9.

Am I missing?

Thanks

@LeviYeoReum ,

In the EFI_MEMORY_RP on free memory commit, we do the below:

So GetPermissionAttributeForMemoryType will return EFI_MEMORY_RP|EFI_MEMORY_XP for EfiConventionalMemory and EFI_MEMORY_XP for EfiBootServicesData, so in the code snippet you sent, OldAttributes != NewAttributes and we'll call gCpu->SetMemoryAttributes, which is what then can lead into the recursion.

[LeviYeoReum]

I see. I've missed that code :) only see the Page.c.
Thanks to let me know! I understand completely

[os-d]

I see. I've missed that code :) only see the Page.c. Thanks to let me know! I understand completely

No worries, thanks for the review!

@kraxel @leiflindholm @samimujawar can you please review?

[os-d]

@samimujawar I addressed your comments, can you please re-review?

[os-d]

@samimujawar friendly ping to re-review

[samimujawar]

Apologies for the delay. I will review this shortly.

[samimujawar]

Hi Oliver,
Apologies for the delay in getting back, and thank you for the updated patch series.

I have some minor comments marked inline.
Other than that do you plan to send a patch series to update the platforms in the edk2-platforms repo?

Regards,

Sami Mujawar

[os-d]

Hi Oliver, Apologies for the delay in getting back, and thank you for the updated patch series.

I have some minor comments marked inline. Other than that do you plan to send a patch series to update the platforms in the edk2-platforms repo?

Thanks for reviewing! I have pushed an update addressing your comments, can you please re-review? I will put up a PR in edk2-platforms to resolve issues there and wait to check it in until this goes in.

[os-d]

Here is the edk2-platforms PR: tianocore/edk2-platforms#235.

[ardbiesheuvel]

This is far too intrusive.

The page table walker does not actually require readable pages, only the CPU needs these pages to be read-/writable while it is manipulating the contents, but this could potentially happen via a different mapping altogether.

Also, I don't think we have fully explored other options, including mapping some or all of the VA space eagerly.

[os-d]

Hi @ardbiesheuvel ,

This is far too intrusive.

This is the same approach x86 CpuDxe takes:

edk2/UefiCpuPkg/CpuDxe/CpuPageTable.c

Line 1117 in f962adc

InitializePageTablePool (

. Having a pre-allocated pool feels less intrusive than eagerly mapping all DRAM ranges. However, I don't disagree that it would be cleaner to not do any memory allocations in the page table code.

The page table walker does not actually require readable pages, only the CPU needs these pages to be read-/writable while it is manipulating the contents, but this could potentially happen via a different mapping altogether.

I'm not quite following, which part of this PR are you referencing as changing the semantics here? And what change would you like to see?

Also, I don't think we have fully explored other options, including mapping some or all of the VA space eagerly.
This is where we left things when Taylor was trying to upstream this:

• Not allocating memory in the page table manipulation code is a good thing
• Our main goal is to bring memory protection improvements to edk2. RP on free memory being one we have in Mu, which we are using this solution (matching x86) to achieve.
• Without this PR, we are blocked on bringing RP on free memory to edk2.
• We don't have the bandwidth to code up and test the eagerly allocating code
• We are concerned about the performance on server systems, but acknowledge that this could be unfounded

Do you have the bandwidth to put up a proof of concept of the eager mapping? We can test on our side and share the performance results if so.

If you do not have the bandwidth, then can we work out an acceptable version of this PR to avoid blocking upstreaming greater memory protections for edk2 and have the eager mapping be revisted (tracked in a GH issue or bugzilla) when there is bandwidth?

[ardbiesheuvel]

Hi @ardbiesheuvel ,

This is far too intrusive.

This is the same approach x86 CpuDxe takes:

edk2/UefiCpuPkg/CpuDxe/CpuPageTable.c

Line 1117 in f962adc

InitializePageTablePool (

I am aware of that. But that doesn't mean it is a good idea.

. Having a pre-allocated pool feels less intrusive than eagerly mapping all DRAM ranges. However, I don't disagree that it would be cleaner to not do any memory allocations in the page table code.

Indeed. We'll always need to perform some allocations, but splitting block entries is tricky for multiple reasons, and there is some rather hairy code that fiddles with the MMU in order not to violate architectural rules regarding Break-Before-Make etc.

In the early code, I don't think mapping eagerly is feasible, as memory might be restricted. But later on, it should be possible, given that the one thing we have in abundance in the boot stage is RAM.

The page table walker does not actually require readable pages, only the CPU needs these pages to be read-/writable while it is manipulating the contents, but this could potentially happen via a different mapping altogether.

I'm not quite following, which part of this PR are you referencing as changing the semantics here? And what change would you like to see?

Yeah, I should clarify that - see my long term plan below.

Also, I don't think we have fully explored other options, including mapping some or all of the VA space eagerly.
This is where we left things when Taylor was trying to upstream this:

• Not allocating memory in the page table manipulation code is a good thing
• Our main goal is to bring memory protection improvements to edk2. RP on free memory being one we have in Mu, which we are using this solution (matching x86) to achieve.
• Without this PR, we are blocked on bringing RP on free memory to edk2.
• We don't have the bandwidth to code up and test the eagerly allocating code
• We are concerned about the performance on server systems, but acknowledge that this could be unfounded

Boot performance on server systems is not going to be impacted by this. And especially on servers, fiddling with the MMU and TLBs etc is likely to have a performance impact as well.

Do you have the bandwidth to put up a proof of concept of the eager mapping? We can test on our side and share the performance results if so.

Sure.

If you do not have the bandwidth, then can we work out an acceptable version of this PR to avoid blocking upstreaming greater memory protections for edk2 and have the eager mapping be revisted (tracked in a GH issue or bugzilla) when there is bandwidth?

I have a short-term and a long-term plan:

The short-term plan is to extend the existing API so that the call that remaps all unused memory RP does so eagerly, so that no additional page allocations are needed to remap these pages when they are allocated

In the long term, what I would like to do is not map the page tables at all. Instead, we could have two sets of page tables, where the second set covers all of DRAM mapped lazily without restricted permissions, and this set is only used by the MMU library code while it is manipulating the mappings (with interrupts disabled). That way, all the dodgy code that fiddles with the MMU can go too, as the code no longer ever operates on live mappings. But it also means that page tables are never accessed via the primary set of page tables, and so all page tables can be unmapped (or mapped RP), and there is no reason to remap them writable, and that provides some robustness as well.

That would also permit us to switch back to lazy mapping, but I honestly don't think it makes a difference.

[os-d]

Do you have the bandwidth to put up a proof of concept of the eager mapping? We can test on our side and share the performance results if so.

Sure.

Thanks, I'll give it a test when it is ready.

I have a short-term and a long-term plan:

The short-term plan is to extend the existing API so that the call that remaps all unused memory RP does so eagerly, so that no additional page allocations are needed to remap these pages when they are allocated

In the long term, what I would like to do is not map the page tables at all. Instead, we could have two sets of page tables, where the second set covers all of DRAM mapped lazily without restricted permissions, and this set is only used by the MMU library code while it is manipulating the mappings (with interrupts disabled). That way, all the dodgy code that fiddles with the MMU can go too, as the code no longer ever operates on live mappings. But it also means that page tables are never accessed via the primary set of page tables, and so all page tables can be unmapped (or mapped RP), and there is no reason to remap them writable, and that provides some robustness as well.

That would also permit us to switch back to lazy mapping, but I honestly don't think it makes a difference.

This plan makes sense to me, thanks for driving this.

[mergify]

PR can not be merged due to conflict. Please rebase and resubmit