On 2023-03-16 14:00, Ard Biesheuvel wrote:
On Thu, 16 Mar 2023 at 14:51, Leif Lindholm <quic_llind...@quicinc.com> wrote:

On Mon, Mar 13, 2023 at 18:17:08 +0100, Ard Biesheuvel wrote:
Currently, we invoke ApplyMemoryProtectionPolicy() after
CoreInternalFreePages() has returned successfully, in order to update
the memory permission attributes of the region to match the policy for
EfiConventionalMemory.

There are two problems with that:
- CoreInternalFreePages() will round up the size of the allocation to
   the appropriate alignment of the memory type, but we only remap the
   number of pages that was passed by the caller, leaving the remaining
   pages freed but mapped with the old permissions;
- in DEBUG builds, we may attempt to clear the entire region while it is
   still mapped with read-only or read-protect attributes.

Let's address both issues, by updating the permissions before performing
the actual conversion.

Signed-off-by: Ard Biesheuvel <a...@kernel.org>
---
  MdeModulePkg/Core/Dxe/Mem/Page.c | 15 ++++++++-------
  1 file changed, 8 insertions(+), 7 deletions(-)

diff --git a/MdeModulePkg/Core/Dxe/Mem/Page.c b/MdeModulePkg/Core/Dxe/Mem/Page.c
index 5903ce7ab525..f5b940bbc25b 100644
--- a/MdeModulePkg/Core/Dxe/Mem/Page.c
+++ b/MdeModulePkg/Core/Dxe/Mem/Page.c
@@ -1519,8 +1519,8 @@ CoreAllocatePages (
    @return EFI_SUCCESS         -Pages successfully freed.

  **/
+STATIC
  EFI_STATUS
-EFIAPI
  CoreInternalFreePages (

This is addressing a historic oversight (possibly caused by the STATIC
function ban), but it's not *really* related to the change in question.


Not entirely. I am moving some logic from a caller into the callee.
This is only safe if there are no other callers, and making it STATIC
makes it more likely that other callers that may exist in one form or
another (i.e., out of tree forks) will fail at build time rather than
misbehave.

Or that was the rationale, anyway. I'm happy to drop it as well.

I'm OK with that rationale, but I think in that case it needs calling out in the commit message.

/
    Leif



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#101280): https://edk2.groups.io/g/devel/message/101280
Mute This Topic: https://groups.io/mt/97586052/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: 
https://edk2.groups.io/g/devel/leave/9847357/21656/1706620634/xyzzy 
[arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-


Reply via email to