tpm: set TPM_CHIP_FLAG_SUSPENDED early

PATCH In review Jarkko Sakkinen 2 participants 7 messages Oct 29, 2024 – Oct 31, 2024

Subsystems (3)

Stable branch Keys-trusted TPM device driver

lore

Share & tools

lore

Jarkko Sakkinen

author maintainer

2024-10-29 22:36 UTC #0 lore raw

PATCH 1/1 tpm: set TPM_CHIP_FLAG_SUSPENDED early

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 [email protected] # v6.4+

Fixes 99d464506255 ("tpm: Prevent hwrng from activating during resume")

Reported-by Mike Seo <[email protected]>

Closes https://bugzilla.kernel.org/show_bug.cgi?id=219383

Signed-off-by Jarkko Sakkinen <[email protected]>

---
 drivers/char/tpm/tpm-interface.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

drivers/char/tpm/tpm-interface.c +2 -2

index 8134f002b121..3f96bc8b95df 100644
@@ -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;

Signature · 1 line

2.47.0

Jerry Snitselaar

Reviewed-by on v1 1/1

2024-10-30 20:09 UTC #1 lore raw

Quoted 39 lines · Jarkko Sakkinen

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 [email protected] # v6.4+
Fixes 99d464506255 ("tpm: Prevent hwrng from activating during resume")
Reported-by Mike Seo <[email protected]>
Closes https://bugzilla.kernel.org/show_bug.cgi?id=219383
Signed-off-by Jarkko Sakkinen <[email protected]>
---
 drivers/char/tpm/tpm-interface.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)
drivers/char/tpm/tpm-interface.c +2 -2
index 8134f002b121..3f96bc8b95df 100644
@@ -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;
Signature · 1 line
2.47.0

Reviewed-by Jerry Snitselaar <[email protected]>

Jarkko Sakkinen

author maintainer

2024-10-30 23:36 UTC #2 lore raw

Quoted 31 lines · Jerry Snitselaar

Quoted 30 lines · Jarkko Sakkinen
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 [email protected] # v6.4+
Fixes 99d464506255 ("tpm: Prevent hwrng from activating during resume")
Reported-by Mike Seo <[email protected]>
Closes https://bugzilla.kernel.org/show_bug.cgi?id=219383
Signed-off-by Jarkko Sakkinen <[email protected]>
---
 drivers/char/tpm/tpm-interface.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)
drivers/char/tpm/tpm-interface.c +2 -0
index 8134f002b121..3f96bc8b95df 100644
@@ -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)
 	}
 

Quoted 11 lines

Quoted 9 lines
drivers/char/tpm/tpm-interface.c +0 -2
 suspended:
-	chip->flags |= TPM_CHIP_FLAG_SUSPENDED;
-
 	if (rc)
 		dev_err(dev, "Ignoring error %d while suspending\n", rc);
 	return 0;
Signature · 1 line
2.47.0
Reviewed-by Jerry Snitselaar <[email protected]>

Thanks but I actually started to look at the function:

https://elixir.bootlin.com/linux/v6.11.5/source/drivers/char/tpm/tpm-interface.c#L365

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

Jerry Snitselaar

2024-10-31 15:02 UTC #3 lore raw

Quoted 75 lines · Jarkko Sakkinen

Quoted 31 lines · Jerry Snitselaar
Quoted 30 lines · Jarkko Sakkinen
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 [email protected] # v6.4+
Fixes 99d464506255 ("tpm: Prevent hwrng from activating during resume")
Reported-by Mike Seo <[email protected]>
Closes https://bugzilla.kernel.org/show_bug.cgi?id=219383
Signed-off-by Jarkko Sakkinen <[email protected]>
---
 drivers/char/tpm/tpm-interface.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)
drivers/char/tpm/tpm-interface.c +2 -0
index 8134f002b121..3f96bc8b95df 100644
@@ -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)
 	}
 
Quoted 11 lines
Quoted 9 lines
drivers/char/tpm/tpm-interface.c +0 -2
 suspended:
-	chip->flags |= TPM_CHIP_FLAG_SUSPENDED;
-
 	if (rc)
 		dev_err(dev, "Ignoring error %d while suspending\n", rc);
 	return 0;
Signature · 1 line
2.47.0
Reviewed-by Jerry Snitselaar <[email protected]>
Thanks but I actually started to look at the function:
https://elixir.bootlin.com/linux/v6.11.5/source/drivers/char/tpm/tpm-interface.c#L365
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);

Quoted 8 lines

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

Jerry Snitselaar

2024-10-31 15:28 UTC #4 lore raw

