diff mbox series

[edk2,3/5] ArmPkg/ArmMmuLib AARCH64: implement support for EFI_MEMORY_RP permissions

Message ID 20190107071504.2431-4-ard.biesheuvel@linaro.org
State New
Headers show
Series memory/MMU hardening for AArch64 | expand

Commit Message

Ard Biesheuvel Jan. 7, 2019, 7:15 a.m. UTC 
Wire up the access flag (AF) page table attribute to the EFI_MEMORY_RP
permission attribute, so that attempts to read from such a region will
trigger an access flag fault.

Note that this is a stronger notion than just read protection, since
it now implies that any write or execute attempt is trapped as well.
However, this does not really matter in practice since we never assume
that a read protected page is writable or executable, and StackGuard
and HeapGuard (which are the primary users of this facility) certainly
don't care.

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

---
 ArmPkg/Drivers/CpuDxe/AArch64/Mmu.c              |  5 +++--
 ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c | 14 +++++++++++---
 2 files changed, 14 insertions(+), 5 deletions(-)

-- 
2.20.1

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

Comments

Leif Lindholm Jan. 14, 2019, 2:29 p.m. UTC | #1 
On Mon, Jan 07, 2019 at 08:15:02AM +0100, Ard Biesheuvel wrote:
> Wire up the access flag (AF) page table attribute to the EFI_MEMORY_RP

> permission attribute, so that attempts to read from such a region will

> trigger an access flag fault.

> 

> Note that this is a stronger notion than just read protection, since

> it now implies that any write or execute attempt is trapped as well.

> However, this does not really matter in practice since we never assume

> that a read protected page is writable or executable, and StackGuard

> and HeapGuard (which are the primary users of this facility) certainly

> don't care.


So ... I'm cautiously positive to this patch.
But this use does contradict the UEFI spec (2.7a, 2.3.6.1 Memory
types), which says EFI_MEMORY_RP is "not used or defined" for AArch64.

Charles?

/
    Leif

> Contributed-under: TianoCore Contribution Agreement 1.1

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

> ---

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

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

>  2 files changed, 14 insertions(+), 5 deletions(-)

> 

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

> index 3e216c7cb235..e62e3fa87112 100644

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

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

> @@ -223,8 +223,9 @@ EfiAttributeToArmAttribute (

>      ArmAttributes = TT_ATTR_INDX_MASK;

>    }

>  

> -  // Set the access flag to match the block attributes

> -  ArmAttributes |= TT_AF;

> +  if ((EfiAttributes & EFI_MEMORY_RP) == 0) {

> +    ArmAttributes |= TT_AF;

> +  }

>  

>    // Determine protection attributes

>    if (EfiAttributes & EFI_MEMORY_RO) {

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

> index e1fabfcbea14..b59c081a7e49 100644

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

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

> @@ -102,6 +102,10 @@ PageAttributeToGcdAttribute (

>      GcdAttributes |= EFI_MEMORY_XP;

>    }

>  

> +  if ((PageAttributes & TT_AF) == 0) {

> +    GcdAttributes |= EFI_MEMORY_RP;

> +  }

> +

>    return GcdAttributes;

>  }

>  

> @@ -451,7 +455,11 @@ GcdAttributeToPageAttribute (

>      PageAttributes |= TT_AP_RO_RO;

>    }

>  

> -  return PageAttributes | TT_AF;

> +  if ((GcdAttributes & EFI_MEMORY_RP) == 0) {

> +    PageAttributes |= TT_AF;

> +  }

> +

> +  return PageAttributes;

>  }

>  

>  EFI_STATUS

> @@ -474,9 +482,9 @@ ArmSetMemoryAttributes (

>      // No memory type was set in Attributes, so we are going to update the

>      // permissions only.

>      //

> -    PageAttributes &= TT_AP_MASK | TT_UXN_MASK | TT_PXN_MASK;

> +    PageAttributes &= TT_AP_MASK | TT_UXN_MASK | TT_PXN_MASK | TT_AF;

>      PageAttributeMask = ~(TT_ADDRESS_MASK_BLOCK_ENTRY | TT_AP_MASK |

> -                          TT_PXN_MASK | TT_XN_MASK);

> +                          TT_PXN_MASK | TT_XN_MASK | TT_AF);

>    }

>  

>    TranslationTable = ArmGetTTBR0BaseAddress ();

> -- 

> 2.20.1

> 

_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Ard Biesheuvel Jan. 14, 2019, 2:59 p.m. UTC | #2 
On Mon, 14 Jan 2019 at 15:29, Leif Lindholm <leif.lindholm@linaro.org> wrote:
>

> On Mon, Jan 07, 2019 at 08:15:02AM +0100, Ard Biesheuvel wrote:

> > Wire up the access flag (AF) page table attribute to the EFI_MEMORY_RP

> > permission attribute, so that attempts to read from such a region will

> > trigger an access flag fault.

> >

> > Note that this is a stronger notion than just read protection, since

> > it now implies that any write or execute attempt is trapped as well.

