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

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

Hi Ard,

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