1. Patchew
  2. EDK2
  3. [edk2-devel] [PATCH v5 00/38] Implement strict memory permissions throughout
  4. View patch

[edk2-devel] [PATCH v5 08/38] ArmPkg/ArmMmuLib: Avoid splitting block entries if possible

Ard Biesheuvel posted 38 patches 1 year, 10 months ago
[edk2-devel] [PATCH v5 08/38] ArmPkg/ArmMmuLib: Avoid splitting block entries if possible
Posted by Ard Biesheuvel 1 year, 10 months ago 
Currently, the ARM MMU page table logic will break down any block entry
that overlaps with the region being mapped, even if the block entry in
question is using the same attributes as the new region.

This means that creating a non-executable mapping inside a region that
is already mapped non-executable at a coarser granularity may trigger a
call to AllocatePages (), which may recurse back into the page table
code to update the attributes on the newly allocated page tables.

Let's avoid this, by preserving the block entry if it already covers the
region being mapped with the correct attributes.

Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
---
 ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c | 10 ++++++++++
 ArmPkg/Library/ArmMmuLib/Arm/ArmMmuLibUpdate.c   | 11 +++++++++++
 2 files changed, 21 insertions(+)

diff --git a/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c b/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c
index 6d21a2e41dd1..1ce200c43c72 100644
--- a/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c
+++ b/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c
@@ -251,6 +251,16 @@ UpdateRegionMappingRecursive (
       ASSERT (Level < 3);
 
       if (!IsTableEntry (*Entry, Level)) {
+        //
+        // If the region we are trying to map is already covered by a block
+        // entry with the right attributes, don't bother splitting it up.
+        //
+        if (IsBlockEntry (*Entry, Level) &&
+            ((*Entry & TT_ATTRIBUTES_MASK & ~AttributeClearMask) == AttributeSetMask))
+        {
+          continue;
+        }
+
         //
         // No table entry exists yet, so we need to allocate a page table
         // for the next level.
diff --git a/ArmPkg/Library/ArmMmuLib/Arm/ArmMmuLibUpdate.c b/ArmPkg/Library/ArmMmuLib/Arm/ArmMmuLibUpdate.c
index 247cf87bf3d3..299d38ad07e8 100644
--- a/ArmPkg/Library/ArmMmuLib/Arm/ArmMmuLibUpdate.c
+++ b/ArmPkg/Library/ArmMmuLib/Arm/ArmMmuLibUpdate.c
@@ -170,6 +170,17 @@ UpdatePageEntries (
 
     // Does this descriptor need to be converted from section entry to 4K pages?
     if (!TT_DESCRIPTOR_SECTION_TYPE_IS_PAGE_TABLE (Descriptor)) {
+      //
+      // If the section mapping covers the requested region with the expected
+      // attributes, splitting it is unnecessary, and should be avoided as it
+      // may result in unbounded recursion when using a strict NX policy.
+      //
+      if ((EntryValue & ~TT_DESCRIPTOR_PAGE_TYPE_MASK & EntryMask) ==
+          (ConvertSectionAttributesToPageAttributes (Descriptor) & EntryMask))
+      {
+        continue;
+      }
+
       Status = ConvertSectionToPages (FirstLevelIdx << TT_DESCRIPTOR_SECTION_BASE_SHIFT);
       if (EFI_ERROR (Status)) {
         // Exit for loop
-- 
2.39.2



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#101112): https://edk2.groups.io/g/devel/message/101112
Mute This Topic: https://groups.io/mt/97585995/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] [PATCH v5 08/38] ArmPkg/ArmMmuLib: Avoid splitting block entries if possible
Posted by Leif Lindholm 1 year, 10 months ago 
On Mon, Mar 13, 2023 at 18:16:44 +0100, Ard Biesheuvel wrote:
> Currently, the ARM MMU page table logic will break down any block entry
> that overlaps with the region being mapped, even if the block entry in
> question is using the same attributes as the new region.
> 
> This means that creating a non-executable mapping inside a region that
> is already mapped non-executable at a coarser granularity may trigger a
> call to AllocatePages (), which may recurse back into the page table
> code to update the attributes on the newly allocated page tables.
> 
> Let's avoid this, by preserving the block entry if it already covers the
> region being mapped with the correct attributes.

So if a later mapping is made inside the same block with conflicting
attributes? That triggers the break down at that point and because the
existing mapping did not conflict, it'll all flush out?

/
    Leif

> Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
> ---
>  ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c | 10 ++++++++++
>  ArmPkg/Library/ArmMmuLib/Arm/ArmMmuLibUpdate.c   | 11 +++++++++++
>  2 files changed, 21 insertions(+)
> 
> diff --git a/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c b/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c
> index 6d21a2e41dd1..1ce200c43c72 100644
> --- a/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c
> +++ b/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c
> @@ -251,6 +251,16 @@ UpdateRegionMappingRecursive (
>        ASSERT (Level < 3);
>  
>        if (!IsTableEntry (*Entry, Level)) {
> +        //
> +        // If the region we are trying to map is already covered by a block
> +        // entry with the right attributes, don't bother splitting it up.
> +        //
> +        if (IsBlockEntry (*Entry, Level) &&
> +            ((*Entry & TT_ATTRIBUTES_MASK & ~AttributeClearMask) == AttributeSetMask))
> +        {
> +          continue;
> +        }
> +
>          //
>          // No table entry exists yet, so we need to allocate a page table
>          // for the next level.
> diff --git a/ArmPkg/Library/ArmMmuLib/Arm/ArmMmuLibUpdate.c b/ArmPkg/Library/ArmMmuLib/Arm/ArmMmuLibUpdate.c
> index 247cf87bf3d3..299d38ad07e8 100644
> --- a/ArmPkg/Library/ArmMmuLib/Arm/ArmMmuLibUpdate.c
> +++ b/ArmPkg/Library/ArmMmuLib/Arm/ArmMmuLibUpdate.c
> @@ -170,6 +170,17 @@ UpdatePageEntries (
>  
>      // Does this descriptor need to be converted from section entry to 4K pages?
>      if (!TT_DESCRIPTOR_SECTION_TYPE_IS_PAGE_TABLE (Descriptor)) {
> +      //
> +      // If the section mapping covers the requested region with the expected
> +      // attributes, splitting it is unnecessary, and should be avoided as it
> +      // may result in unbounded recursion when using a strict NX policy.
> +      //
> +      if ((EntryValue & ~TT_DESCRIPTOR_PAGE_TYPE_MASK & EntryMask) ==
> +          (ConvertSectionAttributesToPageAttributes (Descriptor) & EntryMask))
> +      {
> +        continue;
> +      }
> +
>        Status = ConvertSectionToPages (FirstLevelIdx << TT_DESCRIPTOR_SECTION_BASE_SHIFT);
>        if (EFI_ERROR (Status)) {
>          // Exit for loop
> -- 
> 2.39.2
> 
> 
> 
> 
> 
> 


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#101172): https://edk2.groups.io/g/devel/message/101172
Mute This Topic: https://groups.io/mt/97585995/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/leave/3901457/1787277/102458076/xyzzy [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] [PATCH v5 08/38] ArmPkg/ArmMmuLib: Avoid splitting block entries if possible
Posted by Ard Biesheuvel 1 year, 10 months ago 
On Tue, 14 Mar 2023 at 19:13, Leif Lindholm <quic_llindhol@quicinc.com> wrote:
>
> On Mon, Mar 13, 2023 at 18:16:44 +0100, Ard Biesheuvel wrote:
> > Currently, the ARM MMU page table logic will break down any block entry
> > that overlaps with the region being mapped, even if the block entry in
> > question is using the same attributes as the new region.
> >
> > This means that creating a non-executable mapping inside a region that
> > is already mapped non-executable at a coarser granularity may trigger a
> > call to AllocatePages (), which may recurse back into the page table
> > code to update the attributes on the newly allocated page tables.
> >
> > Let's avoid this, by preserving the block entry if it already covers the
> > region being mapped with the correct attributes.
>
> So if a later mapping is made inside the same block with conflicting
> attributes? That triggers the break down at that point and because the
> existing mapping did not conflict, it'll all flush out?
>

Indeed.

The case here is simply, e.g., mapping a single page XP that is
already covered by a 2 MB XP block: without this patch, we break down
that 2 MB block into page mappings that all have the same attributes.
If the 4k page being remapped is being allocated for a page table, we
may end up with unbounded recursion.

If the attributes are actually different, the split still happens. But
otherwise, the block mapping is retained.


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#101173): https://edk2.groups.io/g/devel/message/101173
Mute This Topic: https://groups.io/mt/97585995/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] [PATCH v5 08/38] ArmPkg/ArmMmuLib: Avoid splitting block entries if possible
Posted by Leif Lindholm 1 year, 10 months ago 
On Tue, Mar 14, 2023 at 19:29:39 +0100, Ard Biesheuvel wrote:
> On Tue, 14 Mar 2023 at 19:13, Leif Lindholm <quic_llindhol@quicinc.com> wrote:
> >
> > On Mon, Mar 13, 2023 at 18:16:44 +0100, Ard Biesheuvel wrote:
> > > Currently, the ARM MMU page table logic will break down any block entry
> > > that overlaps with the region being mapped, even if the block entry in
> > > question is using the same attributes as the new region.
> > >
> > > This means that creating a non-executable mapping inside a region that
> > > is already mapped non-executable at a coarser granularity may trigger a
> > > call to AllocatePages (), which may recurse back into the page table
> > > code to update the attributes on the newly allocated page tables.
> > >
> > > Let's avoid this, by preserving the block entry if it already covers the
> > > region being mapped with the correct attributes.
> >
> > So if a later mapping is made inside the same block with conflicting
> > attributes? That triggers the break down at that point and because the
> > existing mapping did not conflict, it'll all flush out?
> >
> 
> Indeed.
> 
> The case here is simply, e.g., mapping a single page XP that is
> already covered by a 2 MB XP block: without this patch, we break down
> that 2 MB block into page mappings that all have the same attributes.
> If the 4k page being remapped is being allocated for a page table, we
> may end up with unbounded recursion.
> 
> If the attributes are actually different, the split still happens. But
> otherwise, the block mapping is retained.

Makes sense.

Reviewed-by: Leif Lindholm <quic_llindhol@quicinc.com>


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

Patchew 2.1-devel-b67e4c2

© 2016 - 2025 Red Hat, Inc.