* [PATCH V2 2/3] ArmPkg/CpuDxe: Correct EFI_MEMORY_RO usage
@ 2017-01-26 12:32 Jiewen Yao
2017-01-26 12:38 ` Yao, Jiewen
0 siblings, 1 reply; 2+ messages in thread
From: Jiewen Yao @ 2017-01-26 12:32 UTC (permalink / raw)
To: edk2-devel; +Cc: Leif Lindholm, Ard Biesheuvel
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>
Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Jiewen Yao <jiewen.yao@intel.com>
---
ArmPkg/Drivers/CpuDxe/AArch64/Mmu.c | 2 +-
ArmPkg/Drivers/CpuDxe/Arm/Mmu.c | 13 +++++--------
ArmPkg/Drivers/CpuDxe/CpuMmuCommon.c | 4 ++--
ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c | 2 +-
4 files changed, 9 insertions(+), 12 deletions(-)
diff --git a/ArmPkg/Drivers/CpuDxe/AArch64/Mmu.c b/ArmPkg/Drivers/CpuDxe/AArch64/Mmu.c
index d8bb419..4703b33 100644
--- a/ArmPkg/Drivers/CpuDxe/AArch64/Mmu.c
+++ b/ArmPkg/Drivers/CpuDxe/AArch64/Mmu.c
@@ -224,7 +224,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 14fc22d..348b205 100644
--- a/ArmPkg/Drivers/CpuDxe/Arm/Mmu.c
+++ b/ArmPkg/Drivers/CpuDxe/Arm/Mmu.c
@@ -62,7 +62,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 +73,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 +126,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 +137,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 +730,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 +740,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 723604d..acdf1eb 100644
--- a/ArmPkg/Drivers/CpuDxe/CpuMmuCommon.c
+++ b/ArmPkg/Drivers/CpuDxe/CpuMmuCommon.c
@@ -236,7 +236,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 +264,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 540069a..5bc80a6 100644
--- a/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c
+++ b/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c
@@ -89,7 +89,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.windows.1
^ permalink raw reply related [flat|nested] 2+ messages in thread* Re: [PATCH V2 2/3] ArmPkg/CpuDxe: Correct EFI_MEMORY_RO usage
2017-01-26 12:32 [PATCH V2 2/3] ArmPkg/CpuDxe: Correct EFI_MEMORY_RO usage Jiewen Yao
@ 2017-01-26 12:38 ` Yao, Jiewen
0 siblings, 0 replies; 2+ messages in thread
From: Yao, Jiewen @ 2017-01-26 12:38 UTC (permalink / raw)
To: Yao, Jiewen, edk2-devel@lists.01.org
Cc: Leif Lindholm, Ard Biesheuvel, Andrew Fish (afish@apple.com)
Hi Leif
Here is 2nd patch. I did a global replace for EFI_MEMORY_WP.
I have to send this patch because I updated DxeCore to use EFI_MEMORY_RO and EFI_MEMORY_XP for PE image protection.
Assuming the original EFI_MEMORY_WP can work, I think this EFI_MEMORY_RO update is no harm.
But if the original EFI_MEMORY_WP is not validated, this core update has risk.
(Such as X86 CPU, we need apply a patch to support EFI_MEMORY_RO.)
Is that possible that you help me do a quick validation to check if the DxeCore update breaks anything on ARM?
Thank you
Yao Jiewen
> -----Original Message-----
> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Jiewen
> Yao
> Sent: Thursday, January 26, 2017 8:32 PM
> To: edk2-devel@lists.01.org
> Cc: Leif Lindholm <leif.lindholm@linaro.org>; Ard Biesheuvel
> <ard.biesheuvel@linaro.org>
> Subject: [edk2] [PATCH V2 2/3] ArmPkg/CpuDxe: Correct EFI_MEMORY_RO
> usage
>
> 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>
> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Jiewen Yao <jiewen.yao@intel.com>
> ---
> ArmPkg/Drivers/CpuDxe/AArch64/Mmu.c | 2 +-
> ArmPkg/Drivers/CpuDxe/Arm/Mmu.c | 13 +++++--------
> ArmPkg/Drivers/CpuDxe/CpuMmuCommon.c | 4 ++--
> ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c | 2 +-
> 4 files changed, 9 insertions(+), 12 deletions(-)
>
> diff --git a/ArmPkg/Drivers/CpuDxe/AArch64/Mmu.c
> b/ArmPkg/Drivers/CpuDxe/AArch64/Mmu.c
> index d8bb419..4703b33 100644
> --- a/ArmPkg/Drivers/CpuDxe/AArch64/Mmu.c
> +++ b/ArmPkg/Drivers/CpuDxe/AArch64/Mmu.c
> @@ -224,7 +224,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 14fc22d..348b205 100644
> --- a/ArmPkg/Drivers/CpuDxe/Arm/Mmu.c
> +++ b/ArmPkg/Drivers/CpuDxe/Arm/Mmu.c
> @@ -62,7 +62,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 +73,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 +126,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 +137,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 +730,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 +740,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 723604d..acdf1eb 100644
> --- a/ArmPkg/Drivers/CpuDxe/CpuMmuCommon.c
> +++ b/ArmPkg/Drivers/CpuDxe/CpuMmuCommon.c
> @@ -236,7 +236,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 +264,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 540069a..5bc80a6 100644
> --- a/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c
> +++ b/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c
> @@ -89,7 +89,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.windows.1
>
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel
^ permalink raw reply [flat|nested] 2+ messages in thread
end of thread, other threads:[~2017-01-26 12:38 UTC | newest]
Thread overview: 2+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2017-01-26 12:32 [PATCH V2 2/3] ArmPkg/CpuDxe: Correct EFI_MEMORY_RO usage Jiewen Yao
2017-01-26 12:38 ` Yao, Jiewen
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox