[edk2-devel] [RFC PATCH 00/10] Add PPI to manage PEI phase memory attributes

Ard Biesheuvel posted 10 patches 10 months, 1 week ago
Failed in applying to current master (apply log)
There is a newer version of this series
ArmPkg/Drivers/CpuDxe/CpuMmuCommon.c                             |   2 +-
ArmPkg/Drivers/CpuDxe/MemoryAttribute.c                          |  50 +-----
ArmPkg/Drivers/CpuPei/CpuPei.c                                   |  78 +++++++++-
ArmPkg/Drivers/CpuPei/CpuPei.inf                                 |   7 +-
ArmPkg/Include/Library/ArmMmuLib.h                               |  36 ++++-
ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c                 |  52 ++++++-
ArmPkg/Library/ArmMmuLib/Arm/ArmMmuLibUpdate.c                   |  88 +++++++++--
ArmPkg/Library/OpteeLib/Optee.c                                  |   2 +-
MdeModulePkg/Core/DxeIplPeim/Arm/DxeLoadFunc.c                   |  71 ---------
MdeModulePkg/Core/DxeIplPeim/{Ebc/DxeLoadFunc.c => DxeHandoff.c} |  31 +++-
MdeModulePkg/Core/DxeIplPeim/DxeIpl.inf                          |  24 +--
MdeModulePkg/Core/DxeIplPeim/LoongArch64/DxeLoadFunc.c           |  63 --------
MdeModulePkg/Core/DxeIplPeim/RiscV64/DxeLoadFunc.c               |  75 ---------
MdeModulePkg/Core/Pei/Image/Image.c                              | 160 ++++++++++++++++++++
MdeModulePkg/Core/Pei/PeiMain.h                                  |   6 +
MdeModulePkg/Core/Pei/PeiMain.inf                                |   1 +
MdeModulePkg/Include/Ppi/MemoryAttribute.h                       |  78 ++++++++++
MdeModulePkg/MdeModulePkg.dec                                    |   3 +
OvmfPkg/RiscVVirt/RiscVVirtQemu.dsc                              |   6 -
19 files changed, 523 insertions(+), 310 deletions(-)
delete mode 100644 MdeModulePkg/Core/DxeIplPeim/Arm/DxeLoadFunc.c
rename MdeModulePkg/Core/DxeIplPeim/{Ebc/DxeLoadFunc.c => DxeHandoff.c} (62%)
delete mode 100644 MdeModulePkg/Core/DxeIplPeim/LoongArch64/DxeLoadFunc.c
delete mode 100644 MdeModulePkg/Core/DxeIplPeim/RiscV64/DxeLoadFunc.c
create mode 100644 MdeModulePkg/Include/Ppi/MemoryAttribute.h
[edk2-devel] [RFC PATCH 00/10] Add PPI to manage PEI phase memory attributes
Posted by Ard Biesheuvel 10 months, 1 week ago
REF: https://bugzilla.tianocore.org/show_bug.cgi?id=4468

This is a proof-of-concept RFC that implements a PEI phase PPI to manage
memory permission attributes, and wires it up to the PEI image loader so
that shadowed PEIMs as well as the DXE core are remapped with the
appropriate, restricted memory permission attributes before execution.

This means that neither shadowed PEIMs nor the DXE core will ever
execute with writable code regions. It also removes the need on the part
of PEI for memory to be mapped with both writable and executable
permissions by default out of reset. Similar work still needs to be done
to address the early DXE phase (before the CPU arch protocol becomes
available), but once that is out of the way as well, platforms should be
able to map all memory non-executable from the beginning.

This by itself is a major improvement in terms of robustness. It is also
a prerequisite for enabling the WXN MMU control on AArch64, which makes
all writable memory mappings non-executable regardless of the non-exec
page table attribute.

Patches #1 to #4 are prepatory work.
Patch #5 proposes the memory attribute PPI protocol interface.
Patch #6 implements it for ARM and AARCH64.
Patch #7 wires it up into the PEI image loader.
Patches #8 to #10 update the DxeIpl to use this PPI on ARM/AARCH64 for
mapping the stack NX.
instead of an explicit reference to ArmMmuLib. Other architectures
(except IA32/X64) will seamlessly inherit this once they implement the
PPI as well.

Cc: Ray Ni <ray.ni@intel.com>
Cc: Jiewen Yao <jiewen.yao@intel.com>
Cc: Gerd Hoffmann <kraxel@redhat.com>
Cc: Taylor Beebe <t@taylorbeebe.com>
Cc: Oliver Smith-Denny <osd@smith-denny.com>
Cc: Dandan Bi <dandan.bi@intel.com>
Cc: Liming Gao <gaoliming@byosoft.com.cn>
Cc: "Kinney, Michael D" <michael.d.kinney@intel.com>
Cc: Leif Lindholm <quic_llindhol@quicinc.com>
Cc: Sunil V L <sunilvl@ventanamicro.com> 
Cc: Andrei Warkentin <andrei.warkentin@intel.com> 

