* [PATCH v3 1/3] ArmPkg/ArmMmuLib AARCH64: limit recursion when freeing page tables
2020-03-25 15:29 [PATCH v3 0/3] ArmPkg/ArmMmuLib AARCH64: correctness fix Ard Biesheuvel
@ 2020-03-25 15:29 ` Ard Biesheuvel
2020-03-25 15:47 ` Ashish Singhal
2020-03-26 10:22 ` Leif Lindholm
2020-03-25 15:29 ` [PATCH v3 2/3] ArmPkg/ArmMmuLib AARCH64: use helpers to determine table entry types Ard Biesheuvel
` (3 subsequent siblings)
4 siblings, 2 replies; 12+ messages in thread
From: Ard Biesheuvel @ 2020-03-25 15:29 UTC (permalink / raw)
To: devel; +Cc: Ard Biesheuvel, Laszlo Ersek, Leif Lindholm, Ashish Singhal
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
^ permalink raw reply related [flat|nested] 12+ messages in thread* Re: [PATCH v3 1/3] ArmPkg/ArmMmuLib AARCH64: limit recursion when freeing page tables
2020-03-25 15:29 ` [PATCH v3 1/3] ArmPkg/ArmMmuLib AARCH64: limit recursion when freeing page tables Ard Biesheuvel
@ 2020-03-25 15:47 ` Ashish Singhal
2020-03-26 10:22 ` Leif Lindholm
1 sibling, 0 replies; 12+ messages in thread
From: Ashish Singhal @ 2020-03-25 15:47 UTC (permalink / raw)
To: Ard Biesheuvel, devel@edk2.groups.io; +Cc: Laszlo Ersek, Leif Lindholm
[-- Attachment #1: Type: text/plain, Size: 3464 bytes --]
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.
-----------------------------------------------------------------------------------
[-- Attachment #2: Type: text/html, Size: 6857 bytes --]
^ permalink raw reply related [flat|nested] 12+ messages in thread* Re: [PATCH v3 1/3] ArmPkg/ArmMmuLib AARCH64: limit recursion when freeing page tables
2020-03-25 15:29 ` [PATCH v3 1/3] ArmPkg/ArmMmuLib AARCH64: limit recursion when freeing page tables Ard Biesheuvel
2020-03-25 15:47 ` Ashish Singhal
@ 2020-03-26 10:22 ` Leif Lindholm
2020-03-26 10:25 ` Ard Biesheuvel
1 sibling, 1 reply; 12+ messages in thread
From: Leif Lindholm @ 2020-03-26 10:22 UTC (permalink / raw)
To: Ard Biesheuvel; +Cc: devel, Laszlo Ersek, Ashish Singhal
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
>
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: [PATCH v3 1/3] ArmPkg/ArmMmuLib AARCH64: limit recursion when freeing page tables
2020-03-26 10:22 ` Leif Lindholm
@ 2020-03-26 10:25 ` Ard Biesheuvel
0 siblings, 0 replies; 12+ messages in thread
From: Ard Biesheuvel @ 2020-03-26 10:25 UTC (permalink / raw)
To: Leif Lindholm; +Cc: edk2-devel-groups-io, Laszlo Ersek, Ashish Singhal
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
> >
^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH v3 2/3] ArmPkg/ArmMmuLib AARCH64: use helpers to determine table entry types
2020-03-25 15:29 [PATCH v3 0/3] ArmPkg/ArmMmuLib AARCH64: correctness fix Ard Biesheuvel
2020-03-25 15:29 ` [PATCH v3 1/3] ArmPkg/ArmMmuLib AARCH64: limit recursion when freeing page tables Ard Biesheuvel
@ 2020-03-25 15:29 ` Ard Biesheuvel
2020-03-25 15:47 ` Ashish Singhal
2020-03-25 15:29 ` [PATCH v3 3/3] ArmPkg/ArmMmuLib AARCH64: preserve attributes when replacing a table entry Ard Biesheuvel
` (2 subsequent siblings)
4 siblings, 1 reply; 12+ messages in thread
From: Ard Biesheuvel @ 2020-03-25 15:29 UTC (permalink / raw)
To: devel; +Cc: Ard Biesheuvel, Laszlo Ersek, Leif Lindholm, Ashish Singhal
Given how the meaning of the attribute bits for page table entry types
is slightly awkward, and changes between levels, add some helpers to
abstract from this.
Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Reviewed-by: Leif Lindholm <leif@nuviainc.com>
---
ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c | 40 +++++++++++++++++---
1 file changed, 35 insertions(+), 5 deletions(-)
diff --git a/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c b/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c
index d78918cf7ba8..0680ba36d907 100644
--- a/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c
+++ b/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c
@@ -162,6 +162,36 @@ FreePageTablesRecursive (
FreePages (TranslationTable, 1);
}
+STATIC
+BOOLEAN
+IsBlockEntry (
+ IN UINT64 Entry,
+ IN UINTN Level
+ )
+{
+ if (Level == 3) {
+ return (Entry & TT_TYPE_MASK) == TT_TYPE_BLOCK_ENTRY_LEVEL3;
+ }
+ return (Entry & TT_TYPE_MASK) == TT_TYPE_BLOCK_ENTRY;
+}
+
+STATIC
+BOOLEAN
+IsTableEntry (
+ IN UINT64 Entry,
+ IN UINTN Level
+ )
+{
+ if (Level == 3) {
+ //
+ // TT_TYPE_TABLE_ENTRY aliases TT_TYPE_BLOCK_ENTRY_LEVEL3
+ // so we need to take the level into account as well.
+ //
+ return FALSE;
+ }
+ return (Entry & TT_TYPE_MASK) == TT_TYPE_TABLE_ENTRY;
+}
+
STATIC
EFI_STATUS
UpdateRegionMappingRecursive (
@@ -203,7 +233,7 @@ UpdateRegionMappingRecursive (
if (Level == 0 || ((RegionStart | BlockEnd) & BlockMask) != 0) {
ASSERT (Level < 3);
- if ((*Entry & TT_TYPE_MASK) != TT_TYPE_TABLE_ENTRY) {
+ if (!IsTableEntry (*Entry, Level)) {
//
// No table entry exists yet, so we need to allocate a page table
// for the next level.
@@ -221,7 +251,7 @@ UpdateRegionMappingRecursive (
InvalidateDataCacheRange (TranslationTable, EFI_PAGE_SIZE);
}
- if ((*Entry & TT_TYPE_MASK) == TT_TYPE_BLOCK_ENTRY) {
+ if (IsBlockEntry (*Entry, Level)) {
//
// We are splitting an existing block entry, so we have to populate
// the new table with the attributes of the block entry it replaces.
@@ -252,7 +282,7 @@ UpdateRegionMappingRecursive (
AttributeSetMask, AttributeClearMask, TranslationTable,
Level + 1);
if (EFI_ERROR (Status)) {
- if ((*Entry & TT_TYPE_MASK) != TT_TYPE_TABLE_ENTRY) {
+ if (!IsTableEntry (*Entry, Level)) {
//
// We are creating a new table entry, so on failure, we can free all
// allocations we made recursively, given that the whole subhierarchy
@@ -265,10 +295,10 @@ UpdateRegionMappingRecursive (
return Status;
}
- if ((*Entry & TT_TYPE_MASK) != TT_TYPE_TABLE_ENTRY) {
+ if (!IsTableEntry (*Entry, Level)) {
EntryValue = (UINTN)TranslationTable | TT_TYPE_TABLE_ENTRY;
ReplaceTableEntry (Entry, EntryValue, RegionStart,
- (*Entry & TT_TYPE_MASK) == TT_TYPE_BLOCK_ENTRY);
+ IsBlockEntry (*Entry, Level));
}
} else {
EntryValue = (*Entry & AttributeClearMask) | AttributeSetMask;
--
2.17.1
^ permalink raw reply related [flat|nested] 12+ messages in thread* Re: [PATCH v3 2/3] ArmPkg/ArmMmuLib AARCH64: use helpers to determine table entry types
2020-03-25 15:29 ` [PATCH v3 2/3] ArmPkg/ArmMmuLib AARCH64: use helpers to determine table entry types Ard Biesheuvel
@ 2020-03-25 15:47 ` Ashish Singhal
0 siblings, 0 replies; 12+ messages in thread
From: Ashish Singhal @ 2020-03-25 15:47 UTC (permalink / raw)
To: Ard Biesheuvel, devel@edk2.groups.io; +Cc: Laszlo Ersek, Leif Lindholm
[-- Attachment #1: Type: text/plain, Size: 4353 bytes --]
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 2/3] ArmPkg/ArmMmuLib AARCH64: use helpers to determine table entry types
External email: Use caution opening links or attachments
Given how the meaning of the attribute bits for page table entry types
is slightly awkward, and changes between levels, add some helpers to
abstract from this.
Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Reviewed-by: Leif Lindholm <leif@nuviainc.com>
---
ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c | 40 +++++++++++++++++---
1 file changed, 35 insertions(+), 5 deletions(-)
diff --git a/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c b/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c
index d78918cf7ba8..0680ba36d907 100644
--- a/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c
+++ b/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c
@@ -162,6 +162,36 @@ FreePageTablesRecursive (
FreePages (TranslationTable, 1);
}
+STATIC
+BOOLEAN
+IsBlockEntry (
+ IN UINT64 Entry,
+ IN UINTN Level
+ )
+{
+ if (Level == 3) {
+ return (Entry & TT_TYPE_MASK) == TT_TYPE_BLOCK_ENTRY_LEVEL3;
+ }
+ return (Entry & TT_TYPE_MASK) == TT_TYPE_BLOCK_ENTRY;
+}
+
+STATIC
+BOOLEAN
+IsTableEntry (
+ IN UINT64 Entry,
+ IN UINTN Level
+ )
+{
+ if (Level == 3) {
+ //
+ // TT_TYPE_TABLE_ENTRY aliases TT_TYPE_BLOCK_ENTRY_LEVEL3
+ // so we need to take the level into account as well.
+ //
+ return FALSE;
+ }
+ return (Entry & TT_TYPE_MASK) == TT_TYPE_TABLE_ENTRY;
+}
+
STATIC
EFI_STATUS
UpdateRegionMappingRecursive (
@@ -203,7 +233,7 @@ UpdateRegionMappingRecursive (
if (Level == 0 || ((RegionStart | BlockEnd) & BlockMask) != 0) {
ASSERT (Level < 3);
- if ((*Entry & TT_TYPE_MASK) != TT_TYPE_TABLE_ENTRY) {
+ if (!IsTableEntry (*Entry, Level)) {
//
// No table entry exists yet, so we need to allocate a page table
// for the next level.
@@ -221,7 +251,7 @@ UpdateRegionMappingRecursive (
InvalidateDataCacheRange (TranslationTable, EFI_PAGE_SIZE);
}
- if ((*Entry & TT_TYPE_MASK) == TT_TYPE_BLOCK_ENTRY) {
+ if (IsBlockEntry (*Entry, Level)) {
//
// We are splitting an existing block entry, so we have to populate
// the new table with the attributes of the block entry it replaces.
@@ -252,7 +282,7 @@ UpdateRegionMappingRecursive (
AttributeSetMask, AttributeClearMask, TranslationTable,
Level + 1);
if (EFI_ERROR (Status)) {
- if ((*Entry & TT_TYPE_MASK) != TT_TYPE_TABLE_ENTRY) {
+ if (!IsTableEntry (*Entry, Level)) {
//
// We are creating a new table entry, so on failure, we can free all
// allocations we made recursively, given that the whole subhierarchy
@@ -265,10 +295,10 @@ UpdateRegionMappingRecursive (
return Status;
}
- if ((*Entry & TT_TYPE_MASK) != TT_TYPE_TABLE_ENTRY) {
+ if (!IsTableEntry (*Entry, Level)) {
EntryValue = (UINTN)TranslationTable | TT_TYPE_TABLE_ENTRY;
ReplaceTableEntry (Entry, EntryValue, RegionStart,
- (*Entry & TT_TYPE_MASK) == TT_TYPE_BLOCK_ENTRY);
+ IsBlockEntry (*Entry, Level));
}
} else {
EntryValue = (*Entry & AttributeClearMask) | AttributeSetMask;
--
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.
-----------------------------------------------------------------------------------
[-- Attachment #2: Type: text/html, Size: 8314 bytes --]
^ permalink raw reply related [flat|nested] 12+ messages in thread
* [PATCH v3 3/3] ArmPkg/ArmMmuLib AARCH64: preserve attributes when replacing a table entry
2020-03-25 15:29 [PATCH v3 0/3] ArmPkg/ArmMmuLib AARCH64: correctness fix Ard Biesheuvel
2020-03-25 15:29 ` [PATCH v3 1/3] ArmPkg/ArmMmuLib AARCH64: limit recursion when freeing page tables Ard Biesheuvel
2020-03-25 15:29 ` [PATCH v3 2/3] ArmPkg/ArmMmuLib AARCH64: use helpers to determine table entry types Ard Biesheuvel
@ 2020-03-25 15:29 ` Ard Biesheuvel
2020-03-25 15:47 ` Ashish Singhal
2020-03-25 15:48 ` [PATCH v3 0/3] ArmPkg/ArmMmuLib AARCH64: correctness fix Ashish Singhal
2020-03-25 19:49 ` [edk2-devel] " Laszlo Ersek
4 siblings, 1 reply; 12+ messages in thread
From: Ard Biesheuvel @ 2020-03-25 15:29 UTC (permalink / raw)
To: devel; +Cc: Ard Biesheuvel, Laszlo Ersek, Leif Lindholm, Ashish Singhal
Currently, depending on the size of the region being (re)mapped, the
page table manipulation code may replace a table entry with a block entry,
even if the existing table entry uses different mapping attributes to
describe different parts of the region it covers. This is undesirable, and
instead, we should avoid doing so unless we are disregarding the original
attributes anyway. And if we make such a replacement, we should free all
the page tables that have become orphaned in the process.
So let's implement this, by taking the table entry path through the code
for block sized regions if a table entry already exists, and the clear
mask is set (which means we are preserving attributes from the existing
mapping). And when we do replace a table entry with a block entry, free
all the pages that are no longer referenced.
Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
---
ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c | 25 ++++++++++++++++----
1 file changed, 21 insertions(+), 4 deletions(-)
diff --git a/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c b/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c
index 0680ba36d907..3b10ef58f0a2 100644
--- a/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c
+++ b/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c
@@ -229,8 +229,12 @@ UpdateRegionMappingRecursive (
// than a block, and recurse to create the block or page entries at
// the next level. No block mappings are allowed at all at level 0,
// so in that case, we have to recurse unconditionally.
+ // If we are changing a table entry and the AttributeClearMask is non-zero,
+ // we cannot replace it with a block entry without potentially losing
+ // attribute information, so keep the table entry in that case.
//
- if (Level == 0 || ((RegionStart | BlockEnd) & BlockMask) != 0) {
+ if (Level == 0 || ((RegionStart | BlockEnd) & BlockMask) != 0 ||
+ (IsTableEntry (*Entry, Level) && AttributeClearMask != 0)) {
ASSERT (Level < 3);
if (!IsTableEntry (*Entry, Level)) {
@@ -251,6 +255,8 @@ UpdateRegionMappingRecursive (
InvalidateDataCacheRange (TranslationTable, EFI_PAGE_SIZE);
}
+ ZeroMem (TranslationTable, EFI_PAGE_SIZE);
+
if (IsBlockEntry (*Entry, Level)) {
//
// We are splitting an existing block entry, so we have to populate
@@ -268,8 +274,6 @@ UpdateRegionMappingRecursive (
FreePages (TranslationTable, 1);
return Status;
}
- } else {
- ZeroMem (TranslationTable, EFI_PAGE_SIZE);
}
} else {
TranslationTable = (VOID *)(UINTN)(*Entry & TT_ADDRESS_MASK_BLOCK_ENTRY);
@@ -306,7 +310,20 @@ UpdateRegionMappingRecursive (
EntryValue |= (Level == 3) ? TT_TYPE_BLOCK_ENTRY_LEVEL3
: TT_TYPE_BLOCK_ENTRY;
- ReplaceTableEntry (Entry, EntryValue, RegionStart, FALSE);
+ if (IsTableEntry (*Entry, Level)) {
+ //
+ // We are replacing a table entry with a block entry. This is only
+ // possible if we are keeping none of the original attributes.
+ // We can free the table entry's page table, and all the ones below
+ // it, since we are dropping the only possible reference to it.
+ //
+ ASSERT (AttributeClearMask == 0);
+ TranslationTable = (VOID *)(UINTN)(*Entry & TT_ADDRESS_MASK_BLOCK_ENTRY);
+ ReplaceTableEntry (Entry, EntryValue, RegionStart, TRUE);
+ FreePageTablesRecursive (TranslationTable, Level + 1);
+ } else {
+ ReplaceTableEntry (Entry, EntryValue, RegionStart, FALSE);
+ }
}
}
return EFI_SUCCESS;
--
2.17.1
^ permalink raw reply related [flat|nested] 12+ messages in thread* Re: [PATCH v3 3/3] ArmPkg/ArmMmuLib AARCH64: preserve attributes when replacing a table entry
2020-03-25 15:29 ` [PATCH v3 3/3] ArmPkg/ArmMmuLib AARCH64: preserve attributes when replacing a table entry Ard Biesheuvel
@ 2020-03-25 15:47 ` Ashish Singhal
0 siblings, 0 replies; 12+ messages in thread
From: Ashish Singhal @ 2020-03-25 15:47 UTC (permalink / raw)
To: Ard Biesheuvel, devel@edk2.groups.io; +Cc: Laszlo Ersek, Leif Lindholm
[-- Attachment #1: Type: text/plain, Size: 4876 bytes --]
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 3/3] ArmPkg/ArmMmuLib AARCH64: preserve attributes when replacing a table entry
External email: Use caution opening links or attachments
Currently, depending on the size of the region being (re)mapped, the
page table manipulation code may replace a table entry with a block entry,
even if the existing table entry uses different mapping attributes to
describe different parts of the region it covers. This is undesirable, and
instead, we should avoid doing so unless we are disregarding the original
attributes anyway. And if we make such a replacement, we should free all
the page tables that have become orphaned in the process.
So let's implement this, by taking the table entry path through the code
for block sized regions if a table entry already exists, and the clear
mask is set (which means we are preserving attributes from the existing
mapping). And when we do replace a table entry with a block entry, free
all the pages that are no longer referenced.
Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
---
ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c | 25 ++++++++++++++++----
1 file changed, 21 insertions(+), 4 deletions(-)
diff --git a/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c b/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c
index 0680ba36d907..3b10ef58f0a2 100644
--- a/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c
+++ b/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c
@@ -229,8 +229,12 @@ UpdateRegionMappingRecursive (
// than a block, and recurse to create the block or page entries at
// the next level. No block mappings are allowed at all at level 0,
// so in that case, we have to recurse unconditionally.
+ // If we are changing a table entry and the AttributeClearMask is non-zero,
+ // we cannot replace it with a block entry without potentially losing
+ // attribute information, so keep the table entry in that case.
//
- if (Level == 0 || ((RegionStart | BlockEnd) & BlockMask) != 0) {
+ if (Level == 0 || ((RegionStart | BlockEnd) & BlockMask) != 0 ||
+ (IsTableEntry (*Entry, Level) && AttributeClearMask != 0)) {
ASSERT (Level < 3);
if (!IsTableEntry (*Entry, Level)) {
@@ -251,6 +255,8 @@ UpdateRegionMappingRecursive (
InvalidateDataCacheRange (TranslationTable, EFI_PAGE_SIZE);
}
+ ZeroMem (TranslationTable, EFI_PAGE_SIZE);
+
if (IsBlockEntry (*Entry, Level)) {
//
// We are splitting an existing block entry, so we have to populate
@@ -268,8 +274,6 @@ UpdateRegionMappingRecursive (
FreePages (TranslationTable, 1);
return Status;
}
- } else {
- ZeroMem (TranslationTable, EFI_PAGE_SIZE);
}
} else {
TranslationTable = (VOID *)(UINTN)(*Entry & TT_ADDRESS_MASK_BLOCK_ENTRY);
@@ -306,7 +310,20 @@ UpdateRegionMappingRecursive (
EntryValue |= (Level == 3) ? TT_TYPE_BLOCK_ENTRY_LEVEL3
: TT_TYPE_BLOCK_ENTRY;
- ReplaceTableEntry (Entry, EntryValue, RegionStart, FALSE);
+ if (IsTableEntry (*Entry, Level)) {
+ //
+ // We are replacing a table entry with a block entry. This is only
+ // possible if we are keeping none of the original attributes.
+ // We can free the table entry's page table, and all the ones below
+ // it, since we are dropping the only possible reference to it.
+ //
+ ASSERT (AttributeClearMask == 0);
+ TranslationTable = (VOID *)(UINTN)(*Entry & TT_ADDRESS_MASK_BLOCK_ENTRY);
+ ReplaceTableEntry (Entry, EntryValue, RegionStart, TRUE);
+ FreePageTablesRecursive (TranslationTable, Level + 1);
+ } else {
+ ReplaceTableEntry (Entry, EntryValue, RegionStart, FALSE);
+ }
}
}
return EFI_SUCCESS;
--
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.
-----------------------------------------------------------------------------------
[-- Attachment #2: Type: text/html, Size: 8655 bytes --]
^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH v3 0/3] ArmPkg/ArmMmuLib AARCH64: correctness fix
2020-03-25 15:29 [PATCH v3 0/3] ArmPkg/ArmMmuLib AARCH64: correctness fix Ard Biesheuvel
` (2 preceding siblings ...)
2020-03-25 15:29 ` [PATCH v3 3/3] ArmPkg/ArmMmuLib AARCH64: preserve attributes when replacing a table entry Ard Biesheuvel
@ 2020-03-25 15:48 ` Ashish Singhal
2020-03-25 19:49 ` [edk2-devel] " Laszlo Ersek
4 siblings, 0 replies; 12+ messages in thread
From: Ashish Singhal @ 2020-03-25 15:48 UTC (permalink / raw)
To: Ard Biesheuvel, devel@edk2.groups.io; +Cc: Laszlo Ersek, Leif Lindholm
[-- Attachment #1: Type: text/plain, Size: 2970 bytes --]
Patch set version 3 looks good to me. I have reviewed and tested the changes locally.
Reviewed-by: Ashish Singhal <ashishsingha@nvidia.com>
Tested-by: Ashish Singhal <ashishsingha@nvidia.com>
Thanks
Ashish
________________________________
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 0/3] ArmPkg/ArmMmuLib AARCH64: correctness fix
External email: Use caution opening links or attachments
The new ArmMmuLib code is easier to reason about, so that is what I did:
currently, when we create mappings that cover existing table entries, we
may end up overwriting those with block entries without taking the mapping
attributes of the original table entries into account. So let's fix this.
I honestly don't know whether the original code was better at dealing with
this: I do remember some changes from Heyi that may have been related, but
the old code is not easy to follow. In any case, I didn't manage to hit this
case in practice, given that we typically start out with large mappings, and
break them down later (to set permissions), rather than the other way around.
Patch #1 adds some helpers to hide the insane way the type bits change
meaning when you change to level 3.
Patch #2 ensures that we only replace (and free) table entries with block
entries if it is guaranteed that doing so will not lose any attribute
information.
Changes since v2:
- add patch to limit recursion to levels < 3 in FreePageTablesRecursive()
Changes since v1:
- zero newly allocated pages before splitting a block entry into a table
entry, to avoid garbage in that page being misidentified as entry type
attributes - this should fix the crash observed by Laszlo
Cc: Laszlo Ersek <lersek@redhat.com>
Cc: Leif Lindholm <leif@nuviainc.com>
Cc: Ashish Singhal <ashishsingha@nvidia.com>
Ard Biesheuvel (3):
ArmPkg/ArmMmuLib AARCH64: limit recursion when freeing page tables
ArmPkg/ArmMmuLib AARCH64: use helpers to determine table entry types
ArmPkg/ArmMmuLib AARCH64: preserve attributes when replacing a table
entry
.../Library/ArmMmuLib/AArch64/ArmMmuLibCore.c | 83 +++++++++++++++----
1 file changed, 68 insertions(+), 15 deletions(-)
--
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.
-----------------------------------------------------------------------------------
[-- Attachment #2: Type: text/html, Size: 5360 bytes --]
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: [edk2-devel] [PATCH v3 0/3] ArmPkg/ArmMmuLib AARCH64: correctness fix
2020-03-25 15:29 [PATCH v3 0/3] ArmPkg/ArmMmuLib AARCH64: correctness fix Ard Biesheuvel
` (3 preceding siblings ...)
2020-03-25 15:48 ` [PATCH v3 0/3] ArmPkg/ArmMmuLib AARCH64: correctness fix Ashish Singhal
@ 2020-03-25 19:49 ` Laszlo Ersek
2020-03-26 10:35 ` Ard Biesheuvel
4 siblings, 1 reply; 12+ messages in thread
From: Laszlo Ersek @ 2020-03-25 19:49 UTC (permalink / raw)
To: devel, ard.biesheuvel; +Cc: Leif Lindholm, Ashish Singhal
On 03/25/20 16:29, Ard Biesheuvel wrote:
> The new ArmMmuLib code is easier to reason about, so that is what I did:
> currently, when we create mappings that cover existing table entries, we
> may end up overwriting those with block entries without taking the mapping
> attributes of the original table entries into account. So let's fix this.
>
> I honestly don't know whether the original code was better at dealing with
> this: I do remember some changes from Heyi that may have been related, but
> the old code is not easy to follow. In any case, I didn't manage to hit this
> case in practice, given that we typically start out with large mappings, and
> break them down later (to set permissions), rather than the other way around.
>
> Patch #1 adds some helpers to hide the insane way the type bits change
> meaning when you change to level 3.
>
> Patch #2 ensures that we only replace (and free) table entries with block
> entries if it is guaranteed that doing so will not lose any attribute
> information.
>
> Changes since v2:
> - add patch to limit recursion to levels < 3 in FreePageTablesRecursive()
>
> Changes since v1:
> - zero newly allocated pages before splitting a block entry into a table
> entry, to avoid garbage in that page being misidentified as entry type
> attributes - this should fix the crash observed by Laszlo
>
> Cc: Laszlo Ersek <lersek@redhat.com>
> Cc: Leif Lindholm <leif@nuviainc.com>
> Cc: Ashish Singhal <ashishsingha@nvidia.com>
>
> Ard Biesheuvel (3):
> ArmPkg/ArmMmuLib AARCH64: limit recursion when freeing page tables
> ArmPkg/ArmMmuLib AARCH64: use helpers to determine table entry types
> ArmPkg/ArmMmuLib AARCH64: preserve attributes when replacing a table
> entry
>
> .../Library/ArmMmuLib/AArch64/ArmMmuLibCore.c | 83 +++++++++++++++----
> 1 file changed, 68 insertions(+), 15 deletions(-)
>
Tested-by: Laszlo Ersek <lersek@redhat.com>
Thanks!
Laszlo
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: [edk2-devel] [PATCH v3 0/3] ArmPkg/ArmMmuLib AARCH64: correctness fix
2020-03-25 19:49 ` [edk2-devel] " Laszlo Ersek
@ 2020-03-26 10:35 ` Ard Biesheuvel
0 siblings, 0 replies; 12+ messages in thread
From: Ard Biesheuvel @ 2020-03-26 10:35 UTC (permalink / raw)
To: Laszlo Ersek; +Cc: edk2-devel-groups-io, Leif Lindholm, Ashish Singhal
On Wed, 25 Mar 2020 at 20:49, Laszlo Ersek <lersek@redhat.com> wrote:
>
> On 03/25/20 16:29, Ard Biesheuvel wrote:
> > The new ArmMmuLib code is easier to reason about, so that is what I did:
> > currently, when we create mappings that cover existing table entries, we
> > may end up overwriting those with block entries without taking the mapping
> > attributes of the original table entries into account. So let's fix this.
> >
> > I honestly don't know whether the original code was better at dealing with
> > this: I do remember some changes from Heyi that may have been related, but
> > the old code is not easy to follow. In any case, I didn't manage to hit this
> > case in practice, given that we typically start out with large mappings, and
> > break them down later (to set permissions), rather than the other way around.
> >
> > Patch #1 adds some helpers to hide the insane way the type bits change
> > meaning when you change to level 3.
> >
> > Patch #2 ensures that we only replace (and free) table entries with block
> > entries if it is guaranteed that doing so will not lose any attribute
> > information.
> >
> > Changes since v2:
> > - add patch to limit recursion to levels < 3 in FreePageTablesRecursive()
> >
> > Changes since v1:
> > - zero newly allocated pages before splitting a block entry into a table
> > entry, to avoid garbage in that page being misidentified as entry type
> > attributes - this should fix the crash observed by Laszlo
> >
> > Cc: Laszlo Ersek <lersek@redhat.com>
> > Cc: Leif Lindholm <leif@nuviainc.com>
> > Cc: Ashish Singhal <ashishsingha@nvidia.com>
> >
> > Ard Biesheuvel (3):
> > ArmPkg/ArmMmuLib AARCH64: limit recursion when freeing page tables
> > ArmPkg/ArmMmuLib AARCH64: use helpers to determine table entry types
> > ArmPkg/ArmMmuLib AARCH64: preserve attributes when replacing a table
> > entry
> >
> > .../Library/ArmMmuLib/AArch64/ArmMmuLibCore.c | 83 +++++++++++++++----
> > 1 file changed, 68 insertions(+), 15 deletions(-)
> >
>
> Tested-by: Laszlo Ersek <lersek@redhat.com>
>
Pushed to master
Thanks all
^ permalink raw reply [flat|nested] 12+ messages in thread