Re: [PATCH v5 5/5] tpm: flush the auth session only when /dev/tpm0 is open

[James Bottomley <James.Bottomley@HansenPartnership.com>]

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>