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

[Ard Biesheuvel <ard.biesheuvel@linaro.org>]

On 20 December 2017 at 19:28, 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>
> ---
>
> v2: implement changes as requested by Ard.
>

Thanks for the fix, and thanks for reworking it.

Reviewed-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>

Pushed as 889c7ca1b5e8

>  ArmPkg/Library/ArmMmuLib/Arm/ArmMmuLibCore.c | 34 ++++++++++----------
>  1 file changed, 17 insertions(+), 17 deletions(-)
>
> diff --git a/ArmPkg/Library/ArmMmuLib/Arm/ArmMmuLibCore.c b/ArmPkg/Library/ArmMmuLib/Arm/ArmMmuLibCore.c
> index b02f6d7fc590..8f85cf678c98 100644
> --- a/ArmPkg/Library/ArmMmuLib/Arm/ArmMmuLibCore.c
> +++ b/ArmPkg/Library/ArmMmuLib/Arm/ArmMmuLibCore.c
> @@ -128,6 +128,7 @@ PopulateLevel2PageTable (
>    UINT32  SectionDescriptor;
>    UINT32  TranslationTable;
>    UINT32  BaseSectionAddress;
> +  UINT32  FirstPageOffset;
>
>    switch (Attributes) {
>      case ARM_MEMORY_REGION_ATTRIBUTE_WRITE_BACK:
> @@ -199,9 +200,12 @@ PopulateLevel2PageTable (
>          TT_DESCRIPTOR_SECTION_TYPE_PAGE_TABLE;
>    }
>
> -  PageEntry = ((UINT32 *)(TranslationTable) + ((PhysicalBase & TT_DESCRIPTOR_PAGE_INDEX_MASK) >> TT_DESCRIPTOR_PAGE_BASE_SHIFT));
> +  FirstPageOffset = ((PhysicalBase & TT_DESCRIPTOR_PAGE_INDEX_MASK) >> TT_DESCRIPTOR_PAGE_BASE_SHIFT);
> +  PageEntry = ((UINT32 *)(TranslationTable) + FirstPageOffset);
>    Pages     = RemainLength / TT_DESCRIPTOR_PAGE_SIZE;
>
> +  ASSERT (FirstPageOffset + Pages <= TRANSLATION_TABLE_PAGE_COUNT);
> +
>    for (Index = 0; Index < Pages; Index++) {
>      *PageEntry++     =  TT_DESCRIPTOR_PAGE_BASE_ADDRESS(PhysicalBase) | PageAttributes;
>      PhysicalBase += TT_DESCRIPTOR_PAGE_SIZE;
> @@ -220,6 +224,7 @@ FillTranslationTable (
>    UINT32  Attributes;
>    UINT32  PhysicalBase;
>    UINT64  RemainLength;
> +  UINT32  PageMapLength;
>
>    ASSERT(MemoryRegion->Length > 0);
>
> @@ -268,30 +273,25 @@ FillTranslationTable (
>    SectionEntry    = TRANSLATION_TABLE_ENTRY_FOR_VIRTUAL_ADDRESS(TranslationTable, MemoryRegion->VirtualBase);
>
>    while (RemainLength != 0) {
> -    if (PhysicalBase % TT_DESCRIPTOR_SECTION_SIZE == 0) {
> -      if (RemainLength >= TT_DESCRIPTOR_SECTION_SIZE) {
> -        // 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;
> -      } else {
> -        // Case: Physical address aligned on the Section Size (1MB) && the length does not fill a section
> -        PopulateLevel2PageTable (SectionEntry++, PhysicalBase, RemainLength, MemoryRegion->Attributes);
> -
> -        // It must be the last entry
> -        break;
> -      }
> +    if ((PhysicalBase % TT_DESCRIPTOR_SECTION_SIZE == 0) && RemainLength >= TT_DESCRIPTOR_SECTION_SIZE) {
> +      // 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 {
> +      PageMapLength = MIN (RemainLength, TT_DESCRIPTOR_SECTION_SIZE) - (PhysicalBase % TT_DESCRIPTOR_SECTION_SIZE);
> +
>        // 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
>