SecCore evacuation in PeiCore?

SecCore evacuation in PeiCore?

[Marvin Häuser]

Good day everyone,
Good day Michael,

The commit that introduced T-RAM evacuation [1] also introduced the 
function "MigrateSecModulesInFv()". It also is explicitly mentioned as 
part of the control flow in the commit message. As far as I can see, 
since then till today this function has never been called anywhere. Was 
this some draft function that accidentally made it into the patch, or 
did the caller get lost somewhere? The description makes sense to me and 
I'm not experienced enough with the PeiCore control flow to tell whether 
the PEIM migration somehow covers SecCore implicitly. Also I noticed it 
only supports SecCore in a PE/COFF section, not a TE section. Is there a 
rationale for that?

Thank you for your time!

Best regards,
Marvin


[1] 
https://github.com/tianocore/edk2/commit/9bedaec05b7b8ba9aee248361bb61a85a26726cb

Re: [edk2-devel] SecCore evacuation in PeiCore?

[Michael Kubacki]

Hi Marvin,

I apologize for the delayed response, I missed this message earlier. The 
function was called from EvacuateTempRam() in the initial set of patches:
[PATCH 1/6] MdeModulePkg/PeiCore: Enable T-RAM evacuation in PeiCore 
(CVE-2019-11098) (groups.io) <https://edk2.groups.io/g/devel/message/61823>

I was not involved in the patch series on the mailing list (job role 
change at the time) but as a comment in that patch notes, there was an 
inconsistency observed in PE32 section alignment in SEC modules. I don't 
see where this was resolved other than the calls being removed later in 
the series. SecCore migration would not occur implicitly in the PeiCore 
flow but there is functionality for SEC data migration in 
UefiCpuPkg/SecMigrationPei.

Based on what I see now, I'd be happy to send a patch to remove 
MigrateSecModulesInFv().

Thanks,
Michael

On 8/7/2021 2:54 PM, Marvin Häuser wrote:
> Good day everyone,
> Good day Michael,
>
> The commit that introduced T-RAM evacuation [1] also introduced the 
> function "MigrateSecModulesInFv()". It also is explicitly mentioned as 
> part of the control flow in the commit message. As far as I can see, 
> since then till today this function has never been called anywhere. 
> Was this some draft function that accidentally made it into the patch, 
> or did the caller get lost somewhere? The description makes sense to 
> me and I'm not experienced enough with the PeiCore control flow to 
> tell whether the PEIM migration somehow covers SecCore implicitly. 
> Also I noticed it only supports SecCore in a PE/COFF section, not a TE 
> section. Is there a rationale for that?
>
> Thank you for your time!
>
> Best regards,
> Marvin
>
>
> [1] 
> https://github.com/tianocore/edk2/commit/9bedaec05b7b8ba9aee248361bb61a85a26726cb
>
>
> 
>

Re: [edk2-devel] SecCore evacuation in PeiCore?

[Marvin Häuser]

Hey Michael,

Thank you for your response! It was actually quicker than I imagined. :)

I think I understand, but please let me try to get this absolutely 
right. Can I think of "SecMigrationPei" as a sort of "SecCorePostMem", 
which either is loaded into permanent RAM directly or is shadowed 
because it is a PEIM unlike SecCore - and it republishes all public 
data, most especially PPIs, such that the entire PEI stage no longer has 
any references to the original SecCore at all, and the SecCore module 
basically just sits there in the ROM, and its exposed data is either 
discarded or orphaned? Is that about right?

I think I hit the alignment issue of SecCore too, but only for X64 
builds (likely just because the size happens to be lucky for IA32) of 
OVMF. Pretty much sure it's just ResetVector positioning. What would be 
the issue with moving the ResetVector into a separate component, with 
its fixed position in FD (this is actually how UefiCpuPkg/VTF0 works), 
and having SecCore aligned correctly? Not specifically to restore 
MigrateSecModulesInFv(), but as future-proofing to ensure expected 
outputs. In fact, I noticed because my new PE loader code was upset 
about the unaligned XIP load address.

Also thanks for your patch!

Best regards,
Marvin

