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

[Alexey Kardashevskiy]

On 21/01/2020 05:22, Stefan Berger wrote:
> 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.


Then why are we ditching v1.2? I asked multiple times what needs v1.2
and grub never came up and if this is the only version what grub
supports - we need it, no? Or we somehow are going to bypass grub
completely and it is ok?


> 
>>>
>>> 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.

ah yes. Forth is one great write-only language :)


> 
> 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.


Then this should go to 7/7?


> 
> 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
> 
> 

-- 
Alexey