[edk2-devel] [PATCH v5 27/38] BaseTools/GccBase AARCH64: Avoid page sharing between code and data

Ard Biesheuvel posted 38 patches 1 year, 7 months ago
[edk2-devel] [PATCH v5 27/38] BaseTools/GccBase AARCH64: Avoid page sharing between code and data
Posted by Ard Biesheuvel 1 year, 7 months ago
The AArch64 ARM architecture supports a hardware enforcement mode for
mutual exclusion between code and data: any page that is mapped writable
is implicitly non-executable as well.

This means that remapping part of a runtime image for reapplying
relocation fixups may result in any code sharing the same page to lose
its executable permissions.

Let's avoid this, by moving all quantities that are subject to
relocation fixups to a separate page if the build is using 64k section
alignment, which is only the case when building a runtime driver for
AArch64.

Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
---
 BaseTools/Scripts/GccBase.lds | 13 +++++++++++--
 1 file changed, 11 insertions(+), 2 deletions(-)

diff --git a/BaseTools/Scripts/GccBase.lds b/BaseTools/Scripts/GccBase.lds
index 83cebd29d599..63e097e0727c 100644
--- a/BaseTools/Scripts/GccBase.lds
+++ b/BaseTools/Scripts/GccBase.lds
@@ -21,9 +21,8 @@ SECTIONS {
   . = PECOFF_HEADER_SIZE;
 
   .text : ALIGN(CONSTANT(COMMONPAGESIZE)) {
-    *(.text .text.* .stub .gnu.linkonce.t.*)
+    *(.text .text.* .stub .gnu.linkonce.t.* .plt)
     *(.rodata .rodata.* .gnu.linkonce.r.*)
-    *(.got .got.*)
 
     /*
      * The contents of AutoGen.c files are mostly constant from the POV of the
@@ -34,6 +33,16 @@ SECTIONS {
      * emitted GUIDs here.
      */
     *:AutoGen.obj(.data.g*Guid)
+
+    /*
+     * AArch64 runtime drivers use 64k alignment, and may run in a mode where
+     * mutual exclusion of RO and XP mappings are hardware enforced. In such
+     * cases, the input sections below, which carry any quantities that are
+     * subject to relocation fixups at runtime, must not share a 4 KiB page
+     * with any code content.
+     */
+    . = ALIGN(CONSTANT(COMMONPAGESIZE) > 0x1000 ? 0x1000 : 0x20);
+    *(.got .got.* .data.rel.ro)
   }
 
   /*
-- 
2.39.2



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#101131): https://edk2.groups.io/g/devel/message/101131
Mute This Topic: https://groups.io/mt/97586036/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] [PATCH v5 27/38] BaseTools/GccBase AARCH64: Avoid page sharing between code and data
Posted by Leif Lindholm 1 year, 7 months ago
On Mon, Mar 13, 2023 at 18:17:03 +0100, Ard Biesheuvel wrote:
> The AArch64 ARM architecture supports a hardware enforcement mode for
> mutual exclusion between code and data: any page that is mapped writable
> is implicitly non-executable as well.
> 
> This means that remapping part of a runtime image for reapplying
> relocation fixups may result in any code sharing the same page to lose
> its executable permissions.
> 
> Let's avoid this, by moving all quantities that are subject to
> relocation fixups to a separate page if the build is using 64k section
> alignment, which is only the case when building a runtime driver for
> AArch64.
> 
> Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
> ---
>  BaseTools/Scripts/GccBase.lds | 13 +++++++++++--
>  1 file changed, 11 insertions(+), 2 deletions(-)
> 
> diff --git a/BaseTools/Scripts/GccBase.lds b/BaseTools/Scripts/GccBase.lds
> index 83cebd29d599..63e097e0727c 100644
> --- a/BaseTools/Scripts/GccBase.lds
> +++ b/BaseTools/Scripts/GccBase.lds
> @@ -21,9 +21,8 @@ SECTIONS {
>    . = PECOFF_HEADER_SIZE;
>  
>    .text : ALIGN(CONSTANT(COMMONPAGESIZE)) {
> -    *(.text .text.* .stub .gnu.linkonce.t.*)
> +    *(.text .text.* .stub .gnu.linkonce.t.* .plt)
>      *(.rodata .rodata.* .gnu.linkonce.r.*)
> -    *(.got .got.*)
>  
>      /*
>       * The contents of AutoGen.c files are mostly constant from the POV of the
> @@ -34,6 +33,16 @@ SECTIONS {
>       * emitted GUIDs here.
>       */
>      *:AutoGen.obj(.data.g*Guid)
> +
> +    /*
> +     * AArch64 runtime drivers use 64k alignment, and may run in a mode where

Hmm ... is this strictly speaking true?
I.e., yes, all 4k pages within a 64k page need to share the same
permissions, but that could arguably be provided by pooling 4k
allocations together for multiple runtime drivers?

Will this alignment constraint conflict with that, or just help
enforce the mapping compatibility?

/
    Leif

> +     * mutual exclusion of RO and XP mappings are hardware enforced. In such
> +     * cases, the input sections below, which carry any quantities that are
> +     * subject to relocation fixups at runtime, must not share a 4 KiB page
> +     * with any code content.
> +     */
> +    . = ALIGN(CONSTANT(COMMONPAGESIZE) > 0x1000 ? 0x1000 : 0x20);
> +    *(.got .got.* .data.rel.ro)
>    }
>  
>    /*
> -- 
> 2.39.2
> 
> 
> 
> 
> 
> 


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#101271): https://edk2.groups.io/g/devel/message/101271
Mute This Topic: https://groups.io/mt/97586036/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 v5 27/38] BaseTools/GccBase AARCH64: Avoid page sharing between code and data
Posted by Ard Biesheuvel 1 year, 7 months ago
On Thu, 16 Mar 2023 at 14:46, Leif Lindholm <quic_llindhol@quicinc.com> wrote:
>
> On Mon, Mar 13, 2023 at 18:17:03 +0100, Ard Biesheuvel wrote:
> > The AArch64 ARM architecture supports a hardware enforcement mode for
> > mutual exclusion between code and data: any page that is mapped writable
> > is implicitly non-executable as well.
> >
> > This means that remapping part of a runtime image for reapplying
> > relocation fixups may result in any code sharing the same page to lose
> > its executable permissions.
> >
> > Let's avoid this, by moving all quantities that are subject to
> > relocation fixups to a separate page if the build is using 64k section
> > alignment, which is only the case when building a runtime driver for
> > AArch64.
> >
> > Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
> > ---
> >  BaseTools/Scripts/GccBase.lds | 13 +++++++++++--
> >  1 file changed, 11 insertions(+), 2 deletions(-)
> >
> > diff --git a/BaseTools/Scripts/GccBase.lds b/BaseTools/Scripts/GccBase.lds
> > index 83cebd29d599..63e097e0727c 100644
> > --- a/BaseTools/Scripts/GccBase.lds
> > +++ b/BaseTools/Scripts/GccBase.lds
> > @@ -21,9 +21,8 @@ SECTIONS {
> >    . = PECOFF_HEADER_SIZE;
> >
> >    .text : ALIGN(CONSTANT(COMMONPAGESIZE)) {
> > -    *(.text .text.* .stub .gnu.linkonce.t.*)
> > +    *(.text .text.* .stub .gnu.linkonce.t.* .plt)
> >      *(.rodata .rodata.* .gnu.linkonce.r.*)
> > -    *(.got .got.*)
> >
> >      /*
> >       * The contents of AutoGen.c files are mostly constant from the POV of the
> > @@ -34,6 +33,16 @@ SECTIONS {
> >       * emitted GUIDs here.
> >       */
> >      *:AutoGen.obj(.data.g*Guid)
> > +
> > +    /*
> > +     * AArch64 runtime drivers use 64k alignment, and may run in a mode where
>
> Hmm ... is this strictly speaking true?
> I.e., yes, all 4k pages within a 64k page need to share the same
> permissions, but that could arguably be provided by pooling 4k
> allocations together for multiple runtime drivers?
>

That only works for dynamic allocations. The static allocations are
all part of the PE/COFF image, whose size needs to be a multiple of
the section alignment.

If we decide we don't care about the image base and size after loading
the image (which is reasonable, and TE is based on this notion as
well), we could free the leading and trailing memory that we don't
actually use after dispatching the image.

*However*, the code section, which sits in between those, will always
need to be a multiple of 64k, as it will be mapped with R-X
permissions under the OS. This change only ensures that *within* that
region, the relocatable quantities that may need to be rewritten when
SetVirtualAddressMap() is called don't share a 4k page with executable
instructions, as those would then lose their exec permissions under
WXN.




> Will this alignment constraint conflict with that, or just help
> enforce the mapping compatibility?
>
> /
>     Leif
>
> > +     * mutual exclusion of RO and XP mappings are hardware enforced. In such
> > +     * cases, the input sections below, which carry any quantities that are
> > +     * subject to relocation fixups at runtime, must not share a 4 KiB page
> > +     * with any code content.
> > +     */
> > +    . = ALIGN(CONSTANT(COMMONPAGESIZE) > 0x1000 ? 0x1000 : 0x20);
> > +    *(.got .got.* .data.rel.ro)
> >    }
> >
> >    /*
> > --
> > 2.39.2
> >
> >
> >
> >
> >
> >
>
>
> 
>
>


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