Use all active TPM banks for Measured Boot #821
Conversation
SergiiDmytruk
force-pushed
the
all-tpm-banks
branch
5 times, most recently
from
ccbfdc8 to
85106d0
Compare
|
|
||
| int i, j; | ||
| struct tpm_digest digests[ENABLED_TPM_ALGS_NUM + 1]; | ||
| for (i = 0, j = 0; i < ENABLED_TPM_ALGS_NUM; ++i) { |
There was a problem hiding this comment.
@SergiiDmytruk is it intended that j is not incremented in this loop?
There was a problem hiding this comment.
Oh, of course not, thanks for catching this. Turns out CONFIG_VBOOT_NO_TPM is set for some Protectli boards, so vboot doesn't extend any PCRs making this harder to notice in manual tests.
src/security/vboot/tpm_common.c
Outdated
| /* Extending the same data to all banks. It either gets truncated, fits | ||
| perfectly or is padded with zeroes. */ | ||
| digests[j].hash = buffer; | ||
| digests[j].hash_type = algo; |
There was a problem hiding this comment.
We do want to use algo, defined before this loop, not alg defined and used inside this loop?
There was a problem hiding this comment.
Must be alg, thanks. Missing increment of j hid this mistake.
src/security/tpm/tspi/tspi.c
Outdated
| return TPM_CB_HASH_ERROR; | ||
| } | ||
|
|
||
| digests[j].hash = digest; |
There was a problem hiding this comment.
Admittedly, *according to LLM*:
The function computes multiple digests by iterating algorithms, but it finalizes each digest into the same stack buffer uint8_t digest[TPM_PCR_MAX_LEN] and then stores digests[j].hash = digest. All entries end up pointing to the same memory and therefore all banks will extend with the last computed digest, with truncation effects for smaller algorithms. This breaks both PCR values and event log correctness when more than one bank is active.
I'm not sure if this is the case, feel free to disregard
There was a problem hiding this comment.
LLM is actually right in this case. This affects only FMAP measurement, which I did notice (it's the first measurement) but incorrectly attributed this weirdness to vboot extending the same value regardless of the digest type and didn't check the code.
Out of vendorcode/eltan/security/mboot/mboot.c to not duplicate the implementation. Upstream-Status: Pending Signed-off-by: Sergii Dmytruk <sergii.dmytruk@3mdeb.com>
No functional changes are intended, merely changing interface and how places that make use of it. A few places got extra error checks, but their conditions shouldn't be satisfied at this point. Upstream-Status: Pending Signed-off-by: Sergii Dmytruk <sergii.dmytruk@3mdeb.com>
The marshaling code was already there. This change only increases maximum number of hashes and initializes `tpm2_pcr_extend_cmd` with all digests that were passed in. Upstream-Status: Pending Signed-off-by: Sergii Dmytruk <sergii.dmytruk@3mdeb.com>
* Update vendor data to drop entry size and maximum count and add offset to the first unused byte and total number of available bytes. This bumps its major version because the update breaks compatibility. * Represent log table as two structures, tpm_2_log_table and tpm_2_log_bottom, separate by a list of digests in the header. * Remove tpm_2_log_entry. * Add functions to store and parse log entries and use them instead of array operations. * cbmem tool doesn't need an update because it already parses the log as an agile format rather than an array of entries of the same size. Change-Id: I13cebe2a40c220375cc14124ef9b13ea7ee0207f Upstream-Status: Pending Signed-off-by: Sergii Dmytruk <sergii.dmytruk@3mdeb.com>
Get rid of the large if-else statement in intel_cbnt_inject_ibg_measurements() by moving code from branches into separate functions. Upstream-Status: Pending Change-Id: I892c56d37abac1b43c68ac761d428c3560007246 Signed-off-by: Sergii Dmytruk <sergii.dmytruk@3mdeb.com>
Updates the code to support hashing for more than one digest algorithm when that's available, the code should work as before at this point. Change-Id: I4e0ea97946e6c8cafbc21a6418b8cb5e7d087df0 Upstream-Status: Pending Signed-off-by: Sergii Dmytruk <sergii.dmytruk@3mdeb.com>
Updates the code to support hashing for more than one digest algorithm when that's available, the code should work as before at this point. Change-Id: I243531e699d927896278df2822e80c69db2715dd Upstream-Status: Pending Signed-off-by: Sergii Dmytruk <sergii.dmytruk@3mdeb.com>
Updates the code to support hashing for more than one digest algorithm when that's available, the code should work as before at this point. Change-Id: I6a89d8d430986bda7ee77053ca3768a292e1b53b Upstream-Status: Pending Signed-off-by: Sergii Dmytruk <sergii.dmytruk@3mdeb.com>
All of the client has already been updated to permit use of multiple banks, but at most one was ever enabled. TPM 2 log was also updated to permit handling of multiple digests, but similarly only one was in use. From now on, it's possible to configure more than one digest (only SHA1 and SHA256 are selected by default). This changes previous TSPI API of `tpm_log_alg()` (single hash) to `tpm_log_alg_active(enum vb2_hash_algorithm)` coupled with `enabled_tpm_algs` array (multiple hashes). The bulk of the code here is for dealing with the set of banks of TPM: - querying it from the device to know what digests should be used - synchronizing set of digests in the log with the actual set of active banks The latter is needed in case TPM is initialized in ramstage while measurements are accumulated starting from the bootblock. An alternative was to require initializing TPM in the bootblock, but bootblock may not have enough space for the extra code required for TPM, hence a different approach was taken: take all supported hashes before TPM is initialized, trim unnecessary digests after the initialization. Change-Id: Ia326b22869c4983fc4e02e150461e7a9ff94dc4e Upstream-Status: Pending Signed-off-by: Sergii Dmytruk <sergii.dmytruk@3mdeb.com>
SergiiDmytruk
force-pushed
the
all-tpm-banks
branch
2 times, most recently
from
5c23cb0 to
69aee15
Compare
SergiiDmytruk
left a comment
SergiiDmytruk
left a comment
There was a problem hiding this comment.
Updated the code and reran tests on VP4650. Also checked that build with CONFIG_VBOOT_NO_TPM=n produces correct PCR values.
|
|
||
| int i, j; | ||
| struct tpm_digest digests[ENABLED_TPM_ALGS_NUM + 1]; | ||
| for (i = 0, j = 0; i < ENABLED_TPM_ALGS_NUM; ++i) { |
There was a problem hiding this comment.
Oh, of course not, thanks for catching this. Turns out CONFIG_VBOOT_NO_TPM is set for some Protectli boards, so vboot doesn't extend any PCRs making this harder to notice in manual tests.
src/security/vboot/tpm_common.c
Outdated
| /* Extending the same data to all banks. It either gets truncated, fits | ||
| perfectly or is padded with zeroes. */ | ||
| digests[j].hash = buffer; | ||
| digests[j].hash_type = algo; |
There was a problem hiding this comment.
Must be alg, thanks. Missing increment of j hid this mistake.
src/security/tpm/tspi/tspi.c
Outdated
| return TPM_CB_HASH_ERROR; | ||
| } | ||
|
|
||
| digests[j].hash = digest; |
There was a problem hiding this comment.
LLM is actually right in this case. This affects only FMAP measurement, which I did notice (it's the first measurement) but incorrectly attributed this weirdness to vboot extending the same value regardless of the digest type and didn't check the code.
At the moment this is to use CI for checking that commits don't break anything.
ref: prot-1808