From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail-wm1-f66.google.com (mail-wm1-f66.google.com [209.85.128.66]) by mx.groups.io with SMTP id smtpd.web10.33977.1585218371428986910 for ; Thu, 26 Mar 2020 03:26:11 -0700 Authentication-Results: mx.groups.io; dkim=pass header.i=@linaro.org header.s=google header.b=jD9+/WBL; spf=pass (domain: linaro.org, ip: 209.85.128.66, mailfrom: ard.biesheuvel@linaro.org) Received: by mail-wm1-f66.google.com with SMTP id b12so5852424wmj.3 for ; Thu, 26 Mar 2020 03:26:11 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=2qX51Wd7bPYKYdt8VOL5AuVe50eMMfyABDrb6PYcblI=; b=jD9+/WBLRv/el4vh9nq/sx7YwR/6Gnb4G2dz61jzmkpPoA3o8SZIF/zCi8kDK+nNve WjviQC5SahBV0KZACJtYNZzG7lCjy6pcBc0M4jKZy9fxXXVucsEsa7tY9DMnj+Y7g8cJ Zc//oKFC2TkRX+Xl4iVFTrqZuHHY4Lxt+pw4j1SSWas/zmgiiWlh2nqw3mulwLog3QU4 I7sKaJ7uQczNpG92FU1s33bLWA2BhTrn6Wa9ifkfKHPpKyVUFFy8lVpDaXUMawCdy0Uf 2r2m7kgQR7ExQCJtji4F9ThRAAFs/KsRcINHAZ+OChEgoz+xAilL0LdlMlHC+cvnzMan 2UOQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=2qX51Wd7bPYKYdt8VOL5AuVe50eMMfyABDrb6PYcblI=; b=OwxPeFGfM+z8hqUC8LG10RDFY8ir2+Y2ciFskomzpAkqbHbO2tcoujAnUfo8E3AeIz +BzUMpdhUvcwpHiOSLK9LSFZrefqw/gIjIzZcYfd1kcgR3ZUdJYNLRA1ENCO3hD12U7d E3KzmwCIcGpf95lI3hkgdVEc/Ho0jDhBY+CBduDxX9ZfRxdbct2pmVQ3vESeQM0u6g/8 mrXkk6FPTWiF/Id07ARB8N9vRjYVrO+otDptlEXAsHUh80hFSnYwtmQuLYyIzPbD9+fR ZdZ4QwBVvm7r+OTAUvI09hSwR3RIKhWNuyDJo9fwNezxxLFzuVtAe00sMg3KEbWqrpRj FLgQ== X-Gm-Message-State: ANhLgQ0xkAiJM3HbyEynUgepX1u6cKaQ7qyUb2lGBTutTQQGhDbjpLJT xXnUjwQ47JJuuDjxf1xbjfbdMr5mVkGX1tS/cXIvIg== X-Google-Smtp-Source: ADFU+vtJl/+hhpIpRAyqjkUfmrRJ3I+59mm4eGIFbi0sUeeBDDN2vVbDVKCynjcB7rGhnWtM2lZpXm6G5VdYjL21uhs= X-Received: by 2002:a7b:c050:: with SMTP id u16mr2596725wmc.68.1585218369948; Thu, 26 Mar 2020 03:26:09 -0700 (PDT) MIME-Version: 1.0 References: <20200325152940.1492-1-ard.biesheuvel@linaro.org> <20200325152940.1492-2-ard.biesheuvel@linaro.org> <20200326102242.GO22097@bivouac.eciton.net> In-Reply-To: <20200326102242.GO22097@bivouac.eciton.net> From: "Ard Biesheuvel" Date: Thu, 26 Mar 2020 11:25:59 +0100 Message-ID: Subject: Re: [PATCH v3 1/3] ArmPkg/ArmMmuLib AARCH64: limit recursion when freeing page tables To: Leif Lindholm Cc: edk2-devel-groups-io , Laszlo Ersek , Ashish Singhal Content-Type: text/plain; charset="UTF-8" On Thu, 26 Mar 2020 at 11:22, Leif Lindholm wrote: > > 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 > > LGTM > Reviewed-by: Leif Lindholm > > 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? > We also have TT_TYPE_BLOCK_ENTRY_LEVEL3 (as opposed to TT_TYPE_BLOCK_ENTRY for other levels) so the '3' is rather set in stone, I'd say. > > --- > > 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 > >