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

[Hao Wu <hao.wu@rubrik.com>]

> On Jul 2, 2021, at 4:57 AM, Jarkko Sakkinen <jarkko@kernel.org> wrote:
> 
> On Fri, Jul 02, 2021 at 11:42:39AM +0300, Jarkko Sakkinen wrote:
>> On Fri, Jul 02, 2021 at 12:59:18AM -0700, Hao Wu wrote:
>>>> On Jul 2, 2021, at 12:45 AM, Jarkko Sakkinen <jarkko@kernel.org> wrote:
>>>> 
>>>> 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?
>>>> 
>>> There are two checks
>>> - chip->timeout_wait_stat >= TPM_TIMEOUT_WAIT_STAT
>>> It could be invalid when future developer set it to some value less than `TPM_TIMEOUT_USECS_MIN`,
>>> and crash the usleep 
>> 
>> I don't understand this. Why you don't set to appropriate value?
Ok, fair enough, I assume developers will test it anyway to ensure no crash. Will remove this check.

> What you should do, is to define two fields:
> 
> - tpm_timeout_min
> - tpm_timeout_max
> 
> And initialize these to TPM_TIMEOUT_USECS_MIN and TPM_TIMEOUT_USECS_MAX.
> 
> Then fixup those for Atmel (with a simple if-statement, switch-case is
> overkill).
Switch was more for extensibility when other vendor has similar issue,
but we can refactor when needed in the future. I can use if-statement for now.

> The way you work out things right now is broken:
> 
> 1. Before for non-Atmel: usleep_range(100, 500)
> 2. After for non-Atmel: usleep_range(200, 500)
I realized this in day-1, I think this range change does not matter much.
`TPM_TIMEOUT_RANGE_US=300` is already used in the codebase, I assume people define
such if for general use cases for usleep_range in TPM
But we can add two fields if that makes us more comfortable to strictly follow the current code
semantically.

> I.e. the patch changes code semantically that it should not touch in the
> first place.
> 
> /Jarkko
Hao