Lazy flush for the auth session
Subsystems (5)
Jarkko Sakkinen
maintainerv8 cover Lazy flush for the auth session
v7:
https://lore.kernel.org/linux-integrity/[email protected]/
v6:
https://lore.kernel.org/linux-integrity/[email protected]/T/#t
v5:
https://lore.kernel.org/linux-integrity/[email protected]/T/#m527d0466a02abaa448720999ff055de0540e7bb7
v4:
https://lore.kernel.org/linux-integrity/[email protected]/
v3:
https://lore.kernel.org/linux-integrity/[email protected]/
v2:
https://lore.kernel.org/linux-integrity/[email protected]/
v1:
https://lore.kernel.org/linux-integrity/[email protected]/
Jarkko Sakkinen (3):
tpm: Return tpm2_sessions_init() when null key creation fails
tpm: Rollback tpm2_load_null()
tpm: Lazily flush the auth session
drivers/char/tpm/tpm-chip.c | 10 +++ drivers/char/tpm/tpm-dev-common.c | 3 + drivers/char/tpm/tpm-interface.c | 6 +- drivers/char/tpm/tpm2-sessions.c | 100 ++++++++++++++++++------------ 4 files changed, 77 insertions(+), 42 deletions(-)
Signature · 1 line
2.47.0
Jarkko Sakkinen
maintainerPATCH v8 1/3 tpm: Return tpm2_sessions_init() when null key creation fails
Do not continue tpm2_sessions_init() further if the null key pair creation fails.
---
v8:
- Refine commit message.
v7:
- Add the error message back but fix it up a bit:
1. Remove 'TPM:' given dev_err().
2. s/NULL/null/ as this has nothing to do with the macro in libc.
3. Fix the reasoning: null key creation failed
v6:
- Address:
https://lore.kernel.org/linux-integrity/[email protected]/
as TPM RC is taken care of at the call site. Add also the missing documentation for the return values.
v5:
- Do not print klog messages on error, as tpm2_save_context() already takes care of this.
v4:
- Fixed up stable version.
v3:
- Handle TPM and POSIX error separately and return -ENODEV always back to the caller.
v2:
- Refined the commit message.
--- drivers/char/tpm/tpm2-sessions.c | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-)
drivers/char/tpm/tpm2-sessions.c +9 -2
@@ -1347,14 +1347,21 @@ static int tpm2_create_null_primary(struct tpm_chip *chip)* * Derive and context save the null primary and allocate memory in the * struct tpm_chip for the authorizations. +* +* Return: +* * 0 - OK +* * -errno - A system error +* * TPM_RC - A TPM error */ int tpm2_sessions_init (struct tpm_chip *chip) {int rc; rc =tpm2_create_null_primary (chip); -if (rc) -dev_err (&chip->dev,"TPM: security failed (NULL seed derivation): %d\n" , rc); +if (rc) { +dev_err (&chip->dev,"null key creation failed with %d\n" , rc); +return rc; + } chip->auth =kmalloc (sizeof (*chip->auth),GFP_KERNEL );if (!chip->auth)
Signature · 1 line
2.47.0
Re: [PATCH v8 1/3] tpm: Return tpm2_sessions_init() when null key creation fails
Quoted 6 lines · Jarkko Sakkinen
Do not continue
tpm2_sessions_init()further if the null key pair creation fails.Cc [email protected] # v6.10+Fixes d2add27cf2b8 ("tpm: Add NULL primary creation")Signed-off-by Jarkko Sakkinen <[email protected]>
Quoted 55 lines
---
v8:
- Refine commit message.
v7:
- Add the error message back but fix it up a bit:
1. Remove 'TPM:' givendev_err().
2. s/NULL/null/ as this has nothing to do with the macro in libc.
3. Fix the reasoning: null key creation failed
v6:
- Address:
https://lore.kernel.org/linux-integrity/[email protected]/
as TPM RC is taken care of at the call site. Add also the missing documentation for the return values.
v5:
- Do not print klog messages on error, astpm2_save_context()already takes care of this.
v4:
- Fixed up stable version.
v3:
- Handle TPM and POSIX error separately and return-ENODEValways back to the caller.
v2:
- Refined the commit message.--- drivers/char/tpm/tpm2-sessions.c | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-)drivers/char/tpm/tpm2-sessions.c +9
-2@@ -1347,14 +1347,21 @@ static int tpm2_create_null_primary(struct tpm_chip *chip)* * Derive and context save the null primary and allocate memory in the * struct tpm_chip for the authorizations. +* +* Return: +* * 0 - OK +* * -errno - A system error +* * TPM_RC - A TPM error */ int tpm2_sessions_init (struct tpm_chip *chip) {int rc; rc =tpm2_create_null_primary (chip); -if (rc) -dev_err (&chip->dev,"TPM: security failed (NULL seed derivation): %d\n" , rc); +if (rc) { +dev_err (&chip->dev,"null key creation failed with %d\n" , rc); +return rc; + } chip->auth =kmalloc (sizeof (*chip->auth),GFP_KERNEL );if (!chip->auth)
Jarkko Sakkinen
maintainerRe: [PATCH v8 1/3] tpm: Return tpm2_sessions_init() when null key creation fails
Quoted 11 lines · Stefan Berger
Quoted 6 lines · Jarkko Sakkinen
Do not continue
tpm2_sessions_init()further if the null key pair creation fails.Cc [email protected] # v6.10+Fixes d2add27cf2b8 ("tpm: Add NULL primary creation")Signed-off-by Jarkko Sakkinen <[email protected]>Reviewed-by Stefan Berger <[email protected]>
Thanks, patch are applied to
https://git.kernel.org/pub/scm/linux/kernel/git/jarkko/linux-tpmdd.git/log/?h=next
I will amend these with any further changes (such as tags).
BR, Jarkko
Jarkko Sakkinen
maintainerPATCH v8 2/3 tpm: Rollback tpm2_load_null()
In reply to Jarkko SakkinenDo not continue on tpm2_create_primary() failure in tpm2_load_null().
---
v8:
- Fix stray character in a log message.
v7:
- No changes.
v6:
- Address Stefan's remark:
https://lore.kernel.org/linux-integrity/[email protected]/
v5:
- Fix the TPM error code leak from tpm2_load_context(). v4:
- No changes.
v3:
- Update log messages. Previously the log message incorrectly stated on load failure that integrity check had been failed, even tho the check is done *after* the load operation.
v2:
- Refined the commit message.
- Reverted tpm2_create_primary() changes. They are not required if tmp_null_key is used as the parameter.
--- drivers/char/tpm/tpm2-sessions.c | 44 +++++++++++++++++--------------- 1 file changed, 24 insertions(+), 20 deletions(-)
drivers/char/tpm/tpm2-sessions.c +24 -20
@@ -915,33 +915,37 @@ static int tpm2_parse_start_auth_session(struct tpm2_auth *auth,static int tpm2_load_null (struct tpm_chip *chip,u32 *null_key) { -int rc;unsigned int offset =0 ;/* dummy offset for null seed context */ u8 name[SHA256_DIGEST_SIZE +2 ]; +u32 tmp_null_key; +int rc; rc =tpm2_load_context (chip, chip->null_key_context, &offset, - null_key); -if (rc != -EINVAL ) -return rc; + &tmp_null_key); +if (rc != -EINVAL ) { +if (!rc) + *null_key = tmp_null_key; + goto err; + } -/* an integrity failure may mean the TPM has been reset */ -dev_err (&chip->dev,"NULL key integrity failure!\n" ); -/* check the null name against what we know */ -tpm2_create_primary (chip,TPM2_RH_NULL ,NULL , name); -if (memcmp (name, chip->null_key_name,sizeof (name)) ==0 ) -/* name unchanged, assume transient integrity failure */ -return rc; -/* -* Fatal TPM failure: the NULL seed has actually changed, so -* the TPM must have been illegally reset. All in-kernel TPM -* operations will fail because the NULL primary can't be -* loaded to salt the sessions, but disable the TPM anyway so -* userspace programmes can't be compromised by it. -*/ -dev_err (&chip->dev,"NULL name has changed, disabling TPM due to interference\n" ); +/* Try to re-create null key, given the integrity failure: */ + rc =tpm2_create_primary (chip,TPM2_RH_NULL , &tmp_null_key, name); +if (rc) + goto err; + +/* Return null key if the name has not been changed: */ +if (!memcmp (name, chip->null_key_name,sizeof (name))) { + *null_key = tmp_null_key; +return 0 ; + } + +/* Deduce from the name change TPM interference: */ +dev_err (&chip->dev,"null key integrity check failed\n" ); +tpm2_flush_context (chip, tmp_null_key); chip->flags |=TPM_CHIP_FLAG_DISABLE ; -return rc; +err: +return rc ? -ENODEV :0 ; }/**
Signature · 1 line
2.47.0
Re: [PATCH v8 2/3] tpm: Rollback tpm2_load_null()
Dear Jarkko,
Thank you for your patch.
Am 28.10.24 um 06:50 schrieb Jarkko Sakkinen:
Quoted 1 line
Do not continue on
tpm2_create_primary()failure intpm2_load_null().
Could you please elaborate, why this is done, that means the motivation for your change?
Quoted 4 lines
Cc [email protected] # v6.10+Fixes eb24c9788cd9 ("tpm: disable the TPM if NULL name changes")Reviewed-by Stefan Berger <[email protected]>Signed-off-by Jarkko Sakkinen <[email protected]>
Kind regards,
Paul
Jarkko Sakkinen
maintainerRe: [PATCH v8 2/3] tpm: Rollback tpm2_load_null()
Quoted 10 lines · Paul Menzel
Dear Jarkko,
Thank you for your patch.
Am 28.10.24 um 06:50 schrieb Jarkko Sakkinen:
Quoted 1 line
Do not continue on
tpm2_create_primary()failure intpm2_load_null().Could you please elaborate, why this is done, that means the motivation for your change?
Which part of "not properly handling a return value" I should explain?
BR, Jarkko
Re: [PATCH v8 2/3] tpm: Rollback tpm2_load_null()
Dear Jarkko,
Am 28.10.24 um 13:10 schrieb Jarkko Sakkinen:
Quoted 9 lines · Paul Menzel
Quoted 6 lines
Am 28.10.24 um 06:50 schrieb Jarkko Sakkinen:
Quoted 1 line
Do not continue on
tpm2_create_primary()failure intpm2_load_null().Could you please elaborate, why this is done, that means the motivation for your change?
Which part of "not properly handling a return value" I should explain?
Sorry, where is your quote from?
Anyway, maybe explaining why a successful call to tpm2_create_primary() is needed to continue would at least help me.
Kind regards,
Paul
Jarkko Sakkinen
maintainerRe: [PATCH v8 2/3] tpm: Rollback tpm2_load_null()
Quoted 12 lines · Paul Menzel
Dear Jarkko,
Am 28.10.24 um 13:10 schrieb Jarkko Sakkinen:
Quoted 8 lines · Paul Menzel
Quoted 6 lines
Am 28.10.24 um 06:50 schrieb Jarkko Sakkinen:
Quoted 1 line
Do not continue on
tpm2_create_primary()failure intpm2_load_null().Could you please elaborate, why this is done, that means the motivation for your change?
Quoted 6 lines
Quoted 1 line
Which part of "not properly handling a return value" I should explain?
Sorry, where is your quote from?
Anyway, maybe explaining why a successful call to
tpm2_create_primary()is needed to continue would at least help me.
It's not a void function.
BR, Jarkko
James Bottomley
maintainerRe: [PATCH v8 2/3] tpm: Rollback tpm2_load_null()
In reply to Jarkko SakkinenOn Mon, 2024-10-28 at 07:50 +0200, Jarkko Sakkinen wrote:
[...]
Quoted 39 lines
drivers/char/tpm/tpm2-sessions.c +0
-0@@ -915,33 +915,37 @@ static int tpm2_parse_start_auth_session(struct tpm2_auth *auth,
static inttpm2_load_null(struct tpm_chip *chip, u32 *null_key){
- int rc;
unsigned int offset = 0; /* dummy offset for null seed context */
u8 name[SHA256_DIGEST_SIZE + 2];
+ u32tmp_null_key;
+ int rc;
rc = tpm2_load_context(chip,chip->null_key_context, &offset, -null_key);
- if (rc !=-EINVAL)
- return rc;
+&tmp_null_key);
+ if (rc !=-EINVAL) {
+ if (!rc)
+ *null_key =tmp_null_key;
+ goto err;
+ }
- /* an integrity failure may mean the TPM has been reset */ - dev_err(&chip->dev, "NULL key integrity failure!\n"); - /* check the null name against what we know */
- tpm2_create_primary(chip,TPM2_RH_NULL, NULL, name); - if (memcmp(name, chip->null_key_name, sizeof(name))== 0) - /* name unchanged, assume transient integrity failure */
- return rc;
- /*
- * Fatal TPM failure: the NULL seed has actually changed, so - * the TPM must have been illegally reset. All in-kernel TPM - * operations will fail because the NULL primary can't be - * loaded to salt the sessions, but disable the TPM anyway so - * userspace programmes can't be compromised by it.
Quoted 8 lines
- */
- dev_err(&chip->dev, "NULL name has changed, disabling TPM due to interference\n");
+ /* Try to re-create null key, given the integrity failure: */ + rc =tpm2_create_primary(chip, TPM2_RH_NULL, &tmp_null_key, name);
+ if (rc)
+ goto err;
From a security point of view, this probably isn't such a good idea:
the reason the context load failed above is likely the security condition we're checking for: the null seed changed because an interposer did a reset. That means that if the interposer knows about this error leg, it would simply error out the create primary here and the TPM wouldn't be disabled.
Regards,
James
Jarkko Sakkinen
maintainerRe: [PATCH v8 2/3] tpm: Rollback tpm2_load_null()
Quoted 48 lines · James Bottomley
On Mon, 2024-10-28 at 07:50 +0200, Jarkko Sakkinen wrote:
[...]Quoted 46 lines
drivers/char/tpm/tpm2-sessions.c +0
-0@@ -915,33 +915,37 @@ static int tpm2_parse_start_auth_session(struct tpm2_auth *auth,
static inttpm2_load_null(struct tpm_chip *chip, u32 *null_key){
- int rc;
unsigned int offset = 0; /* dummy offset for null seed context */
u8 name[SHA256_DIGEST_SIZE + 2];
+ u32tmp_null_key;
+ int rc;
rc = tpm2_load_context(chip,chip->null_key_context, &offset, -null_key);
- if (rc !=-EINVAL)
- return rc;
+&tmp_null_key);
+ if (rc !=-EINVAL) {
+ if (!rc)
+ *null_key =tmp_null_key;
+ goto err;
+ }
- /* an integrity failure may mean the TPM has been reset */ - dev_err(&chip->dev, "NULL key integrity failure!\n"); - /* check the null name against what we know */
- tpm2_create_primary(chip,TPM2_RH_NULL, NULL, name); - if (memcmp(name, chip->null_key_name, sizeof(name))== 0) - /* name unchanged, assume transient integrity failure */
- return rc;
- /*
- * Fatal TPM failure: the NULL seed has actually changed, so - * the TPM must have been illegally reset. All in-kernel TPM - * operations will fail because the NULL primary can't be - * loaded to salt the sessions, but disable the TPM anyway so - * userspace programmes can't be compromised by it. - */
- dev_err(&chip->dev, "NULL name has changed, disabling TPM due to interference\n");
+ /* Try to re-create null key, given the integrity failure: */ + rc =tpm2_create_primary(chip, TPM2_RH_NULL, &tmp_null_key, name);
+ if (rc)
Quoted 8 lines
Quoted 1 line
+ goto err;
From a security point of view, this probably isn't such a good idea:
the reason the context load failed above is likely the security condition we're checking for: the null seed changed because an interposer did a reset. That means that if the interposer knows about this error leg, it would simply error out the create primary here and the TPM wouldn't be disabled.
If you think there is something that should be still addressed, or there is overlooked issue please do send a patch, and we will review that. There's been plenty of time to comment on patches.
Neither in previous TPM_CHIP_FLAG_DISABLED was set tpm2_load_context() failed. It went like this:
rc = tpm2_load_context(chip, chip->null_key_context, &offset, null_key); if (rc != -EINVAL) return rc;
If you think that this should be addressed, do send a patch but point out the fixes-tag to your original patch then.
BR, Jarkko
Jarkko Sakkinen
maintainerRe: [PATCH v8 2/3] tpm: Rollback tpm2_load_null()
Quoted 65 lines · Jarkko Sakkinen
Quoted 48 lines · James Bottomley
On Mon, 2024-10-28 at 07:50 +0200, Jarkko Sakkinen wrote:
[...]Quoted 46 lines
drivers/char/tpm/tpm2-sessions.c +0
-0@@ -915,33 +915,37 @@ static int tpm2_parse_start_auth_session(struct tpm2_auth *auth,
static inttpm2_load_null(struct tpm_chip *chip, u32 *null_key){
- int rc;
unsigned int offset = 0; /* dummy offset for null seed context */
u8 name[SHA256_DIGEST_SIZE + 2];
+ u32tmp_null_key;
+ int rc;
rc = tpm2_load_context(chip,chip->null_key_context, &offset, -null_key);
- if (rc !=-EINVAL)
- return rc;
+&tmp_null_key);
+ if (rc !=-EINVAL) {
+ if (!rc)
+ *null_key =tmp_null_key;
+ goto err;
+ }
- /* an integrity failure may mean the TPM has been reset */ - dev_err(&chip->dev, "NULL key integrity failure!\n"); - /* check the null name against what we know */
- tpm2_create_primary(chip,TPM2_RH_NULL, NULL, name); - if (memcmp(name, chip->null_key_name, sizeof(name))== 0) - /* name unchanged, assume transient integrity failure */
- return rc;
- /*
- * Fatal TPM failure: the NULL seed has actually changed, so - * the TPM must have been illegally reset. All in-kernel TPM - * operations will fail because the NULL primary can't be - * loaded to salt the sessions, but disable the TPM anyway so - * userspace programmes can't be compromised by it. - */
- dev_err(&chip->dev, "NULL name has changed, disabling TPM due to interference\n");
+ /* Try to re-create null key, given the integrity failure: */ + rc =tpm2_create_primary(chip, TPM2_RH_NULL, &tmp_null_key, name);
+ if (rc)Quoted 8 lines
Quoted 1 line
+ goto err;
From a security point of view, this probably isn't such a good idea:
the reason the context load failed above is likely the security condition we're checking for: the null seed changed because an interposer did a reset. That means that if the interposer knows about this error leg, it would simply error out the create primary here and the TPM wouldn't be disabled.If you think there is something that should be still addressed, or there is overlooked issue please do send a patch, and we will review that. There's been plenty of time to comment on patches.
Neither in previous
TPM_CHIP_FLAG_DISABLEDwas settpm2_load_context()failed. It went like this:
Quoted 7 lines
rc = tpm2_load_context(chip, chip->null_key_context, &offset, null_key); if (rc != -EINVAL) return rc;If you think that this should be addressed, do send a patch but point out the fixes-tag to your original patch then.
Also want to denotat that I specifically did not set flag because I thought you had good reasons not to disable TPM. So it was left like that knowingly, definitely not by ignorance. Good that it became now apparent and I'm happy to take a fix in (with correct fixes tag).
BR, Jarkko
Jarkko Sakkinen
maintainerPATCH v8 3/3 tpm: Lazily flush the auth session
In reply to Jarkko SakkinenMove the allocation of chip->auth to tpm2_start_auth_session() so that this field can be used as flag to tell whether auth session is active or not.
Instead of flushing and reloading the auth session for every transaction separately, keep the session open unless /dev/tpm0 is used.
---
v8:
- Since auth session and null key are flushed at a same time, only either needs to be checked. Addresses and a remark from James Bottomley few revisions ago.
- kfree_sensitive()
- Effectively squash top three patches given the simplifications. v7:
- No changes.
v6:
- No changes.
v5:
- No changes.
v4:
- Changed as bug.
v3:
- Refined the commit message.
- Removed the conditional for applying TPM2_SA_CONTINUE_SESSION only when /dev/tpm0 is open. It is not required as the auth session is flushed, not saved.
v2:
- A new patch.
--- drivers/char/tpm/tpm-chip.c | 10 +++++++ drivers/char/tpm/tpm-dev-common.c | 3 +++ drivers/char/tpm/tpm-interface.c | 6 +++-- drivers/char/tpm/tpm2-sessions.c | 45 ++++++++++++++++++------------- 4 files changed, 44 insertions(+), 20 deletions(-)
drivers/char/tpm/tpm-chip.c +10 -0
@@ -674,6 +674,16 @@ EXPORT_SYMBOL_GPL(tpm_chip_register);*/ void tpm_chip_unregister (struct tpm_chip *chip) { +#ifdef CONFIG_TCG_TPM2_HMAC +int rc; + + rc =tpm_try_get_ops (chip); +if (!rc) { +tpm2_end_auth_session (chip); +tpm_put_ops (chip); + } +#endif +tpm_del_legacy_sysfs (chip);if (tpm_is_hwrng_enabled (chip))hwrng_unregister (&chip->hwrng);
drivers/char/tpm/tpm-dev-common.c +3 -0
@@ -27,6 +27,9 @@ static ssize_t tpm_dev_transmit(struct tpm_chip *chip, struct tpm_space *space,struct tpm_header *header = (void *)buf;ssize_t ret, len; +if (chip->flags &TPM_CHIP_FLAG_TPM2 ) +tpm2_end_auth_session (chip); + ret =tpm2_prepare_space (chip, space, buf, bufsiz);/* If the command is not implemented by the TPM, synthesize a * response with a TPM2_RC_COMMAND_CODE return for user-space.
drivers/char/tpm/tpm-interface.c +4 -2
@@ -379,10 +379,12 @@ int tpm_pm_suspend(struct device *dev) rc =tpm_try_get_ops (chip);if (!rc) { -if (chip->flags &TPM_CHIP_FLAG_TPM2 ) +if (chip->flags &TPM_CHIP_FLAG_TPM2 ) { +tpm2_end_auth_session (chip);tpm2_shutdown (chip,TPM2_SU_STATE ); -else + }else { rc =tpm1_pm_suspend (chip, tpm_suspend_pcr); + }tpm_put_ops (chip); }
drivers/char/tpm/tpm2-sessions.c +27 -18
112 lines not on this page · raw message
Signature · 1 line
2.47.0
Re: [PATCH v8 3/3] tpm: Lazily flush the auth session
Quoted 6 lines · Jarkko Sakkinen
Move the allocation of
chip->authtotpm2_start_auth_session()so that this field can be used as flag to tell whether auth session is active or not.Instead of flushing and reloading the auth session for every transaction separately, keep the session open unless
/dev/tpm0is used.
Quoted 6 lines
Reported-by Pengyu Ma <[email protected]>Cc [email protected] # v6.10+Fixes 7ca110f2679b ("tpm: Address !chip->auth in tpm_buf_append_hmac_session*()")Tested-by Pengyu Ma <[email protected]>Signed-off-by Jarkko Sakkinen <[email protected]>
Quoted 209 lines
---
v8:
- Since auth session and null key are flushed at a same time, only either needs to be checked. Addresses and a remark from James Bottomley few revisions ago.
-kfree_sensitive()
- Effectively squash top three patches given the simplifications. v7:
- No changes.
v6:
- No changes.
v5:
- No changes.
v4:
- Changed as bug.
v3:
- Refined the commit message.
- Removed the conditional for applyingTPM2_SA_CONTINUE_SESSIONonly when/dev/tpm0is open. It is not required as the auth session is flushed, not saved.
v2:
- A new patch.--- drivers/char/tpm/tpm-chip.c | 10 +++++++ drivers/char/tpm/tpm-dev-common.c | 3 +++ drivers/char/tpm/tpm-interface.c | 6 +++-- drivers/char/tpm/tpm2-sessions.c | 45 ++++++++++++++++++------------- 4 files changed, 44 insertions(+), 20 deletions(-)drivers/char/tpm/tpm-chip.c +10
-0@@ -674,6 +674,16 @@ EXPORT_SYMBOL_GPL(tpm_chip_register);*/ void tpm_chip_unregister (struct tpm_chip *chip) { +#ifdef CONFIG_TCG_TPM2_HMAC +int rc; + + rc =tpm_try_get_ops (chip); +if (!rc) { +tpm2_end_auth_session (chip); +tpm_put_ops (chip); + } +#endif +tpm_del_legacy_sysfs (chip);if (tpm_is_hwrng_enabled (chip))hwrng_unregister (&chip->hwrng);drivers/char/tpm/tpm-dev-common.c +3
-0@@ -27,6 +27,9 @@ static ssize_t tpm_dev_transmit(struct tpm_chip *chip, struct tpm_space *space,struct tpm_header *header = (void *)buf;ssize_t ret, len; +if (chip->flags &TPM_CHIP_FLAG_TPM2 ) +tpm2_end_auth_session (chip); + ret =tpm2_prepare_space (chip, space, buf, bufsiz);/* If the command is not implemented by the TPM, synthesize a * response with a TPM2_RC_COMMAND_CODE return for user-space. drivers/char/tpm/tpm-interface.c +4
-2@@ -379,10 +379,12 @@ int tpm_pm_suspend(struct device *dev) rc =tpm_try_get_ops (chip);if (!rc) { -if (chip->flags &TPM_CHIP_FLAG_TPM2 ) +if (chip->flags &TPM_CHIP_FLAG_TPM2 ) { +tpm2_end_auth_session (chip);tpm2_shutdown (chip,TPM2_SU_STATE ); -else + }else { rc =tpm1_pm_suspend (chip, tpm_suspend_pcr); + }tpm_put_ops (chip); }drivers/char/tpm/tpm2-sessions.c +27
-18@@ -333,6 +333,9 @@ void tpm_buf_append_hmac_session(struct tpm_chip *chip, struct tpm_buf *buf, }#ifdef CONFIG_TCG_TPM2_HMAC +/* The first write to /dev/tpm{rm0} will flush the session. */ + attributes |=TPM2_SA_CONTINUE_SESSION ; +/* * The Architecture Guide requires us to strip trailing zeros * before computing the HMAC @@ -484,7 +487,8 @@ static void tpm2_KDFe(u8 z[EC_PT_SZ], const char *str, u8 *pt_u, u8 *pt_v,sha256_final (&sctx, out); } -static void tpm_buf_append_salt (struct tpm_buf *buf,struct tpm_chip *chip) +static void tpm_buf_append_salt (struct tpm_buf *buf,struct tpm_chip *chip, +struct tpm2_auth *auth) {struct crypto_kpp *kpp;struct kpp_request *req;@@ -543,7 +547,7 @@ static void tpm_buf_append_salt(struct tpm_buf *buf, struct tpm_chip *chip)sg_set_buf (&s[0 ], chip->null_ec_key_x,EC_PT_SZ );sg_set_buf (&s[1 ], chip->null_ec_key_y,EC_PT_SZ );kpp_request_set_input (req, s,EC_PT_SZ *2 ); -sg_init_one (d, chip->auth->salt,EC_PT_SZ ); +sg_init_one (d, auth->salt,EC_PT_SZ );kpp_request_set_output (req, d,EC_PT_SZ );crypto_kpp_compute_shared_secret (req);kpp_request_free (req);@@ -554,8 +558,7 @@ static void tpm_buf_append_salt(struct tpm_buf *buf, struct tpm_chip *chip)* This works because KDFe fully consumes the secret before it * writes the salt */ -tpm2_KDFe (chip->auth->salt,"SECRET" , x, chip->null_ec_key_x, - chip->auth->salt); +tpm2_KDFe (auth->salt,"SECRET" , x, chip->null_ec_key_x, auth->salt); out:crypto_free_kpp (kpp);@@ -853,7 +856,9 @@ int tpm_buf_check_hmac_response(struct tpm_chip *chip, struct tpm_buf *buf,if (rc)/* manually close the session if it wasn't consumed */ tpm2_flush_context (chip, auth->handle); -memzero_explicit (auth,sizeof (*auth)); + +kfree_sensitive (auth); + chip->auth =NULL ; }else {/* reset for next use */ auth->session =TPM_HEADER_SIZE ;@@ -881,7 +886,8 @@ void tpm2_end_auth_session(struct tpm_chip *chip)return ;tpm2_flush_context (chip, auth->handle); -memzero_explicit (auth,sizeof (*auth)); +kfree_sensitive (auth); + chip->auth =NULL ; }EXPORT_SYMBOL (tpm2_end_auth_session);@@ -962,16 +968,20 @@ static int tpm2_load_null(struct tpm_chip *chip, u32 *null_key)*/ int tpm2_start_auth_session (struct tpm_chip *chip) { +struct tpm2_auth *auth;struct tpm_buf buf; -struct tpm2_auth *auth = chip->auth; -int rc;u32 null_key; +int rc; -if (!auth) { -dev_warn_once (&chip->dev,"auth session is not active\n" ); +if (chip->auth) { +dev_warn_once (&chip->dev,"auth session is active\n" );return 0 ; } + auth =kzalloc (sizeof (*auth),GFP_KERNEL ); +if (!auth) +return -ENOMEM ; + rc =tpm2_load_null (chip, &null_key);if (rc) goto out;@@ -992,7 +1002,7 @@ int tpm2_start_auth_session(struct tpm_chip *chip)tpm_buf_append (&buf, auth->our_nonce,sizeof (auth->our_nonce));/* append encrypted salt and squirrel away unencrypted in auth */ -tpm_buf_append_salt (&buf, chip); +tpm_buf_append_salt (&buf, chip, auth);/* session type (HMAC, audit or policy) */ tpm_buf_append_u8 (&buf,TPM2_SE_HMAC );@@ -1014,10 +1024,13 @@ int tpm2_start_auth_session(struct tpm_chip *chip)tpm_buf_destroy (&buf); -if (rc) - goto out; +if (rc ==TPM2_RC_SUCCESS ) { + chip->auth = auth; +return 0 ; + } - out: +out: +kfree_sensitive (auth);return rc; }EXPORT_SYMBOL (tpm2_start_auth_session);@@ -1367,10 +1380,6 @@ int tpm2_sessions_init(struct tpm_chip *chip) return rc; } - chip->auth =kmalloc (sizeof (*chip->auth),GFP_KERNEL ); -if (!chip->auth) -return -ENOMEM ; -return rc; }#endif /* CONFIG_TCG_TPM2_HMAC */
Jarkko Sakkinen
maintainerRe: [PATCH v8 3/3] tpm: Lazily flush the auth session
Quoted 15 lines · Stefan Berger
Quoted 6 lines · Jarkko Sakkinen
Move the allocation of
chip->authtotpm2_start_auth_session()so that this field can be used as flag to tell whether auth session is active or not.Instead of flushing and reloading the auth session for every transaction separately, keep the session open unless
/dev/tpm0is used.Quoted 6 lines
Reported-by Pengyu Ma <[email protected]>Cc [email protected] # v6.10+Fixes 7ca110f2679b ("tpm: Address !chip->auth in tpm_buf_append_hmac_session*()")Tested-by Pengyu Ma <[email protected]>Signed-off-by Jarkko Sakkinen <[email protected]>
Quoted 2 lines
Reviewed-by Stefan Berger <[email protected]>Tested-by Stefan Berger <[email protected]>
Thanks!
Next after this: tpm2_get_random() issues reported.
I think biggest problem with that in general, and independent of bugs, is that it does not pool random but instead pulls random small chunks. This is more like performance issue exposed by bus encryption than introducing a new issue (not formally but with better implementation would not be necessarily a problem).
BR, Jarkko