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

Heinrich Schuchardt
2023-01-13T09:27:51Z

On 1/13/23 08:07, Ilias Apalodimas wrote:
> Hi Simon
>
> On Fri, 13 Jan 2023 at 01:43, Simon Glass <sjg@chromium.org> wrote:
>>
>> Hi Eddie,
>>
>> On Thu, 12 Jan 2023 at 09:16, Eddie James <eajames@linux.ibm.com> wrote:
>>>
>>> Add TPM2 functions to support boot measurement. This includes
>>> starting up the TPM, initializing/appending the event log, and
>>> measuring the U-Boot version. Much of the code was used in the
>>> EFI subsystem, so remove it there and use the common functions.
>>>
>>> Signed-off-by: Eddie James <eajames@linux.ibm.com>
>>> ---
>>> include/efi_tcg2.h | 44 ---
>>> include/tpm-v2.h | 211 ++++++++++++
>>> lib/efi_loader/efi_tcg2.c | 362 +------------------
>>> lib/tpm-v2.c | 708 ++++++++++++++++++++++++++++++++++++++
>>> 4 files changed, 938 insertions(+), 387 deletions(-)
>>
>> [..]
>>
>>> diff --git a/lib/tpm-v2.c b/lib/tpm-v2.c
>>> index 697b982e07..00e1b04d74 100644
>>> --- a/lib/tpm-v2.c
>>> +++ b/lib/tpm-v2.c
>>> @@ -4,13 +4,597 @@
>>> * Author: Miquel Raynal <miquel.raynal@bootlin.com>
>>> */
>>>
>
> [...]
>
>>
>>> +static int tcg2_log_init(struct udevice *dev, struct tcg2_event_log *elog)
>>> +{
>>> + struct tcg_efi_spec_id_event *ev;
>>
>> We cannot add EFI things to generic TPM code.
>>
>>> + 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;
>>> + }
>>> +
>>> + log = (struct tcg_pcr_event *)elog->log;
>>> + put_unaligned_le32(0, &log->pcr_index);
>>> + put_unaligned_le32(EV_NO_ACTION, &log->event_type);
>>> + memset(&log->digest, 0, sizeof(log->digest));
>>> + put_unaligned_le32(event_size, &log->event_size);
>>> +
>>> + ev = (struct tcg_efi_spec_id_event *)log->event;
>>> + strlcpy((char *)ev->signature, TCG_EFI_SPEC_ID_EVENT_SIGNATURE_03,
>>
>> Same with all of this.
>>
>>> + sizeof(ev->signature));
>>> + put_unaligned_le32(0, &ev->platform_class);
>>> + ev->spec_version_minor = TCG_EFI_SPEC_ID_EVENT_SPEC_VERSION_MINOR_TPM2;
>>> + ev->spec_version_major = TCG_EFI_SPEC_ID_EVENT_SPEC_VERSION_MAJOR_TPM2;
>>> + ev->spec_errata = TCG_EFI_SPEC_ID_EVENT_SPEC_VERSION_ERRATA_TPM2;
>>
>> I'm not quite sure what is going on here...is this log in a format
>> defined by the EFI spec? What if we are not using EFI? How would a
>> different format be used?
>
> Yes. The TCG eventlog and everything Eddie is trying to add are
> defined by an extension to the EFI spec. I am unaware of any forms of
> measurement (with a TPM). The eventlong is purely a software
> construct. The TPM PCR extension involves taking measurements and
> talking to the hardware. Nothing prevents you from doing this outside
> EFI. What I am curious about is how these measurements are used by
> the OS in Eddie's case.
>
> When booting with EFI, the kernel calls the GetEventlog callback and
> stores the event log in memory. What happens to bootm?

The eventlog will be in reserved memory. The following patch is needed
to access it in Linux:

[PATCH] tpm: Add reserved memory event log
https://lore.kernel.org/lkml/20230103162010.381214-1-eajames@linux.ibm.com/

The concept looks ok to me.

Best regards

Heinrich

>
>>
>> Put another way, people using a TPM should not pull in EFI things just
>> to do that.
>>
>> I'm just not quite sure of the best approach here...
>
> The extensions to the EFI spec defines
> 1. What the eventlog format looks like
> 2. Functions to configure the TPM pcr banks, retrieve the eventlog
> from memory etc.
>
> Regards
> /Ilias
>>
>> Regards,
>> Simon