From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from linux.microsoft.com (linux.microsoft.com [13.77.154.182]) by mx.groups.io with SMTP id smtpd.web09.41451.1629216977020577442 for ; Tue, 17 Aug 2021 09:16:17 -0700 Authentication-Results: mx.groups.io; dkim=fail reason="body hash did not verify" header.i=@linux.microsoft.com header.s=default header.b=jXObcVak; spf=pass (domain: linux.microsoft.com, ip: 13.77.154.182, mailfrom: mikuback@linux.microsoft.com) Received: from [10.0.0.19] (c-73-27-179-174.hsd1.fl.comcast.net [73.27.179.174]) by linux.microsoft.com (Postfix) with ESMTPSA id 2012C20C2F82; Tue, 17 Aug 2021 09:16:16 -0700 (PDT) DKIM-Filter: OpenDKIM Filter v2.11.0 linux.microsoft.com 2012C20C2F82 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linux.microsoft.com; s=default; t=1629216976; bh=6zUYui3z9xOXBnfxBNmXq2PTJvCCtx6ap3bLYwTxl9w=; h=Subject:To:Cc:References:From:Date:In-Reply-To:From; b=jXObcVakXNpJcfqGZpgRdbABgLiXJgSuN9GZ5gN25iNFcku6BN6vScW2F+0ET7+Eo XGfxuwPHWq2vtup8irqiX14c685ub80dTecn1kY3bPIbZOodRek0fIUjHKxWRDpHZh aYQ5jnyOtEYBC55RptAAxefdGoUqpbOxbPNIUUlE= Subject: Re: [edk2-devel] SecCore evacuation in PeiCore? To: =?UTF-8?Q?Marvin_H=c3=a4user?= , devel@edk2.groups.io Cc: Bret Barkelew References: <82c95051-a508-997b-da05-21f8b2dc7c74@posteo.de> From: "Michael Kubacki" Message-ID: <57fc28a5-18e6-d265-3d00-b804e9cb9943@linux.microsoft.com> Date: Tue, 17 Aug 2021 12:16:14 -0400 User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:78.0) Gecko/20100101 Thunderbird/78.13.0 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: quoted-printable Hi Marvin, It would be beneficial in the sense that it could reduce the amount of=20 work manually needed elsewhere. PeiCore can migrate pointers in global databases it knows about such as=20 the PPI list that point to global data areas it has knowledge of such as=20 Temporary RAM. Prior to this change that included the Temporary RAM heap=20 (e.g. memory allocations and data HOBs) and the Temporary RAM stack.=20 PcdMigrateTemporaryRamFirmwareVolumes adds another layer of migration=20 support for firmware volumes. It roughly takes the previous pattern of registering individual PEIMs=20 for shadow and performing a PPI reinstall and automates it where=20 possible within PeiCore. With all of the modules in FVs installed in=20 pre-memory relocated and fixed up, the core can use its knowledge of the=20 FV list to expand its ability to update global databases with the new=20 pointer location for pointers into those images. So this can help with=20 something like the common usage of a PPI interface structure that is a=20 global variable that contains function pointers to functions linked in=20 the module. However, there is still a case PeiCore doesn't know how to handle and=20 that is pointers in a structure that is allocated on the heap. It would=20 be aware of the PPI structure itself that is on the heap but it would be=20 unaware of the custom structure type there that contains the pointers. What I previously meant by the "universal" comment is that a separate=20 PEIM named SecMigrationPei could give a false impression that SEC=20 migration is entirely covered by the PEIM and the case above might not=20 be considered. In UefiCpuPkg/SecCore/SecMain.c, the PPIs installed in the=20 mPeiSecPlatformInformationPpi descriptor array look like those that=20 would benefit from the functionality brought in by=20 PcdMigrateTemporaryRamFirmwareVolumes. In addition to those in=20 SecBist.c. From that standpoint, the primary benefit I see is it would=20 align support with what is provided with PEIMs. It is unfortunate you don't have a physical platform. Perhaps something=20 like the UP Xtreme in edk2-platforms is worth considering -=20 https://github.com/tianocore/edk2-platforms/tree/master/Platform/Intel/Whis= keylakeOpenBoardPkg/UpXtreme. Thanks, Michael On 8/17/2021 2:41 AM, Marvin H=C3=A4user wrote: > On 16/08/2021 18:18, Michael Kubacki wrote: >> Hi Marvin, >> >> Your understanding of SecMigrationPei is correct. It is not ideal as=20 >> it's an unfamiliar pattern that could give the false impression that=20 >> it is a universal SEC migration solution, which it is not. But if=20 >> platforms understand that any additional data published in SecCore=20 >> must be explicitly migrated (potentially via library extension to=20 >> SecMigrationPei), it can be used to serve the SEC post-memory=20 >> migration role. >> >> I assumed it was related to the reset vector due to the 16-bit=20 >> alignment. I think it would be great to have SecCore aligned properly=20 >> if possible. >=20 > I could probably write a patch, but OVMF does not use this SecCore (and= =20 > still something is misaligned :) ), and I don't have any other platform.= =20 > Maybe I can ask Bret to test it as part of some PE loader validation in= =20 > the future. :) >=20 > Would the old solution, which is being removed, be universal? Would it=20 > be beneficial? I know that the ARM world does not use this SecCore=20 > either, but I generally don't have a good idea about how their stuff work= s. >=20 > Thanks! >=20 > Best regards, > Marvin >=20 >> >> Thanks, >> Michael >> >> On 8/14/2021 8:29 AM, Marvin H=C3=A4user 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=20 >>> right. Can I think of "SecMigrationPei" as a sort of=20 >>> "SecCorePostMem", which either is loaded into permanent RAM directly=20 >>> or is shadowed because it is a PEIM unlike SecCore - and it=20 >>> republishes all public data, most especially PPIs, such that the=20 >>> entire PEI stage no longer has any references to the original SecCore= =20 >>> at all, and the SecCore module basically just sits there in the ROM,=20 >>> and its exposed data is either discarded or orphaned? Is that about=20 >>> right? >>> >>> I think I hit the alignment issue of SecCore too, but only for X64=20 >>> builds (likely just because the size happens to be lucky for IA32) of= =20 >>> OVMF. Pretty much sure it's just ResetVector positioning. What would=20 >>> be the issue with moving the ResetVector into a separate component,=20 >>> with its fixed position in FD (this is actually how UefiCpuPkg/VTF0=20 >>> works), and having SecCore aligned correctly? Not specifically to=20 >>> restore MigrateSecModulesInFv(), but as future-proofing to ensure=20 >>> expected outputs. In fact, I noticed because my new PE loader code=20 >>> 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.= =20 >>>> The function was called from EvacuateTempRam() in the initial set of= =20 >>>> patches: >>>> [PATCH 1/6] MdeModulePkg/PeiCore: Enable T-RAM evacuation in PeiCore= =20 >>>> (CVE-2019-11098) (groups.io)=20 >>>> >>>> >>>> I was not involved in the patch series on the mailing list (job role= =20 >>>> change at the time) but as a comment in that patch notes, there was=20 >>>> an inconsistency observed in PE32 section alignment in SEC modules.=20 >>>> I don't see where this was resolved other than the calls being=20 >>>> removed later in the series. SecCore migration would not occur=20 >>>> implicitly in the PeiCore flow but there is functionality for SEC=20 >>>> data migration in UefiCpuPkg/SecMigrationPei. >>>> >>>> Based on what I see now, I'd be happy to send a patch to remove=20 >>>> MigrateSecModulesInFv(). >>>> >>>> Thanks, >>>> Michael >>>> >>>> On 8/7/2021 2:54 PM, Marvin H=C3=A4user wrote: >>>>> Good day everyone, >>>>> Good day Michael, >>>>> >>>>> The commit that introduced T-RAM evacuation [1] also introduced the= =20 >>>>> function "MigrateSecModulesInFv()". It also is explicitly mentioned= =20 >>>>> as part of the control flow in the commit message. As far as I can=20 >>>>> see, since then till today this function has never been called=20 >>>>> anywhere. Was this some draft function that accidentally made it=20 >>>>> into the patch, or did the caller get lost somewhere? The=20 >>>>> description makes sense to me and I'm not experienced enough with=20 >>>>> the PeiCore control flow to tell whether the PEIM migration somehow= =20 >>>>> covers SecCore implicitly. Also I noticed it only supports SecCore=20 >>>>> in a PE/COFF section, not a TE section. Is there a rationale for that= ? >>>>> >>>>> Thank you for your time! >>>>> >>>>> Best regards, >>>>> Marvin >>>>> >>>>> >>>>> [1]=20 >>>>> https://github.com/tianocore/edk2/commit/9bedaec05b7b8ba9aee248361bb6= 1a85a26726cb=20 >>>>> >>>>> >>>>> >>>>>=20 >>>>> >>>> >>