Re: [PATCH v2 7/7] ArmPkg: Extra action to update permissions for S-ELO MM Image

[Achin Gupta <Achin.Gupta@arm.com>]

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 <Achin.Gupta@arm.com> 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 <sughosh.ganu@arm.com> 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 <sughosh.ganu@arm.com> wrote:
> >> >> > >
> >> >> > > From: Achin Gupta <achin.gupta@arm.com>
> >> >> > >
> >> >> > > 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 <sughosh.ganu@arm.com>
> >> >> > 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
> >> edk2-devel@lists.01.org
> >> https://lists.01.org/mailman/listinfo/edk2-devel