Ard Biesheuvel (10):
  ArmPkg/ArmMmuLib: Extend API to manage memory permissions better
  ArmPkg/CpuDxe: Simplify memory attributes protocol implementation
  ArmPkg/CpuPei: Drop bogus DEPEX on PEI permanent memory
  OvmfPkg/RiscVVirt: Remove unimplemented NxForStack configuration
  MdeModulePkg: Define memory attribute PPI
  ArmPkg/CpuPei: Implement the memory attributes PPI
  MdeModulePkg/PeiCore: Apply restricted permissions in image loader
  MdeModulePkg/DxeIpl: Merge EBC, RISCV64 and LOONGARCH code
  MdeModulePkg/DxeIpl: Use memory attribute PPI to remap the stack NX
  MdeModulePkg/DxeIpl ARM AARCH64: Switch to generic handoff code

 ArmPkg/Drivers/CpuDxe/CpuMmuCommon.c                             |   2 +-
 ArmPkg/Drivers/CpuDxe/MemoryAttribute.c                          |  50 +-----
 ArmPkg/Drivers/CpuPei/CpuPei.c                                   |  78 +++++++++-
 ArmPkg/Drivers/CpuPei/CpuPei.inf                                 |   7 +-
 ArmPkg/Include/Library/ArmMmuLib.h                               |  36 ++++-
 ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c                 |  52 ++++++-
 ArmPkg/Library/ArmMmuLib/Arm/ArmMmuLibUpdate.c                   |  88 +++++++++--
 ArmPkg/Library/OpteeLib/Optee.c                                  |   2 +-
 MdeModulePkg/Core/DxeIplPeim/Arm/DxeLoadFunc.c                   |  71 ---------
 MdeModulePkg/Core/DxeIplPeim/{Ebc/DxeLoadFunc.c => DxeHandoff.c} |  31 +++-
 MdeModulePkg/Core/DxeIplPeim/DxeIpl.inf                          |  24 +--
 MdeModulePkg/Core/DxeIplPeim/LoongArch64/DxeLoadFunc.c           |  63 --------
 MdeModulePkg/Core/DxeIplPeim/RiscV64/DxeLoadFunc.c               |  75 ---------
 MdeModulePkg/Core/Pei/Image/Image.c                              | 160 ++++++++++++++++++++
 MdeModulePkg/Core/Pei/PeiMain.h                                  |   6 +
 MdeModulePkg/Core/Pei/PeiMain.inf                                |   1 +
 MdeModulePkg/Include/Ppi/MemoryAttribute.h                       |  78 ++++++++++
 MdeModulePkg/MdeModulePkg.dec                                    |   3 +
 OvmfPkg/RiscVVirt/RiscVVirtQemu.dsc                              |   6 -
 19 files changed, 523 insertions(+), 310 deletions(-)
 delete mode 100644 MdeModulePkg/Core/DxeIplPeim/Arm/DxeLoadFunc.c
 rename MdeModulePkg/Core/DxeIplPeim/{Ebc/DxeLoadFunc.c => DxeHandoff.c} (62%)
 delete mode 100644 MdeModulePkg/Core/DxeIplPeim/LoongArch64/DxeLoadFunc.c
 delete mode 100644 MdeModulePkg/Core/DxeIplPeim/RiscV64/DxeLoadFunc.c
 create mode 100644 MdeModulePkg/Include/Ppi/MemoryAttribute.h

-- 
2.39.2



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#105300): https://edk2.groups.io/g/devel/message/105300
Mute This Topic: https://groups.io/mt/99131172/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] [RFC PATCH 00/10] Add PPI to manage PEI phase memory attributes
Posted by Oliver Smith-Denny 10 months, 1 week ago
On 5/25/2023 7:30 AM, Ard Biesheuvel wrote:
> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=4468
> 
> 
> 
> This is a proof-of-concept RFC that implements a PEI phase PPI to manage
> 
> memory permission attributes, and wires it up to the PEI image loader so
> 
> that shadowed PEIMs as well as the DXE core are remapped with the
> 
> appropriate, restricted memory permission attributes before execution.
> 
> 
> 
> This means that neither shadowed PEIMs nor the DXE core will ever
> 
> execute with writable code regions. It also removes the need on the part
> 
> of PEI for memory to be mapped with both writable and executable
> 
> permissions by default out of reset. Similar work still needs to be done
> 
> to address the early DXE phase (before the CPU arch protocol becomes
> 
> available), but once that is out of the way as well, platforms should be
> 
> able to map all memory non-executable from the beginning.
> 
> 
> 
> This by itself is a major improvement in terms of robustness. It is also
> 
> a prerequisite for enabling the WXN MMU control on AArch64, which makes
> 
> all writable memory mappings non-executable regardless of the non-exec
> 
> page table attribute.
> 
> 
> 
> Patches #1 to #4 are prepatory work.
> 
> Patch #5 proposes the memory attribute PPI protocol interface.
> 
> Patch #6 implements it for ARM and AARCH64.
> 
> Patch #7 wires it up into the PEI image loader.
> 
> Patches #8 to #10 update the DxeIpl to use this PPI on ARM/AARCH64 for
> 
> mapping the stack NX.
> 
> instead of an explicit reference to ArmMmuLib. Other architectures
> 
> (except IA32/X64) will seamlessly inherit this once they implement the
> 
> PPI as well.
> 

