CBnT: inject measurements made by BootGuard into event log #699

SergiiDmytruk · 2025-06-20T22:02:23Z

PCR-0 and (optionally) PCR-7
TigerLake and newer
Commit messages and comments in the code provide more details

Was tested for sanity in QEMU which helped to find bugs, but not on hardware (one attempt resulted in a brick although not necessarily related to the new code which doesn't reset the machine or otherwise prevents boot in any way).

ref: ncm-1811

src/security/intel/cbnt/measurement.c

src/soc/intel/common/block/cse/cse.c

src/security/intel/cbnt/measurement.c

miczyg1 · 2025-06-23T09:50:52Z

src/security/tpm/tspi/log-tpm2.c

-		return 0xFF;
-	}
-}
-
 void *tpm2_log_cbmem_init(void)


One more thing to that function. The log size should be 64K at minimum per the TCG spec:

4. If the ACPI TPM2 table contains the address and size of the Platform Firmware TCG log: a. The Log Area Minimum Length for the TCG event log MUST be at least 64KB.

Here we just allocate 50 entries + size of the log table. Same happens of TPM1.2. Because of that the platform ends up having just a few hundreds of bytes of the log in ACPI TPM table. Not sure if measured boot v2 PRs change that on EDK2 side somehow. Still the EDK2 is not the only payload we use.

coreboot doesn't actually store measured data in the events, so it's enough for coreboot. Measured boot v2 stops coreboot from publishing its event log in ACPI so EDK can publish its version which is much larger than what coreboot uses.

Measured boot v2 stops coreboot from publishing its event log in ACPI so EDK can publish its version which is much larger than what coreboot uses.

yeah, I know, but when EDK2 is not used, the log ends up being tiny.

Right, but then nothing except coreboot likely writes to the log.

@SergiiDmytruk SeaBIOS does IIRC.

It does (discarding anything that coreboot wrote there), although it respects the length reported in ACPI. Added the change anyway: 84fb089.

src/arch/x86/include/arch/fit_table.h

miczyg1 · 2025-07-08T12:46:57Z

src/security/intel/cbnt/measurement.c

+	 * doesn't contain anything useful on modern platforms and suggests SGX-related
+	 * MSRs (0x20::0x23, to be specific, but SDM vol. 4 has these instead).


Where is that suggestion in SDM? I think this comment should be removed/changed.

B.1.21 TXT.PUBLIC.KEY – AC Module Public Key Hash:

Note: Modern platforms utilizing SGX technology implement the Public Key Hash register as
part of the CPU core, not as part of PCH and thus TXT.PUBLIC.KEY register has been
deprecated with no underlying HW functionality. Any value it may contain does not have
meaning and must be disregarded.
Actual Public Key Hash is typically available in CPU MSRs 0x20∷0x23.
Platform manufacturers shall consult respective external data sheets to verify the above
numeric values as it may change.

Also mentioned when EVTYPE_SINIT_PUBKEY_ HASH is described:

PUBKEY_HASH is digest of ACM Signer key. It can be retrieved from MSRs 0x20::0x23

That's a note from MLE SDG. SDM is a different document. But I already see where it came from.

MLE SDG and CBNT spec are right when it comes to MSRs 0x20:0x23, which I already proved.

We have to remove any reference of SDM here. It seems you have mistaken SGXLEPUBKEYHASH with ACM PUBKEYAHSH MSRs, because both have PUBKEYHASH in its names. But the ACM PUBKEY MSRS are nto SGX-related at all. Just the information about ACM pubkey hash has been moved from PCH MMIO registers to CPU MSRs since the introduction of SGX technology. However, SGX has been removed after Comet Lake (10th Gen) from client devices, but the MSRs 0x20:0x23 are still there, because they have different purpose and are not SGX related.

Regardless of the reason, SDM does not have any information about Boot Guard event logging, nor about MSRS 0x20:0x23. Thus the comment should be rephrased and not mention SDM at all:

