On 19 December 2017 at 08:57, M1cha <[email protected]> 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 <[email protected]>
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.
_______________________________________________
edk2-devel mailing list
[email protected]
https://lists.01.org/mailman/listinfo/edk2-devel
