diff mbox series

[edk2,1/4] ArmPkg/CpuDxe: Correct EFI_MEMORY_RO usage

Message ID 1486661891-7888-2-git-send-email-ard.biesheuvel@linaro.org
State Superseded
Headers show
Series ArmPkg: add groundwork for DXE image protection | expand

Commit Message

Ard Biesheuvel Feb. 9, 2017, 5:38 p.m. UTC 
From: Jiewen Yao <jiewen.yao@intel.com>


Current Arm CpuDxe driver uses EFI_MEMORY_WP for write protection,
according to UEFI spec, we should use EFI_MEMORY_RO for write protection.
The EFI_MEMORY_WP is the cache attribute instead of memory attribute.

Cc: Leif Lindholm <leif.lindholm@linaro.org>
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Jiewen Yao <jiewen.yao@intel.com>


Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>

---
 ArmPkg/Drivers/CpuDxe/AArch64/Mmu.c              |  3 ++-
 ArmPkg/Drivers/CpuDxe/Arm/Mmu.c                  | 14 ++++++--------
 ArmPkg/Drivers/CpuDxe/CpuMmuCommon.c             |  5 +++--
 ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c |  3 ++-
 4 files changed, 13 insertions(+), 12 deletions(-)

-- 
2.7.4

_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Comments

Leif Lindholm Feb. 10, 2017, 6:17 p.m. UTC | #1 
On Thu, Feb 09, 2017 at 05:38:08PM +0000, Ard Biesheuvel wrote:
> From: Jiewen Yao <jiewen.yao@intel.com>

> 

> Current Arm CpuDxe driver uses EFI_MEMORY_WP for write protection,

> according to UEFI spec, we should use EFI_MEMORY_RO for write protection.

> The EFI_MEMORY_WP is the cache attribute instead of memory attribute.

> 

> Cc: Leif Lindholm <leif.lindholm@linaro.org>

> Contributed-under: TianoCore Contribution Agreement 1.0

> Signed-off-by: Jiewen Yao <jiewen.yao@intel.com>

> 

> Contributed-under: TianoCore Contribution Agreement 1.0

> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>


No objections to this patch, but I would have expected it to be 4/4,
if it caused issues requiring the other 3 to be created?

/
    Leif

> ---

>  ArmPkg/Drivers/CpuDxe/AArch64/Mmu.c              |  3 ++-

>  ArmPkg/Drivers/CpuDxe/Arm/Mmu.c                  | 14 ++++++--------

>  ArmPkg/Drivers/CpuDxe/CpuMmuCommon.c             |  5 +++--

>  ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c |  3 ++-

>  4 files changed, 13 insertions(+), 12 deletions(-)

> 

> diff --git a/ArmPkg/Drivers/CpuDxe/AArch64/Mmu.c b/ArmPkg/Drivers/CpuDxe/AArch64/Mmu.c

> index d8bb41978066..15d5a8173233 100644

> --- a/ArmPkg/Drivers/CpuDxe/AArch64/Mmu.c

> +++ b/ArmPkg/Drivers/CpuDxe/AArch64/Mmu.c

> @@ -3,6 +3,7 @@

>  Copyright (c) 2009, Hewlett-Packard Company. All rights reserved.<BR>

>  Portions copyright (c) 2010, Apple Inc. All rights reserved.<BR>

>  Portions copyright (c) 2011-2013, ARM Ltd. All rights reserved.<BR>

> +Copyright (c) 2017, Intel Corporation. All rights reserved.<BR>

>  

>  This program and the accompanying materials

>  are licensed and made available under the terms and conditions of the BSD License

