tpm: Lock TPM chip in tpm_pm_suspend() first

Setting TPM_CHIP_FLAG_SUSPENDED in the end of tpm_pm_suspend() can be racy according, as this leaves window for tpm_hwrng_read() to be called while the operation is in progress. The recent bug report gives also evidence of this behaviour.

Aadress this by locking the TPM chip before checking any chip->flags both in tpm_pm_suspend() and tpm_hwrng_read(). Move TPM_CHIP_FLAG_SUSPENDED check inside tpm_get_random() so that it will be always checked only when the lock is reserved.

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]>

cmake -Bbuild -Dbuildroot_defconfig=busybox_x86_64_defconfig && make -Cbuild buildroot-prepare make -Cbuild/buildroot/build
pushd build/buildroot/build
images/run-qemu.sh &
socat - UNIX-CONNECT:images/serial.sock

Setting TPM_CHIP_FLAG_SUSPENDED in the end of tpm_pm_suspend() can be racy according, as this leaves window for tpm_hwrng_read() to be called while the operation is in progress. The recent bug report gives also evidence of this behaviour.

Aadress this by locking the TPM chip before checking any chip->flags both in tpm_pm_suspend() and tpm_hwrng_read(). Move TPM_CHIP_FLAG_SUSPENDED check inside tpm_get_random() so that it will be always checked only when the lock is reserved.

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]>

---
v3:
- Check TPM_CHIP_FLAG_SUSPENDED inside tpm_get_random() so that it is also done under the lock (suggested by Jerry Snitselaar).
v2:
- Addressed my own remark:
https://lore.kernel.org/linux-integrity/[email protected]/

---
 drivers/char/tpm/tpm-chip.c      |  4 ----
 drivers/char/tpm/tpm-interface.c | 32 ++++++++++++++++++++++----------
 2 files changed, 22 insertions(+), 14 deletions(-)

drivers/char/tpm/tpm-chip.c +0 -4

index 1ff99a7091bb..7df7abaf3e52 100644
@@ -525,10 +525,6 @@ static int tpm_hwrng_read(struct hwrng *rng, void *data, size_t max, bool wait)
 {
 	struct tpm_chip *chip = container_of(rng, struct tpm_chip, hwrng);
 
-	/* Give back zero bytes, as TPM chip has not yet fully resumed: */
-	if (chip->flags & TPM_CHIP_FLAG_SUSPENDED)
-		return 0;
-
 	return tpm_get_random(chip, data, max);
 }
 

drivers/char/tpm/tpm-interface.c +7 -8

index 8134f002b121..b1daa0d7b341 100644
@@ -370,6 +370,13 @@ int tpm_pm_suspend(struct device *dev)
 	if (!chip)
 		return -ENODEV;
 
+	rc = tpm_try_get_ops(chip);
+	if (rc) {
+		/* Can be safely set out of locks, as no action cannot race: */
+		chip->flags |= TPM_CHIP_FLAG_SUSPENDED;
+		goto out;
+	}
+
 	if (chip->flags & TPM_CHIP_FLAG_ALWAYS_POWERED)
 		goto suspended;
 

@@ -377,21 +384,19 @@ int tpm_pm_suspend(struct device *dev)
 	    !pm_suspend_via_firmware())
 		goto suspended;
 
-	rc = tpm_try_get_ops(chip);
-	if (!rc) {
-		if (chip->flags & TPM_CHIP_FLAG_TPM2) {
-			tpm2_end_auth_session(chip);
-			tpm2_shutdown(chip, TPM2_SU_STATE);
-		} else {
-			rc = tpm1_pm_suspend(chip, tpm_suspend_pcr);
-		}

drivers/char/tpm/tpm-interface.c +9 -0

 suspended:
 	chip->flags |= TPM_CHIP_FLAG_SUSPENDED;
+	tpm_put_ops(chip);
 
+out:
 	if (rc)
 		dev_err(dev, "Ignoring error %d while suspending\n", rc);
 	return 0;

@@ -440,11 +445,18 @@ int tpm_get_random(struct tpm_chip *chip, u8 *out, size_t max)
 	if (!chip)
 		return -ENODEV;
 
+	/* Give back zero bytes, as TPM chip has not yet fully resumed: */
+	if (chip->flags & TPM_CHIP_FLAG_SUSPENDED) {
+		rc = 0;
+		goto out;
+	}
+
 	if (chip->flags & TPM_CHIP_FLAG_TPM2)
 		rc = tpm2_get_random(chip, out, max);
 	else
 		rc = tpm1_get_random(chip, out, max);
 
+out:
 	tpm_put_ops(chip);
 	return rc;
 }

Signature · 1 line

2.47.0

Quoted 72 lines · Jarkko Sakkinen

Setting TPM_CHIP_FLAG_SUSPENDED in the end of tpm_pm_suspend() can be racy according, as this leaves window for tpm_hwrng_read() to be called while the operation is in progress. The recent bug report gives also evidence of this behaviour.

Aadress this by locking the TPM chip before checking any chip->flags both in tpm_pm_suspend() and tpm_hwrng_read(). Move TPM_CHIP_FLAG_SUSPENDED check inside tpm_get_random() so that it will be always checked only when the lock is reserved.

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]>

---
v3:
- Check TPM_CHIP_FLAG_SUSPENDED inside tpm_get_random() so that it is also done under the lock (suggested by Jerry Snitselaar).
v2:
- Addressed my own remark:
https://lore.kernel.org/linux-integrity/[email protected]/

---
 drivers/char/tpm/tpm-chip.c      |  4 ----
 drivers/char/tpm/tpm-interface.c | 32 ++++++++++++++++++++++----------
 2 files changed, 22 insertions(+), 14 deletions(-)

drivers/char/tpm/tpm-chip.c +0 -4

index 1ff99a7091bb..7df7abaf3e52 100644
@@ -525,10 +525,6 @@ static int tpm_hwrng_read(struct hwrng *rng, void *data, size_t max, bool wait)
 {
 	struct tpm_chip *chip = container_of(rng, struct tpm_chip, hwrng);
 
-	/* Give back zero bytes, as TPM chip has not yet fully resumed: */
-	if (chip->flags & TPM_CHIP_FLAG_SUSPENDED)
-		return 0;
-
 	return tpm_get_random(chip, data, max);
 }
 

drivers/char/tpm/tpm-interface.c +7 -8

index 8134f002b121..b1daa0d7b341 100644
@@ -370,6 +370,13 @@ int tpm_pm_suspend(struct device *dev)
 	if (!chip)
 		return -ENODEV;
 
+	rc = tpm_try_get_ops(chip);
+	if (rc) {
+		/* Can be safely set out of locks, as no action cannot race: */
+		chip->flags |= TPM_CHIP_FLAG_SUSPENDED;
+		goto out;
+	}
+
 	if (chip->flags & TPM_CHIP_FLAG_ALWAYS_POWERED)
 		goto suspended;
 

@@ -377,21 +384,19 @@ int tpm_pm_suspend(struct device *dev)
 	    !pm_suspend_via_firmware())
 		goto suspended;
 
-	rc = tpm_try_get_ops(chip);
-	if (!rc) {
-		if (chip->flags & TPM_CHIP_FLAG_TPM2) {
-			tpm2_end_auth_session(chip);
-			tpm2_shutdown(chip, TPM2_SU_STATE);
-		} else {
-			rc = tpm1_pm_suspend(chip, tpm_suspend_pcr);
-		}

drivers/char/tpm/tpm-interface.c int tpm_pm_suspend(struct device *dev) · line 394

Quoted 10 lines

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

-
-		tpm_put_ops(chip);
+	if (chip->flags & TPM_CHIP_FLAG_TPM2) {
+		tpm2_end_auth_session(chip);
+		tpm2_shutdown(chip, TPM2_SU_STATE);
+		goto suspended;
 	}
 
+	rc = tpm1_pm_suspend(chip, tpm_suspend_pcr);
+

I imagine the above still be wrapped in an else with the if (chip->flags & TPM_CHIP_FLAG_TPM2) otherwise it will call tpm1_pm_suspend for both tpm1 and tpm2 devices, yes?

So:

	if (chip->flags & TPM_CHIP_FLAG_TPM2) {
		tpm2_end_auth_session(chip);
		tpm2_shutdown(chip, TPM2_SU_STATE);
		goto suspended;
	} else {
		rc = tpm1_pm_suspend(chip, tpm_suspend_pcr);
	}

Other than that I think it looks good.

Quoted 99 lines · Jerry Snitselaar

Quoted 72 lines · Jarkko Sakkinen

Setting TPM_CHIP_FLAG_SUSPENDED in the end of tpm_pm_suspend() can be racy according, as this leaves window for tpm_hwrng_read() to be called while the operation is in progress. The recent bug report gives also evidence of this behaviour.

Aadress this by locking the TPM chip before checking any chip->flags both in tpm_pm_suspend() and tpm_hwrng_read(). Move TPM_CHIP_FLAG_SUSPENDED check inside tpm_get_random() so that it will be always checked only when the lock is reserved.

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]>

