[PATCH] ArmPkg/ArmMmuLib ARM: fix page size granularity in initial MMU setting

[PATCH] ArmPkg/ArmMmuLib ARM: fix page size granularity in initial MMU setting

[M1cha]

>From what I can see this bug dates back to the commit from 2011 where
support for this was added: 2cf4b60895f8a

The first problem is that PopulateLevel2PageTable overflows the
translation table buffer because it doesn't verify that the size
actually fits within one level 2 page table.

The second problem is that the loop in FillTranslationTable doesn't
care about the PhysicalBase or the RemainLength and always substracts
one section size from RemainLength.

Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Michael Zimmermann <sigmaepsilon92@gmail.com>
---
 ArmPkg/Library/ArmMmuLib/Arm/ArmMmuLibCore.c | 14 ++++++++++----
 1 file changed, 10 insertions(+), 4 deletions(-)

diff --git a/ArmPkg/Library/ArmMmuLib/Arm/ArmMmuLibCore.c b/ArmPkg/Library/ArmMmuLib/Arm/ArmMmuLibCore.c
index b02f6d7fc590..869c661b9964 100644
--- a/ArmPkg/Library/ArmMmuLib/Arm/ArmMmuLibCore.c
+++ b/ArmPkg/Library/ArmMmuLib/Arm/ArmMmuLibCore.c
@@ -202,6 +202,8 @@ PopulateLevel2PageTable (
   PageEntry = ((UINT32 *)(TranslationTable) + ((PhysicalBase & TT_DESCRIPTOR_PAGE_INDEX_MASK) >> TT_DESCRIPTOR_PAGE_BASE_SHIFT));
   Pages     = RemainLength / TT_DESCRIPTOR_PAGE_SIZE;
 
+  ASSERT (Pages <= TRANSLATION_TABLE_PAGE_COUNT);
+
   for (Index = 0; Index < Pages; Index++) {
     *PageEntry++     =  TT_DESCRIPTOR_PAGE_BASE_ADDRESS(PhysicalBase) | PageAttributes;
     PhysicalBase += TT_DESCRIPTOR_PAGE_SIZE;
@@ -273,6 +275,7 @@ FillTranslationTable (
         // Case: Physical address aligned on the Section Size (1MB) && the length is greater than the Section Size
         *SectionEntry++ = TT_DESCRIPTOR_SECTION_BASE_ADDRESS(PhysicalBase) | Attributes;
         PhysicalBase += TT_DESCRIPTOR_SECTION_SIZE;
+        RemainLength -= TT_DESCRIPTOR_SECTION_SIZE;
       } else {
         // Case: Physical address aligned on the Section Size (1MB) && the length does not fill a section
         PopulateLevel2PageTable (SectionEntry++, PhysicalBase, RemainLength, MemoryRegion->Attributes);
@@ -281,17 +284,20 @@ FillTranslationTable (
         break;
       }
     } else {
+      UINT32 PageMapStart = PhysicalBase % TT_DESCRIPTOR_SECTION_SIZE;
+      UINT32 PageMapLength = MIN(RemainLength, TT_DESCRIPTOR_SECTION_SIZE) - PageMapStart;
+
       // Case: Physical address NOT aligned on the Section Size (1MB)
-      PopulateLevel2PageTable (SectionEntry++, PhysicalBase, RemainLength, MemoryRegion->Attributes);
-      // Aligned the address
-      PhysicalBase = (PhysicalBase + TT_DESCRIPTOR_SECTION_SIZE) & ~(TT_DESCRIPTOR_SECTION_SIZE-1);
+      PopulateLevel2PageTable (SectionEntry++, PhysicalBase, PageMapLength, MemoryRegion->Attributes);
 
       // If it is the last entry
       if (RemainLength < TT_DESCRIPTOR_SECTION_SIZE) {
         break;
       }
+
+      PhysicalBase += PageMapLength;
+      RemainLength -= PageMapLength;
     }
-    RemainLength -= TT_DESCRIPTOR_SECTION_SIZE;
   }
 }
 
-- 
2.15.1

Re: [PATCH] ArmPkg/ArmMmuLib ARM: fix page size granularity in initial MMU setting

[Ard Biesheuvel]

