Re: [PATCH] tpm: fix ATMEL TPM crash caused by too frequent queries

From: Jarkko Sakkinen <jarkko@kernel.org>
To: Hao Wu <hao.wu@rubrik.com>
Cc: Shrihari Kalkar <shrihari.kalkar@rubrik.com>,
	Seungyeop Han <seungyeop.han@rubrik.com>,
	Anish Jhaveri <anish.jhaveri@rubrik.com>,
	peterhuewe@gmx.de, jgg@ziepe.ca, linux-integrity@vger.kernel.org,
	Paul Menzel <pmenzel@molgen.mpg.de>,
	Ken Goldman <kgold@linux.ibm.com>,
	zohar@linux.vnet.ibm.com, why2jjj.linux@gmail.com,
	Hamza Attak <hamza@hpe.com>,
	gregkh@linuxfoundation.org, arnd@arndb.de,
	Nayna <nayna@linux.vnet.ibm.com>,
	James Bottomley <James.Bottomley@hansenpartnership.com>
Subject: Re: [PATCH] tpm: fix ATMEL TPM crash caused by too frequent queries
Date: Fri, 2 Jul 2021 10:45:18 +0300	[thread overview]
Message-ID: <20210702074518.64gyockmqrphbkqx@kernel.org> (raw)
In-Reply-To: <939BC11F-0905-4777-9DD7-630FC28ED205@rubrik.com>

On Fri, Jul 02, 2021 at 12:33:15AM -0700, Hao Wu wrote:
> 
> 
> > On Jul 1, 2021, at 11:35 PM, Jarkko Sakkinen <jarkko@kernel.org> wrote:
> > 
> > On Tue, Jun 29, 2021 at 09:22:05PM -0700, Hao Wu wrote:
> >> This is a fix for the ATMEL TPM crash bug reported in
> >> https://patchwork.kernel.org/project/linux-integrity/patch/20200926223150.109645-1-hao.wu@rubrik.com/
> >> 
> >> According to the discussions in the original thread,
> >> we don't want to revert the timeout of wait_for_tpm_stat
> >> for non-ATMEL chips, which brings back the performance cost.
> >> For investigation and analysis of why wait_for_tpm_stat
> >> caused the issue, and how the regression was introduced,
> >> please read the original thread above.
> >> 
> >> Thus the proposed fix here is to only revert the timeout
> >> for ATMEL chips by checking the vendor ID.
> >> 
> >> Signed-off-by: Hao Wu <hao.wu@rubrik.com>
> >> Fixes: 9f3fc7bcddcb ("tpm: replace msleep() with usleep_range() in TPM 1.2/2.0 generic drivers")
> > 
> > Fixes tag should be before SOB.
> > 
> >> ---
> >> Test Plan:
> >> - Run fixed kernel with ATMEL TPM chips and see crash
> >> has been fixed.
> >> - Run fixed kernel with non-ATMEL TPM chips, and confirm
> >> the timeout has not been changed.
> >> 
> >> drivers/char/tpm/tpm.h          |  9 ++++++++-
> >> drivers/char/tpm/tpm_tis_core.c | 19 +++++++++++++++++--
> >> include/linux/tpm.h             |  2 ++
> >> 3 files changed, 27 insertions(+), 3 deletions(-)
> >> 
> >> diff --git a/drivers/char/tpm/tpm.h b/drivers/char/tpm/tpm.h
> >> index 283f78211c3a..bc6aa7f9e119 100644
> >> --- a/drivers/char/tpm/tpm.h
> >> +++ b/drivers/char/tpm/tpm.h
> >> @@ -42,7 +42,9 @@ enum tpm_timeout {
> >> 	TPM_TIMEOUT_RANGE_US = 300,	/* usecs */
> >> 	TPM_TIMEOUT_POLL = 1,	/* msecs */
> >> 	TPM_TIMEOUT_USECS_MIN = 100,      /* usecs */
> >> -	TPM_TIMEOUT_USECS_MAX = 500      /* usecs */
> >> +	TPM_TIMEOUT_USECS_MAX = 500,	/* usecs */
> > 
> > What is this change?
> Need to add the tailing comma
> 
> > 
> >> +	TPM_TIMEOUT_WAIT_STAT = 500,	/* usecs */
> >> +	TPM_ATML_TIMEOUT_WAIT_STAT = 15000	/* usecs */
> >> };
> >> 
> >> /* TPM addresses */
> >> @@ -189,6 +191,11 @@ static inline void tpm_msleep(unsigned int delay_msec)
> >> 		     delay_msec * 1000);
> >> };
> >> 
> >> +static inline void tpm_usleep(unsigned int delay_usec)
> >> +{
> >> +	usleep_range(delay_usec - TPM_TIMEOUT_RANGE_US, delay_usec);
> >> +};
> > 
> > Please remove this, and open code.
> Ok, will do
> 
> >> +
> >> int tpm_chip_start(struct tpm_chip *chip);
> >> void tpm_chip_stop(struct tpm_chip *chip);
> >> struct tpm_chip *tpm_find_get_ops(struct tpm_chip *chip);
> >> diff --git a/drivers/char/tpm/tpm_tis_core.c b/drivers/char/tpm/tpm_tis_core.c
> >> index 55b9d3965ae1..9ddd4edfe1c2 100644
> >> --- a/drivers/char/tpm/tpm_tis_core.c
> >> +++ b/drivers/char/tpm/tpm_tis_core.c
> >> @@ -80,8 +80,12 @@ static int wait_for_tpm_stat(struct tpm_chip *chip, u8 mask,
> >> 		}
> >> 	} else {
> >> 		do {
> >> -			usleep_range(TPM_TIMEOUT_USECS_MIN,
> >> -				     TPM_TIMEOUT_USECS_MAX);
> >> +			if (chip->timeout_wait_stat && 
> >> +				chip->timeout_wait_stat >= TPM_TIMEOUT_WAIT_STAT) {
> >> +				tpm_usleep((unsigned int)(chip->timeout_wait_stat));
> >> +			} else {
> >> +				tpm_usleep((unsigned int)(TPM_TIMEOUT_WAIT_STAT));
> >> +			}
> > 
> > Invalid use of braces. Please read
> > 
> > https://www.kernel.org/doc/html/v5.13/process/coding-style.html
> > 
> > Why do you have to use this field conditionally anyway? Why doesn't
> > it always contain a legit value?
> The field is legit now, but doesn’t hurt to do addition check for robustness 
> to ensure no crash ? Just in case the value is updated below TPM_TIMEOUT_WAIT_STAT ? 
> 
> Can remove if we think it is not needed.

A simple question: why you use it conditionally? Can the field contain invalid value?

/Jarkko