Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

config/coreboot-librem: Use coreboot's custom TPM event log #1609

Closed
wants to merge 1 commit into from

Conversation

miczyg1
Copy link
Contributor

@miczyg1 miczyg1 commented Feb 14, 2024

No description provided.

@miczyg1
Copy link
Contributor Author

miczyg1 commented Feb 14, 2024

Should address issue: #1608

@JonathonHall-Purism
Copy link
Collaborator

The issue in #1608 is actually occurring the boards changed in #1604 (@tlaurion is working on that). On Librem boards, cbmem does segfault, but it emits the complete and correct TPM log before faulting, so Heads functionality works fine. I had not noticed it was faulting until it was mentioned in #1604.

That said I'm happy to test this and see if it addresses the segfault (may be a bit before I can get to it).

@miczyg1
Copy link
Contributor Author

miczyg1 commented Feb 14, 2024

I had no problem dumping TPM log in TCG format with cbmem on 4.21+ too, but segfault is something new to me.

cbmem -L will also fail to dup the log if the first event is not SpecID event for TCG format. Not sure about that, but there might be a case where the first entry in the log is not SpecID event, i.e. vboot. If vboot is used (as separate stage IIRC) it may happen that measurement of verstage is first, then CRTM initialization and SpecID event is created later... I would have to double check.

But other than that, the SpecID should be always first if vboot is not used (which should be true for all heads builds). Maybe if preram TCG LOG is full, then something is being overwritten?

@tlaurion
Copy link
Collaborator

tlaurion commented Feb 14, 2024

Created a branch I've built on talos II trying to make CircleCI happy which is happening under https://app.circleci.com/pipelines/github/tlaurion/heads?branch=pre423-edp_fhd_in-tpm_tcg_event_log-CircleCi_coreboot_cache_fixed which builds https://github.com/tlaurion/heads/tree/pre423-edp_fhd_in-tpm_tcg_event_log-CircleCi_coreboot_cache_fixed
EDIT: builds successfully there. @miczyg1 please reuse for testing. Building util/cbmem from different coreboot releases on ubuntu shows same segfault as per Heads cbmem, showing its not linked to cbmem being built with musl-cross-make, while coreboot is built with coreboot buildstack which should be working. So not sure why your behavior would be any different unless your buildsystem is not the one of coreboot (which I think is the case).

This changes coreboot config files to reflect changes needed to pass to 4.23 have TPM TCG Event log format, which was used to produce logs on x230 from cbmem built from musl-cross-make. As said under #1604, the errors reported there were for coreboot custom Event Log format, which was default before and where cbmem doesn't seem to have evolved correctly to support changes that happened after TCG addition.

Here, we built 4.23+ from commit including edp/fhd patch yersterday to reflect master state. The PR of course includes transformed coreboot configurations. Notes that I deactivated microcode inclusion there simply because git forks seem to change where blobs are and for the exercice, they were not relevant and made the build fails because not found in expected paths.

@miczyg1 Here are the logs produced from Heads output of cbmem -L (cbmem built from util/cbmem in tree)
cbmem_L_segfault.log
dmesg_cbmem_L_segfault.log

As said under #1604, building cbmem from tarball archives (4.19,4.20,4.21,4.22) produces invalid results. Here, since we switched to TCG under coreboot, the results are different. Here is raw output from my ubuntu testing station:
tcg_cbmem-L_parsing_across_coreboot_versions_atop_ubuntu.log
Of course, new format not recognized under 4.19 nor 4.20.1. And segfaults there for TPM1. Unfortunately, I could not get qemu to output cbmem, will retry and update when successful.


Notes:

@miczyg1
Copy link
Contributor Author

miczyg1 commented Feb 15, 2024

So not sure why your behavior would be any different unless your buildsystem is not the one of coreboot (which I think is the case).

Always using the coreboot toolchain inside docker. In case of heads we also have docker to get all cross toolchains to be built properly, but that still uses heads-built musl-cross and coreboot toolchain still...

@miczyg1
Copy link
Contributor Author

miczyg1 commented Feb 15, 2024

TCPA log entry 10:
	PCR: -967035966
	Event type: Unknown (0xace82028 >= 19)
	Digest: d97f7b94ead60f73575cdf71010ef9fd99ed6b28
	Event data: 

