[edk2,RFC,2/2] ArmVirtQemu: restrict RWX mappings

This reduces the amount of memory mapped as both writable and executable
to the absolute minimum, by mapping all of memory non-executable by
default, and using PermissionsPeCoffExtraActionLib to only map those
regions executable that require it for execution. If possible, these
regions are remapped read-only at the same time, but in some cases
(runtime drivers, TE images, images with < 4 KB section alignment) we
cannot avoid having to map the entire PE/COFF image RWX.

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

---
 ArmVirtPkg/ArmVirtQemu.dsc                                           | 9 ++++++++-
 ArmVirtPkg/Library/ArmVirtMemoryInitPeiLib/ArmVirtMemoryInitPeiLib.c | 3 +++
 2 files changed, 11 insertions(+), 1 deletion(-)

-- 
2.7.4

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

On 05/03/16 14:45, Ard Biesheuvel wrote:
> This reduces the amount of memory mapped as both writable and executable

> to the absolute minimum, by mapping all of memory non-executable by

> default, and using PermissionsPeCoffExtraActionLib to only map those

> regions executable that require it for execution. If possible, these

> regions are remapped read-only at the same time, but in some cases

> (runtime drivers, TE images, images with < 4 KB section alignment) we

> cannot avoid having to map the entire PE/COFF image RWX.

> 

> Contributed-under: TianoCore Contribution Agreement 1.0

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

> ---

>  ArmVirtPkg/ArmVirtQemu.dsc                                           | 9 ++++++++-

>  ArmVirtPkg/Library/ArmVirtMemoryInitPeiLib/ArmVirtMemoryInitPeiLib.c | 3 +++

>  2 files changed, 11 insertions(+), 1 deletion(-)


(1) Could you include a comprehensive table in the commit message that
describes what handling affects what driver type, and where each
treatment is established?

For example, you mention TE images as exempt. According to
"ArmVirtQemu.fdf", this means SEC, PEI_CORE, PEIM. However, below you
link the library into PeiMain.inf (i.e., the PEI_CORE) as well. Why is
that correct?

Also, why must e.g. runtime drivers be mapped fully RWX?

(2) Shouldn't ArmVirtQemuKernel.dsc be modified as well? (Maybe not
identically, since it runs entirely from DRAM, but still?)

(3) I think it's worth keeping in mind that on X64 and Ia32, some
drivers generate code dynamically. For example, in the implementation of
EFI_MP_SERVICES_PROTOCOL. I don't think it should block this series, but
perhaps make a note somewhere (code? commit message?) that such drivers
will have to massage their mappings individually on aarch64.

Thanks!
Laszlo

> diff --git a/ArmVirtPkg/ArmVirtQemu.dsc b/ArmVirtPkg/ArmVirtQemu.dsc

> index 34323bf83d64..fbddd7fac7b5 100644

> --- a/ArmVirtPkg/ArmVirtQemu.dsc

> +++ b/ArmVirtPkg/ArmVirtQemu.dsc

> @@ -82,6 +82,9 @@ [BuildOptions]

>    GCC:*_*_ARM_PLATFORM_FLAGS == -mcpu=cortex-a15 -I$(WORKSPACE)/ArmVirtPkg/Include

>    *_*_AARCH64_PLATFORM_FLAGS == -I$(WORKSPACE)/ArmVirtPkg/Include

>  

> +[BuildOptions.common.EDKII.DXE_CORE,BuildOptions.common.EDKII.DXE_DRIVER,BuildOptions.common.EDKII.UEFI_DRIVER]

> +  GCC:*_*_AARCH64_DLINK_FLAGS = -z common-page-size=0x1000

> +

>  

>  ################################################################################

>  #

> @@ -243,7 +246,10 @@ [Components.common]

>    # PEI Phase modules

>    #

>    ArmPlatformPkg/PrePeiCore/PrePeiCoreUniCore.inf

> -  MdeModulePkg/Core/Pei/PeiMain.inf

> +  MdeModulePkg/Core/Pei/PeiMain.inf {

> +    <LibraryClasses>

> +      PeCoffExtraActionLib|ArmPkg/Library/PermissionsPeCoffExtraActionLib/PermissionsPeCoffExtraActionLib.inf

> +  }

