On Sun, 2021-10-24 at 19:48 +0300, amirmizi6@xxxxxxxxx wrote:
> From: Amir Mizinski <amirmizi6@xxxxxxxxx>
>
> Currently, the driver polls the TPM_STS.stsValid field until TRUE; then it
> reads TPM_STS register again to verify only that TPM_STS.expect field is
> FALSE (i.e., it ignores TPM_STS.stsValid).
> Since TPM_STS.stsValid represents the TPM_STS.expect validity, a check of
> only one of these fields is wrong. Fix this condition so that both fields
> are checked in the same TPM_STS register read.
>
> Modify the signature of wait_for_tpm_stat(), adding an additional
> "result" parameter to its call.
> wait_for_tpm_stat() is now polling the TPM_STS with a mask and waits
> for the value in result. This modification adds the ability to check if
> certain TPM_STS bits have been cleared.
> For example, use the new parameter to check in status that TPM_STS_VALID
> is set and also that TPM_STS_EXPECT is zeroed. This prevents a racy
> check.
>
> Fixes: 27084efee0c3 ("tpm: driver for next generation TPM chips")
Where does this failure occur in practice? If nowhere, this can be
dropped, and the patch can be considered as a feature. Defining it
as a fix makes only sense, if it needs to be backported to stable
kernels. This requires something to be actually broken.
The commit message does not contain a real bug report. It is just
referring to the specification, which is not a workload.
> Suggested-by: Benoit Houyere <benoit.houyere@xxxxxx>
> Signed-off-by: Amir Mizinski <amirmizi6@xxxxxxxxx>
> ---
> drivers/char/tpm/tpm_tis_core.c | 59 ++++++++++++++++++-----------------------
> 1 file changed, 26 insertions(+), 33 deletions(-)
>
> diff --git a/drivers/char/tpm/tpm_tis_core.c b/drivers/char/tpm/tpm_tis_core.c
> index 69579ef..98de2fd 100644
> --- a/drivers/char/tpm/tpm_tis_core.c
> +++ b/drivers/char/tpm/tpm_tis_core.c
> @@ -44,9 +44,9 @@ static bool wait_for_tpm_stat_cond(struct tpm_chip *chip, u8 mask,
> return false;
> }
>
> -static int wait_for_tpm_stat(struct tpm_chip *chip, u8 mask,
> - unsigned long timeout, wait_queue_head_t *queue,
> - bool check_cancel)
> +static int wait_for_tpm_stat(struct tpm_chip *chip, u8 mask, u8 result,
> + unsigned long timeout,
> + wait_queue_head_t *queue, bool check_cancel)
I would consider renaming this as, given that you are changing the
signature anyway:
tpm_tis_wait_for_stat()
This would be more consistent with the other naming, and make e.g.
grepping kernel tree easier.
How did you end up to the name "result"? I have hard time deriving
from that name the actual semantics. E.g. "expected" would already
a way more sane name.
/Jarkko
