On 24 October 2018 at 05:22, Achin Gupta <[email protected]> wrote: > Hi Ard, > > Please see CIL.. >
FYI I will be on leave until 5th of November, so I will get to this once I get back. -- Ard. > On Fri, Aug 24, 2018 at 03:55:29PM +0100, Ard Biesheuvel wrote: >> On 21 August 2018 at 07:50, Sughosh Ganu <[email protected]> wrote: >> > hi Ard, >> > >> > On Tue July 23, 2018 at 11:03PM +0530, Supreeth Venkatesh wrote: >> >> >> >> On Sat, 2018-07-21 at 20:06 +0900, Ard Biesheuvel wrote: >> >> > On 20 July 2018 at 21:38, Sughosh Ganu <[email protected]> wrote: >> >> > > >> >> > > From: Achin Gupta <[email protected]> >> >> > > >> >> > > The Standalone MM drivers runs in S-EL0 in AArch64 on ARM Standard >> >> > > Platforms and is deployed during SEC phase. The memory allocated to >> >> > > the Standalone MM drivers should be marked as RO+X. >> >> > > >> >> > > During PE/COFF Image section parsing, this patch implements extra >> >> > > action "UpdatePeCoffPermissions" to request the privileged firmware >> >> > > in >> >> > > EL3 to update the permissions. >> >> > > >> >> > > Contributed-under: TianoCore Contribution Agreement 1.1 >> >> > > Signed-off-by: Sughosh Ganu <[email protected]> >> >> > Apologies for bringing this up only now, but I don't think I was ever >> >> > cc'ed on these patches. >> >> > >> >> Apologies if you have missed it. But I am pretty sure it was part of >> >> earlier large patch-set on which you and leif were copied, as it was >> >> part of ArmPkg. >> >> > >> >> > We are relying on a debug hook in the PE/COFF loader to ensure that >> >> > we >> >> > don't end up with memory that is both writable and executable in the >> >> > secure world. Do we really think that is a good idea? >> >> > >> >> > (I know this code was derived from a proof of concept that I did >> >> > years >> >> > ago, but that was just a PoC) >> >> I think we need a little bit more details on what is your suggestion? >> >> >> >> A little bit background here: This code runs in S-EL0 and Request gets >> >> sent to secure world SPM to ensure that the region permissions are >> >> updated correctly via the "ArmMmuStandaloneMmCoreLib" SVC - >> >> ARM_SVC_ID_SP_SET_MEM_ATTRIBUTES_AARCH64. >> >> >> >> DebugPeCoffExtraActionLib is just used to extract image region >> >> information, but the region permission >> >> update request is sent to secure world for validation. >> >> >> >> With the above explanation, can you provide an insight into what was >> >> your thinking? >> >> Do you want us to create a separate library and call it >> >> as PeCoffExtraActionLib to avoid the "Debug" word though it is a hook >> >> to PeCoffExtraActionLib in MdePkg or do we want to create this library >> >> in a separate package (may be in MdePkg?) or something totally >> >> different. >> > >> > Supreeth had replied to your comments on the patch. Can you please >> > check this. If you feel that this needs to be implemented differently, >> > can you please suggest it to us. Thanks. >> > >> >> My point is that such a fundamental action that needs to occur while >> loading the PE/COFF image should not be hooked into the loader this >> way. > > Based upon our discussion at the Linaro Connect, we investigated leveraging > the > DXE Image Protection support [1] in Standalone MM (StMM). Amongst other > challenges, there is a chicken and egg problem. I will try and explain. > > DXE Memory protection has dependencies that cannot be fulfilled in StMM. A > non-exhaustive list is: > > 1. Dependency on CPU_ARCH protocol > 2. Dependency on Loaded Image patch protocol > 3. Dependency on Boot services > > There is an inherent assumption that this support will never be used in > SMM. Furthermore, in StMM, permissions are changed when the StMM drivers are > first dispatched. A dependency on a driver to change the permissions is the > chicken and egg. So we need a library. > > One option is to introduce a memory protection library in StMM i.e. a library > interface like StandaloneMmImageProtect(). This function will be called from > generic code after the PE-COFF loader has loaded and relocated the StMM driver > image. However, this support is not required on x86. They will have to > include a > NULL library implementation. This would be in addition to the NULL > PeCoffExtraActionLib they already include through MdePkg.dsc. > > I am hesitant to take this approach in the absence of a requirement on x86. At > the same time, the current approach of leveraging the > DebugPeCoffExtraActionLib > in ArmPkg does not make sense either. > > IMO, the better approach would be to add a AArch64 specific > StandaloneMmPeCoffExtraActionLib in the StandaloneMmPkg. Memory protection > will > be implemented in the relocation hook. There will be no impact on x86 or the > ArmPkg. If in future there is a requirement to support this feature on x86 as > well, then a separate library could be implemented. > > Please let us know if this sounds reasonable to you. Sughosh will be posting > the > patches with this approach in a bit to aid the discussion. > > Cheers, > Achin > > [1] MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c > >> _______________________________________________ >> edk2-devel mailing list >> [email protected] >> https://lists.01.org/mailman/listinfo/edk2-devel _______________________________________________ edk2-devel mailing list [email protected] https://lists.01.org/mailman/listinfo/edk2-devel
