Unbalanced TPM2 HMAC session calls

PATCH In review Jonathan McDowell 4 participants 12 messages Mar 6, 2025 – Mar 7, 2025

Subsystems (2)

Keys-trusted TPM device driver

lore

Share & tools

lore

Jonathan McDowell

author

2025-03-06 15:15 UTC #0 lore raw

We're seeing a lot of:

tpm tpm0: auth session is active

messages in our logs. This is emitted (once per boot) by tpm2_start_auth_session() if the auth sessions is already active when it is called.

Investigating I think this is because tpm2_pcr_extend() calls tpm_buf_append_hmac_session() which sets TPM2_SA_CONTINUE_SESSION so tpm_buf_check_hmac_response() does not cleanup the auth session, but then doesn't call tpm2_end_auth_session().

Looking at tpm2_get_random() it uses TPM2_SA_CONTINUE_SESSION but *also* cleans up with tpm2_end_auth_session().

I'd be sending a patch proposing the addition of tpm2_end_auth_session() to the end of tpm2_pcr_extend() but I recall a bunch of discussion about trying to cache the HMAC session to improve IMA performance, so I don't know if perhaps we should be dropping the warning instead?

(As an aside, I'm not clear dropping the warning is enough, as I can't see where the session otherwise gets cleaned up other than by accident when the RNG tries to get more randomness.)

Signature · 1 line

I've got a trigger inside.

Mimi Zohar

maintainer

2025-03-06 18:59 UTC #1 lore raw

Quoted 13 lines · Jonathan McDowell

We're seeing a lot of:
tpm tpm0: auth session is active
messages in our logs. This is emitted (once per boot) by tpm2_start_auth_session() if the auth sessions is already active when it is called.
Investigating I think this is because tpm2_pcr_extend() calls tpm_buf_append_hmac_session() which sets TPM2_SA_CONTINUE_SESSION so tpm_buf_check_hmac_response() does not cleanup the auth session, but then doesn't call tpm2_end_auth_session().

Quoted 7 lines

Looking at tpm2_get_random() it uses TPM2_SA_CONTINUE_SESSION but *also* cleans up with tpm2_end_auth_session().
I'd be sending a patch proposing the addition of tpm2_end_auth_session() to the end of tpm2_pcr_extend() but I recall a bunch of discussion about trying to cache the HMAC session to improve IMA performance, so I don't know if perhaps we should be dropping the warning instead?

Hi Jonathan,