---
v3:
- Check TPM_CHIP_FLAG_SUSPENDED inside tpm_get_random() so that it is also done under the lock (suggested by Jerry Snitselaar).
v2:
- Addressed my own remark:
https://lore.kernel.org/linux-integrity/[email protected]/

---
 drivers/char/tpm/tpm-chip.c      |  4 ----
 drivers/char/tpm/tpm-interface.c | 32 ++++++++++++++++++++++----------
 2 files changed, 22 insertions(+), 14 deletions(-)

drivers/char/tpm/tpm-chip.c +0 -4

index 1ff99a7091bb..7df7abaf3e52 100644
@@ -525,10 +525,6 @@ static int tpm_hwrng_read(struct hwrng *rng, void *data, size_t max, bool wait)
 {
 	struct tpm_chip *chip = container_of(rng, struct tpm_chip, hwrng);
 
-	/* Give back zero bytes, as TPM chip has not yet fully resumed: */
-	if (chip->flags & TPM_CHIP_FLAG_SUSPENDED)
-		return 0;
-
 	return tpm_get_random(chip, data, max);
 }
 

drivers/char/tpm/tpm-interface.c +7 -8

index 8134f002b121..b1daa0d7b341 100644
@@ -370,6 +370,13 @@ int tpm_pm_suspend(struct device *dev)
 	if (!chip)
 		return -ENODEV;
 
+	rc = tpm_try_get_ops(chip);
+	if (rc) {
+		/* Can be safely set out of locks, as no action cannot race: */
+		chip->flags |= TPM_CHIP_FLAG_SUSPENDED;
+		goto out;
+	}
+
 	if (chip->flags & TPM_CHIP_FLAG_ALWAYS_POWERED)
 		goto suspended;
 

@@ -377,21 +384,19 @@ int tpm_pm_suspend(struct device *dev)
 	    !pm_suspend_via_firmware())
 		goto suspended;
 
-	rc = tpm_try_get_ops(chip);
-	if (!rc) {
-		if (chip->flags & TPM_CHIP_FLAG_TPM2) {
-			tpm2_end_auth_session(chip);
-			tpm2_shutdown(chip, TPM2_SU_STATE);
-		} else {
-			rc = tpm1_pm_suspend(chip, tpm_suspend_pcr);
-		}

drivers/char/tpm/tpm-interface.c int tpm_pm_suspend(struct device *dev) · line 394

Quoted 10 lines

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

-
-		tpm_put_ops(chip);
+	if (chip->flags & TPM_CHIP_FLAG_TPM2) {
+		tpm2_end_auth_session(chip);
+		tpm2_shutdown(chip, TPM2_SU_STATE);
+		goto suspended;
 	}
 
+	rc = tpm1_pm_suspend(chip, tpm_suspend_pcr);
+

I imagine the above still be wrapped in an else with the if (chip->flags & TPM_CHIP_FLAG_TPM2) otherwise it will call tpm1_pm_suspend for both tpm1 and tpm2 devices, yes?

So:

	if (chip->flags & TPM_CHIP_FLAG_TPM2) {
		tpm2_end_auth_session(chip);
		tpm2_shutdown(chip, TPM2_SU_STATE);
		goto suspended;
	} else {
		rc = tpm1_pm_suspend(chip, tpm_suspend_pcr);
	}

Quoted 1 line

Other than that I think it looks good.

It should be fine because after tpm2_shutdown() is called there is "goto suspended;". This is IMHO more readable as it matches the structure of previous exits before it. In future if this needs to be improved it will easier to move the logic to a helper function (e.g. __tpm_pm_suspend()) where gotos are substituted with return-statements.

BR, Jarkko

Quoted 108 lines · Jarkko Sakkinen

Quoted 99 lines · Jerry Snitselaar

Quoted 72 lines · Jarkko Sakkinen

Setting TPM_CHIP_FLAG_SUSPENDED in the end of tpm_pm_suspend() can be racy according, as this leaves window for tpm_hwrng_read() to be called while the operation is in progress. The recent bug report gives also evidence of this behaviour.

Aadress this by locking the TPM chip before checking any chip->flags both in tpm_pm_suspend() and tpm_hwrng_read(). Move TPM_CHIP_FLAG_SUSPENDED check inside tpm_get_random() so that it will be always checked only when the lock is reserved.

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]>