On 13/08/2021 18:51, Michael Kubacki wrote:
> Hi Marvin,
>
> I apologize for the delayed response, I missed this message earlier. 
> The function was called from EvacuateTempRam() in the initial set of 
> patches:
> [PATCH 1/6] MdeModulePkg/PeiCore: Enable T-RAM evacuation in PeiCore 
> (CVE-2019-11098) (groups.io) 
> <https://edk2.groups.io/g/devel/message/61823>
>
> I was not involved in the patch series on the mailing list (job role 
> change at the time) but as a comment in that patch notes, there was an 
> inconsistency observed in PE32 section alignment in SEC modules. I 
> don't see where this was resolved other than the calls being removed 
> later in the series. SecCore migration would not occur implicitly in 
> the PeiCore flow but there is functionality for SEC data migration in 
> UefiCpuPkg/SecMigrationPei.
>
> Based on what I see now, I'd be happy to send a patch to remove 
> MigrateSecModulesInFv().
>
> Thanks,
> Michael
>
> On 8/7/2021 2:54 PM, Marvin Häuser wrote:
>> Good day everyone,
>> Good day Michael,
>>
>> The commit that introduced T-RAM evacuation [1] also introduced the 
>> function "MigrateSecModulesInFv()". It also is explicitly mentioned 
>> as part of the control flow in the commit message. As far as I can 
>> see, since then till today this function has never been called 
>> anywhere. Was this some draft function that accidentally made it into 
>> the patch, or did the caller get lost somewhere? The description 
>> makes sense to me and I'm not experienced enough with the PeiCore 
>> control flow to tell whether the PEIM migration somehow covers 
>> SecCore implicitly. Also I noticed it only supports SecCore in a 
>> PE/COFF section, not a TE section. Is there a rationale for that?
>>
>> Thank you for your time!
>>
>> Best regards,
>> Marvin
>>
>>
>> [1] 
>> https://github.com/tianocore/edk2/commit/9bedaec05b7b8ba9aee248361bb61a85a26726cb
>>
>>
>> 
>>
>

Re: [edk2-devel] SecCore evacuation in PeiCore?

[Michael Kubacki]

Hi Marvin,

Your understanding of SecMigrationPei is correct. It is not ideal as 
it's an unfamiliar pattern that could give the false impression that it 
is a universal SEC migration solution, which it is not. But if platforms 
understand that any additional data published in SecCore must be 
explicitly migrated (potentially via library extension to 
SecMigrationPei), it can be used to serve the SEC post-memory migration 
role.

I assumed it was related to the reset vector due to the 16-bit 
alignment. I think it would be great to have SecCore aligned properly if 
possible.

Thanks,
Michael

On 8/14/2021 8:29 AM, Marvin Häuser wrote:
> Hey Michael,
>
> Thank you for your response! It was actually quicker than I imagined. :)
>
> I think I understand, but please let me try to get this absolutely 
> right. Can I think of "SecMigrationPei" as a sort of "SecCorePostMem", 
> which either is loaded into permanent RAM directly or is shadowed 
> because it is a PEIM unlike SecCore - and it republishes all public 
> data, most especially PPIs, such that the entire PEI stage no longer 
> has any references to the original SecCore at all, and the SecCore 
> module basically just sits there in the ROM, and its exposed data is 
> either discarded or orphaned? Is that about right?
>
> I think I hit the alignment issue of SecCore too, but only for X64 
> builds (likely just because the size happens to be lucky for IA32) of 
> OVMF. Pretty much sure it's just ResetVector positioning. What would 
> be the issue with moving the ResetVector into a separate component, 
> with its fixed position in FD (this is actually how UefiCpuPkg/VTF0 
> works), and having SecCore aligned correctly? Not specifically to 
> restore MigrateSecModulesInFv(), but as future-proofing to ensure 
> expected outputs. In fact, I noticed because my new PE loader code was 
> upset about the unaligned XIP load address.
>
> Also thanks for your patch!
>
> Best regards,
> Marvin
>
> On 13/08/2021 18:51, Michael Kubacki wrote:
>> Hi Marvin,
>>
>> I apologize for the delayed response, I missed this message earlier. 
>> The function was called from EvacuateTempRam() in the initial set of 
>> patches:
>> [PATCH 1/6] MdeModulePkg/PeiCore: Enable T-RAM evacuation in PeiCore 
>> (CVE-2019-11098) (groups.io) 
>> <https://edk2.groups.io/g/devel/message/61823>
>>
>> I was not involved in the patch series on the mailing list (job role 
>> change at the time) but as a comment in that patch notes, there was 
>> an inconsistency observed in PE32 section alignment in SEC modules. I 
>> don't see where this was resolved other than the calls being removed 
>> later in the series. SecCore migration would not occur implicitly in 
>> the PeiCore flow but there is functionality for SEC data migration in 
>> UefiCpuPkg/SecMigrationPei.
>>
>> Based on what I see now, I'd be happy to send a patch to remove 
>> MigrateSecModulesInFv().
>>
>> Thanks,
>> Michael
>>
>> On 8/7/2021 2:54 PM, Marvin Häuser wrote:
>>> Good day everyone,
>>> Good day Michael,
>>>
>>> The commit that introduced T-RAM evacuation [1] also introduced the 
>>> function "MigrateSecModulesInFv()". It also is explicitly mentioned 
>>> as part of the control flow in the commit message. As far as I can 
>>> see, since then till today this function has never been called 
>>> anywhere. Was this some draft function that accidentally made it 
>>> into the patch, or did the caller get lost somewhere? The 
>>> description makes sense to me and I'm not experienced enough with 
>>> the PeiCore control flow to tell whether the PEIM migration somehow 
>>> covers SecCore implicitly. Also I noticed it only supports SecCore 
>>> in a PE/COFF section, not a TE section. Is there a rationale for that?
>>>
>>> Thank you for your time!
>>>
>>> Best regards,
>>> Marvin
>>>
>>>
>>> [1] 
>>> https://github.com/tianocore/edk2/commit/9bedaec05b7b8ba9aee248361bb61a85a26726cb
>>>
>>>
>>> 
>>>
>>

