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

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

Hi Ard,

Apologies for the delay! CIL.

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.

This is a bit misleading. For Standalone MM drivers, memory is allocated from
the heap which is marked as RW+XN by the SPM in the S-EL1 page tables. This
allows PeCoffLoaderLoadImage() to load the image from the FV into its newly
allocated buffer. Before the driver can be executed, its rodata and code
sections need a permission update from RW+XN to RO+XN and RO+X respectively. 

This patch does these permission changes after the relocation fixups have been
done in PeCoffLoaderRelocateImageExtraAction().

> >> > >
> >> > > 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.

IIUC, your concern is about the way the PeCoffLoaderRelocateImageExtraAction()
has been used? Is it meant to fulfil a different purpose? PE-COFF image loading
and relocation is done by generic code and this seemed like the best mechanism
to introduce an arch. specific hook.

I agree that implementing the hooks in a DebugPeCoffExtraActionLib is not
suitable for upstreaming. We could implement this within the StandaloneMmPkg as
an Aarch64 library since it is unlikely it will ever be used in the ArmPkg
itself.

Please let us know what you think.

cheers,
Achin
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel