Repository navigation
Conversation
|
Reviewed cd15dec (architecture, correctness, lifetimes, security, style, tests). No new issues found, 2 still open. Must fix
Suggestions
Nits
|
| err = tpm_log_tagged_event( | ||
| TPM2_PCR_KERNEL_CONFIG, | ||
| POINTER_TO_PHYSICAL_ADDRESS(dtb.iov_base), | ||
| dtb.iov_len, | ||
| DEVICETREE_EVENT_TAG_ID, | ||
| entry->devicetree, | ||
| NULL); | ||
| if (err != EFI_SUCCESS) | ||
| log_error_status( | ||
| err, | ||
| "Unable to extend PCR %i with DTB '%ls': %m", | ||
| TPM2_PCR_KERNEL_CONFIG, | ||
| entry->devicetree); |
There was a problem hiding this comment.
Claude: suggestion: This measurement fails open: devicetree_install() has already published the blob via BS->InstallConfigurationTable(EFI_DTB_TABLE) by the time we get here, so when tpm_log_tagged_event() returns an error we log it and fall through to BS->StartImage(). The kernel then consumes the (Secure-Boot-unverified) DTB while PCR 12 is left at exactly the value it would have had for a "no DTB" boot, i.e. a PCR 12 policy sealed against the DTB-less state still unseals. Since tpm_log_tagged_event() already returns EFI_SUCCESS with measured=false when neither the TCG2 nor the CC protocol is present, a non-EFI_SUCCESS return here means a protocol was present and the extend genuinely failed, so treating it as fatal would not regress TPM-less machines. Given that the whole point of the !secure_boot_enabled() guard above is that this DTB is unverified, the measurement is the compensating control and losing it silently is worth avoiding: either measure before the blob is published, or propagate the error so the entry is not started (the _cleanup_(devicetree_cleanup) on dtstate will unwind the install). Note the asymmetry with install_addon_devicetrees() in stub.c, which does abort on measurement failure. Also consider passing a real ret_measured out-parameter instead of NULL, as stub.c does, so the loader knows whether the measurement actually happened.
There was a problem hiding this comment.
You're right that boot continues after a measurement error. However, install_addon_devicetrees() also returns only from the helper; its caller continues booting with the DTB installed.
Also, a non-success return does not necessarily mean the PCR was unchanged. For example, EFI_VOLUME_FULL means the extend succeeded but the event could not be logged.
These points do not eliminate the concern about an actual extend failure. Making that fatal is a reasonable policy to consider, but it would differ from the existing stub behavior.
| [[ -d /sys/firmware/efi ]] | ||
| systemd-analyze has-tpm2 | ||
| [[ -x /usr/lib/systemd/systemd-pcrlock ]] |
There was a problem hiding this comment.
Claude: suggestion: These prerequisites hard-fail instead of skipping. Sibling tests print a reason and exit 77 when EFI, TPM2 or systemd-pcrlock is unavailable (see TEST-92-TPM2-SWTPM.sh:35, TEST-70-TPM2.pcrlock.sh:15).
There was a problem hiding this comment.
I'll add a skip when systemd-pcrlock is unavailable, but keep the EFI and TPM checks as assertions. This test explicitly requests UEFI firmware and a TPM, so skipping those failures could hide setup or detection regressions.
f095658 to
b22a1ef
Compare
Reject incomplete DTB reads before installing the blob. Keep measurement errors non-fatal and make that behavior explicit in the warning. Clarify the shared DeviceTree event tag and normalized path description, and move the unchanged SMBIOS Type 0 payload to a module-level constant.
Skip the test on its first boot when systemd-pcrlock is unavailable. Keep UEFI, TPM and Secure Boot checks as assertions, since the test explicitly requests the required firmware and TPM setup. Share the common TPM2 integration test configuration without changing either test's effective settings.
233c7e1 to
cd15dec
Compare
Summary
Testing
systemd-bootaa64.efion Fedora 44/aarch64 with QEMU, AAVMF, and swtpm; verified that the Type #1 DTB was measured exactly once into PCR 12 and that the event log replay matched the PCR value