Re: [edk2-devel] SecCore evacuation in PeiCore?

[Marvin Häuser]

On 16/08/2021 18:18, Michael Kubacki wrote:
> Hi Marvin,
>
> Your understanding of SecMigrationPei is correct. It is not ideal as 
> it's an unfamiliar pattern that could give the false impression that 
> it is a universal SEC migration solution, which it is not. But if 
> platforms understand that any additional data published in SecCore 
> must be explicitly migrated (potentially via library extension to 
> SecMigrationPei), it can be used to serve the SEC post-memory 
> migration role.
>
> I assumed it was related to the reset vector due to the 16-bit 
> alignment. I think it would be great to have SecCore aligned properly 
> if possible.

I could probably write a patch, but OVMF does not use this SecCore (and 
still something is misaligned :) ), and I don't have any other platform. 
Maybe I can ask Bret to test it as part of some PE loader validation in 
the future. :)

Would the old solution, which is being removed, be universal? Would it 
be beneficial? I know that the ARM world does not use this SecCore 
either, but I generally don't have a good idea about how their stuff works.

Thanks!

Best regards,
Marvin

>
> Thanks,
> Michael
>
> On 8/14/2021 8:29 AM, Marvin Häuser wrote:
>> Hey Michael,
>>
>> Thank you for your response! It was actually quicker than I imagined. :)
>>
>> I think I understand, but please let me try to get this absolutely 
>> right. Can I think of "SecMigrationPei" as a sort of 
>> "SecCorePostMem", which either is loaded into permanent RAM directly 
>> or is shadowed because it is a PEIM unlike SecCore - and it 
>> republishes all public data, most especially PPIs, such that the 
>> entire PEI stage no longer has any references to the original SecCore 
>> at all, and the SecCore module basically just sits there in the ROM, 
>> and its exposed data is either discarded or orphaned? Is that about 
>> right?
>>
>> I think I hit the alignment issue of SecCore too, but only for X64 
>> builds (likely just because the size happens to be lucky for IA32) of 
>> OVMF. Pretty much sure it's just ResetVector positioning. What would 
>> be the issue with moving the ResetVector into a separate component, 
>> with its fixed position in FD (this is actually how UefiCpuPkg/VTF0 
>> works), and having SecCore aligned correctly? Not specifically to 
>> restore MigrateSecModulesInFv(), but as future-proofing to ensure 
>> expected outputs. In fact, I noticed because my new PE loader code 
>> was upset about the unaligned XIP load address.
>>
>> Also thanks for your patch!
>>
>> Best regards,
>> Marvin
>>
>> On 13/08/2021 18:51, Michael Kubacki wrote:
>>> Hi Marvin,
>>>
>>> I apologize for the delayed response, I missed this message earlier. 
>>> The function was called from EvacuateTempRam() in the initial set of 
>>> patches:
>>> [PATCH 1/6] MdeModulePkg/PeiCore: Enable T-RAM evacuation in PeiCore 
>>> (CVE-2019-11098) (groups.io) 
>>> <https://edk2.groups.io/g/devel/message/61823>
>>>
>>> I was not involved in the patch series on the mailing list (job role 
>>> change at the time) but as a comment in that patch notes, there was 
>>> an inconsistency observed in PE32 section alignment in SEC modules. 
>>> I don't see where this was resolved other than the calls being 
>>> removed later in the series. SecCore migration would not occur 
>>> implicitly in the PeiCore flow but there is functionality for SEC 
>>> data migration in UefiCpuPkg/SecMigrationPei.
>>>
>>> Based on what I see now, I'd be happy to send a patch to remove 
>>> MigrateSecModulesInFv().
>>>
>>> Thanks,
>>> Michael
>>>
>>> On 8/7/2021 2:54 PM, Marvin Häuser wrote:
>>>> Good day everyone,
>>>> Good day Michael,
>>>>
>>>> The commit that introduced T-RAM evacuation [1] also introduced the 
>>>> function "MigrateSecModulesInFv()". It also is explicitly mentioned 
>>>> as part of the control flow in the commit message. As far as I can 
>>>> see, since then till today this function has never been called 
>>>> anywhere. Was this some draft function that accidentally made it 
>>>> into the patch, or did the caller get lost somewhere? The 
>>>> description makes sense to me and I'm not experienced enough with 
>>>> the PeiCore control flow to tell whether the PEIM migration somehow 
>>>> covers SecCore implicitly. Also I noticed it only supports SecCore 
>>>> in a PE/COFF section, not a TE section. Is there a rationale for that?
>>>>
>>>> Thank you for your time!
>>>>
>>>> Best regards,
>>>> Marvin
>>>>
>>>>
>>>> [1] 
>>>> https://github.com/tianocore/edk2/commit/9bedaec05b7b8ba9aee248361bb61a85a26726cb
>>>>
>>>>
>>>> 
>>>>
>>>
>

