Re: [PATCH v3 1/3] ArmPkg/ArmMmuLib AARCH64: limit recursion when freeing page tables

["Leif Lindholm" <leif@nuviainc.com>]

On Wed, Mar 25, 2020 at 16:29:38 +0100, Ard Biesheuvel wrote:
> FreePageTablesRecursive () traverses the page table tree depth first
> to free all pages that it finds, without taking into account the
> level at which it is operating.
> 
> Since TT_TYPE_TABLE_ENTRY aliases TT_TYPE_BLOCK_ENTRY_LEVEL3, we cannot
> distinguish table entries from block entries unless we take the level
> into account, and so we may be dereferencing garbage if we happen to
> try and free a hierarchy of page tables that has level 3 pages in it.
> 
> Let's fix this by passing the level into FreePageTablesRecursive (),
> and limit the recursion to levels < 3.
> 
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>

LGTM
Reviewed-by: Leif Lindholm <leif@nuviainc.com>

One question, which does not require any action on this patch:
Could that 3 be a #define (or Pcd) for future-proofing, or do the
bindings already restrict us in ways that can't reasonably be tweaked?

> ---
>  ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c | 18 ++++++++++++------
>  1 file changed, 12 insertions(+), 6 deletions(-)
> 
> diff --git a/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c b/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c
> index a43d468b73ca..d78918cf7ba8 100644
> --- a/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c
> +++ b/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c
> @@ -142,15 +142,21 @@ ReplaceTableEntry (
>  STATIC
>  VOID
>  FreePageTablesRecursive (
> -  IN  UINT64  *TranslationTable
> +  IN  UINT64  *TranslationTable,
> +  IN  UINTN   Level
>    )
>  {
>    UINTN   Index;
>  
> -  for (Index = 0; Index < TT_ENTRY_COUNT; Index++) {
> -    if ((TranslationTable[Index] & TT_TYPE_MASK) == TT_TYPE_TABLE_ENTRY) {
> -      FreePageTablesRecursive ((VOID *)(UINTN)(TranslationTable[Index] &
> -                                               TT_ADDRESS_MASK_BLOCK_ENTRY));
> +  ASSERT (Level <= 3);
> +
> +  if (Level < 3) {
> +    for (Index = 0; Index < TT_ENTRY_COUNT; Index++) {
> +      if ((TranslationTable[Index] & TT_TYPE_MASK) == TT_TYPE_TABLE_ENTRY) {
> +        FreePageTablesRecursive ((VOID *)(UINTN)(TranslationTable[Index] &
> +                                                 TT_ADDRESS_MASK_BLOCK_ENTRY),
> +                                 Level + 1);
> +      }
>      }
>    }
>    FreePages (TranslationTable, 1);
> @@ -254,7 +260,7 @@ UpdateRegionMappingRecursive (
>            // possible for existing table entries, since we cannot revert the
>            // modifications we made to the subhierarchy it represents.)
>            //
> -          FreePageTablesRecursive (TranslationTable);
> +          FreePageTablesRecursive (TranslationTable, Level + 1);
>          }
>          return Status;
>        }
> -- 
> 2.17.1
>