| 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: <D4ENBNZ5715P.1A1NWQMN9MC44@kernel.org>
Date: Tue, 24 Sep 2024 19:13:42 +0300
From: "Jarkko Sakkinen" <jarkko@...nel.org>
To: "James Bottomley" <James.Bottomley@...senPartnership.com>,
<linux-integrity@...r.kernel.org>
Cc: <roberto.sassu@...wei.com>, <mapengyu@...il.com>,
<stable@...r.kernel.org>, "Mimi Zohar" <zohar@...ux.ibm.com>, "David
Howells" <dhowells@...hat.com>, "Paul Moore" <paul@...l-moore.com>, "James
Morris" <jmorris@...ei.org>, "Serge E. Hallyn" <serge@...lyn.com>, "Peter
Huewe" <peterhuewe@....de>, "Jason Gunthorpe" <jgg@...pe.ca>,
<keyrings@...r.kernel.org>, <linux-security-module@...r.kernel.org>,
<linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v5 5/5] tpm: flush the auth session only when /dev/tpm0
is open
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@...il.com>
> > Closes: https://bugzilla.kernel.org/show_bug.cgi?id=219229
> > Cc: stable@...r.kernel.org # v6.10+
> > Fixes: 7ca110f2679b ("tpm: Address !chip->auth in
> > tpm_buf_append_hmac_session*()")
> > Tested-by: Pengyu Ma <mapengyu@...il.com>
> > Signed-off-by: Jarkko Sakkinen <jarkko@...nel.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@...senPartnership.com>
OK, thank you.
BR, Jarkko
Powered by blists - more mailing lists