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

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

> On Jul 4, 2021, at 10:19 PM, Jarkko Sakkinen <jarkko@kernel.org> wrote:
> 
> On Fri, Jul 02, 2021 at 12:16:12PM -0700, Hao Wu wrote:
>> 
>>> 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.
> 
> Make things more fancy *only* when you actually need more fancy.
> 
>>> 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.
> 
> By saying that you are actually saying that *undocumented* semantic changes
> to the kernel code are fine as long as they don't change things "too much"
> 
> Are you serious about this?
Fair enough, I agree that keeping things as it avoid potential issues. Thanks for pointing this out!
> 
>> `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.
> 
> This has absolutely nothing to do with "comfortable". It's black and white
> wrong.
> 
> /Jarkko

I believe the comments are addressed in 
https://patchwork.kernel.org/project/linux-integrity/patch/20210704000754.1384-1-hao.wu@rubrik.com/

Have tested it with ATMEL 1.2 chip. 

Thanks
Hao