Looks like coreboot didn't terminate the event log... cbmem checks for a zero_block of one TCPA entry length, if it isn;t found, then it goes further... We could add more safety checks to cbmem to avoid that but also make sure coreboot properly terminates the eventlog (or at least clear the cbmem entry with the log while creating it...)

@SergiiDmytruk cc

EDIT:

coreboot is clearing the cbmem areas with TCG logs in src/security/tpm/tspi/log-tpm1.c and src/security/tpm/tspi/log-tpm2.c: memset(tclt, 0, tpm_log_len);, so a memory corruption?

@SergiiDmytruk
Copy link
Contributor

Of course, new format not recognized under 4.19 nor 4.20.1. And segfaults there for TPM1.

Each log is passed in a separate CBMEM entry, old versions of cbmem should just not find the log and be done.

coreboot is clearing the cbmem areas with TCG logs in src/security/tpm/tspi/log-tpm1.c and src/security/tpm/tspi/log-tpm2.c: memset(tclt, 0, tpm_log_len);, so a memory corruption?

Memory issue or some other bug is always possible. Endianness should be fine (inverting bytes doesn't seem to result in anything sensible). But given that the entry follows Event data: CBFS: fallback/payload, maybe something other than coreboot added it?

If vboot is used (as separate stage IIRC) it may happen that measurement of verstage is first, then CRTM initialization and SpecID event is created later... I would have to double check.

It isn't appending of bytes after bytes, those entries are copied individually into the area allocated for entries:

void tpm1_log_copy_entries(const void *from, void *to)
{
	const struct tpm_1_log_table *from_log = from;
	struct tpm_1_log_table *to_log = to;
	int i;

	for (i = 0; i < le16toh(from_log->vendor.num_entries); i++) {
		if (le16toh(to_log->vendor.num_entries) >= le16toh(to_log->vendor.max_entries)) {
			printk(BIOS_WARNING, "TPM LOG: log table is full\n");
			return;
		}

		struct tpm_1_log_entry *tce =
			&to_log->entries[le16toh(to_log->vendor.num_entries)];
		memcpy(tce, &from_log->entries[i], sizeof(*tce));

		to_log->vendor.num_entries = htole16(le16toh(to_log->vendor.num_entries) + 1);
	}
}

That area is after the header which contains SpecID:

struct tpm_1_log_table {
	/* The first entry of the log is inlined and describes the log itself */
	uint32_t pcr;
	uint32_t event_type;
	uint8_t digest[TPM_1_LOG_DIGEST_MAX_LENGTH];
	uint32_t spec_id_size;
	struct spec_id_event_data spec_id;
	struct tpm_1_vendor vendor;

	struct tpm_1_log_entry entries[]; /* Variable number of entries */
} __packed;

@tlaurion
Copy link
Collaborator

tlaurion commented Feb 16, 2024

@SergiiDmytruk seems like patches are needed on coreboot master and this behavior affects all measured boot configured platforms. Where can that be tracked?

CC @miczyg1

@SergiiDmytruk
Copy link
Contributor

@tlaurion Not necessarily, we still don't know why this happens. If my guess about something else updating event log is correct, it might be happening due to standard logs, unlike coreboot-specific one, being published in ACPI tables where they can be found by e.g. Linux. If patching src/acpi/acpi.c to do nothing in acpi_create_tcpa() and acpi_create_tpm2() changes anything, then it could be it. However, not publishing the logs seems like a workaround rather than a fix if coreboot doesn't do anything wrong.

@tlaurion
Copy link
Collaborator

tlaurion commented Feb 19, 2024

@SergiiDmytruk @miczyg1 @krystian-hebel @JonathonHall-Purism I started a discussion at https://matrix.to/#/!pAlHOfxQNPXOgFGTmo:matrix.org/$CQDLxAckANKmXAu8ZDtcDIHUn1dgvgfzyifZidHr7SA?via=matrix.org&via=nitro.chat&via=fairydust.space to ease the process of understanding the current issue at stake tagging peple that should be involved.

EDIT: further documented at #1608.

@tlaurion
Copy link
Collaborator

tlaurion commented Feb 19, 2024

Issue opened with logs under #1608. This PR doesn't fix anything at all as documented at #1608 (comment)

@tlaurion tlaurion closed this Feb 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants