Re: [PATCH v5 32/38] MdeModulePkg/DxeCore: Update memory protections before freeing a region

["Leif Lindholm" <quic_llindhol@quicinc.com>]

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 <ardb@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

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

/
    Leif

>  CoreInternalFreePages (
>    IN EFI_PHYSICAL_ADDRESS  Memory,
>    IN UINTN                 NumberOfPages,
> @@ -1574,6 +1574,13 @@ CoreInternalFreePages (
>    NumberOfPages += EFI_SIZE_TO_PAGES (Alignment) - 1;
>    NumberOfPages &= ~(EFI_SIZE_TO_PAGES (Alignment) - 1);
>  
> +  ApplyMemoryProtectionPolicy (
> +    Entry->Type,
> +    EfiConventionalMemory,
> +    Memory,
> +    EFI_PAGES_TO_SIZE (NumberOfPages)
> +    );
> +
>    if (MemoryType != NULL) {
>      *MemoryType = Entry->Type;
>    }
> @@ -1628,12 +1635,6 @@ CoreFreePages (
>        NULL
>        );
>      InstallMemoryAttributesTableOnMemoryAllocation (MemoryType);
> -    ApplyMemoryProtectionPolicy (
> -      MemoryType,
> -      EfiConventionalMemory,
> -      Memory,
> -      EFI_PAGES_TO_SIZE (NumberOfPages)
> -      );
>    }
>  
>    return Status;
> -- 
> 2.39.2
>