Re: [PATCH v3 2/6] tpm: Support boot measurements

Eddie James
2023-01-23T20:15:33Z

On 1/16/23 06:00, Ilias Apalodimas wrote:
> Hi Eddie
>
>
>> +static inline u16 tpm2_algorithm_to_len(enum tpm2_algorithms a)
>> +{
>> + switch (a) {
>> + case TPM2_ALG_SHA1:
>> + return TPM2_SHA1_DIGEST_SIZE;
>> + case TPM2_ALG_SHA256:
>> + return TPM2_SHA256_DIGEST_SIZE;
>> + case TPM2_ALG_SHA384:
>> + return TPM2_SHA384_DIGEST_SIZE;
>> + case TPM2_ALG_SHA512:
>> + return TPM2_SHA512_DIGEST_SIZE;
>> + default:
>> + return 0;
>> + }
>> +}
> Any reason we can't move the static 'const struct digest_info
> hash_algo_list' from the efi_tcg.c here? We can then move the
> functions defined in there alg_to_mask and alg_to_len.
>
> And since alg_to_mask is really just a bitshift maybe replace that?

Hi,

It seems more efficient to keep the switch statement rather than iterate
through the structure array looking for the matching hash algorithm? I
could remove the 'static const struct digest_info hash_algo_list' in
efi_tcg2.c and instead use the tpm2_algorithm_to_len and
tpm2_algorithm_to_mask in efi_tcg2.c. What do you think?

