Skip to content

[do not merge] Combined TPM eventlog #517

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.

Sign up for GitHub

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

Jump to bottom
Open
wants to merge 6 commits into
base: dasharo
Choose a base branch
from
Open

[do not merge] Combined TPM eventlog #517

wants to merge 6 commits into from

Conversation

SergiiDmytruk
Copy link
Member

@SergiiDmytruk SergiiDmytruk commented Jun 12, 2024 •
edited
Loading

Actual code changes are in EDK, coreboot only needs to not publish related ACPI entries.

Testing on APUs won't work without reverting modifications in src/drivers/pc80/tpm/tis.c which somehow disables a working TPM.

EDK PR: Dasharo/edk2#139

This now also includes picking TCG log format at runtime.

@SergiiDmytruk SergiiDmytruk requested a review from miczyg1 June 12, 2024 13:35
@SergiiDmytruk SergiiDmytruk mentioned this pull request Jun 12, 2024
Open
@SergiiDmytruk SergiiDmytruk mentioned this pull request Jul 4, 2024
Open
krystian-hebel
krystian-hebel previously approved these changes Jul 5, 2024
View reviewed changes
Copy link
Contributor

@krystian-hebel krystian-hebel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approved, but edk2 must be merged first.

@m-iwanicki
Copy link

@SergiiDmytruk Not sure if it's due to your PR but when I tested latest artifact: https://github.com/Dasharo/coreboot/actions/runs/9800367204/job/27062155366 on Protectli VP2420 Dasharo System Features menu disappeared.

@krystian-hebel
Copy link
Contributor

@SergiiDmytruk Not sure if it's due to your PR but when I tested latest artifact: https://github.com/Dasharo/coreboot/actions/runs/9800367204/job/27062155366 on Protectli VP2420 Dasharo System Features menu disappeared.

Probably caused by lack of Dasharo/edk2#148 after the bug was introduced in Dasharo/edk2#144 / Dasharo/DasharoModulePkg#50

@SergiiDmytruk
Copy link
Member Author

@m-iwanicki I saw that but assumed a messed up .config. It's actually an issue fixed by Dasharo/edk2@d130aec commit which was missing on rebased. Added it there and updated both PRs to include that fix.

@miczyg1
Copy link
Contributor

miczyg1 commented Jul 8, 2024

Testing on APUs won't work without reverting modifications in src/drivers/pc80/tpm/tis.c which somehow disables a working TPM.

This was supposed to fix it: d3b8531

Do we have some insights why it doesn't work/what is the problem exactly?

@SergiiDmytruk
Copy link
Member Author

Testing on APUs won't work without reverting modifications in src/drivers/pc80/tpm/tis.c which somehow disables a working TPM.

This was supposed to fix it: d3b8531

Do we have some insights why it doesn't work/what is the problem exactly?

This PR is older than that commit.

@miczyg1
Copy link
Contributor

miczyg1 commented Jul 16, 2024 •
edited
Loading

@macpijan do we have some working setups with socketable TPM1.2 and TPM2.0 in the lab?

@krystian-hebel
Copy link
Contributor

IIRC one of KGPEs should still have TPM1.2 which was used for AEM. Note that it currently has ASUS BIOS, on 16Mb chip.

@miczyg1
Copy link
Contributor

miczyg1 commented Jul 16, 2024

IIRC one of KGPEs should still have TPM1.2 which was used for AEM. Note that it currently has ASUS BIOS, on 16Mb chip.

@krystian-hebel something that will run on this coreboot version :)

@SergiiDmytruk
Copy link
Member Author

Force-pushed to get deterministic variable measurements.

@m-iwanicki m-iwanicki mentioned this pull request Jul 30, 2024
Merged
@SergiiDmytruk SergiiDmytruk mentioned this pull request Aug 7, 2024
Open
@SergiiDmytruk
Copy link
Member Author

Rebased: updated EDK2 hash which also fixed merge conflict.

@macpijan macpijan changed the title Combined TPM eventlog [do not merge] Combined TPM eventlog Sep 5, 2024
@miczyg1
Copy link
Contributor

miczyg1 commented Dec 20, 2024

Rebased on top of recent dasharo branch

@miczyg1 miczyg1 changed the base branch from dasharo to dasharo-24.02.1 March 28, 2025 12:14
@SergiiDmytruk
configs: bump Dasharo EDK2 to get combined TPM log
3f44011 
Change-Id: Iacee23253d2766d9f3835860d0d64d84b5bcd880
Signed-off-by: Sergii Dmytruk <[email protected]>
@SergiiDmytruk
acpi: don't publish TPM logs if EDK payload is used
baf9c30 
EDK will publish its own version of the log after parsing and
importing coreboot's log discovered through CBMEM.

Publishing is done by appending a table, so coreboot must avoid
adding corresponding log tables to let OS find a more complete one
from EDK.

Change-Id: Iec92c2b5c426ee003e81996937862d81cb4ead24
Signed-off-by: Sergii Dmytruk <[email protected]>
@SergiiDmytruk
security/tpm: replace TPM_MEASURE_ALGO with tpm_log_alg()
9c636c3 
No functional changes.  This replaces a macro with an inline function to
make code more readable and more convenient to extend in the future.

Change-Id: I456bc3bb749a9b58fba72f5562195525e55290bf
Signed-off-by: Sergii Dmytruk <[email protected]>
@SergiiDmytruk
security/tpm: add CONFIG_TPM_LOG_TCG
3cdcbbe 
This event log format option automatically selects TCG log format
depending on which TPM is present.

Change-Id: I1997396f24ff6362fe64ac56f8e61efcf2ffb0f7
Signed-off-by: Sergii Dmytruk <[email protected]>
@SergiiDmytruk
payloads/edk2: ask EDK2 to disable PCR banks unsupported by coreboot
3d75cd6 
coreboot can only extend a single PCR bank of TPM2 and this change
results in all available PCRs being extended.

This is an alternative to making coreboot extend all active PCRs.

Change-Id: I4e21ab77f191e9b36cb467cd61ad0a3e347035cb
Signed-off-by: Sergii Dmytruk <[email protected]>
@SergiiDmytruk SergiiDmytruk changed the base branch from dasharo-24.02.1 to dasharo May 4, 2025 22:31
@macpijan @SergiiDmytruk
config: pcengines: v0.9.1-rc1
8df91b7 
Change-Id: I8573a65bdbd7727dc11f0634fdbd62711b36ddac
Signed-off-by: Maciej Pijanowski <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@krystian-hebel krystian-hebel krystian-hebel left review comments

@miczyg1 miczyg1 Awaiting requested review from miczyg1

@mkopec mkopec Awaiting requested review from mkopec

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@SergiiDmytruk @m-iwanicki @krystian-hebel @miczyg1 @macpijan