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

[Breno Leitao <leitao@debian.org>]

On Thu, Nov 12, 2026 at 04:03:06PM +0200, Jarkko Sakkinen wrote:
> On Thu, Nov 12, 2026 at 04:00:50PM +0200, Jarkko Sakkinen wrote:
> > On Thu, Nov 12, 2026 at 04:30:17AM -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 once 60 commands in a row have timed out.
> > > 
> > > A completed command, whatever its TPM return code, resets the count.
> > > Some TPMs stall for half a minute, time out a handful of commands, and
> > > then recover, so a short streak must not disable them.
> > > 
> > > With only the hwrng retrying every 10 seconds, 60 timeouts take more
> > > than ten minutes. A userspace poller gets there sooner, but each of its
> > > commands still waits out the transport timeout.
> > > 
> > > The limit is the tpm.max_unanswered boot parameter, so that slow
> > > pre-production parts, such as FPGA prototypes, can raise it, or set it
> > > to 0 to never disable the TPM. A disabled TPM stays disabled until the
> > > driver is rebound or the machine reboots.
> > > 
> > > Signed-off-by: Breno Leitao <leitao@debian.org>
> > > ---
> > >  drivers/char/tpm/tpm-interface.c | 15 +++++++++++++++
> > >  include/linux/tpm.h              |  1 +
> > >  2 files changed, 16 insertions(+)
> > > 
> > > diff --git a/drivers/char/tpm/tpm-interface.c b/drivers/char/tpm/tpm-interface.c
> > > index e0385b16616b46..b7865ccb799289 100644
> > > --- a/drivers/char/tpm/tpm-interface.c
> > > +++ b/drivers/char/tpm/tpm-interface.c
> > > @@ -40,6 +40,11 @@ module_param_named(suspend_pcr, tpm_suspend_pcr, uint, 0644);
> > >  MODULE_PARM_DESC(suspend_pcr,
> > >  		 "PCR to use for dummy writes to facilitate flush on suspend.");
> > >  
> > > +static unsigned int tpm_max_unanswered = 60;
> > > +module_param_named(max_unanswered, tpm_max_unanswered, uint, 0444);
> > > +MODULE_PARM_DESC(max_unanswered,
> > > +		 "Disable the TPM after this many commands in a row time out (0 = never).");
> > 
> > Documentation/admin/kernel-parameters.txt update is also needed.
> > 
> > tpm_max_missed_responses is what I would suggest, because it is more
> > accurate e.g., on stating what is being counted :-)
> > 
> > 
> > > +
> > >  /**
> > >   * tpm_calc_ordinal_duration() - calculate the maximum command duration
> > >   * @chip:    TPM chip to use.
> > > @@ -268,6 +273,16 @@ ssize_t tpm_transmit(struct tpm_chip *chip, u8 *buf, size_t bufsiz)
> > >  		delay_msec *= 2;
> > >  		memcpy(buf, save, save_size);
> > >  	}
> > > +
> > > +	if (ret >= 0) {
> > > +		/* we are good, reset nr_unanswered */
> > 
> > I'm not a huge fan in general of third person plural in the source code
> > comments. Probably a comment here is not needed at all as it does not
> > state anything that would not be obvious (i.e. something nothing
> > non-obvious is documented).
> > 
> > > +		chip->nr_unanswered = 0;
> > > +	} else if (ret == -ETIME &&
> > > +		   ++chip->nr_unanswered == tpm_max_unanswered) {
> > > +		dev_err(&chip->dev, "%u commands in a row timed out, disabling the TPM\n",
> > > +			chip->nr_unanswered);
> > > +		chip->flags |= TPM_CHIP_FLAG_DISABLE;
> > > +	}
> > >  	return ret;
> > >  }
> > >  
> > > diff --git a/include/linux/tpm.h b/include/linux/tpm.h
> > > index 0db277af45c393..e8191585a68e7a 100644
> > > --- a/include/linux/tpm.h
> > > +++ b/include/linux/tpm.h
> > > @@ -131,6 +131,7 @@ struct tpm_chip {
> > >  	struct hwrng hwrng;
> > >  
> > >  	struct mutex tpm_mutex;	/* tpm is processing */
> > > +	unsigned int nr_unanswered;	/* commands in a row that timed out */
> > 
> > This would be then nr_lost_responses.
> 
> Oops sorry.
> 
> I mean nr_missed_responses (and I would prefer "missed" over "lost").

Ack, respining...

--breno