---
v3:
- Check TPM_CHIP_FLAG_SUSPENDED inside tpm_get_random() so that it is also done under the lock (suggested by Jerry Snitselaar).
v2:
- Addressed my own remark:
https://lore.kernel.org/linux-integrity/[email protected]/

---
 drivers/char/tpm/tpm-chip.c      |  4 ----
 drivers/char/tpm/tpm-interface.c | 32 ++++++++++++++++++++++----------
 2 files changed, 22 insertions(+), 14 deletions(-)

drivers/char/tpm/tpm-chip.c +0 -4

index 1ff99a7091bb..7df7abaf3e52 100644
@@ -525,10 +525,6 @@ static int tpm_hwrng_read(struct hwrng *rng, void *data, size_t max, bool wait)
 {
 	struct tpm_chip *chip = container_of(rng, struct tpm_chip, hwrng);
 
-	/* Give back zero bytes, as TPM chip has not yet fully resumed: */
-	if (chip->flags & TPM_CHIP_FLAG_SUSPENDED)
-		return 0;
-
 	return tpm_get_random(chip, data, max);
 }
 

drivers/char/tpm/tpm-interface.c +7 -8

index 8134f002b121..b1daa0d7b341 100644
@@ -370,6 +370,13 @@ int tpm_pm_suspend(struct device *dev)
 	if (!chip)
 		return -ENODEV;
 
+	rc = tpm_try_get_ops(chip);
+	if (rc) {
+		/* Can be safely set out of locks, as no action cannot race: */
+		chip->flags |= TPM_CHIP_FLAG_SUSPENDED;
+		goto out;
+	}
+
 	if (chip->flags & TPM_CHIP_FLAG_ALWAYS_POWERED)
 		goto suspended;
 

@@ -377,21 +384,19 @@ int tpm_pm_suspend(struct device *dev)
 	    !pm_suspend_via_firmware())
 		goto suspended;
 
-	rc = tpm_try_get_ops(chip);
-	if (!rc) {
-		if (chip->flags & TPM_CHIP_FLAG_TPM2) {
-			tpm2_end_auth_session(chip);
-			tpm2_shutdown(chip, TPM2_SU_STATE);
-		} else {
-			rc = tpm1_pm_suspend(chip, tpm_suspend_pcr);
-		}

drivers/char/tpm/tpm-interface.c int tpm_pm_suspend(struct device *dev) · line 394

Quoted 10 lines

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

-
-		tpm_put_ops(chip);
+	if (chip->flags & TPM_CHIP_FLAG_TPM2) {
+		tpm2_end_auth_session(chip);
+		tpm2_shutdown(chip, TPM2_SU_STATE);
+		goto suspended;
 	}
 
+	rc = tpm1_pm_suspend(chip, tpm_suspend_pcr);
+

I imagine the above still be wrapped in an else with the if (chip->flags & TPM_CHIP_FLAG_TPM2) otherwise it will call tpm1_pm_suspend for both tpm1 and tpm2 devices, yes?

So:

	if (chip->flags & TPM_CHIP_FLAG_TPM2) {
		tpm2_end_auth_session(chip);
		tpm2_shutdown(chip, TPM2_SU_STATE);
		goto suspended;
	} else {
		rc = tpm1_pm_suspend(chip, tpm_suspend_pcr);
	}

Quoted 1 line

Other than that I think it looks good.

It should be fine because after tpm2_shutdown() is called there is "goto suspended;". This is IMHO more readable as it matches the structure of previous exits before it. In future if this needs to be improved it will easier to move the logic to a helper function (e.g. __tpm_pm_suspend()) where gotos are substituted with return-statements.

Quoted 2 lines

BR, Jarkko

Heh, yep.

Reviewed-by Jerry Snitselaar <[email protected]>