* BTG BWG says to use TXT.PUBLIC.KEY, but TXT SDG (#315168-017.4) and CBnT BWG * says that it doesn't contain anything useful on modern platforms utilizing SGX and * suggests to use MSRs 0x20::0x23.

One may also calculate the hash of the key from ACM header itself and compare against TXT.PUBLIC.KEY or the MSRs to be sure. But I think the Skylake+ (6th gen) will have the key in MSRs already.

2. It seems you have mistaken SGXLEPUBKEYHASH with ACM PUBKEYAHSH MSRs, because both have PUBKEYHASH in its names.

That wasn't a mistake. I picked it because I didn't find anything that fits the description nor description of MSRs 0x20:0x23 in SDM, and since SGX was mentioned in both places decided to go with it which is what the comment was explaining. I should have updated it when switching to 0x20:0x23, did it now: https://github.com/Dasharo/coreboot/compare/5820a99a5ee7959893db963edf3b597bc5e63554..84fb089a040e938ced09b2d74d1b5dd9d17e3348#diff-1725d4cd7234cc446584ac6ebad791ed8c8e640970d4adb2e99e5b5b233030f7R494

miczyg1 · 2025-07-08T12:54:00Z

src/security/intel/cbnt/measurement.c

+			   always returns the first one which might be wrong. */
+			printk(BIOS_WARNING, "CBnT: picking SACM from FIT at random!\n");
+		}
+		*size = ((struct acm_header_v0 *)addr)->size * 4;


The acm_header_v0 might need an update. There are newer versions of acm_header. I believe ADL or MTL uses v3 already which has slightly different structure (RSAPubKey/RSASig is bigger, depends on KeySize field and RSAPubExp is omitted, Scratch and User Area also get shifted). SO maybe a union of acm header versions would be needed.

Key data is at different offsets indeed, thanks.

v3 should cover our needs, it's also the first version with CBnT support. TGL and ADL use v3, MTL has v4. The signature part is compatible between v3.0 and v5.4 (v5 seems to have no change for the header compared to v4, just version number change).

code changes: https://github.com/Dasharo/coreboot/compare/4f0d0aa19a293ff4329d846c6f39af07d848a04c..3eb202f68e21e208692f41e80043eddf08d69588#diff-1725d4cd7234cc446584ac6ebad791ed8c8e640970d4adb2e99e5b5b233030f7R145-R187
declaration: https://github.com/Dasharo/coreboot/compare/4f0d0aa19a293ff4329d846c6f39af07d848a04c..3eb202f68e21e208692f41e80043eddf08d69588#diff-1013ce554074c41208e8c9b5239fbe83423c40bf24d9a07ff5b98440bba4a74c

Is it good that we assume V3 always? Maybe a union would be better + a check for ACM header version.

Added a version check in https://github.com/Dasharo/coreboot/compare/5820a99a5ee7959893db963edf3b597bc5e63554..84fb089a040e938ced09b2d74d1b5dd9d17e3348#diff-1725d4cd7234cc446584ac6ebad791ed8c8e640970d4adb2e99e5b5b233030f7.

I wanted to add a union last time, but it wouldn't do anything useful in this case (v4/v5 adds and modifies some fields at the end), so I didn't even bother committing the header for v4.

SergiiDmytruk

Other changes apart from rebasing and review:

removed Q35 and CBFS leftovers
made more things const which wasn't possible due to CBFS
added error checks after FIT lookup since find_in_fit() is returning NULL on error

src/arch/x86/include/arch/fit_table.h

SergiiDmytruk · 2025-07-08T18:30:38Z

src/security/tpm/tspi/log-tpm2.c

-		return 0xFF;
-	}
-}
-
 void *tpm2_log_cbmem_init(void)


Right, but then nothing except coreboot likely writes to the log.

SergiiDmytruk · 2025-07-08T18:36:01Z

src/security/intel/cbnt/measurement.c

+	 * doesn't contain anything useful on modern platforms and suggests SGX-related
+	 * MSRs (0x20::0x23, to be specific, but SDM vol. 4 has these instead).