Re: [edk2-devel] SecCore evacuation in PeiCore?

[Michael Kubacki]

Hi Marvin,

It would be beneficial in the sense that it could reduce the amount of 
work manually needed elsewhere.

PeiCore can migrate pointers in global databases it knows about such as 
the PPI list that point to global data areas it has knowledge of such as 
Temporary RAM. Prior to this change that included the Temporary RAM heap 
(e.g. memory allocations and data HOBs) and the Temporary RAM stack. 
PcdMigrateTemporaryRamFirmwareVolumes adds another layer of migration 
support for firmware volumes.

It roughly takes the previous pattern of registering individual PEIMs 
for shadow and performing a PPI reinstall and automates it where 
possible within PeiCore. With all of the modules in FVs installed in 
pre-memory relocated and fixed up, the core can use its knowledge of the 
FV list to expand its ability to update global databases with the new 
pointer location for pointers into those images. So this can help with 
something like the common usage of a PPI interface structure that is a 
global variable that contains function pointers to functions linked in 
the module.

However, there is still a case PeiCore doesn't know how to handle and 
that is pointers in a structure that is allocated on the heap. It would 
be aware of the PPI structure itself that is on the heap but it would be 
unaware of the custom structure type there that contains the pointers.

What I previously meant by the "universal" comment is that a separate 
PEIM named SecMigrationPei could give a false impression that SEC 
migration is entirely covered by the PEIM and the case above might not 
be considered.

In UefiCpuPkg/SecCore/SecMain.c, the PPIs installed in the 
mPeiSecPlatformInformationPpi descriptor array look like those that 
would benefit from the functionality brought in by 
PcdMigrateTemporaryRamFirmwareVolumes. In addition to those in 
SecBist.c. From that standpoint, the primary benefit I see is it would 
align support with what is provided with PEIMs.

It is unfortunate you don't have a physical platform. Perhaps something 
like the UP Xtreme in edk2-platforms is worth considering - 
https://github.com/tianocore/edk2-platforms/tree/master/Platform/Intel/WhiskeylakeOpenBoardPkg/UpXtreme.

Thanks,
Michael