> @@ -224,7 +225,7 @@ EfiAttributeToArmAttribute (

>    ArmAttributes |= TT_AF;

>  

>    // Determine protection attributes

> -  if (EfiAttributes & EFI_MEMORY_WP) {

> +  if (EfiAttributes & EFI_MEMORY_RO) {

>      ArmAttributes |= TT_AP_RO_RO;

>    }

>  

> diff --git a/ArmPkg/Drivers/CpuDxe/Arm/Mmu.c b/ArmPkg/Drivers/CpuDxe/Arm/Mmu.c

> index 14fc22d7a59f..6dcfba69e879 100644

> --- a/ArmPkg/Drivers/CpuDxe/Arm/Mmu.c

> +++ b/ArmPkg/Drivers/CpuDxe/Arm/Mmu.c

> @@ -3,6 +3,7 @@

>  Copyright (c) 2009, Hewlett-Packard Company. All rights reserved.<BR>

>  Portions copyright (c) 2010, Apple Inc. All rights reserved.<BR>

>  Portions copyright (c) 2013, ARM Ltd. All rights reserved.<BR>

> +Copyright (c) 2017, Intel Corporation. All rights reserved.<BR>

>  

>  This program and the accompanying materials

>  are licensed and made available under the terms and conditions of the BSD License

> @@ -62,7 +63,7 @@ SectionToGcdAttributes (

>    // determine protection attributes

>    switch(SectionAttributes & TT_DESCRIPTOR_SECTION_AP_MASK) {

>      case TT_DESCRIPTOR_SECTION_AP_NO_NO: // no read, no write

> -      //*GcdAttributes |= EFI_MEMORY_WP | EFI_MEMORY_RP;

> +      //*GcdAttributes |= EFI_MEMORY_RO | EFI_MEMORY_RP;

>        break;

>  

>      case TT_DESCRIPTOR_SECTION_AP_RW_NO:

> @@ -73,7 +74,7 @@ SectionToGcdAttributes (

>      // read only cases map to write-protect

>      case TT_DESCRIPTOR_SECTION_AP_RO_NO:

>      case TT_DESCRIPTOR_SECTION_AP_RO_RO:

> -      *GcdAttributes |= EFI_MEMORY_WP;

> +      *GcdAttributes |= EFI_MEMORY_RO;

>        break;

>  

>      default:

> @@ -126,7 +127,7 @@ PageToGcdAttributes (

>    // determine protection attributes

>    switch(PageAttributes & TT_DESCRIPTOR_PAGE_AP_MASK) {

>      case TT_DESCRIPTOR_PAGE_AP_NO_NO: // no read, no write

> -      //*GcdAttributes |= EFI_MEMORY_WP | EFI_MEMORY_RP;

> +      //*GcdAttributes |= EFI_MEMORY_RO | EFI_MEMORY_RP;

>        break;

>  

>      case TT_DESCRIPTOR_PAGE_AP_RW_NO:

> @@ -137,7 +138,7 @@ PageToGcdAttributes (

>      // read only cases map to write-protect

>      case TT_DESCRIPTOR_PAGE_AP_RO_NO:

>      case TT_DESCRIPTOR_PAGE_AP_RO_RO:

> -      *GcdAttributes |= EFI_MEMORY_WP;

> +      *GcdAttributes |= EFI_MEMORY_RO;

>        break;

>  

>      default:

> @@ -730,9 +731,6 @@ EfiAttributeToArmAttribute (

>        ArmAttributes = TT_DESCRIPTOR_SECTION_CACHE_POLICY_WRITE_BACK_ALLOC; // TEX [2:0] = 001, C=1, B=1

>        break;

>  

> -    case EFI_MEMORY_WP:

> -    case EFI_MEMORY_XP:

> -    case EFI_MEMORY_RP:

>      case EFI_MEMORY_UCE:

>      default:

>        // Cannot be implemented UEFI definition unclear for ARM

> @@ -743,7 +741,7 @@ EfiAttributeToArmAttribute (

>    }

>  

>    // Determine protection attributes

> -  if (EfiAttributes & EFI_MEMORY_WP) {

> +  if (EfiAttributes & EFI_MEMORY_RO) {

>      ArmAttributes |= TT_DESCRIPTOR_SECTION_AP_RO_RO;

>    } else {

>      ArmAttributes |= TT_DESCRIPTOR_SECTION_AP_RW_RW;

> diff --git a/ArmPkg/Drivers/CpuDxe/CpuMmuCommon.c b/ArmPkg/Drivers/CpuDxe/CpuMmuCommon.c

> index 723604d1df96..54d9b0163331 100644

> --- a/ArmPkg/Drivers/CpuDxe/CpuMmuCommon.c

> +++ b/ArmPkg/Drivers/CpuDxe/CpuMmuCommon.c

> @@ -1,6 +1,7 @@

>  /** @file

>  *

>  *  Copyright (c) 2013, ARM Limited. All rights reserved.

> +*  Copyright (c) 2017, Intel Corporation. All rights reserved.<BR>

>  *

>  *  This program and the accompanying materials

>  *  are licensed and made available under the terms and conditions of the BSD License

> @@ -236,7 +237,7 @@ CpuConvertPagesToUncachedVirtualAddress (

>    // be the PCI address. Code should always use the CPU address, and we will or in VirtualMask

>    // to that address.

>    //

> -  Status = SetMemoryAttributes (Address, Length, EFI_MEMORY_WP, 0);

> +  Status = SetMemoryAttributes (Address, Length, EFI_MEMORY_RO, 0);

>    if (!EFI_ERROR (Status)) {

>      Status = SetMemoryAttributes (Address | VirtualMask, Length, EFI_MEMORY_UC, VirtualMask);

>    }

> @@ -264,7 +265,7 @@ CpuReconvertPages (

>    //

>    // Unmap the aliased Address

>    //

> -  Status = SetMemoryAttributes (Address | VirtualMask, Length, EFI_MEMORY_WP, 0);

> +  Status = SetMemoryAttributes (Address | VirtualMask, Length, EFI_MEMORY_RO, 0);

>    if (!EFI_ERROR (Status)) {

>      //

>      // Restore atttributes

> diff --git a/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c b/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c

> index 540069a59b2e..6aa970bc0514 100644

> --- a/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c

> +++ b/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c

> @@ -3,6 +3,7 @@

>  *

>  *  Copyright (c) 2011-2014, ARM Limited. All rights reserved.

>  *  Copyright (c) 2016, Linaro Limited. All rights reserved.

> +*  Copyright (c) 2017, Intel Corporation. All rights reserved.<BR>

>  *

>  *  This program and the accompanying materials

>  *  are licensed and made available under the terms and conditions of the BSD License

> @@ -89,7 +90,7 @@ PageAttributeToGcdAttribute (

>    // Determine protection attributes

>    if (((PageAttributes & TT_AP_MASK) == TT_AP_NO_RO) || ((PageAttributes & TT_AP_MASK) == TT_AP_RO_RO)) {

>      // Read only cases map to write-protect

> -    GcdAttributes |= EFI_MEMORY_WP;

> +    GcdAttributes |= EFI_MEMORY_RO;

>    }

>  

>    // Process eXecute Never attribute

> -- 

> 2.7.4

> 

_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Ard Biesheuvel Feb. 10, 2017, 6:25 p.m. UTC | #2 
On 10 February 2017 at 18:17, Leif Lindholm <leif.lindholm@linaro.org> wrote:
> On Thu, Feb 09, 2017 at 05:38:08PM +0000, Ard Biesheuvel wrote:

>> From: Jiewen Yao <jiewen.yao@intel.com>

>>

>> Current Arm CpuDxe driver uses EFI_MEMORY_WP for write protection,

>> according to UEFI spec, we should use EFI_MEMORY_RO for write protection.

>> The EFI_MEMORY_WP is the cache attribute instead of memory attribute.

>>

>> Cc: Leif Lindholm <leif.lindholm@linaro.org>

>> Contributed-under: TianoCore Contribution Agreement 1.0

>> Signed-off-by: Jiewen Yao <jiewen.yao@intel.com>

>>

>> Contributed-under: TianoCore Contribution Agreement 1.0

>> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>

>

> No objections to this patch, but I would have expected it to be 4/4,

> if it caused issues requiring the other 3 to be created?

>


Not quite: it is the feature itself that requires these fixes, and
this patch actually makes sense as 1/4, since it removes uses of
EFI_MEMORY_WP that are no longer appropriate. Implementing 2-4 with
EFI_MEMORY_WP instead of EFI_MEMORY_RO and then changing it at the end
would make no sense at all.
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Leif Lindholm Feb. 10, 2017, 7:36 p.m. UTC | #3 
On Fri, Feb 10, 2017 at 06:25:00PM +0000, Ard Biesheuvel wrote:
> On 10 February 2017 at 18:17, Leif Lindholm <leif.lindholm@linaro.org> wrote:

> > On Thu, Feb 09, 2017 at 05:38:08PM +0000, Ard Biesheuvel wrote:

> >> From: Jiewen Yao <jiewen.yao@intel.com>

> >>

> >> Current Arm CpuDxe driver uses EFI_MEMORY_WP for write protection,

> >> according to UEFI spec, we should use EFI_MEMORY_RO for write protection.

> >> The EFI_MEMORY_WP is the cache attribute instead of memory attribute.

> >>

> >> Cc: Leif Lindholm <leif.lindholm@linaro.org>

> >> Contributed-under: TianoCore Contribution Agreement 1.0

> >> Signed-off-by: Jiewen Yao <jiewen.yao@intel.com>

> >>

> >> Contributed-under: TianoCore Contribution Agreement 1.0

> >> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>

> >

> > No objections to this patch, but I would have expected it to be 4/4,

> > if it caused issues requiring the other 3 to be created?

> >

> 

> Not quite: it is the feature itself that requires these fixes, and

> this patch actually makes sense as 1/4, since it removes uses of

> EFI_MEMORY_WP that are no longer appropriate. Implementing 2-4 with

> EFI_MEMORY_WP instead of EFI_MEMORY_RO and then changing it at the end

> would make no sense at all.


OK, so basically, the issue was already in the existing code?

In that case:
Reviewed-by: Leif Lindholm <leif.lindholm@linaro.org>

_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
diff mbox series

Patch

diff --git a/ArmPkg/Drivers/CpuDxe/AArch64/Mmu.c b/ArmPkg/Drivers/CpuDxe/AArch64/Mmu.c
index d8bb41978066..15d5a8173233 100644
--- a/ArmPkg/Drivers/CpuDxe/AArch64/Mmu.c
+++ b/ArmPkg/Drivers/CpuDxe/AArch64/Mmu.c
@@ -3,6 +3,7 @@ 
 Copyright (c) 2009, Hewlett-Packard Company. All rights reserved.<BR>
 Portions copyright (c) 2010, Apple Inc. All rights reserved.<BR>
 Portions copyright (c) 2011-2013, ARM Ltd. All rights reserved.<BR>
+Copyright (c) 2017, Intel Corporation. All rights reserved.<BR>
 
 This program and the accompanying materials
 are licensed and made available under the terms and conditions of the BSD License
@@ -224,7 +225,7 @@  EfiAttributeToArmAttribute (
   ArmAttributes |= TT_AF;
 
   // Determine protection attributes
-  if (EfiAttributes & EFI_MEMORY_WP) {
+  if (EfiAttributes & EFI_MEMORY_RO) {
     ArmAttributes |= TT_AP_RO_RO;
   }
 
diff --git a/ArmPkg/Drivers/CpuDxe/Arm/Mmu.c b/ArmPkg/Drivers/CpuDxe/Arm/Mmu.c
index 14fc22d7a59f..6dcfba69e879 100644
--- a/ArmPkg/Drivers/CpuDxe/Arm/Mmu.c
+++ b/ArmPkg/Drivers/CpuDxe/Arm/Mmu.c
@@ -3,6 +3,7 @@ 
 Copyright (c) 2009, Hewlett-Packard Company. All rights reserved.<BR>
 Portions copyright (c) 2010, Apple Inc. All rights reserved.<BR>
 Portions copyright (c) 2013, ARM Ltd. All rights reserved.<BR>
+Copyright (c) 2017, Intel Corporation. All rights reserved.<BR>
 
 This program and the accompanying materials
 are licensed and made available under the terms and conditions of the BSD License
@@ -62,7 +63,7 @@  SectionToGcdAttributes (
   // determine protection attributes
   switch(SectionAttributes & TT_DESCRIPTOR_SECTION_AP_MASK) {
     case TT_DESCRIPTOR_SECTION_AP_NO_NO: // no read, no write
-      //*GcdAttributes |= EFI_MEMORY_WP | EFI_MEMORY_RP;
+      //*GcdAttributes |= EFI_MEMORY_RO | EFI_MEMORY_RP;
       break;
 
     case TT_DESCRIPTOR_SECTION_AP_RW_NO:
@@ -73,7 +74,7 @@  SectionToGcdAttributes (
     // read only cases map to write-protect
     case TT_DESCRIPTOR_SECTION_AP_RO_NO:
     case TT_DESCRIPTOR_SECTION_AP_RO_RO:
-      *GcdAttributes |= EFI_MEMORY_WP;
+      *GcdAttributes |= EFI_MEMORY_RO;
       break;
 
     default:
@@ -126,7 +127,7 @@  PageToGcdAttributes (
   // determine protection attributes
   switch(PageAttributes & TT_DESCRIPTOR_PAGE_AP_MASK) {
     case TT_DESCRIPTOR_PAGE_AP_NO_NO: // no read, no write
-      //*GcdAttributes |= EFI_MEMORY_WP | EFI_MEMORY_RP;
+      //*GcdAttributes |= EFI_MEMORY_RO | EFI_MEMORY_RP;
       break;
 
     case TT_DESCRIPTOR_PAGE_AP_RW_NO:
@@ -137,7 +138,7 @@  PageToGcdAttributes (
     // read only cases map to write-protect
     case TT_DESCRIPTOR_PAGE_AP_RO_NO:
     case TT_DESCRIPTOR_PAGE_AP_RO_RO:
-      *GcdAttributes |= EFI_MEMORY_WP;
+      *GcdAttributes |= EFI_MEMORY_RO;
       break;
 
     default:
@@ -730,9 +731,6 @@  EfiAttributeToArmAttribute (
       ArmAttributes = TT_DESCRIPTOR_SECTION_CACHE_POLICY_WRITE_BACK_ALLOC; // TEX [2:0] = 001, C=1, B=1
       break;
 
-    case EFI_MEMORY_WP:
-    case EFI_MEMORY_XP:
-    case EFI_MEMORY_RP:
     case EFI_MEMORY_UCE:
     default:
       // Cannot be implemented UEFI definition unclear for ARM
@@ -743,7 +741,7 @@  EfiAttributeToArmAttribute (
   }
 
   // Determine protection attributes
-  if (EfiAttributes & EFI_MEMORY_WP) {
+  if (EfiAttributes & EFI_MEMORY_RO) {
     ArmAttributes |= TT_DESCRIPTOR_SECTION_AP_RO_RO;
   } else {
     ArmAttributes |= TT_DESCRIPTOR_SECTION_AP_RW_RW;
diff --git a/ArmPkg/Drivers/CpuDxe/CpuMmuCommon.c b/ArmPkg/Drivers/CpuDxe/CpuMmuCommon.c
index 723604d1df96..54d9b0163331 100644
--- a/ArmPkg/Drivers/CpuDxe/CpuMmuCommon.c
+++ b/ArmPkg/Drivers/CpuDxe/CpuMmuCommon.c
@@ -1,6 +1,7 @@ 
 /** @file
 *
 *  Copyright (c) 2013, ARM Limited. All rights reserved.
+*  Copyright (c) 2017, Intel Corporation. All rights reserved.<BR>
 *
 *  This program and the accompanying materials
 *  are licensed and made available under the terms and conditions of the BSD License
@@ -236,7 +237,7 @@  CpuConvertPagesToUncachedVirtualAddress (
   // be the PCI address. Code should always use the CPU address, and we will or in VirtualMask
   // to that address.
   //
-  Status = SetMemoryAttributes (Address, Length, EFI_MEMORY_WP, 0);
+  Status = SetMemoryAttributes (Address, Length, EFI_MEMORY_RO, 0);
   if (!EFI_ERROR (Status)) {
     Status = SetMemoryAttributes (Address | VirtualMask, Length, EFI_MEMORY_UC, VirtualMask);
   }
@@ -264,7 +265,7 @@  CpuReconvertPages (
   //
   // Unmap the aliased Address
   //
-  Status = SetMemoryAttributes (Address | VirtualMask, Length, EFI_MEMORY_WP, 0);
+  Status = SetMemoryAttributes (Address | VirtualMask, Length, EFI_MEMORY_RO, 0);
   if (!EFI_ERROR (Status)) {
     //
     // Restore atttributes
diff --git a/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c b/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c
index 540069a59b2e..6aa970bc0514 100644
--- a/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c
+++ b/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c
@@ -3,6 +3,7 @@ 
 *
 *  Copyright (c) 2011-2014, ARM Limited. All rights reserved.
 *  Copyright (c) 2016, Linaro Limited. All rights reserved.
+*  Copyright (c) 2017, Intel Corporation. All rights reserved.<BR>
 *
 *  This program and the accompanying materials
 *  are licensed and made available under the terms and conditions of the BSD License
@@ -89,7 +90,7 @@  PageAttributeToGcdAttribute (
   // Determine protection attributes
   if (((PageAttributes & TT_AP_MASK) == TT_AP_NO_RO) || ((PageAttributes & TT_AP_MASK) == TT_AP_RO_RO)) {
     // Read only cases map to write-protect
-    GcdAttributes |= EFI_MEMORY_WP;
+    GcdAttributes |= EFI_MEMORY_RO;
   }
 
   // Process eXecute Never attribute