Quoted 83 lines · Jerry Snitselaar

Quoted 82 lines · Jarkko Sakkinen
Quoted 31 lines · Jerry Snitselaar
Quoted 30 lines · Jarkko Sakkinen
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 [email protected] # v6.4+
Fixes 99d464506255 ("tpm: Prevent hwrng from activating during resume")
Reported-by Mike Seo <[email protected]>
Closes https://bugzilla.kernel.org/show_bug.cgi?id=219383
Signed-off-by Jarkko Sakkinen <[email protected]>
---
 drivers/char/tpm/tpm-interface.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)
drivers/char/tpm/tpm-interface.c +2 -0
index 8134f002b121..3f96bc8b95df 100644
@@ -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)
 	}
 
Quoted 11 lines
Quoted 9 lines
drivers/char/tpm/tpm-interface.c +0 -2
 suspended:
-	chip->flags |= TPM_CHIP_FLAG_SUSPENDED;
-
 	if (rc)
 		dev_err(dev, "Ignoring error %d while suspending\n", rc);
 	return 0;
Signature · 1 line
2.47.0
Reviewed-by Jerry Snitselaar <[email protected]>
Thanks but I actually started to look at the function:
https://elixir.bootlin.com/linux/v6.11.5/source/drivers/char/tpm/tpm-interface.c#L365
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.

Quoted 9 lines

Quoted 1 line
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

Jarkko Sakkinen

author maintainer

2024-10-31 15:59 UTC #5 lore raw

Quoted 94 lines · Jerry Snitselaar

Quoted 83 lines · Jerry Snitselaar
Quoted 82 lines · Jarkko Sakkinen
Quoted 31 lines · Jerry Snitselaar
Quoted 30 lines · Jarkko Sakkinen
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 [email protected] # v6.4+
Fixes 99d464506255 ("tpm: Prevent hwrng from activating during resume")
Reported-by Mike Seo <[email protected]>
Closes https://bugzilla.kernel.org/show_bug.cgi?id=219383
Signed-off-by Jarkko Sakkinen <[email protected]>
---
 drivers/char/tpm/tpm-interface.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)
drivers/char/tpm/tpm-interface.c +2 -0
index 8134f002b121..3f96bc8b95df 100644
@@ -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)
 	}
 
Quoted 11 lines
Quoted 9 lines
drivers/char/tpm/tpm-interface.c +0 -2
 suspended:
-	chip->flags |= TPM_CHIP_FLAG_SUSPENDED;
-
 	if (rc)
 		dev_err(dev, "Ignoring error %d while suspending\n", rc);
 	return 0;
Signature · 1 line
2.47.0
Reviewed-by Jerry Snitselaar <[email protected]>
Thanks but I actually started to look at the function:
https://elixir.bootlin.com/linux/v6.11.5/source/drivers/char/tpm/tpm-interface.c#L365
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.
Quoted 9 lines
Quoted 1 line
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

Quoted 5 lines

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).

Quoted 2 lines

Regards,
Jerry

BR, Jarkko

Jarkko Sakkinen

author maintainer

2024-10-31 16:02 UTC #6 lore raw

In reply to Jerry Snitselaar

Quoted 80 lines · Jerry Snitselaar

Quoted 79 lines · Jarkko Sakkinen
Quoted 31 lines · Jerry Snitselaar
Quoted 30 lines · Jarkko Sakkinen
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 [email protected] # v6.4+
Fixes 99d464506255 ("tpm: Prevent hwrng from activating during resume")
Reported-by Mike Seo <[email protected]>
Closes https://bugzilla.kernel.org/show_bug.cgi?id=219383
Signed-off-by Jarkko Sakkinen <[email protected]>
---
 drivers/char/tpm/tpm-interface.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)
drivers/char/tpm/tpm-interface.c +2 -0
index 8134f002b121..3f96bc8b95df 100644
@@ -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)
 	}
 
Quoted 11 lines
Quoted 9 lines
drivers/char/tpm/tpm-interface.c +0 -2
 suspended:
-	chip->flags |= TPM_CHIP_FLAG_SUSPENDED;
-
 	if (rc)
 		dev_err(dev, "Ignoring error %d while suspending\n", rc);
 	return 0;
Signature · 1 line
2.47.0
Reviewed-by Jerry Snitselaar <[email protected]>
Thanks but I actually started to look at the function:
https://elixir.bootlin.com/linux/v6.11.5/source/drivers/char/tpm/tpm-interface.c#L365
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.

Quoted 8 lines

Quoted 4 lines
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.

Quoted 3 lines

Regards,
Jerry

BR, Jarkko