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

["Ard Biesheuvel" <ardb@kernel.org>]

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.