| Message ID | 20221101020352.939691-2-jsd@semihalf.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | char: tpm: Adjust cr50_i2c locking mechanism | expand |
On Mon, Oct 31, 2022 at 8:04 PM Jan Dabros <jsd@semihalf.com> wrote: > > - if (!tpm_chip_start(chip)) { > + rc = tpm_try_get_ops(chip); > + if (!rc) { > if (chip->flags & TPM_CHIP_FLAG_TPM2) > tpm2_shutdown(chip, TPM2_SU_STATE); > else > rc = tpm1_pm_suspend(chip, tpm_suspend_pcr); This if-else block is still interacting with the TPM even though you're not guaranteed to have the lock, which could lead to racy/inchorent results. Would it be better to just bail out entirely since we can't safely attempt any recovery at this point. If it's still worth attempting the shutdown command, it would at least be good to add a comment admitting that we have no choice but to communicate with the TPM without a lock.
> > - if (!tpm_chip_start(chip)) { > > + rc = tpm_try_get_ops(chip); > > + if (!rc) { > > if (chip->flags & TPM_CHIP_FLAG_TPM2) > > tpm2_shutdown(chip, TPM2_SU_STATE); > > else > > rc = tpm1_pm_suspend(chip, tpm_suspend_pcr); > > This if-else block is still interacting with the TPM even though > you're not guaranteed to have the lock, which could lead to > racy/inchorent results. Would it be better to just bail out entirely > since we can't safely attempt any recovery at this point. If it's > still worth attempting the shutdown command, it would at least be good > to add a comment admitting that we have no choice but to communicate > with the TPM without a lock. If tpm_try_get_ops() returns 0 it means that we have a lock. And if we don't have a lock, then we are not executing any TPM commands. Are you referring to tpm_mutex or something different? Best Regards, Jan
On Wed, Nov 2, 2022 at 3:28 PM Jan Dąbroś <jsd@semihalf.com> wrote: > > > > - if (!tpm_chip_start(chip)) { > > > + rc = tpm_try_get_ops(chip); > > > + if (!rc) { > > > if (chip->flags & TPM_CHIP_FLAG_TPM2) > > > tpm2_shutdown(chip, TPM2_SU_STATE); > > > else > > > rc = tpm1_pm_suspend(chip, tpm_suspend_pcr); > > > > This if-else block is still interacting with the TPM even though > > you're not guaranteed to have the lock, which could lead to > > racy/inchorent results. Would it be better to just bail out entirely > > since we can't safely attempt any recovery at this point. If it's > > still worth attempting the shutdown command, it would at least be good > > to add a comment admitting that we have no choice but to communicate > > with the TPM without a lock. > > If tpm_try_get_ops() returns 0 it means that we have a lock. And if we > don't have a lock, then we are not executing any TPM commands. Are you > referring to tpm_mutex or something different? Ah, yup, I was reading this backwards, thinking that something had gone wrong when entering this block. Nevermind.
diff --git a/drivers/char/tpm/tpm-interface.c b/drivers/char/tpm/tpm-interface.c index 1621ce8187052..d69905233aff2 100644 --- a/drivers/char/tpm/tpm-interface.c +++ b/drivers/char/tpm/tpm-interface.c @@ -401,13 +401,14 @@ int tpm_pm_suspend(struct device *dev) !pm_suspend_via_firmware()) goto suspended; - if (!tpm_chip_start(chip)) { + rc = tpm_try_get_ops(chip); + if (!rc) { if (chip->flags & TPM_CHIP_FLAG_TPM2) tpm2_shutdown(chip, TPM2_SU_STATE); else rc = tpm1_pm_suspend(chip, tpm_suspend_pcr); - tpm_chip_stop(chip); + tpm_put_ops(chip); } suspended:
Currently tpm transactions are executed unconditionally in tpm_pm_suspend() function, what may lead to races with other tpm accessors in the system. Add proper locking mechanisms by calling tpm_try_get_ops() which is a wrapper on tpm_chip_start(). Signed-off-by: Jan Dabros <jsd@semihalf.com> --- drivers/char/tpm/tpm-interface.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-)