[SLOF] [PATCH v6 6/7] tcgbios: Add TPM 2.0 support and firmware API

[Stefan Berger]

On 1/20/20 12:08 PM, Stefan Berger wrote:
> On 1/20/20 3:09 AM, Alexey Kardashevskiy wrote:
>>

I accidentally pressed the send button before. So here are more responses.

>
>>> +
>>> +\ firmware API call
>>> +: get-failure-reason ( -- reason )
>>> +    " get-failure-reason" vtpm-call-forward IF
>>> +        \ vtpm-call-forward failed; return a value
>>> +        0 \ invalid
>>> +    THEN
>>> +;
>>> +
>>> +0 0 s" ibm,sml-efi-reformat-supported" property
>>> +
>>> +\ firmware API call
>>> +: reformat-sml-to-efi-alignment ( -- success )
>>> +    " reformat-sml-to-efi-alignment" vtpm-call-forward IF
>>> +        false
>>> +    THEN
>>> +;
>>> +
>>> +: open ( )
>>> +    vtpm-debug? IF ." VTPM: vTPM open()" cr THEN
>>> +    true
>>> +;
>>> +
>>> +: close ( )
>>> +    vtpm-debug? IF ." VTPM: vTPM close()" cr THEN
>>> +;
>>> +
>>> +\ setup alias and the RTAS bypass
>>> +vtpm-init
>>> +
>>> +\ setup the log
>>> +include vtpm-sml.fs
>>> +
>>> +s" /ibm,vtpm" find-node ?dup IF
>>> +  s" measure-scrtm" rot $call-static
>>> +THEN
>>
>> The above 22 lines confuse me a lot.
>> Why vtpm-sml.fs?
>
> You mean why does vtpm-sml.fs exist at all?Or why not just put all 
> vTPM code into one file?
>
>
>> Why "open" does not open?
>> Why vtpm-init is not in "open"?
>> Why the device methods are in vtpm-sml.fs?


The straight forward answer is that I have some 'challenges' with the 
FORTH part. I do not completely understand how to organize the code 
there. What is there right now is at least 'working'.


>>
>> The Linux finds the device, opens it and calls methods (passing
>> ihandle), why this complication?


Grub could call the API as well. I have removed all the other APIs now 
since the removal of TPM 1.2 support since I only knew of callers of the 
API from 'trusted' grub code supporting TPM 1.2. It doesn't support TPM 2.


>>
>> I am missing the point in all of this and 2 lines commit log does not
>> help at all.
>>




>> +
>> +/*
>> + * Prepare TPM for boot; this function has to be called before
>> + * the firmware transitions to the boot loader.
>>
>> I do not see any caller of this one.
>>
>>
>>> + */
>>> +uint32_t tpm_leave_firmware(void)
>> static?


No, FORTH is calling it.

board-qemu/slof/vtpm-sml.fs-: leave-firmware ( -- )
board-qemu/slof/vtpm-sml.fs: tpm-leave-firmware                         
( errcode )
board-qemu/slof/vtpm-sml.fs-    ?dup IF
board-qemu/slof/vtpm-sml.fs:        ." VTPM: Error code from 
tpm-leave-firmware: " . cr
board-qemu/slof/vtpm-sml.fs-    THEN
board-qemu/slof/vtpm-sml.fs-;


>>
>>
>>> +/*
>>> + * Add event separators for a range of PCRs
>>> + */
>>> +uint32_t tpm_add_event_separators(uint32_t start_pcr, uint32_t 
>>> end_pcr)
>>> +{
>>> +    static const uint8_t evt_separator[] = {0xff,0xff,0xff,0xff};
>>> +    uint32_t rc = 0;
>>> +    uint32_t pcrIndex;
>>> +
>>> +    if (!tpm_is_working())
>>> +        return TCGBIOS_GENERAL_ERROR;
>>> +
>>> +    if (start_pcr >= 24 || start_pcr > end_pcr)
>>> +        return TCGBIOS_INVALID_INPUT_PARA;
>>> +
>>> +    /* event separators need to be extended and logged for PCRs 0-7 */
>>> +    for (pcrIndex = start_pcr; pcrIndex <= end_pcr; pcrIndex++) {
>>> +        rc = tpm_add_measurement_to_log(pcrIndex, EV_SEPARATOR,
>>> +                        NULL, 0,
>>> +                        evt_separator,
>>> +                        sizeof(evt_separator));
>>> +        if (rc)
>>> +            break;
>> return rc;


Fixed.



>>
>>
>>> +    }
>>> +
>>> +    return rc;
>>
>> return 0;
>>
>>
>>> +}
>>> +
>>> +uint32_t tpm_measure_bcv_mbr(uint32_t bootdrv, const uint8_t *addr,
>>> +                 uint32_t length)
>>> +{
>>> +    uint32_t rc;
>>> +    const char *string;
>>> +
>>> +    if (!tpm_is_working())
>>> +        return TCGBIOS_GENERAL_ERROR;
>>> +
>>> +    if (length < 0x200)
>>> +        return TCGBIOS_INVALID_INPUT_PARA;
>>> +
>>> +    string = "Booting BCV device 00h (Floppy)";
>>> +    if (bootdrv == BCV_DEVICE_HDD)
>>> +        string = "Booting BCV device 80h (HDD)";
>>> +
>>> +    rc = tpm_add_action(4, string);
>>> +    if (rc)
>>> +        return rc;
>>> +
>>> +    /*
>>> +     * equivalent to: dd if=/dev/hda ibs=1 count=440 | sha1sum
>>> +     */
>>> +    string = "MBR";
>>
>> What about GPT (I am not extemely familiar with MBR/GPT but I see my
>> ubuntu images use GPT these days)?


I have a patch on top of this series that does measure GPT. I will post 
on the next round.

+
>>> +struct tpm2_req_pcr_allocate {
>>> +    struct tpm_req_header hdr;
>>> +    uint32_t authhandle;
>>> +    uint32_t authblocksize;
>>> +    struct tpm2_authblock authblock;
>>> +    uint32_t count;
>>> +    uint8_t tpms_pcr_selections[4];
>>> +} __attribute__((packed));
>>> +
>>> +struct tpms_pcr_selection {
>>> +    uint16_t hashAlg;
>>> +    uint8_t sizeOfSelect;
>>> +    uint8_t pcrSelect[0];
>>> +} __attribute__((packed));
>>> +
>>> +struct tpml_pcr_selection {
>>> +    uint32_t count;
>>> +    struct tpms_pcr_selection selections[0];
>>> +} __attribute__((packed));
>>> +
>>
>> Please separate somehow what is defined for SLOF only (is
>> TPM2_ALG_SHA1_FLAG/etc?) and for everything else - what comes from what
>> spec.


All are TPM 2 definitions. Most are related to commands, others are 
related to logging.

I will try to leave some references to names of specs.



>> +
>>> +/************************************************/
>>> +/* Check whether the TPM is working             */
>>> +/* SLOF:   tpm-is-working  ( -- true | false )  */
>>> +/* LIBTPM: bool = tpm_is_working()              */
>>> +/************************************************/
>>
>> What is calling this one? Now I am really not sure they all are actually
>> have a user. Thanks,


There is a user for this one.

board-qemu/slof/vtpm-sml.fs-
board-qemu/slof/vtpm-sml.fs-: vtpm-menu
board-qemu/slof/vtpm-sml.fs:    tpm-is-working IF
board-qemu/slof/vtpm-sml.fs-        tpm20-menu
board-qemu/slof/vtpm-sml.fs-    THEN