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

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

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