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 (
   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



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#101136): https://edk2.groups.io/g/devel/message/101136
Mute This Topic: https://groups.io/mt/97586052/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

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
> 


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#101273): https://edk2.groups.io/g/devel/message/101273
Mute This Topic: https://groups.io/mt/97586052/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/leave/3901457/1787277/102458076/xyzzy [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

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.


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#101274): https://edk2.groups.io/g/devel/message/101274
Mute This Topic: https://groups.io/mt/97586052/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

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



-=-=-=-=-=-=-=-=-=-=-=-
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/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/leave/3901457/1787277/102458076/xyzzy [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-