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

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

On 2023-03-16 14:00, Ard Biesheuvel wrote:
> On Thu, 16 Mar 2023 at 14:51, Leif Lindholm <quic_llindhol@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 <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
>>>   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