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: <3c9a6d63-f892-4809-b48d-5fecc3817ad9@linux.ibm.com>
Date: Thu, 3 Oct 2024 17:59:25 -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/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 
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),