| Message ID | 20240921120811.1264985-6-jarkko@kernel.org (mailing list archive) |
|---|---|
| State | Handled Elsewhere |
| Headers | show |
| Series | Lazy flush for the auth session | expand |
On Sat, 2024-09-21 at 15:08 +0300, Jarkko Sakkinen wrote: > Instead of flushing and reloading the auth session for every single > transaction, keep the session open unless /dev/tpm0 is used. In > practice this means applying TPM2_SA_CONTINUE_SESSION to the session > attributes. Flush the session always when /dev/tpm0 is written. Patch looks fine but this description is way too terse to explain how it works. I would suggest: Boot time elongation as a result of adding sessions has been reported as an issue in https://bugzilla.kernel.org/show_bug.cgi?id=219229 The root cause is the addition of session overhead to tpm2_pcr_extend(). This overhead can be reduced by not creating and destroying a session for each invocation of the function. Do this by keeping a session resident in the TPM for reuse by any session based TPM command. The current flow of TPM commands in the kernel supports this because tpm2_end_session() is only called for tpm errors because most commands don't continue the session and expect the session to be flushed on success. Thus we can add the continue session flag to session creation to ensure the session won't be flushed except on error, which is a rare case. Since the session consumes a slot in the TPM it must not be seen by userspace but we can flush it whenever a command write occurs and re- create it again on the next kernel session use. Since TPM use in boot is somewhat rare this allows considerable reuse of the in-kernel session and shortens boot time: <give figures> > > Reported-by: Pengyu Ma <mapengyu@gmail.com> > Closes: https://bugzilla.kernel.org/show_bug.cgi?id=219229 > Cc: stable@vger.kernel.org # v6.10+ > Fixes: 7ca110f2679b ("tpm: Address !chip->auth in > tpm_buf_append_hmac_session*()") > Tested-by: Pengyu Ma <mapengyu@gmail.com> > Signed-off-by: Jarkko Sakkinen <jarkko@kernel.org> > --- > 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 | 1 + > drivers/char/tpm/tpm-dev-common.c | 1 + > drivers/char/tpm/tpm-interface.c | 1 + > drivers/char/tpm/tpm2-sessions.c | 3 +++ > 4 files changed, 6 insertions(+) > > diff --git a/drivers/char/tpm/tpm-chip.c b/drivers/char/tpm/tpm- > chip.c > index 0ea00e32f575..7a6bb30d1f32 100644 > --- a/drivers/char/tpm/tpm-chip.c > +++ b/drivers/char/tpm/tpm-chip.c > @@ -680,6 +680,7 @@ void tpm_chip_unregister(struct tpm_chip *chip) > rc = tpm_try_get_ops(chip); > if (!rc) { > if (chip->flags & TPM_CHIP_FLAG_TPM2) { > + tpm2_end_auth_session(chip); > tpm2_flush_context(chip, chip->null_key); > chip->null_key = 0; > } > diff --git a/drivers/char/tpm/tpm-dev-common.c > b/drivers/char/tpm/tpm-dev-common.c > index 4eaa8e05c291..a3ed7a99a394 100644 > --- a/drivers/char/tpm/tpm-dev-common.c > +++ b/drivers/char/tpm/tpm-dev-common.c > @@ -29,6 +29,7 @@ static ssize_t tpm_dev_transmit(struct tpm_chip > *chip, struct tpm_space *space, > > #ifdef CONFIG_TCG_TPM2_HMAC > if (chip->flags & TPM_CHIP_FLAG_TPM2) { > + tpm2_end_auth_session(chip); > tpm2_flush_context(chip, chip->null_key); > chip->null_key = 0; > } > diff --git a/drivers/char/tpm/tpm-interface.c b/drivers/char/tpm/tpm- > interface.c > index bfa47d48b0f2..2363018fa8fb 100644 > --- a/drivers/char/tpm/tpm-interface.c > +++ b/drivers/char/tpm/tpm-interface.c > @@ -381,6 +381,7 @@ int tpm_pm_suspend(struct device *dev) > if (!rc) { > if (chip->flags & TPM_CHIP_FLAG_TPM2) { > #ifdef CONFIG_TCG_TPM2_HMAC > + tpm2_end_auth_session(chip); > tpm2_flush_context(chip, chip->null_key); > chip->null_key = 0; > #endif > diff --git a/drivers/char/tpm/tpm2-sessions.c > b/drivers/char/tpm/tpm2-sessions.c > index a8d3d5d52178..38b92ad9e75f 100644 > --- a/drivers/char/tpm/tpm2-sessions.c > +++ b/drivers/char/tpm/tpm2-sessions.c > @@ -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 Code is fine, with the change log update, you can add Reviewed-by: James Bottomley <James.Bottomley@HansenPartnership.com>
On Tue Sep 24, 2024 at 4:43 PM EEST, James Bottomley wrote: > On Sat, 2024-09-21 at 15:08 +0300, Jarkko Sakkinen wrote: > > Instead of flushing and reloading the auth session for every single > > transaction, keep the session open unless /dev/tpm0 is used. In > > practice this means applying TPM2_SA_CONTINUE_SESSION to the session > > attributes. Flush the session always when /dev/tpm0 is written. > > Patch looks fine but this description is way too terse to explain how > it works. > > I would suggest: > > Boot time elongation as a result of adding sessions has been reported > as an issue in https://bugzilla.kernel.org/show_bug.cgi?id=219229 > > The root cause is the addition of session overhead to > tpm2_pcr_extend(). This overhead can be reduced by not creating and > destroying a session for each invocation of the function. Do this by > keeping a session resident in the TPM for reuse by any session based > TPM command. The current flow of TPM commands in the kernel supports > this because tpm2_end_session() is only called for tpm errors because > most commands don't continue the session and expect the session to be > flushed on success. Thus we can add the continue session flag to > session creation to ensure the session won't be flushed except on > error, which is a rare case. > > Since the session consumes a slot in the TPM it must not be seen by > userspace but we can flush it whenever a command write occurs and re- > create it again on the next kernel session use. Since TPM use in boot > is somewhat rare this allows considerable reuse of the in-kernel > session and shortens boot time: > > <give figures> > > > > > > > Reported-by: Pengyu Ma <mapengyu@gmail.com> > > Closes: https://bugzilla.kernel.org/show_bug.cgi?id=219229 > > Cc: stable@vger.kernel.org # v6.10+ > > Fixes: 7ca110f2679b ("tpm: Address !chip->auth in > > tpm_buf_append_hmac_session*()") > > Tested-by: Pengyu Ma <mapengyu@gmail.com> > > Signed-off-by: Jarkko Sakkinen <jarkko@kernel.org> > > --- > > 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 | 1 + > > drivers/char/tpm/tpm-dev-common.c | 1 + > > drivers/char/tpm/tpm-interface.c | 1 + > > drivers/char/tpm/tpm2-sessions.c | 3 +++ > > 4 files changed, 6 insertions(+) > > > > diff --git a/drivers/char/tpm/tpm-chip.c b/drivers/char/tpm/tpm- > > chip.c > > index 0ea00e32f575..7a6bb30d1f32 100644 > > --- a/drivers/char/tpm/tpm-chip.c > > +++ b/drivers/char/tpm/tpm-chip.c > > @@ -680,6 +680,7 @@ void tpm_chip_unregister(struct tpm_chip *chip) > > rc = tpm_try_get_ops(chip); > > if (!rc) { > > if (chip->flags & TPM_CHIP_FLAG_TPM2) { > > + tpm2_end_auth_session(chip); > > tpm2_flush_context(chip, chip->null_key); > > chip->null_key = 0; > > } > > diff --git a/drivers/char/tpm/tpm-dev-common.c > > b/drivers/char/tpm/tpm-dev-common.c > > index 4eaa8e05c291..a3ed7a99a394 100644 > > --- a/drivers/char/tpm/tpm-dev-common.c > > +++ b/drivers/char/tpm/tpm-dev-common.c > > @@ -29,6 +29,7 @@ static ssize_t tpm_dev_transmit(struct tpm_chip > > *chip, struct tpm_space *space, > > > > #ifdef CONFIG_TCG_TPM2_HMAC > > if (chip->flags & TPM_CHIP_FLAG_TPM2) { > > + tpm2_end_auth_session(chip); > > tpm2_flush_context(chip, chip->null_key); > > chip->null_key = 0; > > } > > diff --git a/drivers/char/tpm/tpm-interface.c b/drivers/char/tpm/tpm- > > interface.c > > index bfa47d48b0f2..2363018fa8fb 100644 > > --- a/drivers/char/tpm/tpm-interface.c > > +++ b/drivers/char/tpm/tpm-interface.c > > @@ -381,6 +381,7 @@ int tpm_pm_suspend(struct device *dev) > > if (!rc) { > > if (chip->flags & TPM_CHIP_FLAG_TPM2) { > > #ifdef CONFIG_TCG_TPM2_HMAC > > + tpm2_end_auth_session(chip); > > tpm2_flush_context(chip, chip->null_key); > > chip->null_key = 0; > > #endif > > diff --git a/drivers/char/tpm/tpm2-sessions.c > > b/drivers/char/tpm/tpm2-sessions.c > > index a8d3d5d52178..38b92ad9e75f 100644 > > --- a/drivers/char/tpm/tpm2-sessions.c > > +++ b/drivers/char/tpm/tpm2-sessions.c > > @@ -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 > > Code is fine, with the change log update, you can add > > Reviewed-by: James Bottomley <James.Bottomley@HansenPartnership.com> OK, thank you. BR, Jarkko
On Tue Sep 24, 2024 at 4:43 PM EEST, James Bottomley wrote: > On Sat, 2024-09-21 at 15:08 +0300, Jarkko Sakkinen wrote: > > Instead of flushing and reloading the auth session for every single > > transaction, keep the session open unless /dev/tpm0 is used. In > > practice this means applying TPM2_SA_CONTINUE_SESSION to the session > > attributes. Flush the session always when /dev/tpm0 is written. > > Patch looks fine but this description is way too terse to explain how > it works. > > I would suggest: > > Boot time elongation as a result of adding sessions has been reported > as an issue in https://bugzilla.kernel.org/show_bug.cgi?id=219229 > > The root cause is the addition of session overhead to > tpm2_pcr_extend(). This overhead can be reduced by not creating and > destroying a session for each invocation of the function. Do this by > keeping a session resident in the TPM for reuse by any session based > TPM command. The current flow of TPM commands in the kernel supports > this because tpm2_end_session() is only called for tpm errors because > most commands don't continue the session and expect the session to be > flushed on success. Thus we can add the continue session flag to > session creation to ensure the session won't be flushed except on > error, which is a rare case. I need to disagree on this as I don't even have PCR extends in my boot sequence and it still adds overhead. Have you verified this from the reporter? There's bunch of things that use auth session, like trusted keys. Making such claim that PCR extend is the reason is nonsense. BR, Jarkko
On Tue, 2024-09-24 at 21:07 +0300, Jarkko Sakkinen wrote: > On Tue Sep 24, 2024 at 4:43 PM EEST, James Bottomley wrote: > > On Sat, 2024-09-21 at 15:08 +0300, Jarkko Sakkinen wrote: > > > Instead of flushing and reloading the auth session for every > > > single transaction, keep the session open unless /dev/tpm0 is > > > used. In practice this means applying TPM2_SA_CONTINUE_SESSION to > > > the session attributes. Flush the session always when /dev/tpm0 > > > is written. > > > > Patch looks fine but this description is way too terse to explain > > how it works. > > > > I would suggest: > > > > Boot time elongation as a result of adding sessions has been > > reported as an issue in > > https://bugzilla.kernel.org/show_bug.cgi?id=219229 > > > > The root cause is the addition of session overhead to > > tpm2_pcr_extend(). This overhead can be reduced by not creating > > and destroying a session for each invocation of the function. Do > > this by keeping a session resident in the TPM for reuse by any > > session based TPM command. The current flow of TPM commands in the > > kernel supports this because tpm2_end_session() is only called for > > tpm errors because most commands don't continue the session and > > expect the session to be flushed on success. Thus we can add the > > continue session flag to session creation to ensure the session > > won't be flushed except on error, which is a rare case. > > I need to disagree on this as I don't even have PCR extends in my > boot sequence and it still adds overhead. Have you verified this > from the reporter? > > There's bunch of things that use auth session, like trusted keys. > Making such claim that PCR extend is the reason is nonsense. Well, the bug report does say it's the commit adding sessions to the PCR extends that causes the delay: https://bugzilla.kernel.org/show_bug.cgi?id=219229#c5 I don't know what else to tell you. James
On Tue Sep 24, 2024 at 9:40 PM EEST, James Bottomley wrote: > On Tue, 2024-09-24 at 21:07 +0300, Jarkko Sakkinen wrote: > > On Tue Sep 24, 2024 at 4:43 PM EEST, James Bottomley wrote: > > > On Sat, 2024-09-21 at 15:08 +0300, Jarkko Sakkinen wrote: > > > > Instead of flushing and reloading the auth session for every > > > > single transaction, keep the session open unless /dev/tpm0 is > > > > used. In practice this means applying TPM2_SA_CONTINUE_SESSION to > > > > the session attributes. Flush the session always when /dev/tpm0 > > > > is written. > > > > > > Patch looks fine but this description is way too terse to explain > > > how it works. > > > > > > I would suggest: > > > > > > Boot time elongation as a result of adding sessions has been > > > reported as an issue in > > > https://bugzilla.kernel.org/show_bug.cgi?id=219229 > > > > > > The root cause is the addition of session overhead to > > > tpm2_pcr_extend(). This overhead can be reduced by not creating > > > and destroying a session for each invocation of the function. Do > > > this by keeping a session resident in the TPM for reuse by any > > > session based TPM command. The current flow of TPM commands in the > > > kernel supports this because tpm2_end_session() is only called for > > > tpm errors because most commands don't continue the session and > > > expect the session to be flushed on success. Thus we can add the > > > continue session flag to session creation to ensure the session > > > won't be flushed except on error, which is a rare case. > > > > I need to disagree on this as I don't even have PCR extends in my > > boot sequence and it still adds overhead. Have you verified this > > from the reporter? > > > > There's bunch of things that use auth session, like trusted keys. > > Making such claim that PCR extend is the reason is nonsense. > > Well, the bug report does say it's the commit adding sessions to the > PCR extends that causes the delay: > > https://bugzilla.kernel.org/show_bug.cgi?id=219229#c5 > > I don't know what else to tell you. As far as I've tested this bug I've been able to generate similar costs with anything using HMAC encryption. PCR extend op itself should have same cost with or without encryption AFAIK. I guess I need provide benchmarks on this to prove that PCR extend is not the only site that is affected. BR, Jarkko
On Wed, 2024-09-25 at 00:35 +0300, Jarkko Sakkinen wrote: > On Tue Sep 24, 2024 at 9:40 PM EEST, James Bottomley wrote: > > On Tue, 2024-09-24 at 21:07 +0300, Jarkko Sakkinen wrote: > > > On Tue Sep 24, 2024 at 4:43 PM EEST, James Bottomley wrote: > > > > On Sat, 2024-09-21 at 15:08 +0300, Jarkko Sakkinen wrote: > > > > > Instead of flushing and reloading the auth session for every > > > > > single transaction, keep the session open unless /dev/tpm0 is > > > > > used. In practice this means applying > > > > > TPM2_SA_CONTINUE_SESSION to the session attributes. Flush the > > > > > session always when /dev/tpm0 is written. > > > > > > > > Patch looks fine but this description is way too terse to > > > > explain how it works. > > > > > > > > I would suggest: > > > > > > > > Boot time elongation as a result of adding sessions has been > > > > reported as an issue in > > > > https://bugzilla.kernel.org/show_bug.cgi?id=219229 > > > > > > > > The root cause is the addition of session overhead to > > > > tpm2_pcr_extend(). This overhead can be reduced by not > > > > creating and destroying a session for each invocation of the > > > > function. Do this by keeping a session resident in the TPM for > > > > reuse by any session based TPM command. The current flow of > > > > TPM commands in the kernel supports this because > > > > tpm2_end_session() is only called for tpm errors because most > > > > commands don't continue the session and expect the session to > > > > be flushed on success. Thus we can add the continue session > > > > flag to session creation to ensure the session won't be flushed > > > > except on error, which is a rare case. > > > > > > I need to disagree on this as I don't even have PCR extends in my > > > boot sequence and it still adds overhead. Have you verified this > > > from the reporter? > > > > > > There's bunch of things that use auth session, like trusted keys. > > > Making such claim that PCR extend is the reason is nonsense. > > > > Well, the bug report does say it's the commit adding sessions to > > the PCR extends that causes the delay: > > > > https://bugzilla.kernel.org/show_bug.cgi?id=219229#c5 > > > > I don't know what else to tell you. > > As far as I've tested this bug I've been able to generate similar > costs with anything using HMAC encryption. PCR extend op itself > should have same cost with or without encryption AFAIK. That's true, but the only significant TPM operation in the secure boot path is the PCR extend for IMA. The RNG stuff is there a bit, but there are other significant delays in seeding the entropy pool. During boot with IMA enabled, you can do hundreds of binary measurements, hence the slow down. > I guess I need provide benchmarks on this to prove that PCR extend is > not the only site that is affected. Well, on the per operation figures, it's obviously not, a standard TPM operation gets a significant overhead because of sessions. However, it is the only site that causes a large boot slowdown because of the number of the number of measurements IMA does on boot. Regards, James
On Wed Sep 25, 2024 at 12:51 AM EEST, James Bottomley wrote: > On Wed, 2024-09-25 at 00:35 +0300, Jarkko Sakkinen wrote: > > On Tue Sep 24, 2024 at 9:40 PM EEST, James Bottomley wrote: > > > On Tue, 2024-09-24 at 21:07 +0300, Jarkko Sakkinen wrote: > > > > On Tue Sep 24, 2024 at 4:43 PM EEST, James Bottomley wrote: > > > > > On Sat, 2024-09-21 at 15:08 +0300, Jarkko Sakkinen wrote: > > > > > > Instead of flushing and reloading the auth session for every > > > > > > single transaction, keep the session open unless /dev/tpm0 is > > > > > > used. In practice this means applying > > > > > > TPM2_SA_CONTINUE_SESSION to the session attributes. Flush the > > > > > > session always when /dev/tpm0 is written. > > > > > > > > > > Patch looks fine but this description is way too terse to > > > > > explain how it works. > > > > > > > > > > I would suggest: > > > > > > > > > > Boot time elongation as a result of adding sessions has been > > > > > reported as an issue in > > > > > https://bugzilla.kernel.org/show_bug.cgi?id=219229 > > > > > > > > > > The root cause is the addition of session overhead to > > > > > tpm2_pcr_extend(). This overhead can be reduced by not > > > > > creating and destroying a session for each invocation of the > > > > > function. Do this by keeping a session resident in the TPM for > > > > > reuse by any session based TPM command. The current flow of > > > > > TPM commands in the kernel supports this because > > > > > tpm2_end_session() is only called for tpm errors because most > > > > > commands don't continue the session and expect the session to > > > > > be flushed on success. Thus we can add the continue session > > > > > flag to session creation to ensure the session won't be flushed > > > > > except on error, which is a rare case. > > > > > > > > I need to disagree on this as I don't even have PCR extends in my > > > > boot sequence and it still adds overhead. Have you verified this > > > > from the reporter? > > > > > > > > There's bunch of things that use auth session, like trusted keys. > > > > Making such claim that PCR extend is the reason is nonsense. > > > > > > Well, the bug report does say it's the commit adding sessions to > > > the PCR extends that causes the delay: > > > > > > https://bugzilla.kernel.org/show_bug.cgi?id=219229#c5 > > > > > > I don't know what else to tell you. > > > > As far as I've tested this bug I've been able to generate similar > > costs with anything using HMAC encryption. PCR extend op itself > > should have same cost with or without encryption AFAIK. > > That's true, but the only significant TPM operation in the secure boot > path is the PCR extend for IMA. The RNG stuff is there a bit, but > there are other significant delays in seeding the entropy pool. During > boot with IMA enabled, you can do hundreds of binary measurements, > hence the slow down. > > > I guess I need provide benchmarks on this to prove that PCR extend is > > not the only site that is affected. > > Well, on the per operation figures, it's obviously not, a standard TPM > operation gets a significant overhead because of sessions. However, it > is the only site that causes a large boot slowdown because of the > number of the number of measurements IMA does on boot. Fair enough. I can buy this. I'll phrase it that (since it was mentioned in the bugzilla comment) in the bug in question the root is in PCR extend but since in my own tests I got overhead from trusted keys I also mention that it overally affects also that and tpm2_get_random(). > > Regards, > > James BR, Jarkko
On Wed Sep 25, 2024 at 10:42 AM EEST, Jarkko Sakkinen wrote: > Fair enough. I can buy this. > > I'll phrase it that (since it was mentioned in the bugzilla comment) > in the bug in question the root is in PCR extend but since in my own > tests I got overhead from trusted keys I also mention that it overally > affects also that and tpm2_get_random(). I do not want to take null key flushing away although I got the reasoning given the small amount of time is saved (maybe 25-50 ms in my QEMU setup if I recall correctly) but it would make sense to squash it auth session patch. I'll also check 1/2 and 2/2 if I'm doing too much in them. Not adding any tags to v6 and it really makes sense to develop benchmarks and not rush with the new version now that I got also your feedback, since it is past rc1 timeline. Good target rcX would be around rc3. BR, Jarkko
On Wed Sep 25, 2024 at 10:46 AM EEST, Jarkko Sakkinen wrote: > On Wed Sep 25, 2024 at 10:42 AM EEST, Jarkko Sakkinen wrote: > > Fair enough. I can buy this. > > > > I'll phrase it that (since it was mentioned in the bugzilla comment) > > in the bug in question the root is in PCR extend but since in my own > > tests I got overhead from trusted keys I also mention that it overally > > affects also that and tpm2_get_random(). > > I do not want to take null key flushing away although I got the > reasoning given the small amount of time is saved (maybe 25-50 ms > in my QEMU setup if I recall correctly) but it would make sense to > squash it auth session patch. > > I'll also check 1/2 and 2/2 if I'm doing too much in them. Not > adding any tags to v6 and it really makes sense to develop > benchmarks and not rush with the new version now that I got > also your feedback, since it is past rc1 timeline. > > Good target rcX would be around rc3. I have to admit this: I had blind spot on that PCR extend comment because I did not get hits on that when testing this so it definitely needs to be documented. I spotted it only yesterday. BR, Jarkko
diff --git a/drivers/char/tpm/tpm-chip.c b/drivers/char/tpm/tpm-chip.c index 0ea00e32f575..7a6bb30d1f32 100644 --- a/drivers/char/tpm/tpm-chip.c +++ b/drivers/char/tpm/tpm-chip.c @@ -680,6 +680,7 @@ void tpm_chip_unregister(struct tpm_chip *chip) rc = tpm_try_get_ops(chip); if (!rc) { if (chip->flags & TPM_CHIP_FLAG_TPM2) { + tpm2_end_auth_session(chip); tpm2_flush_context(chip, chip->null_key); chip->null_key = 0; } diff --git a/drivers/char/tpm/tpm-dev-common.c b/drivers/char/tpm/tpm-dev-common.c index 4eaa8e05c291..a3ed7a99a394 100644 --- a/drivers/char/tpm/tpm-dev-common.c +++ b/drivers/char/tpm/tpm-dev-common.c @@ -29,6 +29,7 @@ static ssize_t tpm_dev_transmit(struct tpm_chip *chip, struct tpm_space *space, #ifdef CONFIG_TCG_TPM2_HMAC if (chip->flags & TPM_CHIP_FLAG_TPM2) { + tpm2_end_auth_session(chip); tpm2_flush_context(chip, chip->null_key); chip->null_key = 0; } diff --git a/drivers/char/tpm/tpm-interface.c b/drivers/char/tpm/tpm-interface.c index bfa47d48b0f2..2363018fa8fb 100644 --- a/drivers/char/tpm/tpm-interface.c +++ b/drivers/char/tpm/tpm-interface.c @@ -381,6 +381,7 @@ int tpm_pm_suspend(struct device *dev) if (!rc) { if (chip->flags & TPM_CHIP_FLAG_TPM2) { #ifdef CONFIG_TCG_TPM2_HMAC + tpm2_end_auth_session(chip); tpm2_flush_context(chip, chip->null_key); chip->null_key = 0; #endif diff --git a/drivers/char/tpm/tpm2-sessions.c b/drivers/char/tpm/tpm2-sessions.c index a8d3d5d52178..38b92ad9e75f 100644 --- a/drivers/char/tpm/tpm2-sessions.c +++ b/drivers/char/tpm/tpm2-sessions.c @@ -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
Instead of flushing and reloading the auth session for every single transaction, keep the session open unless /dev/tpm0 is used. In practice this means applying TPM2_SA_CONTINUE_SESSION to the session attributes. Flush the session always when /dev/tpm0 is written. Reported-by: Pengyu Ma <mapengyu@gmail.com> Closes: https://bugzilla.kernel.org/show_bug.cgi?id=219229 Cc: stable@vger.kernel.org # v6.10+ Fixes: 7ca110f2679b ("tpm: Address !chip->auth in tpm_buf_append_hmac_session*()") Tested-by: Pengyu Ma <mapengyu@gmail.com> Signed-off-by: Jarkko Sakkinen <jarkko@kernel.org> --- 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 | 1 + drivers/char/tpm/tpm-dev-common.c | 1 + drivers/char/tpm/tpm-interface.c | 1 + drivers/char/tpm/tpm2-sessions.c | 3 +++ 4 files changed, 6 insertions(+)