On 19 December 2017 at 08:57, M1cha <sigmaepsilon92@gmail.com> wrote:
> From what I can see this bug dates back to the commit from 2011 where
> support for this was added: 2cf4b60895f8a
>
> The first problem is that PopulateLevel2PageTable overflows the
> translation table buffer because it doesn't verify that the size
> actually fits within one level 2 page table.
>
> The second problem is that the loop in FillTranslationTable doesn't
> care about the PhysicalBase or the RemainLength and always substracts
> one section size from RemainLength.
>
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Michael Zimmermann <sigmaepsilon92@gmail.com>

Thanks for the fix

Some comments below

> ---
>  ArmPkg/Library/ArmMmuLib/Arm/ArmMmuLibCore.c | 14 ++++++++++----
>  1 file changed, 10 insertions(+), 4 deletions(-)
>
> diff --git a/ArmPkg/Library/ArmMmuLib/Arm/ArmMmuLibCore.c b/ArmPkg/Library/ArmMmuLib/Arm/ArmMmuLibCore.c
> index b02f6d7fc590..869c661b9964 100644
> --- a/ArmPkg/Library/ArmMmuLib/Arm/ArmMmuLibCore.c
> +++ b/ArmPkg/Library/ArmMmuLib/Arm/ArmMmuLibCore.c
> @@ -202,6 +202,8 @@ PopulateLevel2PageTable (
>    PageEntry = ((UINT32 *)(TranslationTable) + ((PhysicalBase & TT_DESCRIPTOR_PAGE_INDEX_MASK) >> TT_DESCRIPTOR_PAGE_BASE_SHIFT));
>    Pages     = RemainLength / TT_DESCRIPTOR_PAGE_SIZE;
>
> +  ASSERT (Pages <= TRANSLATION_TABLE_PAGE_COUNT);
> +

This ASSERT by itself is not complete, given that PageEntry could
point halfway into the page. Could you rewrite this so it takes that
into account?

>    for (Index = 0; Index < Pages; Index++) {
>      *PageEntry++     =  TT_DESCRIPTOR_PAGE_BASE_ADDRESS(PhysicalBase) | PageAttributes;
>      PhysicalBase += TT_DESCRIPTOR_PAGE_SIZE;
> @@ -273,6 +275,7 @@ FillTranslationTable (
>          // Case: Physical address aligned on the Section Size (1MB) && the length is greater than the Section Size
>          *SectionEntry++ = TT_DESCRIPTOR_SECTION_BASE_ADDRESS(PhysicalBase) | Attributes;
>          PhysicalBase += TT_DESCRIPTOR_SECTION_SIZE;
> +        RemainLength -= TT_DESCRIPTOR_SECTION_SIZE;
>        } else {
>          // Case: Physical address aligned on the Section Size (1MB) && the length does not fill a section
>          PopulateLevel2PageTable (SectionEntry++, PhysicalBase, RemainLength, MemoryRegion->Attributes);

It seems to me that we can fold this call into the one below after
your change (and merge the nested if() conditions into a logical AND
of the two)

> @@ -281,17 +284,20 @@ FillTranslationTable (
>          break;
>        }
>      } else {
> +      UINT32 PageMapStart = PhysicalBase % TT_DESCRIPTOR_SECTION_SIZE;

Please declare variables at function scope. Also, no need for
PageMapStart, you can just subtract PhysicalBase %
TT_DESCRIPTOR_SECTION_SIZE directly.

> +      UINT32 PageMapLength = MIN(RemainLength, TT_DESCRIPTOR_SECTION_SIZE) - PageMapStart;

Space before ( please

> +
>        // Case: Physical address NOT aligned on the Section Size (1MB)
> -      PopulateLevel2PageTable (SectionEntry++, PhysicalBase, RemainLength, MemoryRegion->Attributes);
> -      // Aligned the address
> -      PhysicalBase = (PhysicalBase + TT_DESCRIPTOR_SECTION_SIZE) & ~(TT_DESCRIPTOR_SECTION_SIZE-1);
> +      PopulateLevel2PageTable (SectionEntry++, PhysicalBase, PageMapLength, MemoryRegion->Attributes);
>
>        // If it is the last entry
>        if (RemainLength < TT_DESCRIPTOR_SECTION_SIZE) {
>          break;
>        }
> +
> +      PhysicalBase += PageMapLength;
> +      RemainLength -= PageMapLength;
>      }
> -    RemainLength -= TT_DESCRIPTOR_SECTION_SIZE;
>    }
>  }
>
> --
> 2.15.1
>

Thanks,
Ard.