On 8/17/2021 2:41 AM, Marvin Häuser wrote:
> On 16/08/2021 18:18, Michael Kubacki wrote:
>> Hi Marvin,
>>
>> Your understanding of SecMigrationPei is correct. It is not ideal as 
>> it's an unfamiliar pattern that could give the false impression that 
>> it is a universal SEC migration solution, which it is not. But if 
>> platforms understand that any additional data published in SecCore 
>> must be explicitly migrated (potentially via library extension to 
>> SecMigrationPei), it can be used to serve the SEC post-memory 
>> migration role.
>>
>> I assumed it was related to the reset vector due to the 16-bit 
>> alignment. I think it would be great to have SecCore aligned properly 
>> if possible.
> 
> I could probably write a patch, but OVMF does not use this SecCore (and 
> still something is misaligned :) ), and I don't have any other platform. 
> Maybe I can ask Bret to test it as part of some PE loader validation in 
> the future. :)
> 
> Would the old solution, which is being removed, be universal? Would it 
> be beneficial? I know that the ARM world does not use this SecCore 
> either, but I generally don't have a good idea about how their stuff works.
> 
> Thanks!
> 
> Best regards,
> Marvin
> 
>>
>> Thanks,
>> Michael
>>
>> On 8/14/2021 8:29 AM, Marvin Häuser wrote:
>>> Hey Michael,
>>>
>>> Thank you for your response! It was actually quicker than I imagined. :)
>>>
>>> I think I understand, but please let me try to get this absolutely 
>>> right. Can I think of "SecMigrationPei" as a sort of 
>>> "SecCorePostMem", which either is loaded into permanent RAM directly 
>>> or is shadowed because it is a PEIM unlike SecCore - and it 
>>> republishes all public data, most especially PPIs, such that the 
>>> entire PEI stage no longer has any references to the original SecCore 
>>> at all, and the SecCore module basically just sits there in the ROM, 
>>> and its exposed data is either discarded or orphaned? Is that about 
>>> right?
>>>
>>> I think I hit the alignment issue of SecCore too, but only for X64 
>>> builds (likely just because the size happens to be lucky for IA32) of 
>>> OVMF. Pretty much sure it's just ResetVector positioning. What would 
>>> be the issue with moving the ResetVector into a separate component, 
>>> with its fixed position in FD (this is actually how UefiCpuPkg/VTF0 
>>> works), and having SecCore aligned correctly? Not specifically to 
>>> restore MigrateSecModulesInFv(), but as future-proofing to ensure 
>>> expected outputs. In fact, I noticed because my new PE loader code 
>>> was upset about the unaligned XIP load address.
>>>
>>> Also thanks for your patch!
>>>
>>> Best regards,
>>> Marvin
>>>
>>> On 13/08/2021 18:51, Michael Kubacki wrote:
>>>> Hi Marvin,
>>>>
>>>> I apologize for the delayed response, I missed this message earlier. 
>>>> The function was called from EvacuateTempRam() in the initial set of 
>>>> patches:
>>>> [PATCH 1/6] MdeModulePkg/PeiCore: Enable T-RAM evacuation in PeiCore 
>>>> (CVE-2019-11098) (groups.io) 
>>>> <https://edk2.groups.io/g/devel/message/61823>
>>>>
>>>> I was not involved in the patch series on the mailing list (job role 
>>>> change at the time) but as a comment in that patch notes, there was 
>>>> an inconsistency observed in PE32 section alignment in SEC modules. 
>>>> I don't see where this was resolved other than the calls being 
>>>> removed later in the series. SecCore migration would not occur 
>>>> implicitly in the PeiCore flow but there is functionality for SEC 
>>>> data migration in UefiCpuPkg/SecMigrationPei.
>>>>
>>>> Based on what I see now, I'd be happy to send a patch to remove 
>>>> MigrateSecModulesInFv().
>>>>
>>>> Thanks,
>>>> Michael
>>>>
>>>> On 8/7/2021 2:54 PM, Marvin Häuser wrote:
>>>>> Good day everyone,
>>>>> Good day Michael,
>>>>>
>>>>> The commit that introduced T-RAM evacuation [1] also introduced the 
>>>>> function "MigrateSecModulesInFv()". It also is explicitly mentioned 
>>>>> as part of the control flow in the commit message. As far as I can 
>>>>> see, since then till today this function has never been called 
>>>>> anywhere. Was this some draft function that accidentally made it 
>>>>> into the patch, or did the caller get lost somewhere? The 
>>>>> description makes sense to me and I'm not experienced enough with 
>>>>> the PeiCore control flow to tell whether the PEIM migration somehow 
>>>>> covers SecCore implicitly. Also I noticed it only supports SecCore 
>>>>> in a PE/COFF section, not a TE section. Is there a rationale for that?
>>>>>
>>>>> Thank you for your time!
>>>>>
>>>>> Best regards,
>>>>> Marvin
>>>>>
>>>>>
>>>>> [1] 
>>>>> https://github.com/tianocore/edk2/commit/9bedaec05b7b8ba9aee248361bb61a85a26726cb 
>>>>>
>>>>>
>>>>>
>>>>> 
>>>>>
>>>>
>>