[SeaBIOS] [PATCH 3/3] tpm: Support 2.0 TPM devices connected to a TIS host

[Stefan Berger]

On 02/27/2018 12:35 PM, Stephen Douthit wrote:
> On 02/27/2018 12:17 PM, Stefan Berger wrote:
>> On 02/27/2018 12:14 PM, Stephen Douthit wrote:
>>> On 02/26/2018 07:45 PM, Stefan Berger wrote:
>>>> On 02/26/2018 06:24 PM, Stephen Douthit wrote:
>>>>> On 02/26/2018 06:02 PM, Stefan Berger wrote:
>>>>>> On 02/26/2018 05:44 PM, Stephen Douthit wrote:
>>>>>>> On 02/26/2018 05:09 PM, Stefan Berger wrote:
>>>>>>>> On 02/26/2018 03:37 PM, Stephen Douthit wrote:
>>>>>>>>> tis_get_tpm_version() was returning the interface version, 
>>>>>>>>> which is always
>>>>>>>>> 1.2 since tis_probe() would have failed if the interface 
>>>>>>>>> wasn't TIS.
>>>>>>>>>
>>>>>>>>> New version check is based on the tpm2_probe() function from 
>>>>>>>>> the Linux
>>>>>>>>> tpm_tis driver.
>>>>>>>>>
>>>>>>>>> Signed-off-by: Stephen Douthit <stephend at silicom-usa.com>
>>>>>>>>> Tested-by: Stephen Douthit <stephend at silicom-usa.com>
>>>>>>>>> ---
>>>>>>>>>   src/hw/tpm_drivers.c | 21 +++++++++++++++------
>>>>>>>>>   src/std/tcg.h        |  1 +
>>>>>>>>>   src/tcgbios.c        |  2 +-
>>>>>>>>>   src/tcgbios.h        |  4 ++++
>>>>>>>>>   4 files changed, 21 insertions(+), 7 deletions(-)
>>>>>>>>>
>>>>>>>>> diff --git a/src/hw/tpm_drivers.c b/src/hw/tpm_drivers.c
>>>>>>>>> index 789d70a..32c34df 100644
>>>>>>>>> --- a/src/hw/tpm_drivers.c
>>>>>>>>> +++ b/src/hw/tpm_drivers.c
>>>>>>>>> @@ -11,6 +11,7 @@
>>>>>>>>>   #include "config.h" // CONFIG_TPM_TIS_SHA1THRESHOLD
>>>>>>>>>   #include "hw/tpm_drivers.h" // struct tpm_driver
>>>>>>>>>   #include "std/tcg.h" // TCG_RESPONSE_TIMEOUT
>>>>>>>>> +#include "tcgbios.h" // tpm20_getcapability
>>>>>>>>>   #include "output.h" // warn_timeout
>>>>>>>>>   #include "stacks.h" // yield
>>>>>>>>>   #include "string.h" // memcpy
>>>>>>>>> @@ -119,7 +120,7 @@ static u32 tis_probe(void)
>>>>>>>>>       if ((didvid != 0) && (didvid != 0xffffffff))
>>>>>>>>>           rc = 1;
>>>>>>>>>
>>>>>>>>> -    /* TPM 2 has an interface register */
>>>>>>>>> +    /* Low 32 bits of CRB interface register overlap TIS 
>>>>>>>>> interface register */
>>>>>>>>>       u32 ifaceid = readl(TIS_REG(0, TIS_REG_IFACE_ID));
>>>>>>>>
>>>>>>>> Since this is tis_probe, I don't think we need to mention the 
>>>>>>>> CRB here ...
>>>>>>>
>>>>>>> I think that was a note to myself I forgot to strip out when I
>>>>>>> dropped my debug prints.  I'll change the comment back.
>>>>>>>
>>>>>>>>>
>>>>>>>>>       if ((ifaceid & 0xf) != 0xf) {
>>>>>>>>> @@ -142,13 +143,21 @@ static u32 tis_probe(void)
>>>>>>>>>
>>>>>>>>>   static TPMVersion tis_get_tpm_version(void)
>>>>>>>>>   {
>>>>>>>>> -    /* TPM 2 has an interface register */
>>>>>>>>> -    u32 ifaceid = readl(TIS_REG(0, TIS_REG_IFACE_ID));
>>>>>>>>> +    u8 buffer[128];
>>>>>>>>> +    int ret;
>>>>>>>>> +    struct tpm2_res_getcapability *trg =
>>>>>>>>> +      (struct tpm2_res_getcapability *)&buffer;
>>>>>>>>> +
>>>>>>>>> +    ret = tpm20_getcapability(TPM2_CAP_TPM_PROPERTIES, 
>>>>>>>>> cpu_to_be32(0x100),
>>>>>>>>> +                              cpu_to_be32(1), &trg->hdr, 
>>>>>>>>> sizeof(buffer));
>>>>>>>>> +    if (ret == TPM2_RC_INITIALIZE)
>>>>>>>>> +        return TPM_VERSION_2;
>>>>>>>>> +    else if (ret)
>>>>>>>>> +        return TPM_VERSION_NONE;
>>>>>>>> I haven't tried yet, but likely this will not work for TPM 1.2 
>>>>>>>> since it will also return an error.
>>>>>>>
>>>>>>> Doh.  That makes sense.  I'll need to find a 1.2 system to test 
>>>>>>> on to
>>>>>>> avoid stupid mistakes like that.
>>>>>>>
>>>>>>>> Wouldn't the check for the tag below not be sufficient? We are 
>>>>>>>> doing something similar in QEMU when probing for the TPM. Here 
>>>>>>>> we send TPM 2 command TPM_CC_ReadClock and check the tag for 
>>>>>>>> TPM 2 type and then TPM 1.2 command TPM_ORD_GetTicks and check 
>>>>>>>> for TPM 1.2 tag.
>>>>>>>
>>>>>>> It might be, it wasn't clear to me if the tag could be trusted 
>>>>>>> if the
>>>>>>> return code wasn't success.  The Linux tpm2_probe() code bails 
>>>>>>> before
>>>>>>> the tag comparison if tpm_transmit_cmd() returns an error.
>>>>>>>
>>>>>>> Is there a block of QEMU code you would recommend as a reference 
>>>>>>> for
>>>>>>> probing TPMs?  I'm not very familiar with that code base, and a
>>>>>>> pointer would be handy.
>>>>>>
>>>>>> https://github.com/stefanberger/qemu-tpm/blob/v2.4.1%2Btpm/hw/tpm/tpm_util.c#L94 
>>>>>>
>>>>>>
>>>>>> What I don't like about the sending of commands is that we're 
>>>>>> breaking the layer here. We basically cannot send commands using 
>>>>>> this higher level API. The lowest we could go would be to call 
>>>>>> tpmhw_transmit, but that function cannot be used as it is but has 
>>>>>> to be split up to take tpm_driver as a parameter. You would pass 
>>>>>> to it &tpm_drivers[TIS_DRIVER_INDEX].
>>>>>>
>>>>>> As far as I know, TPM 1.2 didn't have that register, so they 
>>>>>> either return static 0 or 0xffffffff in that register.
>>>>>>
>>>>>> Though if reading the flags from the interface register really 
>>>>>> turns out to be insufficient, I think you should try to send a 
>>>>>> command like TPM2_CC_ReadClock. If you get back a 
>>>>>> TPM2_ST_NO_SESSION, it's a TPM 2. If you get a 
>>>>>> TPM_TAG_RSP_COMMAND it's a TPM 12.
>>>>>
>>>>> Thanks for all the info.  This gives me some experiments to run, and
>>>>> I'll try to find a solution that avoids sending commands if possible.
>>>>
>>>> The following document on pdf page 100 defines bits 28-30 of the 
>>>> interface capability register. If the value there is 0x3 (mask 
>>>> 0x7), we should be sure to have a TPM 2. Hopefully that's 
>>>> sufficient also for your device for determining that it's a TPM 2.
>>>>
>>>> https://trustedcomputinggroup.org/wp-content/uploads/TCG_PC_Client_Platform_TPM_Profile_PTP_Specification_Family_2.0_Revision_1.3v22.pdf 
>>>>
>>>>
>>>>
>>>
>>> My interface ID reads as 0xffffffff, and the interface capability
>>> register reads as intf_cap 0x30000697.  So your suggestion of
>>> checking for 3 in that bitfield of the interface capability register
>>> works here.
>>>
>>> If I change the version check function to the following and drop all
>>> the other changes from patch 3/3 would that be acceptable?
>>>
>>> static TPMVersion tis_get_tpm_version(void)
>>> {
>>>     u32 reg = readl(TIS_REG(0, TIS_REG_IFACE_ID));
>>>
>>>     /* FIFO interface as defined in PTP for TPM 2.0 is active */
>>>     if ((reg & 0xf) == 0) {
>>>         return TPM_VERSION_2;
>>>     }
>>>     /*
>>>      * FIFO interface as defined in TIS1.3 is active
>>>      * Interface capabilities are defined in TIS_REG_INTF_CAPABILITY
>>>      */
>>>     else if ((reg & 0xf) == 0xf) {
>>>         reg = readl(TIS_REG(0, TIS_REG_INTF_CAPABILITY));
>>>         /* Interface 1.3 for TPM 2.0 */
>>>         if (((reg >> 28) & 0x7) == 3)
>>>             return TPM_VERSION_2;
>>
>>
>> I think this is actually the better test and you should put this one 
>> up front.
>
> By "up front" do you want me to swap the order of if/else if tests,
> or make this the first patch in the series?

Make this the first test in tis_get_tpm_version().


>
>>     Stefan
>>
>>>     }
>>>
>>>     return TPM_VERSION_1_2;
>>> }
>>>
>>
>