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

[Heinrich Schuchardt <xypron.glpk@gmx.de>]

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