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

[Sughosh Ganu <sughosh.ganu@arm.com>]

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.

-sughosh