| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-Id: <D53ZZEUWMSY9.2Y3DY0CA1MWZC@kernel.org>
Date: Thu, 24 Oct 2024 14:25:35 +0300
From: "Jarkko Sakkinen" <jarkko@...nel.org>
To: "Stefan Berger" <stefanb@...ux.ibm.com>,
<linux-integrity@...r.kernel.org>, "Peter Huewe" <peterhuewe@....de>,
"Jason Gunthorpe" <jgg@...pe.ca>, "James Bottomley"
<James.Bottomley@...senPartnership.com>
Cc: "David Howells" <dhowells@...hat.com>, "Mimi Zohar"
<zohar@...ux.ibm.com>, "Roberto Sassu" <roberto.sassu@...wei.com>,
<linux-kernel@...r.kernel.org>, <stable@...r.kernel.org>, "Pengyu Ma"
<mapengyu@...il.com>
Subject: Re: [PATCH v7 3/5] tpm: flush the null key only when /dev/tpm0 is
accessed
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: stable@...r.kernel.org # v6.10+
> > Fixes: d2add27cf2b8 ("tpm: Add NULL primary creation")
> > Tested-by: Pengyu Ma <mapengyu@...il.com>
> > Signed-off-by: Jarkko Sakkinen <jarkko@...nel.org>
> > ---
> > 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 <mapengyu@...il.com>.
> > - 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
Powered by blists - more mailing lists