1. Patchew
  2. linux
  3. View series

[PATCH] tpm: do not start chip while suspended

Thadeu Lima de Souza Cascardo posted 1 patch 10 months, 1 week ago
There is a newer version of this series
drivers/char/tpm/tpm-chip.c      | 5 +++++
drivers/char/tpm/tpm-interface.c | 8 +-------
2 files changed, 6 insertions(+), 7 deletions(-)
[PATCH] tpm: do not start chip while suspended
Posted by Thadeu Lima de Souza Cascardo 10 months, 1 week ago 
tpm_chip_start may issue IO that should not be done while the chip is
suspended. In the particular case of I2C, it will issue the following
warning:

[35985.503769] ------------[ cut here ]------------
[35985.503771] i2c i2c-1: Transfer while suspended
[35985.503796] WARNING: CPU: 0 PID: 74 at drivers/i2c/i2c-core.h:56 __i2c_transfer+0xbe/0x810
[35985.503802] Modules linked in:
[35985.503808] CPU: 0 UID: 0 PID: 74 Comm: hwrng Tainted: G        W          6.13.0-next-20250203-00005-gfa0cb5642941 #19 9c3d7f78192f2d38e32010ac9c90fdc71109ef6f
[35985.503814] Tainted: [W]=WARN
[35985.503817] Hardware name: Google Morphius/Morphius, BIOS Google_Morphius.13434.858.0 10/26/2023
[35985.503819] RIP: 0010:__i2c_transfer+0xbe/0x810
[35985.503825] Code: 30 01 00 00 4c 89 f7 e8 40 fe d8 ff 48 8b 93 80 01 00 00 48 85 d2 75 03 49 8b 16 48 c7 c7 0a fb 7c a7 48 89 c6 e8 32 ad b0 fe <0f> 0b b8 94 ff ff ff e9 33 04 00 00 be 02 00 00 00 83 fd 02 0f 5
[35985.503828] RSP: 0018:ffffa106c0333d30 EFLAGS: 00010246
[35985.503833] RAX: 074ba64aa20f7000 RBX: ffff8aa4c1167120 RCX: 0000000000000000
[35985.503836] RDX: 0000000000000000 RSI: ffffffffa77ab0e4 RDI: 0000000000000001
[35985.503838] RBP: 0000000000000001 R08: 0000000000000001 R09: 0000000000000000
[35985.503841] R10: 0000000000000004 R11: 00000001000313d5 R12: ffff8aa4c10f1820
[35985.503843] R13: ffff8aa4c0e243c0 R14: ffff8aa4c1167250 R15: ffff8aa4c1167120
[35985.503846] FS:  0000000000000000(0000) GS:ffff8aa4eae00000(0000) knlGS:0000000000000000
[35985.503849] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[35985.503852] CR2: 00007fab0aaf1000 CR3: 0000000105328000 CR4: 00000000003506f0
[35985.503855] Call Trace:
[35985.503859]  <TASK>
[35985.503863]  ? __warn+0xd4/0x260
[35985.503868]  ? __i2c_transfer+0xbe/0x810
[35985.503874]  ? report_bug+0xf3/0x210
[35985.503882]  ? handle_bug+0x63/0xb0
[35985.503887]  ? exc_invalid_op+0x16/0x50
[35985.503892]  ? asm_exc_invalid_op+0x16/0x20
[35985.503904]  ? __i2c_transfer+0xbe/0x810
[35985.503913]  tpm_cr50_i2c_transfer_message+0x24/0xf0
[35985.503920]  tpm_cr50_i2c_read+0x8e/0x120
[35985.503928]  tpm_cr50_request_locality+0x75/0x170
[35985.503935]  tpm_chip_start+0x116/0x160
[35985.503942]  tpm_try_get_ops+0x57/0x90
[35985.503948]  tpm_find_get_ops+0x26/0xd0
[35985.503955]  tpm_get_random+0x2d/0x80
[35985.503960]  hwrng_fillfn+0x13b/0x2e0
[35985.503964]  ? __cfi_hwrng_fillfn+0x10/0x10
[35985.503969]  kthread+0x23d/0x250
[35985.503974]  ? srso_return_thunk+0x5/0x5f
[35985.503979]  ? lockdep_hardirqs_on+0x95/0x150
[35985.503985]  ? __cfi_kthread+0x10/0x10
[35985.503989]  ret_from_fork+0x44/0x50
[35985.503994]  ? __cfi_kthread+0x10/0x10
[35985.503998]  ret_from_fork_asm+0x11/0x20
[35985.504013]  </TASK>
[35985.504015] irq event stamp: 107462
[35985.504017] hardirqs last  enabled at (107461): [<ffffffffa6b82d41>] _raw_spin_unlock_irqrestore+0x31/0x60
[35985.504022] hardirqs last disabled at (107462): [<ffffffffa6b77309>] __schedule+0x159/0x16f0
[35985.504027] softirqs last  enabled at (104760): [<ffffffffa4f62ef5>] __irq_exit_rcu+0x75/0x160
[35985.504032] softirqs last disabled at (104755): [<ffffffffa4f62ef5>] __irq_exit_rcu+0x75/0x160
[35985.504036] ---[ end trace 0000000000000000 ]---
[35985.504126] tpm tpm0: i2c transfer failed (attempt 2/3): -108
[35985.504207] tpm tpm0: i2c transfer failed (attempt 3/3): -108
[35985.504395] tpm tpm0: i2c transfer failed (attempt 2/3): -108
[35985.504474] tpm tpm0: i2c transfer failed (attempt 3/3): -108