B.1.21 TXT.PUBLIC.KEY – AC Module Public Key Hash:

Note: Modern platforms utilizing SGX technology implement the Public Key Hash register as
part of the CPU core, not as part of PCH and thus TXT.PUBLIC.KEY register has been
deprecated with no underlying HW functionality. Any value it may contain does not have
meaning and must be disregarded.
Actual Public Key Hash is typically available in CPU MSRs 0x20∷0x23.
Platform manufacturers shall consult respective external data sheets to verify the above
numeric values as it may change.

Also mentioned when EVTYPE_SINIT_PUBKEY_ HASH is described:

PUBKEY_HASH is digest of ACM Signer key. It can be retrieved from MSRs 0x20::0x23

src/security/intel/cbnt/measurement.c

SergiiDmytruk · 2025-07-09T17:36:06Z

src/security/intel/cbnt/measurement.c

+			   always returns the first one which might be wrong. */
+			printk(BIOS_WARNING, "CBnT: picking SACM from FIT at random!\n");
+		}
+		*size = ((struct acm_header_v0 *)addr)->size * 4;


Key data is at different offsets indeed, thanks.

v3 should cover our needs, it's also the first version with CBnT support. TGL and ADL use v3, MTL has v4. The signature part is compatible between v3.0 and v5.4 (v5 seems to have no change for the header compared to v4, just version number change).

code changes: https://github.com/Dasharo/coreboot/compare/4f0d0aa19a293ff4329d846c6f39af07d848a04c..3eb202f68e21e208692f41e80043eddf08d69588#diff-1725d4cd7234cc446584ac6ebad791ed8c8e640970d4adb2e99e5b5b233030f7R145-R187
declaration: https://github.com/Dasharo/coreboot/compare/4f0d0aa19a293ff4329d846c6f39af07d848a04c..3eb202f68e21e208692f41e80043eddf08d69588#diff-1013ce554074c41208e8c9b5239fbe83423c40bf24d9a07ff5b98440bba4a74c