> > However, this does not really matter in practice since we never assume

> > that a read protected page is writable or executable, and StackGuard

> > and HeapGuard (which are the primary users of this facility) certainly

> > don't care.

>

> So ... I'm cautiously positive to this patch.

> But this use does contradict the UEFI spec (2.7a, 2.3.6.1 Memory

> types), which says EFI_MEMORY_RP is "not used or defined" for AArch64.

>

> Charles?

>


Not defined by the spec means we can use it do whatever we bloody want
with it, at least that is what a typical compiler engineer will tell
you :-)

I think there was a pending ECR to update the AArch64 binding code to
reflect reality, but I don't think we included EFI_MEMORY_RP.


> > Contributed-under: TianoCore Contribution Agreement 1.1

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

> > ---

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

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

> >  2 files changed, 14 insertions(+), 5 deletions(-)

> >

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

> > index 3e216c7cb235..e62e3fa87112 100644

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

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

> > @@ -223,8 +223,9 @@ EfiAttributeToArmAttribute (

> >      ArmAttributes = TT_ATTR_INDX_MASK;

> >    }

> >

> > -  // Set the access flag to match the block attributes

> > -  ArmAttributes |= TT_AF;

> > +  if ((EfiAttributes & EFI_MEMORY_RP) == 0) {

> > +    ArmAttributes |= TT_AF;

> > +  }

> >

> >    // Determine protection attributes

> >    if (EfiAttributes & EFI_MEMORY_RO) {

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

> > index e1fabfcbea14..b59c081a7e49 100644

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

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

> > @@ -102,6 +102,10 @@ PageAttributeToGcdAttribute (

> >      GcdAttributes |= EFI_MEMORY_XP;

> >    }

> >

> > +  if ((PageAttributes & TT_AF) == 0) {

> > +    GcdAttributes |= EFI_MEMORY_RP;

> > +  }

> > +

> >    return GcdAttributes;

> >  }

> >

> > @@ -451,7 +455,11 @@ GcdAttributeToPageAttribute (

> >      PageAttributes |= TT_AP_RO_RO;

> >    }

> >

> > -  return PageAttributes | TT_AF;

> > +  if ((GcdAttributes & EFI_MEMORY_RP) == 0) {

> > +    PageAttributes |= TT_AF;

> > +  }

> > +

> > +  return PageAttributes;

> >  }

> >

> >  EFI_STATUS

> > @@ -474,9 +482,9 @@ ArmSetMemoryAttributes (

> >      // No memory type was set in Attributes, so we are going to update the

> >      // permissions only.

> >      //

> > -    PageAttributes &= TT_AP_MASK | TT_UXN_MASK | TT_PXN_MASK;

> > +    PageAttributes &= TT_AP_MASK | TT_UXN_MASK | TT_PXN_MASK | TT_AF;

> >      PageAttributeMask = ~(TT_ADDRESS_MASK_BLOCK_ENTRY | TT_AP_MASK |

> > -                          TT_PXN_MASK | TT_XN_MASK);

> > +                          TT_PXN_MASK | TT_XN_MASK | TT_AF);

> >    }

> >

> >    TranslationTable = ArmGetTTBR0BaseAddress ();

> > --

> > 2.20.1

> >

_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Leif Lindholm Jan. 14, 2019, 3:06 p.m. UTC | #3 
On Mon, Jan 14, 2019 at 03:59:08PM +0100, Ard Biesheuvel wrote:
> On Mon, 14 Jan 2019 at 15:29, Leif Lindholm <leif.lindholm@linaro.org> wrote:

> >

> > On Mon, Jan 07, 2019 at 08:15:02AM +0100, Ard Biesheuvel wrote:

> > > Wire up the access flag (AF) page table attribute to the EFI_MEMORY_RP

> > > permission attribute, so that attempts to read from such a region will

> > > trigger an access flag fault.

> > >

> > > Note that this is a stronger notion than just read protection, since

> > > it now implies that any write or execute attempt is trapped as well.

> > > However, this does not really matter in practice since we never assume

> > > that a read protected page is writable or executable, and StackGuard

> > > and HeapGuard (which are the primary users of this facility) certainly

> > > don't care.

> >

> > So ... I'm cautiously positive to this patch.

> > But this use does contradict the UEFI spec (2.7a, 2.3.6.1 Memory

> > types), which says EFI_MEMORY_RP is "not used or defined" for AArch64.

> >

> > Charles?

> 

> Not defined by the spec means we can use it do whatever we bloody want

> with it, at least that is what a typical compiler engineer will tell

> you :-)


Not defined, yes. Not used, less so. For a reciprocal compiler
engineer analogy, think pointer tagging :)

> I think there was a pending ECR to update the AArch64 binding code to

> reflect reality, but I don't think we included EFI_MEMORY_RP.


Right.

Anyway, I'm reasonably happy with stretching the rules slightly here,
and I believe it to be safe, but I do want Charles' take on it.

/
    Leif