Test for the suspended flag inside tpm_try_get_ops while holding the chip
tpm_mutex before calling tpm_chip_start. That will also prevent
tpm_get_random from doing IO while the TPM is suspended.

Fixes: 9265fed6db60 ("tpm: Lock TPM chip in tpm_pm_suspend() first")
Signed-off-by: Thadeu Lima de Souza Cascardo <cascardo@igalia.com>
Cc: stable@vger.kernel.org
Cc: Jerry Snitselaar <jsnitsel@redhat.com>
Cc: Mike Seo <mikeseohyungjin@gmail.com>
Cc: Jarkko Sakkinen <jarkko@kernel.org>
---
 drivers/char/tpm/tpm-chip.c      | 5 +++++
 drivers/char/tpm/tpm-interface.c | 8 +-------
 2 files changed, 6 insertions(+), 7 deletions(-)

diff --git a/drivers/char/tpm/tpm-chip.c b/drivers/char/tpm/tpm-chip.c
index 7df7abaf3e526bf7e85ac9dfbaa1087a51d2ab7e..6db864696a583bf59c534ec8714900a6be7b5156 100644
--- a/drivers/char/tpm/tpm-chip.c
+++ b/drivers/char/tpm/tpm-chip.c
@@ -168,6 +168,11 @@ int tpm_try_get_ops(struct tpm_chip *chip)
 		goto out_ops;
 
 	mutex_lock(&chip->tpm_mutex);
+
+	/* tmp_chip_start may issue IO that is denied while suspended */
+	if (chip->flags & TPM_CHIP_FLAG_SUSPENDED)
+		goto out_lock;
+
 	rc = tpm_chip_start(chip);
 	if (rc)
 		goto out_lock;
diff --git a/drivers/char/tpm/tpm-interface.c b/drivers/char/tpm/tpm-interface.c
index b1daa0d7b341b1a4c71a200115f0d29d2e87512d..e6d786ce4e36970428b75d288a066e832c5b2af1 100644
--- a/drivers/char/tpm/tpm-interface.c
+++ b/drivers/char/tpm/tpm-interface.c
@@ -441,22 +441,16 @@ int tpm_get_random(struct tpm_chip *chip, u8 *out, size_t max)
 	if (!out || max > TPM_MAX_RNG_DATA)
 		return -EINVAL;
 
+	/* NULL will be returned if chip is suspended */
 	chip = tpm_find_get_ops(chip);
 	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;
 }

---
base-commit: 2014c95afecee3e76ca4a56956a936e23283f05b
change-id: 20250205-tpm-suspend-b22f745f3124

Best regards,
-- 
Thadeu Lima de Souza Cascardo <cascardo@igalia.com>
Re: [PATCH] tpm: do not start chip while suspended
Posted by Jarkko Sakkinen 10 months, 1 week ago 
On Wed, Feb 05, 2025 at 09:20:07AM -0300, Thadeu Lima de Souza Cascardo wrote:
> tpm_chip_start may issue IO that should not be done while the chip is
> suspended. In the particular case of I2C, it will issue the following
> warning:

The bug is legit but this description needs some rework:

"Checking TPM_CHIP_FLAG_SUSPENDED after the call to tpm_chip_find_get_ops()
can lead to a spurious tpm_chip_start_call()":