That suggestion was nixed. Instead the boot command line option "tpm.disable_pcr_integrity" was upstreamed to disable the HMAC for TPM extends. Refer to commit 27184f8905ba ("tpm: Opt-in in disable PCR integrity protection').

Mimi

Quoted 5 lines

(As an aside, I'm not clear dropping the warning is enough, as I can't see where the session otherwise gets cleaned up other than by accident when the RNG tries to get more randomness.)

James Bottomley

maintainer

2025-03-06 19:30 UTC #2 lore raw

Quoted 23 lines · Mimi Zohar

Quoted 13 lines · Jonathan McDowell
We're seeing a lot of:
tpm tpm0: auth session is active
messages in our logs. This is emitted (once per boot) by tpm2_start_auth_session() if the auth sessions is already active when it is called.
Investigating I think this is because tpm2_pcr_extend() calls tpm_buf_append_hmac_session() which sets TPM2_SA_CONTINUE_SESSION so tpm_buf_check_hmac_response() does not cleanup the auth session, but then doesn't call tpm2_end_auth_session().
Quoted 8 lines
Looking at tpm2_get_random() it uses TPM2_SA_CONTINUE_SESSION but *also* cleans up with tpm2_end_auth_session().
I'd be sending a patch proposing the addition of tpm2_end_auth_session() to the end of tpm2_pcr_extend() but I recall a bunch of discussion about trying to cache the HMAC session to improve IMA performance, so I don't know if perhaps we should be dropping the warning instead?

Quoted 3 lines

Hi Jonathan,
That suggestion was nixed.

Well it's also upstream as

https://web.git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=df745e25098dcb2f706399c0d06dd8d1bab6b6ec

even though it didn't work out for large numbers of extends. However, that commit introduced this message on a warn once behaviour, so it seems to be expected that this gets printed once for some reason. From a code flow point of view, the lazy session handling seems to be working correctly, so I think just dropping the warn is correct.

Regards,

James

Mimi Zohar

maintainer

2025-03-06 19:43 UTC #3 lore raw

Quoted 26 lines · James Bottomley

Quoted 25 lines · Mimi Zohar
Quoted 13 lines · Jonathan McDowell
We're seeing a lot of:
tpm tpm0: auth session is active
messages in our logs. This is emitted (once per boot) by tpm2_start_auth_session() if the auth sessions is already active when it is called.
Investigating I think this is because tpm2_pcr_extend() calls tpm_buf_append_hmac_session() which sets TPM2_SA_CONTINUE_SESSION so tpm_buf_check_hmac_response() does not cleanup the auth session, but then doesn't call tpm2_end_auth_session().
Quoted 8 lines
Looking at tpm2_get_random() it uses TPM2_SA_CONTINUE_SESSION but *also* cleans up with tpm2_end_auth_session().
I'd be sending a patch proposing the addition of tpm2_end_auth_session() to the end of tpm2_pcr_extend() but I recall a bunch of discussion about trying to cache the HMAC session to improve IMA performance, so I don't know if perhaps we should be dropping the warning instead?
Hi Jonathan,

Quoted 7 lines

Quoted 1 line
That suggestion was nixed.
Well it's also upstream as
https://web.git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=df745e25098dcb2f706399c0d06dd8d1bab6b6ec
even though it didn't work out for large numbers of extends.

Oh, right! That patch definitely helped with performance, just not enough. Without that patch, some of my test systems wouldn't boot.

Mimi

Quoted 10 lines

However,
that commit introduced this message on a warn once behaviour, so it seems to be expected that this gets printed once for some reason. From a code flow point of view, the lazy session handling seems to be working correctly, so I think just dropping the warn is correct.
Regards,
James

Jarkko Sakkinen

maintainer

2025-03-06 21:40 UTC #4 lore raw

In reply to James Bottomley

Quoted 30 lines · James Bottomley

Quoted 23 lines · Mimi Zohar
Quoted 13 lines · Jonathan McDowell
We're seeing a lot of:
tpm tpm0: auth session is active
messages in our logs. This is emitted (once per boot) by tpm2_start_auth_session() if the auth sessions is already active when it is called.
Investigating I think this is because tpm2_pcr_extend() calls tpm_buf_append_hmac_session() which sets TPM2_SA_CONTINUE_SESSION so tpm_buf_check_hmac_response() does not cleanup the auth session, but then doesn't call tpm2_end_auth_session().
Quoted 8 lines
Looking at tpm2_get_random() it uses TPM2_SA_CONTINUE_SESSION but *also* cleans up with tpm2_end_auth_session().
I'd be sending a patch proposing the addition of tpm2_end_auth_session() to the end of tpm2_pcr_extend() but I recall a bunch of discussion about trying to cache the HMAC session to improve IMA performance, so I don't know if perhaps we should be dropping the warning instead?
Quoted 3 lines
Hi Jonathan,
That suggestion was nixed.
Well it's also upstream as

Quoted 7 lines

https://web.git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=df745e25098dcb2f706399c0d06dd8d1bab6b6ec
even though it didn't work out for large numbers of extends. However, that commit introduced this message on a warn once behaviour, so it seems to be expected that this gets printed once for some reason. From a code flow point of view, the lazy session handling seems to be working correctly, so I think just dropping the warn is correct.

Quoted 4 lines

Regards,
James

BR, Jarkko

Jarkko Sakkinen

maintainer

2025-03-06 21:39 UTC #5 lore raw

In reply to Jonathan McDowell

Quoted 7 lines · Jonathan McDowell

We're seeing a lot of:
tpm tpm0: auth session is active
messages in our logs. This is emitted (once per boot) by tpm2_start_auth_session() if the auth sessions is already active when it is called.

It's by design actually:

https://web.git.kernel.org/pub/scm/linux/kernel/git/jarkko/linux-tpmdd.git/commit/?id=df745e25098dc

Quoted 8 lines

Investigating I think this is because tpm2_pcr_extend() calls tpm_buf_append_hmac_session() which sets TPM2_SA_CONTINUE_SESSION so tpm_buf_check_hmac_response() does not cleanup the auth session, but then doesn't call tpm2_end_auth_session().
Looking at tpm2_get_random() it uses TPM2_SA_CONTINUE_SESSION but *also* cleans up with tpm2_end_auth_session().

Quoted 8 lines

I'd be sending a patch proposing the addition of tpm2_end_auth_session() to the end of tpm2_pcr_extend() but I recall a bunch of discussion about trying to cache the HMAC session to improve IMA performance, so I don't know if perhaps we should be dropping the warning instead?
(As an aside, I'm not clear dropping the warning is enough, as I can't see where the session otherwise gets cleaned up other than by accident when the RNG tries to get more randomness.)

It would be appropriate action, or relaxing it into pr_debug().

Quoted 5 lines

J.
Signature · 1 line
I've got a trigger inside.

BR, Jarkko

Jonathan McDowell

author

2025-03-07 10:56 UTC #6 lore raw

PATCH 1/1 tpm: Drop warning when an auth session is active

Auth sessions are lazily flushed since commit df745e25098dc ("tpm:
Lazily flush the auth session"), so it's expected that we might try to start a new session when one is still active.

Signed-off-by Jonathan McDowell <[email protected]>

---
 drivers/char/tpm/tpm2-sessions.c | 1 -
 1 file changed, 1 deletion(-)

drivers/char/tpm/tpm2-sessions.c +0 -1

index b70165b588ec..2d2c192ebb14 100644
@@ -982,7 +982,6 @@ int tpm2_start_auth_session(struct tpm_chip *chip)
 	int rc;
 
 	if (chip->auth) {
-		dev_warn_once(&chip->dev, "auth session is active\n");
 		return 0;
 	}

Signature · 1 line

2.48.1

Jarkko Sakkinen

maintainer

2025-03-07 16:36 UTC #7 lore raw

Re: [PATCH] tpm: Drop warning when an auth session is active

Quoted 14 lines · Jonathan McDowell

Auth sessions are lazily flushed since commit df745e25098dc ("tpm:
Lazily flush the auth session"), so it's expected that we might try to start a new session when one is still active.
Signed-off-by Jonathan McDowell <[email protected]>
---
 drivers/char/tpm/tpm2-sessions.c | 1 -
 1 file changed, 1 deletion(-)
drivers/char/tpm/tpm2-sessions.c +0 -0
index b70165b588ec..2d2c192ebb14 100644

drivers/char/tpm/tpm2-sessions.c int tpm2_start_auth_session(struct tpm_chip *chip) · line 986

Quoted 6 lines

drivers/char/tpm/tpm2-sessions.c +0 -1

@@ -982,7 +982,6 @@ int tpm2_start_auth_session(struct tpm_chip *chip)
 	int rc;
 
 	if (chip->auth) {
-		dev_warn_once(&chip->dev, "auth session is active\n");
 		return 0;
 	}

OK so curly faces should be also removed but I can adjust this (if you don't mind), so we save all of us from trouble of going through additional review round?

Quoted 4 lines

Signature · 2 lines
-- 
2.48.1

BR, Jarkko

Jonathan McDowell

author

2025-03-07 17:25 UTC #8 lore raw

Re: [PATCH] tpm: Drop warning when an auth session is active

Quoted 17 lines · Jarkko Sakkinen

Quoted 16 lines · Jonathan McDowell
Auth sessions are lazily flushed since commit df745e25098dc ("tpm:
Lazily flush the auth session"), so it's expected that we might try to start a new session when one is still active.
Signed-off-by Jonathan McDowell <[email protected]>
---
 drivers/char/tpm/tpm2-sessions.c | 1 -
 1 file changed, 1 deletion(-)
drivers/char/tpm/tpm2-sessions.c +0 -0
index b70165b588ec..2d2c192ebb14 100644
@@ -982,7 +982,6 @@ int tpm2_start_auth_session(struct tpm_chip *chip)
 	int rc;
 

Quoted 8 lines

drivers/char/tpm/tpm2-sessions.c int tpm2_start_auth_session(struct tpm_chip *chip) · line 986
Quoted 4 lines
drivers/char/tpm/tpm2-sessions.c +0 -1
 	if (chip->auth) {
-		dev_warn_once(&chip->dev, "auth session is active\n");
 		return 0;
 	}
OK so curly faces should be also removed but I can adjust this (if you don't mind), so we save all of us from trouble of going through additional review round?

Doh! Shoulda caught that. Feel free to do the fix up.

Signature · 4 lines

   "Why? - because it's f***ing    |  .''`.  Debian GNU/Linux Developer
     there!" -- Edmund Hilary      | : :' :  Happy to accept PGP signed
                                   | `. `'   or encrypted mail - RSA
                                   |   `-    key on the keyservers.

Jarkko Sakkinen

maintainer

2025-03-07 19:49 UTC #9 lore raw

Re: [PATCH] tpm: Drop warning when an auth session is active

Quoted 27 lines · Jonathan McDowell

Quoted 17 lines · Jarkko Sakkinen
Quoted 16 lines · Jonathan McDowell
Auth sessions are lazily flushed since commit df745e25098dc ("tpm:
Lazily flush the auth session"), so it's expected that we might try to start a new session when one is still active.
Signed-off-by Jonathan McDowell <[email protected]>
---
 drivers/char/tpm/tpm2-sessions.c | 1 -
 1 file changed, 1 deletion(-)
drivers/char/tpm/tpm2-sessions.c +0 -0
index b70165b588ec..2d2c192ebb14 100644
@@ -982,7 +982,6 @@ int tpm2_start_auth_session(struct tpm_chip *chip)
 	int rc;
 
Quoted 8 lines
drivers/char/tpm/tpm2-sessions.c int tpm2_start_auth_session(struct tpm_chip *chip) · line 986
Quoted 4 lines
drivers/char/tpm/tpm2-sessions.c +0 -1
 	if (chip->auth) {
-		dev_warn_once(&chip->dev, "auth session is active\n");
 		return 0;
 	}
OK so curly faces should be also removed but I can adjust this (if you don't mind), so we save all of us from trouble of going through additional review round?

Quoted 1 line

Doh! Shoulda caught that. Feel free to do the fix up.

Sure np!

Quoted 1 line

J.

BR, Jarkko

Jonathan McDowell

author

2025-03-07 10:58 UTC #10 lore raw

PATCH 1/1 tpm: Lazily flush auth session when getting random data

In reply to Jarkko Sakkinen

From: Jonathan McDowell <[email protected]>

Lazy flushing of TPM auth sessions was introduced to speed up IMA measurments into the TPM. Make use of it in tpm2_get_random as well, which has the added benefit of not needlessly cleaning up the session that IMA is using when there are no userspace accesses taking place.

Command trace before for every call:

hwrng (0x00000161): 14 (52965242 ns)
hwrng (0x00000176): 48 (161612432 ns)
hwrng (0x00000165): 10 (2410494 ns)
hwrng (0x0000017B): 117 (70699883 ns)
hwrng (0x0000017B): 117 (70959666 ns)
hwrng (0x00000165): 10 (2756827 ns)

After, with repeated calls showing no setup:

hwrng (0x00000161): 14 (53044582 ns)
hwrng (0x00000176): 48 (160491333 ns)
hwrng (0x00000165): 10 (2408220 ns)
hwrng (0x0000017B): 117 (70695037 ns)
hwrng (0x0000017B): 117 (70994984 ns)
hwrng (0x0000017B): 117 (70195388 ns)
hwrng (0x0000017B): 117 (70973835 ns)

Signed-off-by Jonathan McDowell <[email protected]>

---
 drivers/char/tpm/tpm2-cmd.c | 1 -
 1 file changed, 1 deletion(-)

drivers/char/tpm/tpm2-cmd.c +0 -1

index dfdcbd009720..524d802ede26 100644
@@ -359,7 +359,6 @@ int tpm2_get_random(struct tpm_chip *chip, u8 *dest, size_t max)
 	} while (retries-- && total < max);
 
 	tpm_buf_destroy(&buf);
-	tpm2_end_auth_session(chip);
 
 	return total ? total : -EIO;
 out:

Signature · 1 line

2.48.1

Jarkko Sakkinen

maintainer Reviewed-by on v1 1/1

2025-03-07 16:34 UTC #11 lore raw

Re: [PATCH] tpm: Lazily flush auth session when getting random data

Quoted 46 lines · Jonathan McDowell

From: Jonathan McDowell <[email protected]>
Lazy flushing of TPM auth sessions was introduced to speed up IMA measurments into the TPM. Make use of it in tpm2_get_random as well, which has the added benefit of not needlessly cleaning up the session that IMA is using when there are no userspace accesses taking place.
Command trace before for every call:
hwrng (0x00000161): 14 (52965242 ns)
hwrng (0x00000176): 48 (161612432 ns)
hwrng (0x00000165): 10 (2410494 ns)
hwrng (0x0000017B): 117 (70699883 ns)
hwrng (0x0000017B): 117 (70959666 ns)
hwrng (0x00000165): 10 (2756827 ns)
After, with repeated calls showing no setup:
hwrng (0x00000161): 14 (53044582 ns)
hwrng (0x00000176): 48 (160491333 ns)
hwrng (0x00000165): 10 (2408220 ns)
hwrng (0x0000017B): 117 (70695037 ns)
hwrng (0x0000017B): 117 (70994984 ns)
hwrng (0x0000017B): 117 (70195388 ns)
hwrng (0x0000017B): 117 (70973835 ns)
Signed-off-by Jonathan McDowell <[email protected]>
---
 drivers/char/tpm/tpm2-cmd.c | 1 -
 1 file changed, 1 deletion(-)
drivers/char/tpm/tpm2-cmd.c +0 -1
index dfdcbd009720..524d802ede26 100644
@@ -359,7 +359,6 @@ int tpm2_get_random(struct tpm_chip *chip, u8 *dest, size_t max)
 	} while (retries-- && total < max);
 
 	tpm_buf_destroy(&buf);
-	tpm2_end_auth_session(chip);
 
 	return total ? total : -EIO;
 out:
Signature · 1 line
2.48.1

Thanks for this. It is a good catch! I'll apply this over the weekend when I apply Arm FF-A driver patches.

Reviewed-by Jarkko Sakkinen <[email protected]>

BR, Jarkko