Re: [edk2-devel] [RFC PATCH 07/10] MdeModulePkg/PeiCore: Apply restricted permissions in image loader

["Oliver Smith-Denny" <osde@linux.microsoft.com>]

On 5/25/2023 2:29 PM, Ard Biesheuvel wrote:
> 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:
>>> +  if (Private->MemoryAttributePpi == NULL) {
>>>
>>> +    return EFI_SUCCESS;
>>
>> We will have a gap here before the MemoryAttributePpi is installed,
>> obviously, when CpuPei is installed. Is the expectation that only
>> dependencies for CpuPei will be launched before and everything else
>> will have a dependency on CpuPei?
>>
>> Or, is it that shadowed PEIMs won't happen before CpuPei? I am curious
>> how big or small of a gap this really is.
>>
> 
> There are two different cases to consider here:
> - First, there is the DxeIpl, which will rely on the PPI (via the
> image loader and directly) to map the DXE core and the NX stack. The
> DxeIpl will not proceed with that until all PEIMs are dispatched, so
> if the PPI is going to be exposed, it will be available by that time.
> - Then there are the shadowed PEIMs, and given that shadowed PEIMs
> implicitly depend on PEI permanent memory having been installed, the
> only requirement here is that, if the platform needs/wants this for
> shadowed PEIMs as well, the PEIM that installs the PEI permanent
> memory should depex on the PPI.
> 

I see, thanks for the explanation. While this is not the main point
of this patchset, I do think that the core should own the memory
protections of the shadowed PEIMs, instead of relying on the platform
to do the right thing (which it never does :). Sure, this is gated by
PCDs that the platform sets, but that is a simpler interface than
requiring a new depex.

Perhaps this is not trivial, as I assume the permanent memory
installation happens in a platform specific PEIM? And as such would
require a PPI notification in the core for the memory attributes PPI,
which would lead to additional undesirable complexity.

>>>
>>> +  }
>>>
>>> +
>>>
>>> +  //
>>>
>>> +  // PEI phase executables must be able to execute in place from read-only NOR
>>>
>>> +  // flash, and so they can be mapped read-only in their entirety.
>>>
>>> +  //
>>>
>>> +  if ((FileType == EFI_FV_FILETYPE_PEI_CORE) ||
>>>
>>> +      (FileType == EFI_FV_FILETYPE_PEIM) ||
>>>
>>> +      (FileType == EFI_FV_FILETYPE_COMBINED_PEIM_DRIVER))
>>>
>>> +  {
>>
>> We are calling this from PEI Core, will we have more images of type
>> PEI Core? I understand if this is for completeness, but just making
>> sure I understand the flow.
>>
> 
> PEI core itself may be shadowed as well, and will be mapped read-only
> as well if it is included here.
> 

Gotcha, thanks.

Oliver