Publish EDK's TPM eventlog that contains coreboot's entries #139
Conversation
SergiiDmytruk
force-pushed
the
combined-eventlog
branch
from
f5283a2 to
21bf698
Compare
SergiiDmytruk
force-pushed
the
combined-eventlog
branch
from
21bf698 to
48e3529
Compare
SergiiDmytruk
force-pushed
the
combined-eventlog
branch
from
48e3529 to
23da581
Compare
SergiiDmytruk
force-pushed
the
combined-eventlog
branch
from
23da581 to
0444e65
Compare
SergiiDmytruk
force-pushed
the
combined-eventlog
branch
from
9af3e30 to
0444e65
Compare
SergiiDmytruk
force-pushed
the
combined-eventlog
branch
from
0444e65 to
2708d2d
Compare
|
@miczyg1 any more comments here? |
Actually not anymore. I would like to give this PR and this a try on some HW that has a socketable TPM 1.2 and TPM 2.0. Dasharo/coreboot#517 |
SergiiDmytruk
force-pushed
the
combined-eventlog
branch
from
2708d2d to
c0078b2
Compare
|
Added a fix to make measurements of Dasharo variables reproducible (c0078b2) here because it's kind of related. https://github.com/Dasharo/DasharoModulePkg has the issue as well, but don't know if we need to fix it there. |
We should, there are still releases coming that will use DasharoModulePkg submodule in EDKII |
SergiiDmytruk
force-pushed
the
combined-eventlog
branch
from
c0078b2 to
13373e8
Compare
|
Rebased. |
SergiiDmytruk
force-pushed
the
rebased
branch
from
8eccede to
6b645e4
Compare
There was a problem hiding this comment.
Tested it on various platforms and it works well.
However I am worried about the flexibility of the solution. We are forcing only one hashing algorithm on both sides: coreboot and EDK2. It will break the PCR bank switching feature. I would expect both coreboot and EDK2 to provide measurement using all hashes that are currently enabled in TPM.
| @@ -307,7 +307,7 @@ SyncPcrAllocationsAndPcrMask ( | |||
| { | |||
| DEBUG ((DEBUG_INFO, "TpmActivePcrBanks & Tpm2PcrMask = 0x%08x\n", (TpmActivePcrBanks & Tpm2PcrMask))); | |||
| DEBUG ((DEBUG_INFO, "TpmActivePcrBanks & BiosHashAlgorithmBitmap = 0x%08x\n", (TpmActivePcrBanks & BiosHashAlgorithmBitmap))); | |||
| NewTpmActivePcrBanks = TpmActivePcrBanks; | |||
| NewTpmActivePcrBanks = TpmHashAlgorithmBitmap; | |||
There was a problem hiding this comment.
This will break the feature of changing PCR banks via menu. Am I right? It always forces to use the single bank defined by PCD at coreboot build time.
There was a problem hiding this comment.
That is correct, but coreboot never extends more than a single bank and EDK2 was constrained to do the same to align the two parts of the firmware as a simpler option: Dasharo/dasharo-issues#982
There was a problem hiding this comment.
Yes, and that should also change per my comment: #139 (review)
Find the log using DasharoPayloadPkg/CbParseLib in DasharoPayloadPkg/BlSupportPei and create HOBs like those produced by TcgPei and Tcg2Pei all of which will be picked up by TcgDxe and Tcg2Dxe. TPM1 case is quite simple: - use coreboot's Spec ID Event as EDK doesn't seem to add one of its own TPM2 case is more advanced and is more complicated: - don't create a HOB for coreboot's Spec ID Event (the first entry) because TPM2 can have multiple digests and coreboot produces at most one - when importing HOBs in Tcg2Dxe add missing hashes of OneDigest kind from TXT spec (0x01 followed by 0x00 bytes) just to not come up with some custom placeholder Signed-off-by: Sergii Dmytruk <[email protected]>
Basically a copy&paste from Tcg2Smm. Intentionally not making any changes (like dropping use of PCDs to pass data) beyond what's necessary to make it work. No need for an analogous change for TPM1 because TcgDxe already publishes the log. Signed-off-by: Sergii Dmytruk <[email protected]>
Prior to this change the code could only disable banks unsupported by the BIOS and not enable those which are supported. This resulted in not touching TPM configuration if an unsupported bank was already selected instead of automatically switching on supported banks. Signed-off-by: Sergii Dmytruk <[email protected]>
Selecting them won't result in enabling them, so they shouldn't show up in the UI. Signed-off-by: Sergii Dmytruk <[email protected]>
miczyg1
force-pushed
the
combined-eventlog
branch
from
9c5dd7b to
19b8bc9
Compare
|
Rebased on top of recent dasharo |
Missing entries from coreboot's log are filled in as
0x01 0x00....which affects expected SHA-1 PCR-2 value computed bytpm2_eventlog, but TCG specification says that all entries need to have identical set of digests listed in the same order, hence I've added those fake values as described by TXT spec because I don't think TCG covers such a case.See commit messages for more details.
coreboot PR: Dasharo/coreboot#517