linux-kernel - Re: [RFC PATCH] tpm: Workaround failed command reception on Infineon devices

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <979629af-e224-4308-a9a1-c66a60003d2d@linux.ibm.com>
Date: Thu, 3 Oct 2024 18:25:14 -0400
From: Stefan Berger <stefanb@...ux.ibm.com>
To: Jonathan McDowell <noodles@...th.li>, linux-integrity@...r.kernel.org,
        Jarkko Sakkinen <jarkko@...nel.org>,
        James Bottomley <James.Bottomley@...senpartnership.com>
Cc: Peter Huewe <peterhuewe@....de>, Jason Gunthorpe <jgg@...pe.ca>,
        linux-kernel@...r.kernel.org
Subject: Re: [RFC PATCH] tpm: Workaround failed command reception on Infineon
 devices



On 10/3/24 5:59 PM, Stefan Berger wrote:
> 
> 
> On 10/2/24 1:04 PM, Jonathan McDowell wrote:
>> (I'm still in the process of testing this to confirm it fixes the
>> errata I've seen, but I wanted to send it out for comments to make sure
>> it's a reasonable approach.)
>>
>> Some Infineon devices have a issue where the status register will get
>> stuck with a quick REQUEST_USE / COMMAND_READY sequence. The work around
> 
> Did you tell Infineon about it? Maybe they should have a look at their 
> firmware.
> 
> What are the TPMs in your fleet doing? I heared that some TPMs 
> pre-create keys in the background once users requested a few. I would 
> try to create a few primary keys with different combination of key flags 

Actually make this child keys of primary keys:

 > tsscreateprimary
Handle 80000000
 > while :; do time tsscreate -hp 80000000 -si  -opem pubkey.pem ; cat 
pubkey.pem; done

This should give a different key every time and maybe key creation time 
goes up at some point...

> set and in different hierarchies (and flush them) to use up these 
> precreated  keys and see whether that leads to any issues with the TIS 
> responsiveness once presumably the device starts creating keys in the 
> background...
> 
>> is to retry the command submission. Add appropriate logic to do this in
> 
> I would describe it as 'retrying to set the TPM_STS_COMMAND_READY flag 
> on the TIS STS register' because you are not retring a (TPM) command 
> submission, like resubmitting a TPM2_PCR_Extend for example.
> 
> 
> 
>> the send path.
>>
>> Signed-off-by: Jonathan McDowell <noodles@...a.com>
>> ---
>>   drivers/char/tpm/tpm_tis_core.c | 24 +++++++++++++++++++-----
>>   drivers/char/tpm/tpm_tis_core.h |  1 +
>>   include/linux/tpm.h             |  1 +
>>   3 files changed, 21 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/char/tpm/tpm_tis_core.c 
>> b/drivers/char/tpm/tpm_tis_core.c
>> index f6aa0dfadb93..940abd1a868e 100644
>> --- a/drivers/char/tpm/tpm_tis_core.c
>> +++ b/drivers/char/tpm/tpm_tis_core.c
>> @@ -432,16 +432,27 @@ static int tpm_tis_recv(struct tpm_chip *chip, 
>> u8 *buf, size_t count)
>>   static int tpm_tis_send_data(struct tpm_chip *chip, const u8 *buf, 
>> size_t len)
>>   {
>>       struct tpm_tis_data *priv = dev_get_drvdata(&chip->dev);
>> -    int rc, status, burstcnt;
>> +    int rc, status, burstcnt, retry;
>> +    bool status_fix = test_bit(TPM_TIS_STATUS_WORKAROUND, &priv->flags);
> 
> This should probably be moved to the top.
> int retry = status_fix ? 3 : 1;
> 
>>       size_t count = 0;
>>       bool itpm = test_bit(TPM_TIS_ITPM_WORKAROUND, &priv->flags);
>>       status = tpm_tis_status(chip);
>>       if ((status & TPM_STS_COMMAND_READY) == 0) {
>> -        tpm_tis_ready(chip);
>> -        if (wait_for_tpm_stat
>> -            (chip, TPM_STS_COMMAND_READY, chip->timeout_b,
>> -             &priv->int_queue, false) < 0) {
>> +        retry = status_fix ? 3 : 1;
>> +
>> +        while (retry > 0) {
>> +            tpm_tis_ready(chip);
>> +            if (wait_for_tpm_stat
>> +                (chip, TPM_STS_COMMAND_READY, chip->timeout_b,
>> +                 &priv->int_queue, false) >= 0) {
>> +                break;
>> +            }
>> +
>> +            retry--;
>> +        }
>> +
>> +        if (retry == 0) {
>>               rc = -ETIME;
>>               goto out_err;
>>           }
>> @@ -1147,6 +1158,9 @@ int tpm_tis_core_init(struct device *dev, struct 
>> tpm_tis_data *priv, int irq,
>>           priv->timeout_max = TIS_TIMEOUT_MAX_ATML;
>>       }
>> +    if (priv->manufacturer_id == TPM_VID_IFX)
>> +        set_bit(TPM_TIS_STATUS_WORKAROUND, &priv->flags);
>> +
>>       if (is_bsw()) {
>>           priv->ilb_base_addr = ioremap(INTEL_LEGACY_BLK_BASE_ADDR,
>>                       ILB_REMAP_SIZE);
>> diff --git a/drivers/char/tpm/tpm_tis_core.h 
>> b/drivers/char/tpm/tpm_tis_core.h
>> index 13e99cf65efe..f888da57535d 100644
>> --- a/drivers/char/tpm/tpm_tis_core.h
>> +++ b/drivers/char/tpm/tpm_tis_core.h
>> @@ -89,6 +89,7 @@ enum tpm_tis_flags {
>>       TPM_TIS_INVALID_STATUS        = 1,
>>       TPM_TIS_DEFAULT_CANCELLATION    = 2,
>>       TPM_TIS_IRQ_TESTED        = 3,
>> +    TPM_TIS_STATUS_WORKAROUND    = 4,
>>   };
>>   struct tpm_tis_data {
>> diff --git a/include/linux/tpm.h b/include/linux/tpm.h
>> index 4ee9d13749ad..5f4998626a98 100644
>> --- a/include/linux/tpm.h
>> +++ b/include/linux/tpm.h
>> @@ -272,6 +272,7 @@ enum tpm2_cc_attrs {
>>   #define TPM_VID_WINBOND  0x1050
>>   #define TPM_VID_STM      0x104A
>>   #define TPM_VID_ATML     0x1114
>> +#define TPM_VID_IFX      0x15D1
>>   enum tpm_chip_flags {
>>       TPM_CHIP_FLAG_BOOTSTRAPPED        = BIT(0),
>