On Wed Oct 23, 2024 at 9:18 PM EEST, Stefan Berger wrote:
>
>
> On 10/21/24 1:39 AM, Jarkko Sakkinen wrote:
> > Instead of flushing and reloading the null key for every single auth
> > session, flush it only when:
> >
> > 1. User space needs to access /dev/tpm{rm}0.
> > 2. When going to sleep.
> > 3. When unregistering the chip.
> >
> > This removes the need to load and swap the null key between TPM and
> > regular memory per transaction, when the user space is not using the
> > chip.
> >
> > Cc: [email protected] # v6.10+
> > Fixes: d2add27cf2b8 ("tpm: Add NULL primary creation")
> > Tested-by: Pengyu Ma <[email protected]>
> > Signed-off-by: Jarkko Sakkinen <[email protected]>
> > ---
> > v5:
> > - No changes.
> > v4:
> > - Changed to bug fix as not having the patch there is a major hit
> > to bootup times.
> > v3:
> > - Unchanged.
> > v2:
> > - Refined the commit message.
> > - Added tested-by from Pengyu Ma <[email protected]>.
> > - Removed spurious pr_info() statement.
> > ---
> > drivers/char/tpm/tpm-chip.c | 13 +++++++++++++
> > drivers/char/tpm/tpm-dev-common.c | 7 +++++++
> > drivers/char/tpm/tpm-interface.c | 9 +++++++--
> > drivers/char/tpm/tpm2-cmd.c | 3 +++
> > drivers/char/tpm/tpm2-sessions.c | 17 ++++++++++++++---
> > include/linux/tpm.h | 2 ++
> > 6 files changed, 46 insertions(+), 5 deletions(-)
> >
> > diff --git a/drivers/char/tpm/tpm-chip.c b/drivers/char/tpm/tpm-chip.c
> > index 854546000c92..0ea00e32f575 100644
> > --- a/drivers/char/tpm/tpm-chip.c
> > +++ b/drivers/char/tpm/tpm-chip.c
> > @@ -674,6 +674,19 @@ 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) {
> > + if (chip->flags & TPM_CHIP_FLAG_TPM2) {
> > + tpm2_flush_context(chip, chip->null_key);
>
> If chip->null_key is already 0, the above function will not do anything
> good, but you could avoid this whole block then by checking for 0 before
> tpm_try_get_ops().
>
> > + chip->null_key = 0;
> > + }
> > + tpm_put_ops(chip);
> > + }
> > +#endif
> > +
> > tpm_del_legacy_sysfs(chip);
> > if (tpm_is_hwrng_enabled(chip))
> > hwrng_unregister(&chip->hwrng);
> > diff --git a/drivers/char/tpm/tpm-dev-common.c
> > b/drivers/char/tpm/tpm-dev-common.c
> > index 30b4c288c1bb..4eaa8e05c291 100644
> > --- a/drivers/char/tpm/tpm-dev-common.c
> > +++ b/drivers/char/tpm/tpm-dev-common.c
> > @@ -27,6 +27,13 @@ static ssize_t tpm_dev_transmit(struct tpm_chip *chip,
> > struct tpm_space *space,
> > struct tpm_header *header = (void *)buf;
> > ssize_t ret, len;
> >
> > +#ifdef CONFIG_TCG_TPM2_HMAC
> > + if (chip->flags & TPM_CHIP_FLAG_TPM2) {
> > + tpm2_flush_context(chip, chip->null_key);
> > + chip->null_key = 0;
> > + }
> > +#endif
> > +
> > 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.
> > diff --git a/drivers/char/tpm/tpm-interface.c
> > b/drivers/char/tpm/tpm-interface.c
> > index 5da134f12c9a..bfa47d48b0f2 100644
> > --- a/drivers/char/tpm/tpm-interface.c
> > +++ b/drivers/char/tpm/tpm-interface.c
> > @@ -379,10 +379,15 @@ 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) {
> > +#ifdef CONFIG_TCG_TPM2_HMAC
> > + tpm2_flush_context(chip, chip->null_key);
> > + chip->null_key = 0;
> > +#endif
>
> Worth using an inline on this repeating pattern? Up to you.
>
> static inline void tpm2_flush_null_key(struct tpm_chip *chip)
> {
> #ifdef CONFIG_TCG_TPM2_HMAC
> if (chip->flags & TPM_CHIP_FLAG_TPM2 && chip->null_key) {
> tpm2_flush_context(chip, chip->null_key);
> chip->null_key = 0;
> }
> #endif
> }
>
> > tpm2_shutdown(chip, TPM2_SU_STATE);
> > - else
> > + } else {
> > rc = tpm1_pm_suspend(chip, tpm_suspend_pcr);
> > + }
> >
> > tpm_put_ops(chip);
> > }
> > diff --git a/drivers/char/tpm/tpm2-cmd.c b/drivers/char/tpm/tpm2-cmd.c
> > index 1e856259219e..aba024cbe7c5 100644
> > --- a/drivers/char/tpm/tpm2-cmd.c
> > +++ b/drivers/char/tpm/tpm2-cmd.c
> > @@ -364,6 +364,9 @@ void tpm2_flush_context(struct tpm_chip *chip, u32
> > handle)
> > struct tpm_buf buf;
> > int rc;
> >
> > + if (!handle)
> > + return;
> > +
>
> wouldn't be necessary with inline.
True!
>
> > rc = tpm_buf_init(&buf, TPM2_ST_NO_SESSIONS, TPM2_CC_FLUSH_CONTEXT);
> > if (rc) {
> > dev_warn(&chip->dev, "0x%08x was not flushed, out of memory\n",
> > diff --git a/drivers/char/tpm/tpm2-sessions.c
> > b/drivers/char/tpm/tpm2-sessions.c
> > index bdac11964b55..78c650ce4c9f 100644
> > --- a/drivers/char/tpm/tpm2-sessions.c
> > +++ b/drivers/char/tpm/tpm2-sessions.c
> > @@ -920,11 +920,19 @@ static int tpm2_load_null(struct tpm_chip *chip, u32
> > *null_key)
> > u32 tmp_null_key;
> > int rc;
> >
> > + /* fast path */
> > + if (chip->null_key) {
> > + *null_key = chip->null_key;
> > + return 0;
> > + }
> > +
> > rc = tpm2_load_context(chip, chip->null_key_context, &offset,
> > &tmp_null_key);
> > if (rc != -EINVAL) {
> > - if (!rc)
> > + if (!rc) {
> > + chip->null_key = tmp_null_key;
> > *null_key = tmp_null_key;
> > + }
> > goto err;
> > }
> >
> > @@ -934,6 +942,7 @@ static int tpm2_load_null(struct tpm_chip *chip, u32
> > *null_key)
> >
> > /* Return the null key if the name has not been changed: */
> > if (memcmp(name, chip->null_key_name, sizeof(name)) == 0) {
> > + chip->null_key = tmp_null_key;
> > *null_key = tmp_null_key;
> > return 0;
> > }
> > @@ -1006,7 +1015,6 @@ int tpm2_start_auth_session(struct tpm_chip *chip)
> > tpm_buf_append_u16(&buf, TPM_ALG_SHA256);
> >
> > rc = tpm_transmit_cmd(chip, &buf, 0, "start auth session");
> > - tpm2_flush_context(chip, null_key);
> >
> > if (rc == TPM2_RC_SUCCESS)
> > rc = tpm2_parse_start_auth_session(auth, &buf);
> > @@ -1338,7 +1346,10 @@ static int tpm2_create_null_primary(struct tpm_chip
> > *chip)
> >
> > rc = tpm2_save_context(chip, null_key, chip->null_key_context,
> > sizeof(chip->null_key_context), &offset);
> > - tpm2_flush_context(chip, null_key);
> > + if (rc)
> > + tpm2_flush_context(chip, null_key);
> > + else
> > + chip->null_key = null_key;
> > }
> >
> > return rc;
> > diff --git a/include/linux/tpm.h b/include/linux/tpm.h
> > index e93ee8d936a9..4eb39db80e05 100644
> > --- a/include/linux/tpm.h
> > +++ b/include/linux/tpm.h
> > @@ -205,6 +205,8 @@ struct tpm_chip {
> > #ifdef CONFIG_TCG_TPM2_HMAC
> > /* details for communication security via sessions */
> >
> > + /* loaded null key */
>
> nit: handle of loaded null key
>
> > + u32 null_key;
> > /* saved context for NULL seed */
> > u8 null_key_context[TPM2_MAX_CONTEXT_SIZE];
> > /* name of NULL seed */
I agree with all of your remarks. I'll send one more round because there
is enough changes. Thank you.
BR, Jarkko
