coreboot

If there are no objections, I'll use an API like this instead:

struct tpm_digest {
    const uint8_t *hash;
    size_t len;
    enum vb2_hash_algorithm hash_type;
};

/**
 * Ask vboot for a digest and extend a TPM PCR with it.
 * @param pcr sets the pcr index
 * @param digests An array of digests terminated by an entry with VB2_HASH_NONE
 * @param name sets additional info where the digest comes from
 * @return TPM_SUCCESS on success. If not a tpm error is returned
 */
uint32_t tpm_extend_pcr(int pcr, const tpm_digest *digests, const char *name);

Passing in length and arrays which must be kept in sync seems to make things needlessly complicated.

Can you explain what use case you have that requires you to use multiple algorithms? And why is it not enough to just call tpm_extend_pcr() several times, once for each algorithm?

Let's clarify what your high-level goal here is first before we discuss how to modify low-level function APIs to enable it.

Can you explain what use case you have that requires you to use multiple algorithms?

I'll let Krystian and Michał correct me, but I'm not sure if we have an actual need for multiple algorithms right away. The API change is probably motivated by the fact that agile format supports multiple algorithms unlike the coreboot-specific one.

And why is it not enough to just call tpm_extend_pcr() several times, once for each algorithm?

This won't work well with tcpa_log_add_table_entry(), which will then add a log entry per algorithm. tpm_extend_pcr() would be updated for consistency here, but it's not strictly necessary.

Sergii Dmytruk wrote in #note-5:

tpm_extend_pcr() would be updated for consistency here, but it's not strictly necessary.

Correction: tpm_extend_pcr() can invoke tcpa_log_add_table_entry() in its body, so it should have a similar interface. Similar considerations apply to tlcl_extend().

Julius Werner wrote in #note-4:

Can you explain what use case you have that requires you to use multiple algorithms?

This allows for greater flexibility, where multiple coexisting programs may have different expectations, e.g. one is old enough to not know anything but SHA1, and another that considers SHA1 not secure enough.

And why is it not enough to just call tpm_extend_pcr() several times, once for each algorithm?

This would call tcpa_log_add_table_entry() (or its corresponding new version), which would create multiple entries. This is not allowed by specification [1], 10.1.6:

For each Hash algorithm enumerated in the TCG_PCClientPCREvent entry, there SHALL be a corresponding digest in all TCG_PCR_EVENT2 structures.

[1] https://trustedcomputinggroup.org/wp-content/uploads/TCG_PCClient_PFP_r1p05_v23_pub.pdf

Can you explain what use case you have that requires you to use multiple algorithms?

This allows for greater flexibility, where multiple coexisting programs may have different expectations, e.g. one is old enough to not know anything but SHA1, and another that considers SHA1 not secure enough.

Sorry, but that sounds kinda vague... I mean, do you actually have a case right now where you need this on one of the coreboot platforms you're building? And how is the algorithm supposed to get in there? Right now we just have a TPM_MEASURE_ALGO constant that's either SHA1 for TPM 1.2 or SHA256 for TPM 2.0... are you planning to make a bunch of Kconfigs to select this instead or something?

Basically, I understand that the log format allows multiple algorithms, and that's fine. And I'm also not saying that we can never expand it to allow logging multiple algorithms if a real need comes up in the future. I'm just saying there's no need to make things any more complicated than they need to be right now and implement support for a bunch of stuff in the lower level APIs that the higher level APIs wouldn't actually be using yet. If your goal for the time being is just to support the new log format, why don't you just do that in a way where tpm_extend_pcr() always creates a log entry with exactly one algorithm? If we ever get to the point where we actually need to log multiple algorithms somewhere we can still expand that later.

Julius Werner wrote in #note-8:

are you planning to make a bunch of Kconfigs to select this

vboot2 supports 4 matching hashes, so 4 corresponding options.

Basically, I understand that the log format allows multiple algorithms, and that's fine. And I'm also not saying that we can never expand it to allow logging multiple algorithms if a real need comes up in the future. I'm just saying there's no need to make things any more complicated than they need to be right now and implement support for a bunch of stuff in the lower level APIs that the higher level APIs wouldn't actually be using yet. If your goal for the time being is just to support the new log format, why don't you just do that in a way where tpm_extend_pcr() always creates a log entry with exactly one algorithm? If we ever get to the point where we actually need to log multiple algorithms somewhere we can still expand that later.

