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

[Ard Biesheuvel <ard.biesheuvel@linaro.org>]

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.