> [35985.503771] i2c i2c-1: Transfer while suspended
> [35985.503796] WARNING: CPU: 0 PID: 74 at drivers/i2c/i2c-core.h:56 __i2c_transfer+0xbe/0x810
> [35985.503802] Modules linked in:
> [35985.503808] CPU: 0 UID: 0 PID: 74 Comm: hwrng Tainted: G        W          6.13.0-next-20250203-00005-gfa0cb5642941 #19 9c3d7f78192f2d38e32010ac9c90fdc71109ef6f
> [35985.503814] Tainted: [W]=WARN
> [35985.503817] Hardware name: Google Morphius/Morphius, BIOS Google_Morphius.13434.858.0 10/26/2023
> [35985.503819] RIP: 0010:__i2c_transfer+0xbe/0x810
> [35985.503825] Code: 30 01 00 00 4c 89 f7 e8 40 fe d8 ff 48 8b 93 80 01 00 00 48 85 d2 75 03 49 8b 16 48 c7 c7 0a fb 7c a7 48 89 c6 e8 32 ad b0 fe <0f> 0b b8 94 ff ff ff e9 33 04 00 00 be 02 00 00 00 83 fd 02 0f 5
> [35985.503828] RSP: 0018:ffffa106c0333d30 EFLAGS: 00010246
> [35985.503833] RAX: 074ba64aa20f7000 RBX: ffff8aa4c1167120 RCX: 0000000000000000
> [35985.503836] RDX: 0000000000000000 RSI: ffffffffa77ab0e4 RDI: 0000000000000001
> [35985.503838] RBP: 0000000000000001 R08: 0000000000000001 R09: 0000000000000000
> [35985.503841] R10: 0000000000000004 R11: 00000001000313d5 R12: ffff8aa4c10f1820
> [35985.503843] R13: ffff8aa4c0e243c0 R14: ffff8aa4c1167250 R15: ffff8aa4c1167120
> [35985.503846] FS:  0000000000000000(0000) GS:ffff8aa4eae00000(0000) knlGS:0000000000000000
> [35985.503849] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> [35985.503852] CR2: 00007fab0aaf1000 CR3: 0000000105328000 CR4: 00000000003506f0
> [35985.503855] Call Trace:
> [35985.503859]  <TASK>
> [35985.503863]  ? __warn+0xd4/0x260
> [35985.503868]  ? __i2c_transfer+0xbe/0x810
> [35985.503874]  ? report_bug+0xf3/0x210
> [35985.503882]  ? handle_bug+0x63/0xb0
> [35985.503887]  ? exc_invalid_op+0x16/0x50
> [35985.503892]  ? asm_exc_invalid_op+0x16/0x20
> [35985.503904]  ? __i2c_transfer+0xbe/0x810
> [35985.503913]  tpm_cr50_i2c_transfer_message+0x24/0xf0
> [35985.503920]  tpm_cr50_i2c_read+0x8e/0x120
> [35985.503928]  tpm_cr50_request_locality+0x75/0x170
> [35985.503935]  tpm_chip_start+0x116/0x160

Only put this snippe to the commit message.

> Test for the suspended flag inside tpm_try_get_ops while holding the chip
> tpm_mutex before calling tpm_chip_start. That will also prevent
> tpm_get_random from doing IO while the TPM is suspended.

Remove and:

"Don't move forward with tpm_chip_start() inside tpm_chip_find_get(),
unless TPM_CHIP_FLAG_SUSPENDED is set. Return NULl in the failure
case."

> 
> Fixes: 9265fed6db60 ("tpm: Lock TPM chip in tpm_pm_suspend() first")
> Signed-off-by: Thadeu Lima de Souza Cascardo <cascardo@igalia.com>
> Cc: stable@vger.kernel.org
> Cc: Jerry Snitselaar <jsnitsel@redhat.com>
> Cc: Mike Seo <mikeseohyungjin@gmail.com>
> Cc: Jarkko Sakkinen <jarkko@kernel.org>
> ---
>  drivers/char/tpm/tpm-chip.c      | 5 +++++
>  drivers/char/tpm/tpm-interface.c | 8 +-------
>  2 files changed, 6 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/char/tpm/tpm-chip.c b/drivers/char/tpm/tpm-chip.c
> index 7df7abaf3e526bf7e85ac9dfbaa1087a51d2ab7e..6db864696a583bf59c534ec8714900a6be7b5156 100644
> --- a/drivers/char/tpm/tpm-chip.c
> +++ b/drivers/char/tpm/tpm-chip.c
> @@ -168,6 +168,11 @@ int tpm_try_get_ops(struct tpm_chip *chip)
>  		goto out_ops;
>  
>  	mutex_lock(&chip->tpm_mutex);
> +
> +	/* tmp_chip_start may issue IO that is denied while suspended */
> +	if (chip->flags & TPM_CHIP_FLAG_SUSPENDED)
> +		goto out_lock;
> +
>  	rc = tpm_chip_start(chip);
>  	if (rc)
>  		goto out_lock;
> diff --git a/drivers/char/tpm/tpm-interface.c b/drivers/char/tpm/tpm-interface.c
> index b1daa0d7b341b1a4c71a200115f0d29d2e87512d..e6d786ce4e36970428b75d288a066e832c5b2af1 100644
> --- a/drivers/char/tpm/tpm-interface.c
> +++ b/drivers/char/tpm/tpm-interface.c
> @@ -441,22 +441,16 @@ int tpm_get_random(struct tpm_chip *chip, u8 *out, size_t max)
>  	if (!out || max > TPM_MAX_RNG_DATA)
>  		return -EINVAL;
>  
> +	/* NULL will be returned if chip is suspended */

spurious diff, remove

>  	chip = tpm_find_get_ops(chip);
>  	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;
>  }
> 
> ---
> base-commit: 2014c95afecee3e76ca4a56956a936e23283f05b
> change-id: 20250205-tpm-suspend-b22f745f3124
> 
> Best regards,
> -- 
> Thadeu Lima de Souza Cascardo <cascardo@igalia.com>
> 

BR, Jarkko

Patchew 2.1-devel-b67e4c2

© 2016 - 2025 Red Hat, Inc.