>> +
>> +#define tpm2_algorithm_to_mask(a) (1 << (a))
>> +
>> /* NV index attributes */
>> enum tpm_index_attrs {
>> TPMA_NV_PPWRITE = 1UL << 0,
>> @@ -419,6 +481,142 @@ enum {
>> HR_NV_INDEX = TPM_HT_NV_INDEX << HR_SHIFT,
>> };
>>
>> +/**
>> + * struct tcg2_event_log - Container for managing the platform event log
>> + *
>> + * @log: Address of the log
>> + * @log_position: Current entry position
>> + * @log_size: Log space available
>> + */
>> +struct tcg2_event_log {
>> + u8 *log;
>> + u32 log_position;
<snip>
>> - }
>> - }
>> -
>> - *pcr_banks = pcrs.count;
>> -
>> - return 0;
>> -out:
>> - return -1;
>> -}
>> -
>> /**
>> * __get_active_pcr_banks() - returns the currently active PCR banks
>> *
>> @@ -638,7 +378,7 @@ static efi_status_t __get_active_pcr_banks(u32 *active_pcr_banks)
>> efi_status_t ret;
>> int err;
>>
>> - ret = platform_get_tpm2_device(&dev);
>> + ret = tcg2_platform_get_tpm2(&dev);
>> if (ret != EFI_SUCCESS)
>> goto out;
>>
> We can get rid of this entirely and just define the
> efi_tcg2_get_active_pcr_banks in the efi_tcg.c now.
> __get_active_pcr_banks == tcg2_get_active_pcr_banks with the only
> diffence being the udevice which is now an argument

Ack.

>
>> @@ -654,70 +394,6 @@ out:
>> return ret;
>> }
>>
>> * efi_tcg2_get_capability() - protocol capability information and state information
>> *
>> @@ -759,7 +435,7 @@ efi_tcg2_get_capability(struct efi_tcg2_protocol *this,
>> capability->protocol_version.major = 1;
>> capability->protocol_version.minor = 1;
<snip>
>> +
>> +static int tcg2_log_append_check(struct tcg2_event_log *elog, u32 pcr_index,
>> + u32 event_type,
>> + struct tpml_digest_values *digest_list,
>> + u32 size, const u8 *event)
>> +{
>> + u32 event_size;
>> + u8 *log;
>> +
>> + event_size = size + tcg2_event_get_size(digest_list);
>> + if (elog->log_position + event_size > elog->log_size) {
>> + printf("%s: log too large: %u + %u > %u\n", __func__,
>> + elog->log_position, event_size, elog->log_size);
>> + return -ENOBUFS;
>> + }
>> +
>> + log = elog->log + elog->log_position;
>> + elog->log_position += event_size;
>> +
>> + tcg2_log_append(pcr_index, event_type, digest_list, size, event, log);
>> +
>> + return 0;
>> +}
>> +
>> +static int tcg2_log_init(struct udevice *dev, struct tcg2_event_log *elog)
>> +{
> I think we need to re-use efi_init_event_log here. The reason is that on
> Arm devices TF-A is capable of constructing an eventlog and passing it
> along in memory. That code takes that into account and tries to reuse the
> existing EventLog passed from previous boot stages.
>
> The main difference between the EFI function and this one
> is the allocated memory of the EventLog itself. But even in this case, it
> would be better to tweak the EFI code and do
> create log -> Allocate EFI memory -> copy log and then use that for EFI

OK... I'll try and get that to work. I see some potential issues like
the fact that EFI finds the platform event log differently.

>
>> + struct tcg_efi_spec_id_event *ev;
>> + struct tcg_pcr_event *log;
>> + u32 event_size;
>> + u32 count = 0;
>> + u32 log_size;
>> + u32 active;
>> + u32 mask;
>> + size_t i;
>> + u16 len;
>> + int rc;
>> +
>> + rc = tcg2_get_active_pcr_banks(dev, &active);
>> + if (rc)
>> + return rc;
>> +
>> + event_size = offsetof(struct tcg_efi_spec_id_event, digest_sizes);
>> + for (i = 0; i < ARRAY_SIZE(tcg2algos); ++i) {
>> + mask = tpm2_algorithm_to_mask(tcg2algos[i]);
>> +
>> + if (!(active & mask))
>> + continue;
>> +
>> + switch (tcg2algos[i]) {
>> + case TPM2_ALG_SHA1:
>> + case TPM2_ALG_SHA256:
>> + case TPM2_ALG_SHA384:
>> + case TPM2_ALG_SHA512:
>> + count++;
>> + break;
>> + default:
>> + continue;
>> + }
>> + }
>> +
>> + event_size += 1 +
>> + (sizeof(struct tcg_efi_spec_id_event_algorithm_size) * count);
>> + log_size = offsetof(struct tcg_pcr_event, event) + event_size;
>> +
>> + if (log_size > elog->log_size) {
>> + printf("%s: log too large: %u > %u\n", __func__, log_size,
>> + elog->log_size);
>> + return -ENOBUFS;
>> + }
>> +
<snip>
>> +
>> +int tcg2_measure_event(struct udevice *dev, struct tcg2_event_log *elog,
>> + u32 pcr_index, u32 event_type, u32 size,
>> + const u8 *event)
>> +{
>> + struct tpml_digest_values digest_list;
>> + int rc;
>> +
>> + rc = tcg2_create_digest(dev, event, size, &digest_list);
>> + if (rc)
>> + return rc;
>> +
>> + rc = tcg2_pcr_extend(dev, pcr_index, &digest_list);
>> + if (rc)
>> + return rc;
>> +
>> + return tcg2_log_append_check(elog, pcr_index, event_type, &digest_list,
>> + size, event);
>> +}
> There's a static efi_status_t tcg2_measure_event(...) left in efi_tcg.c
> which breaks compilation. WE should just use the one you added in tpm-v2.c

Hmm. The EFI version does a slightly different way of event logging, so
I'm not sure it's that simple. There is EFI specific stuff
(ExitBootServices?) so I'm not sure it can be common... I can change the
name in efi_tcg2.c to compile correctly.

Thanks,

Eddie

>
>> +
>> + return 0;
>> +}
>> +
>> u32 tpm2_dam_reset(struct udevice *dev, const char *pw, const ssize_t pw_sz)
>> {
>> u8 command_v2[COMMAND_BUFFER_SIZE] = {
>> --
>> 2.31.1
>>
> Regards
> /Ilias