This fixes the following error Possible precedence problem between ! and pattern match (m//) because `!$filename =~ /~$/` was apparently interpreted as `(!$filename) =~ /~$/`. Fix that by wrapping `=~` expression in parenthesis. Upstream-Status: Pending Change-Id: If63a4b6981e5b562a9af7f2f8ac64947fad7f7b0 Signed-off-by: Sergii Dmytruk <[email protected]>

It is useful for more than event log management. Upstream-Status: Pending Change-Id: I779f948115af786ecda873cd01c9ef1506252c7d Signed-off-by: Sergii Dmytruk <[email protected]>

To make it accessible outside of logging.c file. Upstream-Status: Pending Change-Id: I3bd7fb126fb197c88bec933a88ded59232d520a1 Signed-off-by: Sergii Dmytruk <[email protected]>

Extract the definition from src/cpu/intel/fit/fit_table.c, change first field to a union and add defines for types. This provides a basis for parsing FIT data from coreboot. Upstream-Status: Pending Change-Id: If857e21864fd8be2f15a35c154e1f22802fafddd Signed-off-by: Sergii Dmytruk <[email protected]>

The event seems to be specific to TPM2, so not adding to TPM1 log. Upstream-Status: Pending Change-Id: I99b9c23a5089b039b01090eec08a808346f7c389 Signed-off-by: Sergii Dmytruk <[email protected]>

BootGuard/CbnT can optionally measure data into PCRs before passing control to BIOS which must be taken into account by coreboot for Measured Boot to work correctly. This is done by constructing the data the same way BootGuard does, hashing it and creating an entry in the event log without extending the corresponding PCR. BootGuard measurements must precede all coreboot measurements, so the new code is hooked to tspi_init_crtm() which is responsible for event log initialization and performing initial measurements (starting with measuring FMAP). This also includes 2 other kinds of entries: - creation of Startup Locality event on log initialization when startup locality of the TPM is locality 3 - optionally capping PCRs with a separator after a TPM error (the case of capping unsupported but active PCR bank is not implemented as coreboot hard-codes PCR it works with) Upstream-Status: Pending Change-Id: Ib56cadac85d9ef1d747ecf3cfc2976dc6785262a Signed-off-by: Sergii Dmytruk <[email protected]>

BootGuard/CbnT can optionally measure data into PCRs before passing control to BIOS which must be taken into account by coreboot for Measured Boot to work correctly. This is done by constructing the data the same way BootGuard does, hashing it and creating an entry in the event log without extending the corresponding PCR. Extending PCR-7 is optional and breaks BitLocker on Windows 10, so may not be utilized. However, it doesn't require large amount of code and might as well be implemented along with PCR-0 since both use the same data structures. Upstream-Status: Pending Change-Id: Ic1bfb016600f9f00b8f2fa1965aedfa99647a8e0 Signed-off-by: Sergii Dmytruk <[email protected]>

That's the minimum log size for client platforms per "TCG PC Client Platform Firmware Profile Specification" [0]. [0]: https://trustedcomputinggroup.org/resource/pc-client-specific-platform-firmware-profile-specification/ Upstream-Status: Pending Change-Id: Ifed85a91d5e2cf28fcc40262710e0ec688479a62 Signed-off-by: Sergii Dmytruk <[email protected]>

SergiiDmytruk requested review from mkopec, miczyg1 and filipleple June 20, 2025 22:02

miczyg1 requested changes Jun 23, 2025

View reviewed changes

SergiiDmytruk force-pushed the ibg-measurements branch from ffa516f to 4b50161 Compare June 27, 2025 14:02

SergiiDmytruk requested a review from miczyg1 July 8, 2025 12:24

miczyg1 reviewed Jul 8, 2025

View reviewed changes

src/arch/x86/include/arch/fit_table.h Show resolved Hide resolved

miczyg1 reviewed Jul 8, 2025

View reviewed changes

SergiiDmytruk force-pushed the ibg-measurements branch 2 times, most recently from 4f0d0aa to 3eb202f Compare July 9, 2025 21:35

SergiiDmytruk commented Jul 9, 2025

View reviewed changes

SergiiDmytruk requested a review from miczyg1 July 11, 2025 14:15

SergiiDmytruk added 5 commits July 17, 2025 00:37

security/tpm: expose tpmalg_from_vb2_hash()

0948a8f

It is useful for more than event log management. Upstream-Status: Pending Change-Id: I779f948115af786ecda873cd01c9ef1506252c7d Signed-off-by: Sergii Dmytruk <[email protected]>

security/intel/cbnt: move union cbnt_biosacm_policy to header

b9a6fbe

To make it accessible outside of logging.c file. Upstream-Status: Pending Change-Id: I3bd7fb126fb197c88bec933a88ded59232d520a1 Signed-off-by: Sergii Dmytruk <[email protected]>

security/tpm/tspi: add a function to log Startup Locality event

de6e423

The event seems to be specific to TPM2, so not adding to TPM1 log. Upstream-Status: Pending Change-Id: I99b9c23a5089b039b01090eec08a808346f7c389 Signed-off-by: Sergii Dmytruk <[email protected]>

SergiiDmytruk force-pushed the ibg-measurements branch from 3eb202f to 5820a99 Compare July 16, 2025 21:47

SergiiDmytruk added 3 commits July 17, 2025 00:55

SergiiDmytruk force-pushed the ibg-measurements branch from 5820a99 to 84fb089 Compare July 16, 2025 22:00

		* doesn't contain anything useful on modern platforms and suggests SGX-related
		* MSRs (0x20::0x23, to be specific, but SDM vol. 4 has these instead).

Uh oh!

CBnT: inject measurements made by BootGuard into event log #699

Are you sure you want to change the base?

CBnT: inject measurements made by BootGuard into event log #699

Conversation

SergiiDmytruk commented Jun 20, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

miczyg1 Jun 23, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

miczyg1 Jul 8, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SergiiDmytruk left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

miczyg1 Jun 23, 2025 •

edited

Loading

miczyg1 Jul 8, 2025 •

edited

Loading