Re: [PATCH 1/2] tpm: Disable a TPM that stops responding

[Jarkko Sakkinen <jarkko@kernel.org>]

On Wed, Nov 11, 2026 at 01:57:33AM -0800, Breno Leitao wrote:
> A TPM that stops responding fails each command only after the transport
> timeout, and its callers keep sending more: the hwrng thread every 10
> seconds forever, and userspace through /dev/tpmrm0. Each attempt holds
> tpm_mutex for the whole timeout and logs an error:
> 
>   tpm tpm0: invalid TPM_STS.x 0xff, dumping stack for forensics
>   tpm tpm0: tpm_try_transmit: send(): error -62
>   tpm tpm0: tpm_try_transmit: send(): error -62
>   ...
> 
> The core already stops using a TPM it cannot trust: tpm_try_get_ops()
> fails with -EIO once TPM_CHIP_FLAG_DISABLE is set, which
> tpm2_load_null() does when it cannot restore the null key. Set the same
> flag when commands have kept timing out for ten minutes, with none
> completing in between.
> 
> A completed command, whatever its TPM return code, resets the clock. A
> disabled TPM stays disabled until the driver is rebound or the machine
> reboots.
> 
> Signed-off-by: Breno Leitao <leitao@debian.org>

Some other word than "timeout" should be used for added variables, and
this simply because it makes code more confusing to read given the
collision with the TCG spec terminology.

> ---
>  drivers/char/tpm/tpm-interface.c | 29 +++++++++++++++++++++++++++++
>  include/linux/tpm.h              |  2 ++
>  2 files changed, 31 insertions(+)
> 
> diff --git a/drivers/char/tpm/tpm-interface.c b/drivers/char/tpm/tpm-interface.c
> index e0385b16616b46..c6278ef8d1095e 100644
> --- a/drivers/char/tpm/tpm-interface.c
> +++ b/drivers/char/tpm/tpm-interface.c
> @@ -30,6 +30,8 @@
>  
>  #include "tpm.h"
>  
> +#define TPM_UNRESPONSIVE_TIMEOUT	(10 * 60 * HZ)

What is measured here is the accumulated time of the whole operation.

The problem with having compile-time only constant is a problem because
it makes FPGA pre-production phase more difficult. And I think to this
extent also considering pre-production makes sense even tho for most
part we only care about in-production ASIC :-)

So... It should be instead a boot-time parameter with a default value;
nothing more complicated than that.

Essentially this is watchdog timeout i.e. it detects hw malfunction
with a deadline timer.

So I'd suggest TPM_WDT_TIMEOUT because I think that will better separate
it from chip state transition timeouts.


> +
>  /*
>   * Bug workaround - some TPM's don't flush the most
>   * recently changed pcr on suspend, so force the flush
> @@ -206,6 +208,31 @@ static ssize_t tpm_try_transmit(struct tpm_chip *chip, void *buf, size_t bufsiz)
>  	return rc ? rc : len;
>  }
>  
> +/*
> + * Disable the TPM once commands keep timing out for TPM_UNRESPONSIVE_TIMEOUT.
> + * A few timeouts are not enough: some TPMs stall for half a minute and recover.
> + */
> +static void tpm_check_unresponsive(struct tpm_chip *chip, ssize_t ret)
> +{
> +	if (ret >= 0) {
> +		chip->nr_timeouts = 0;
> +		return;
> +	}
> +	if (ret != -ETIME || (chip->flags & TPM_CHIP_FLAG_DISABLE))
> +		return;
> +	if (!chip->nr_timeouts++) {
> +		chip->first_timeout = jiffies;
> +		return;
> +	}
> +	if (time_before(jiffies,
> +			chip->first_timeout + TPM_UNRESPONSIVE_TIMEOUT))
> +		return;
> +
> +	dev_err(&chip->dev, "%u commands timed out in %lu seconds, disabling the TPM\n",
> +		chip->nr_timeouts, (jiffies - chip->first_timeout) / HZ);
> +	chip->flags |= TPM_CHIP_FLAG_DISABLE;
> +}
> +
>  /**
>   * tpm_transmit - Internal kernel interface to transmit TPM commands.
>   * @chip:	a TPM chip to use
> @@ -268,6 +295,8 @@ ssize_t tpm_transmit(struct tpm_chip *chip, u8 *buf, size_t bufsiz)
>  		delay_msec *= 2;
>  		memcpy(buf, save, save_size);
>  	}
> +
> +	tpm_check_unresponsive(chip, ret);

This wrapper is only making things more confusing. Should be open-coded.

>  	return ret;
>  }
>  
> diff --git a/include/linux/tpm.h b/include/linux/tpm.h
> index 0db277af45c393..4264c87a04d98c 100644
> --- a/include/linux/tpm.h
> +++ b/include/linux/tpm.h
> @@ -131,6 +131,8 @@ struct tpm_chip {
>  	struct hwrng hwrng;
>  
>  	struct mutex tpm_mutex;	/* tpm is processing */
> +	unsigned int nr_timeouts;	/* consecutive command timeouts */


Isn't this a bool? Could be a chip flag instead if this is the right
approach in the first place.

> +	unsigned long first_timeout;	/* jiffies */

Don't really understand this name when comparing what gets recorded
into it.

>  
>  	unsigned long timeout_a; /* jiffies */
>  	unsigned long timeout_b; /* jiffies */
> 
> -- 
> 2.53.0-Meta
> 

I'm not really confident whether a new timeout is needed. Couldn't
alternative be just have a max number of trials and count -ETIME's.

Br, Jarkko