Hi Ard,

Thanks for the RFC. This same issue exists for X64, I saw your mailing
list question about IA32 PEI, do you have plans for extending this to
X64 PEI (I'm not sure its state of readiness)?

If so, do you think it is feasible to merge the X64 DxeLoadFunc with
the generic one you have created?

Overall, this seems a reasonable approach to me towards making DXE more
protected with the additional benefit of protecting shadowed PEIMs.

I did wonder if the same discussion we are having on the DXE side about
moving these MMU manipulations to the core instead of the CPU driver
make sense in PEI, too, but with the greatly reduced complexity of the
environment (no GCD, etc.), I don't think it makes sense now and that
focusing on the DXE reorganization and expansion is going to deliver
a much greater impact.

Thanks,
Oliver


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#105319): https://edk2.groups.io/g/devel/message/105319
Mute This Topic: https://groups.io/mt/99131172/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] [RFC PATCH 00/10] Add PPI to manage PEI phase memory attributes
Posted by Ard Biesheuvel 10 months, 1 week ago
On Thu, 25 May 2023 at 19:21, Oliver Smith-Denny
<osde@linux.microsoft.com> wrote:
>
> On 5/25/2023 7:30 AM, Ard Biesheuvel wrote:
> > REF: https://bugzilla.tianocore.org/show_bug.cgi?id=4468
> >
> >
> >
> > This is a proof-of-concept RFC that implements a PEI phase PPI to manage
> >
> > memory permission attributes, and wires it up to the PEI image loader so
> >
> > that shadowed PEIMs as well as the DXE core are remapped with the
> >
> > appropriate, restricted memory permission attributes before execution.
> >
> >
> >
> > This means that neither shadowed PEIMs nor the DXE core will ever
> >
> > execute with writable code regions. It also removes the need on the part
> >
> > of PEI for memory to be mapped with both writable and executable
> >
> > permissions by default out of reset. Similar work still needs to be done
> >
> > to address the early DXE phase (before the CPU arch protocol becomes
> >
> > available), but once that is out of the way as well, platforms should be
> >
> > able to map all memory non-executable from the beginning.
> >
> >
> >
> > This by itself is a major improvement in terms of robustness. It is also
> >
> > a prerequisite for enabling the WXN MMU control on AArch64, which makes
> >
> > all writable memory mappings non-executable regardless of the non-exec
> >
> > page table attribute.
> >
> >
> >
> > Patches #1 to #4 are prepatory work.
> >
> > Patch #5 proposes the memory attribute PPI protocol interface.
> >
> > Patch #6 implements it for ARM and AARCH64.
> >
> > Patch #7 wires it up into the PEI image loader.
> >
> > Patches #8 to #10 update the DxeIpl to use this PPI on ARM/AARCH64 for
> >
> > mapping the stack NX.
> >
> > instead of an explicit reference to ArmMmuLib. Other architectures
> >
> > (except IA32/X64) will seamlessly inherit this once they implement the
> >
> > PPI as well.
> >
>
> Hi Ard,
>

Hi,

Thanks for taking a look.

> Thanks for the RFC. This same issue exists for X64, I saw your mailing
> list question about IA32 PEI, do you have plans for extending this to
> X64 PEI (I'm not sure its state of readiness)?
>

Yes, but I need to wrap my head around that code first (I'm not as
familiar with low-level X64 as I am with ARM). I though I'd send this
RFC out in the meantime to get some alignment on the general shape of
things.

> If so, do you think it is feasible to merge the X64 DxeLoadFunc with
> the generic one you have created?
>

Hopefully. There are some pieces there that are highly X64 specific,
though, but at least the permissions management code could be shared,
in which case it can be moved out of the arch-specific source file and
into the generic one (this is one of the reasons I merged that code
between ARM, AARCH64, RISC-V and LoongArch64). Pure IA32 will simply
not implement the PPI, and the long mode switch stuff can remain IA32
specific afaict (although we'll be retaining that only for diagnostic
purposes)

> Overall, this seems a reasonable approach to me towards making DXE more
> protected with the additional benefit of protecting shadowed PEIMs.
>
> I did wonder if the same discussion we are having on the DXE side about
> moving these MMU manipulations to the core instead of the CPU driver
> make sense in PEI, too, but with the greatly reduced complexity of the
> environment (no GCD, etc.), I don't think it makes sense now and that
> focusing on the DXE reorganization and expansion is going to deliver
> a much greater impact.
>

IMHO the modularity has its advantages in some cases, and this
particular use case does not force us to deviate from that principle,
so I'd rather keep that as a separate discussion. On ARM (and now on
X64 for TDX) we already have PEI-less platforms where SEC dispatches
the DXE core directly, so I'm not convinced we really need something
in between.

-- 
Ard.


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