This approach will require returning to these changes, understanding them and spec again, changing, testing and reviewing changes again and only then adding a second/third algorithm. This can even prevent such development in the future by making the cost of change high enough. I think we might as well implement agile format properly (no fixed-size buffers in structures, any number of algorithms) right away. Lower level API is called in two places, both of which need to loop over an array of enabled algorithms, which doesn't complicate the code much, just generalizes it.

I think we might as well implement agile format properly (no fixed-size buffers in structures, any number of algorithms) right away.

You are implementing the format properly (the format defines how the TCPA log is supposed to look in memory, not what kind of APIs the code that writes it internally need to provide). You are just not implementing all possible ways for coreboot to fill it out.

I still feel strongly that we shouldn't overcomplicate APIs and increase maintenance burden by implementing options that nobody has a use case for right now and likely never will. https://en.wikipedia.org/wiki/YAGNI

Julius Werner wrote in #note-10:

I still feel strongly that we shouldn't overcomplicate APIs

It would actually simplify the API by making parameter lists shorter and input data better grouped.

increase maintenance burden by implementing options that nobody has a use case for right now and likely never will.

skiboot writes both SHA1 and SHA256 hashes to TPM2 log. I didn't count it as a use case because so far we were using TPM1.2, but it does show existence of logs with multiple hashes in the wild.

It would actually simplify the API by making parameter lists shorter and input data better grouped.

Having to construct a separate parameter struct rather than just throwing in two scalars is not "simpler".

skiboot writes both SHA1 and SHA256 hashes to TPM2 log. I didn't count it as a use case because so far we were using TPM1.2, but it does show existence of logs with multiple hashes in the wild.

I don't know what skiboot is... is that coreboot? Do they have a real use case for having both hashes in the log or is it just another bootloader where someone decided "might as well write all the hashes in advance just because the spec technically allows for it"?

My question is: is there any user of coreboot right now who would actually turn on multiple hashes for production purposes because otherwise something they need doesn't work for them?

I don't know what skiboot is... is that coreboot? Do they have a real use case for having both hashes in the log or is it just another bootloader where someone decided "might as well write all the hashes in advance just because the spec technically allows for it"?

My question is: is there any user of coreboot right now who would actually turn on multiple hashes for production purposes because otherwise something they need doesn't work for them?

Skiboot is a payload currently used by OpenPOWER systems [1], like QEMU POWER9 or Talos II that is slowly being upstreamed [2]. With additional changes both to format of log created by coreboot and to the payload itself (latter breaks TPM2.0 logs), it could be persuaded to work with one hash, we did that as PoC for our setup that uses TPM1.2 (due to supply chain issues and low ability of I2C TPMs in general). However, instead of following existing standards, be it TCG or coreboot, such approach creates yet another one. Having the ability to use more than one would make transition to TPM2.0 easier, if not no-op. Since we are going to have to change event log generation code anyway, we want to do it properly, instead of putting another half-measure in place.

So no, there are no users of coreboot that would use it right now, but there will (hopefully) soon be. As this is a change that will impact many platforms, we want to push it upstream sooner rather than later, leaving as much time for review and testing as possible.

[1] https://github.com/coreboot/coreboot/tree/master/payloads/external/skiboot
[2] https://review.coreboot.org/q/topic:talos-2

However, instead of following existing standards, be it TCG or coreboot, such approach creates yet another one. Having the ability to use more than one would make transition to TPM2.0 easier, if not no-op. Since we are going to have to change event log generation code anyway, we want to do it properly, instead of putting another half-measure in place.

Sorry, I still get the impression that we have a fundamental misunderstanding here. The TCG does not dictate how many hashes need to be logged in the TCPA log! (Or does it? If I'm wrong about this please clarify the exact spec and section that defines what hash algorithms must be present in the long, because I am not aware of any such requirement.) The TCG defined a log format that allows an implementation to log one or more hashes of different algorithms for each measurement entry. What exact algorithms and how many of them to use is entirely up to the implementation.

So no, we would not be "putting another half-measure in place" that creates "yet another" standard. We would be switching to the exact TCG standard that you want (which I am generally not opposed to at all), we would use that exact format, and we would just choose to only log one hash for one algorithm in that data structure that is designed to hold one or more hashes depending on how the writer decides to fill it out. Because we don't have a use case for more than one hash. That's all I'm talking about.

There may not be a strong need to have multiple hashes in the log entries yet. I have also recently spotted Intel fTPMs that can have only one PCR bank active at a time, so only discrete TPMs are capable of having multiple PCR banks active. Let's stick with a single hash algorithm in the entries for now. If needed we may add additional algorithms later indeed.