On Thu, Oct 31, 2024 at 01:36:46AM +0200, Jarkko Sakkinen wrote:
On Wed Oct 30, 2024 at 10:09 PM EET, Jerry Snitselaar wrote:
On Wed, Oct 30, 2024 at 12:36:47AM +0200, Jarkko Sakkinen wrote:
Setting TPM_CHIP_FLAG_SUSPENDED in the end of tpm_pm_suspend() can be racy according to the bug report, as this leaves window for tpm_hwrng_read() to be called while the operation is in progress. Move setting of the flag into the beginning.
Cc: stable@vger.kernel.org # v6.4+ Fixes: 99d464506255 ("tpm: Prevent hwrng from activating during resume") Reported-by: Mike Seo mikeseohyungjin@gmail.com Closes: https://bugzilla.kernel.org/show_bug.cgi?id=219383 Signed-off-by: Jarkko Sakkinen jarkko@kernel.org
drivers/char/tpm/tpm-interface.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/char/tpm/tpm-interface.c b/drivers/char/tpm/tpm-interface.c index 8134f002b121..3f96bc8b95df 100644 --- a/drivers/char/tpm/tpm-interface.c +++ b/drivers/char/tpm/tpm-interface.c @@ -370,6 +370,8 @@ int tpm_pm_suspend(struct device *dev) if (!chip) return -ENODEV;
- chip->flags |= TPM_CHIP_FLAG_SUSPENDED;
- if (chip->flags & TPM_CHIP_FLAG_ALWAYS_POWERED) goto suspended;
@@ -390,8 +392,6 @@ int tpm_pm_suspend(struct device *dev) } suspended:
- chip->flags |= TPM_CHIP_FLAG_SUSPENDED;
- if (rc) dev_err(dev, "Ignoring error %d while suspending\n", rc); return 0;
-- 2.47.0
Reviewed-by: Jerry Snitselaar jsnitsel@redhat.com
Thanks but I actually started to look at the function:
https://elixir.bootlin.com/linux/v6.11.5/source/drivers/char/tpm/tpm-interfa...
The absolutely safe-play way considering concurrency would be to do tpm_try_get_ops() before checking any flags. That way tpm_hwrng_read() is guaranteed not conflict.
So the way I would fix this instead would be to (untested wrote inline here):
int tpm_pm_suspend(struct device *dev) { struct tpm_chip *chip = dev_get_drvdata(dev); int rc = 0;
if (!chip) return -ENODEV;
rc = tpm_try_get_ops(chip); if (rc) { chip->flags = |= TPM_CHIP_FLAG_SUSPENDED; return rc; }
/* ... */
suspended: chip->flags |= TPM_CHIP_FLAG_SUSPENDED; tpm_put_ops(chip);
It does not really affect performance but guarantees that tpm_hwrng_read() is guaranteed either fully finish or never happens given that both sides take chip->lock.
So I'll put one more round of this and then this should be stable and fully fixed.
BR, Jarkko
Ah, yeah better to set it while it has the mutex. That should still be 'if (!rc)' after the tpm_try_get_ops() right? (I'm assuming that is just a transcription error).
Regards, Jerry