>    MdeModulePkg/Universal/PCD/Pei/Pcd.inf {

>      <LibraryClasses>

>        PcdLib|MdePkg/Library/BasePcdLibNull/BasePcdLibNull.inf

> @@ -265,6 +271,7 @@ [Components.common]

>    MdeModulePkg/Core/Dxe/DxeMain.inf {

>      <LibraryClasses>

>        NULL|MdeModulePkg/Library/DxeCrc32GuidedSectionExtractLib/DxeCrc32GuidedSectionExtractLib.inf

> +      PeCoffExtraActionLib|ArmPkg/Library/PermissionsPeCoffExtraActionLib/PermissionsPeCoffExtraActionLib.inf

>    }

>    MdeModulePkg/Universal/PCD/Dxe/Pcd.inf {

>      <LibraryClasses>

> diff --git a/ArmVirtPkg/Library/ArmVirtMemoryInitPeiLib/ArmVirtMemoryInitPeiLib.c b/ArmVirtPkg/Library/ArmVirtMemoryInitPeiLib/ArmVirtMemoryInitPeiLib.c

> index f6c69152848e..c5899aa2ac3c 100644

> --- a/ArmVirtPkg/Library/ArmVirtMemoryInitPeiLib/ArmVirtMemoryInitPeiLib.c

> +++ b/ArmVirtPkg/Library/ArmVirtMemoryInitPeiLib/ArmVirtMemoryInitPeiLib.c

> @@ -111,6 +111,9 @@ MemoryPeim (

>    // Build Memory Allocation Hob

>    InitMmu ();

>  

> +  ArmSetMemoryRegionNoExec ((EFI_PHYSICAL_ADDRESS)PcdGet64 (PcdSystemMemoryBase),

> +                            PcdGet64 (PcdSystemMemorySize));

> +

>    if (FeaturePcdGet (PcdPrePiProduceMemoryTypeInformationHob)) {

>      // Optional feature that helps prevent EFI memory map fragmentation.

>      BuildMemoryTypeInformationHob ();

> 


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

On 3 May 2016 at 16:01, Laszlo Ersek <lersek@redhat.com> wrote:
> On 05/03/16 14:45, Ard Biesheuvel wrote:

>> This reduces the amount of memory mapped as both writable and executable

>> to the absolute minimum, by mapping all of memory non-executable by

>> default, and using PermissionsPeCoffExtraActionLib to only map those

>> regions executable that require it for execution. If possible, these

>> regions are remapped read-only at the same time, but in some cases

>> (runtime drivers, TE images, images with < 4 KB section alignment) we

>> cannot avoid having to map the entire PE/COFF image RWX.

>>

>> Contributed-under: TianoCore Contribution Agreement 1.0

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

>> ---

>>  ArmVirtPkg/ArmVirtQemu.dsc                                           | 9 ++++++++-

>>  ArmVirtPkg/Library/ArmVirtMemoryInitPeiLib/ArmVirtMemoryInitPeiLib.c | 3 +++

>>  2 files changed, 11 insertions(+), 1 deletion(-)

>

> (1) Could you include a comprehensive table in the commit message that

> describes what handling affects what driver type, and where each

> treatment is established?

>


Sure

> For example, you mention TE images as exempt. According to

> "ArmVirtQemu.fdf", this means SEC, PEI_CORE, PEIM. However, below you

> link the library into PeiMain.inf (i.e., the PEI_CORE) as well. Why is

> that correct?

>


PeiCore and DxeCore both contain PE/COFF loaders, which is why they
need this library resolution. This is independent of whether these
modules are TE themselves.

> Also, why must e.g. runtime drivers be mapped fully RWX?

>


Runtime drivers are relocated a second time when
SetVirtualAddressMap() is called, and the boot time MMU configuration
may still be live at this point. Since these relocations point into
the .text section as well (e.g., to relocate const structures
containing function pointers), they cannot be mapped read-only. This
is a bit disappointing, actually, and I wonder if there is a better
solution. In fact, i was hoping for some discussion on this topic in
general, not necessarily on the patches.

> (2) Shouldn't ArmVirtQemuKernel.dsc be modified as well? (Maybe not

> identically, since it runs entirely from DRAM, but still?)

>


The fact that it runs from DRAM is a problem. In fact, this patch will
break if applies as is. The reason is that remapping all of RAM noexec
will affect the code that is currently being executed. Here,
ArmVirtQemu has a big advantage since NOR flash is R-X by nature.

> (3) I think it's worth keeping in mind that on X64 and Ia32, some

> drivers generate code dynamically. For example, in the implementation of

> EFI_MP_SERVICES_PROTOCOL. I don't think it should block this series, but

> perhaps make a note somewhere (code? commit message?) that such drivers

> will have to massage their mappings individually on aarch64.

>


This is exactly the kind of feedback i was hoping for (hence the RFC
in the subject line). There may be other cases where executable code
is copied (and now that I think of it, we have such code in AArch64 as
well). When it is arch-specific code, that can be handled ad hoc, but
if there are generic ways of putting code on the heap, it would
require some standardized protocol to manage this.
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel