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>
---
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
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#56301): https://edk2.groups.io/g/devel/message/56301
Mute This Topic: https://groups.io/mt/72543072/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
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
>
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#56386): https://edk2.groups.io/g/devel/message/56386
Mute This Topic: https://groups.io/mt/72543072/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
On Thu, 26 Mar 2020 at 11:22, Leif Lindholm <leif@nuviainc.com> 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 <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?
>
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
> >
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#56392): https://edk2.groups.io/g/devel/message/56392
Mute This Topic: https://groups.io/mt/72543072/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
Reviewed-by: Ashish Singhal <ashishsingha@nvidia.com>
Tested-by: Ashish Singhal <ashishsingha@nvidia.com>
________________________________
From: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Sent: Wednesday, March 25, 2020 9:29 AM
To: devel@edk2.groups.io <devel@edk2.groups.io>
Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>; Laszlo Ersek <lersek@redhat.com>; Leif Lindholm <leif@nuviainc.com>; Ashish Singhal <ashishsingha@nvidia.com>
Subject: [PATCH v3 1/3] ArmPkg/ArmMmuLib AARCH64: limit recursion when freeing page tables
External email: Use caution opening links or attachments
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>
---
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
-----------------------------------------------------------------------------------
This email message is for the sole use of the intended recipient(s) and may contain
confidential information. Any unauthorized review, use, disclosure or distribution
is prohibited. If you are not the intended recipient, please contact the sender by
reply email and destroy all copies of the original message.
-----------------------------------------------------------------------------------
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#56304): https://edk2.groups.io/g/devel/message/56304
Mute This Topic: https://groups.io/mt/72543072/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
© 2016 - 2026 Red Hat, Inc.