> > > Contributed-under: TianoCore Contribution Agreement 1.1

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

> > > ---

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

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

> > >  2 files changed, 14 insertions(+), 5 deletions(-)

> > >

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

> > > index 3e216c7cb235..e62e3fa87112 100644

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

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

> > > @@ -223,8 +223,9 @@ EfiAttributeToArmAttribute (

> > >      ArmAttributes = TT_ATTR_INDX_MASK;

> > >    }

> > >

> > > -  // Set the access flag to match the block attributes

> > > -  ArmAttributes |= TT_AF;

> > > +  if ((EfiAttributes & EFI_MEMORY_RP) == 0) {

> > > +    ArmAttributes |= TT_AF;

> > > +  }

> > >

> > >    // Determine protection attributes

> > >    if (EfiAttributes & EFI_MEMORY_RO) {

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

> > > index e1fabfcbea14..b59c081a7e49 100644

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

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

> > > @@ -102,6 +102,10 @@ PageAttributeToGcdAttribute (

> > >      GcdAttributes |= EFI_MEMORY_XP;

> > >    }

> > >

> > > +  if ((PageAttributes & TT_AF) == 0) {

> > > +    GcdAttributes |= EFI_MEMORY_RP;

> > > +  }

> > > +

> > >    return GcdAttributes;

> > >  }

> > >

> > > @@ -451,7 +455,11 @@ GcdAttributeToPageAttribute (

> > >      PageAttributes |= TT_AP_RO_RO;

> > >    }

> > >

> > > -  return PageAttributes | TT_AF;

> > > +  if ((GcdAttributes & EFI_MEMORY_RP) == 0) {

> > > +    PageAttributes |= TT_AF;

> > > +  }

> > > +

> > > +  return PageAttributes;

> > >  }

> > >

> > >  EFI_STATUS

> > > @@ -474,9 +482,9 @@ ArmSetMemoryAttributes (

> > >      // No memory type was set in Attributes, so we are going to update the

> > >      // permissions only.

> > >      //

> > > -    PageAttributes &= TT_AP_MASK | TT_UXN_MASK | TT_PXN_MASK;

> > > +    PageAttributes &= TT_AP_MASK | TT_UXN_MASK | TT_PXN_MASK | TT_AF;

> > >      PageAttributeMask = ~(TT_ADDRESS_MASK_BLOCK_ENTRY | TT_AP_MASK |

> > > -                          TT_PXN_MASK | TT_XN_MASK);

> > > +                          TT_PXN_MASK | TT_XN_MASK | TT_AF);

> > >    }

> > >

> > >    TranslationTable = ArmGetTTBR0BaseAddress ();

> > > --

> > > 2.20.1

> > >

_______________________________________________
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 3e216c7cb235..e62e3fa87112 100644
--- a/ArmPkg/Drivers/CpuDxe/AArch64/Mmu.c
+++ b/ArmPkg/Drivers/CpuDxe/AArch64/Mmu.c
@@ -223,8 +223,9 @@  EfiAttributeToArmAttribute (
     ArmAttributes = TT_ATTR_INDX_MASK;
   }
 
-  // Set the access flag to match the block attributes
-  ArmAttributes |= TT_AF;
+  if ((EfiAttributes & EFI_MEMORY_RP) == 0) {
+    ArmAttributes |= TT_AF;
+  }
 
   // Determine protection attributes
   if (EfiAttributes & EFI_MEMORY_RO) {
diff --git a/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c b/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c
index e1fabfcbea14..b59c081a7e49 100644
--- a/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c
+++ b/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c
@@ -102,6 +102,10 @@  PageAttributeToGcdAttribute (
     GcdAttributes |= EFI_MEMORY_XP;
   }
 
+  if ((PageAttributes & TT_AF) == 0) {
+    GcdAttributes |= EFI_MEMORY_RP;
+  }
+
   return GcdAttributes;
 }
 
@@ -451,7 +455,11 @@  GcdAttributeToPageAttribute (
     PageAttributes |= TT_AP_RO_RO;
   }
 
-  return PageAttributes | TT_AF;
+  if ((GcdAttributes & EFI_MEMORY_RP) == 0) {
+    PageAttributes |= TT_AF;
+  }
+
+  return PageAttributes;
 }
 
 EFI_STATUS
@@ -474,9 +482,9 @@  ArmSetMemoryAttributes (
     // No memory type was set in Attributes, so we are going to update the
     // permissions only.
     //
-    PageAttributes &= TT_AP_MASK | TT_UXN_MASK | TT_PXN_MASK;
+    PageAttributes &= TT_AP_MASK | TT_UXN_MASK | TT_PXN_MASK | TT_AF;
     PageAttributeMask = ~(TT_ADDRESS_MASK_BLOCK_ENTRY | TT_AP_MASK |
-                          TT_PXN_MASK | TT_XN_MASK);
+                          TT_PXN_MASK | TT_XN_MASK | TT_AF);
   }
 
   TranslationTable = ArmGetTTBR0BaseAddress ();