ArmPkg/ArmMmuLib: Drop buggy secure memory type check

[edk2-devel] [PATCH] ArmPkg/ArmMmuLib: Drop buggy secure memory type check

Posted by Ard Biesheuvel via groups.io 1 year, 1 month ago

From: Ard Biesheuvel <ardb@kernel.org>

Jake reports that the IS_ARM_MEMORY_REGION_ATTRIBUTES_SECURE() macro is
no longer accurate since commit 852227a9d52e3cb9 ("ArmPkg/Mmu: Remove
handling of NONSECURE memory regions").

Fortunately, it only affects the NS bit in level 1 short descriptors,
which is ignored when executing in non-secure mode. And given that
running UEFI in the secure world is not a use case we aim to support,
let's just drop this logic altogether.

Reported-by: Jake Garver <jake@nvidia.com>
Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
---
 ArmPkg/Include/Library/ArmLib.h              | 2 --
 ArmPkg/Library/ArmMmuLib/Arm/ArmMmuLibCore.c | 2 --
 2 files changed, 4 deletions(-)

diff --git a/ArmPkg/Include/Library/ArmLib.h b/ArmPkg/Include/Library/ArmLib.h
index fb1ae57b3522..6c5315d7f45f 100644
--- a/ArmPkg/Include/Library/ArmLib.h
+++ b/ArmPkg/Include/Library/ArmLib.h
@@ -45,8 +45,6 @@ typedef enum {
   ARM_MEMORY_REGION_ATTRIBUTE_DEVICE,
 } ARM_MEMORY_REGION_ATTRIBUTES;

-#define IS_ARM_MEMORY_REGION_ATTRIBUTES_SECURE(attr)  ((UINT32)(attr) & 1)
-
 typedef struct {
   EFI_PHYSICAL_ADDRESS            PhysicalBase;
   EFI_VIRTUAL_ADDRESS             VirtualBase;
diff --git a/ArmPkg/Library/ArmMmuLib/Arm/ArmMmuLibCore.c b/ArmPkg/Library/ArmMmuLib/Arm/ArmMmuLibCore.c
index 00c5f42cd91a..28e4cd9f1a77 100644
--- a/ArmPkg/Library/ArmMmuLib/Arm/ArmMmuLibCore.c
+++ b/ArmPkg/Library/ArmMmuLib/Arm/ArmMmuLibCore.c
@@ -169,7 +169,6 @@ PopulateLevel2PageTable (

       // Overwrite the section entry to point to the new Level2 Translation Table
       *SectionEntry = (TranslationTable & TT_DESCRIPTOR_SECTION_PAGETABLE_ADDRESS_MASK) |
-                      (IS_ARM_MEMORY_REGION_ATTRIBUTES_SECURE (Attributes) ? (1 << 3) : 0) |
                       TT_DESCRIPTOR_SECTION_TYPE_PAGE_TABLE;
     } else {
       // We do not support the other section type (16MB Section)
@@ -192,7 +191,6 @@ PopulateLevel2PageTable (
     ZeroMem ((VOID *)TranslationTable, TRANSLATION_TABLE_PAGE_SIZE);

     *SectionEntry = (TranslationTable & TT_DESCRIPTOR_SECTION_PAGETABLE_ADDRESS_MASK) |
-                    (IS_ARM_MEMORY_REGION_ATTRIBUTES_SECURE (Attributes) ? (1 << 3) : 0) |
                     TT_DESCRIPTOR_SECTION_TYPE_PAGE_TABLE;
   }

--
2.42.0.283.g2d96d420d3-goog



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#108522): https://edk2.groups.io/g/devel/message/108522
Mute This Topic: https://groups.io/mt/101311374/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH] ArmPkg/ArmMmuLib: Drop buggy secure memory type check

Posted by Leif Lindholm 1 year, 1 month ago

On Tue, Sep 12, 2023 at 07:42:30 +0000, Ard Biesheuvel wrote:
> From: Ard Biesheuvel <ardb@kernel.org>
> 
> Jake reports that the IS_ARM_MEMORY_REGION_ATTRIBUTES_SECURE() macro is
> no longer accurate since commit 852227a9d52e3cb9 ("ArmPkg/Mmu: Remove
> handling of NONSECURE memory regions").
> 
> Fortunately, it only affects the NS bit in level 1 short descriptors,
> which is ignored when executing in non-secure mode. And given that
> running UEFI in the secure world is not a use case we aim to support,
> let's just drop this logic altogether.
> 
> Reported-by: Jake Garver <jake@nvidia.com>
> Signed-off-by: Ard Biesheuvel <ardb@kernel.org>

Good riddance.
Reviewed-by: Leif Lindholm <quic_llindhol@quicinc.com>

> ---
>  ArmPkg/Include/Library/ArmLib.h              | 2 --
>  ArmPkg/Library/ArmMmuLib/Arm/ArmMmuLibCore.c | 2 --
>  2 files changed, 4 deletions(-)
> 
> diff --git a/ArmPkg/Include/Library/ArmLib.h b/ArmPkg/Include/Library/ArmLib.h
> index fb1ae57b3522..6c5315d7f45f 100644
> --- a/ArmPkg/Include/Library/ArmLib.h
> +++ b/ArmPkg/Include/Library/ArmLib.h
> @@ -45,8 +45,6 @@ typedef enum {
>    ARM_MEMORY_REGION_ATTRIBUTE_DEVICE,
>  } ARM_MEMORY_REGION_ATTRIBUTES;
> 
> -#define IS_ARM_MEMORY_REGION_ATTRIBUTES_SECURE(attr)  ((UINT32)(attr) & 1)
> -
>  typedef struct {
>    EFI_PHYSICAL_ADDRESS            PhysicalBase;
>    EFI_VIRTUAL_ADDRESS             VirtualBase;
> diff --git a/ArmPkg/Library/ArmMmuLib/Arm/ArmMmuLibCore.c b/ArmPkg/Library/ArmMmuLib/Arm/ArmMmuLibCore.c
> index 00c5f42cd91a..28e4cd9f1a77 100644
> --- a/ArmPkg/Library/ArmMmuLib/Arm/ArmMmuLibCore.c
> +++ b/ArmPkg/Library/ArmMmuLib/Arm/ArmMmuLibCore.c
> @@ -169,7 +169,6 @@ PopulateLevel2PageTable (
> 
>        // Overwrite the section entry to point to the new Level2 Translation Table
>        *SectionEntry = (TranslationTable & TT_DESCRIPTOR_SECTION_PAGETABLE_ADDRESS_MASK) |
> -                      (IS_ARM_MEMORY_REGION_ATTRIBUTES_SECURE (Attributes) ? (1 << 3) : 0) |
>                        TT_DESCRIPTOR_SECTION_TYPE_PAGE_TABLE;
>      } else {
>        // We do not support the other section type (16MB Section)
> @@ -192,7 +191,6 @@ PopulateLevel2PageTable (
>      ZeroMem ((VOID *)TranslationTable, TRANSLATION_TABLE_PAGE_SIZE);
> 
>      *SectionEntry = (TranslationTable & TT_DESCRIPTOR_SECTION_PAGETABLE_ADDRESS_MASK) |
> -                    (IS_ARM_MEMORY_REGION_ATTRIBUTES_SECURE (Attributes) ? (1 << 3) : 0) |
>                      TT_DESCRIPTOR_SECTION_TYPE_PAGE_TABLE;
>    }
> 
> --
> 2.42.0.283.g2d96d420d3-goog
> 


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#108527): https://edk2.groups.io/g/devel/message/108527
Mute This Topic: https://groups.io/mt/101311374/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/leave/3901457/1787277/102458076/xyzzy [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH] ArmPkg/ArmMmuLib: Drop buggy secure memory type check

Posted by Ard Biesheuvel 1 year, 1 month ago

On Tue, 12 Sept 2023 at 12:01, Leif Lindholm <quic_llindhol@quicinc.com> wrote:
>
> On Tue, Sep 12, 2023 at 07:42:30 +0000, Ard Biesheuvel wrote:
> > From: Ard Biesheuvel <ardb@kernel.org>
> >
> > Jake reports that the IS_ARM_MEMORY_REGION_ATTRIBUTES_SECURE() macro is
> > no longer accurate since commit 852227a9d52e3cb9 ("ArmPkg/Mmu: Remove
> > handling of NONSECURE memory regions").
> >
> > Fortunately, it only affects the NS bit in level 1 short descriptors,
> > which is ignored when executing in non-secure mode. And given that
> > running UEFI in the secure world is not a use case we aim to support,
> > let's just drop this logic altogether.
> >
> > Reported-by: Jake Garver <jake@nvidia.com>
> > Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
>
> Good riddance.
> Reviewed-by: Leif Lindholm <quic_llindhol@quicinc.com>
>

Thanks. Merged as #4822.


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#108528): https://edk2.groups.io/g/devel/message/108528
Mute This Topic: https://groups.io/mt/101311374/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-