Hi Ard, Just a polite poke that Sughosh had posted the patches as I had described below here [1] & here [2]. Please let us know what you think.
cheers, Achin [1] https://lists.01.org/pipermail/edk2-devel/2018-October/031377.html [2] https://lists.01.org/pipermail/edk2-devel/2018-October/031384.html On Wed, Oct 24, 2018 at 08:05:22AM -0300, Ard Biesheuvel wrote: > 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
