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;
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
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
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
On Thu, Oct 31, 2024 at 08:02:37AM -0700, Jerry Snitselaar wrote:
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
It has been a while since I've looked at TPM code. Since tpm_hwrng_read doesn't check the flag with the mutex held is there a point later where it will bail out if the suspend has occurred? I'm wondering if the check for the suspend flag in tpm_hwrng_read should be after the tpm_find_get_ops in tpm_get_random.
Regards, Jerry
On Thu Oct 31, 2024 at 5:28 PM EET, Jerry Snitselaar wrote:
On Thu, Oct 31, 2024 at 08:02:37AM -0700, Jerry Snitselaar wrote:
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
It has been a while since I've looked at TPM code. Since tpm_hwrng_read doesn't check the flag with the mutex held is there a point later where it will bail out if the suspend has occurred? I'm wondering if the check for the suspend flag in tpm_hwrng_read should be after the tpm_find_get_ops in tpm_get_random.
Right, I ignored that side in v2. Yeah, I agree that in both cases it would be best that all checks are done when the lock is taken.
It means open-coding tpm2_get_random() but I think it is anyway good idea (as tpm_get_random() is meant for outside callers).
Regards, Jerry
BR, Jarkko
On Thu Oct 31, 2024 at 5:02 PM EET, Jerry Snitselaar wrote:
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).
Can you check v2 of the patch? It misses the tpm_hwrng_read() change that you suggested. I think rc is checked there correctly but it is always possible that I overlook/ignore something...
So no tags for that since an update is still coming but just the parts that are already in it make sense.
Regards, Jerry
BR, Jarkko
linux-stable-mirror@lists.linaro.org
-
Jarkko Sakkinen -
Jerry Snitselaar