| 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: <b510e556621e3826dd6df043fde817192b9f12ea.camel@linux.ibm.com>
Date: Wed, 06 Nov 2024 21:48:16 -0500
From: Mimi Zohar <zohar@...ux.ibm.com>
To: Jarkko Sakkinen <jarkko@...nel.org>, linux-integrity@...r.kernel.org,
Jonathan Corbet <corbet@....net>, Peter Huewe <peterhuewe@....de>,
Jason
Gunthorpe <jgg@...pe.ca>,
James Bottomley
<James.Bottomley@...senPartnership.com>
Cc: Roberto Sassu <roberto.sassu@...wei.com>, linux-doc@...r.kernel.org,
linux-kernel@...r.kernel.org
Subject: Re: [PATCH] tpm: Opt-in in disable PCR encryption on TPM2 chips
On Thu, 2024-11-07 at 02:51 +0200, Jarkko Sakkinen wrote:
> On Thu Nov 7, 2024 at 2:47 AM EET, Jarkko Sakkinen wrote:
> > From: Mimi Zohar <zohar@...ux.ibm.com>
> >
> > The initial encrypted HMAC session feature added TPM bus encryption to
> > various in-kernel TPM operations. This can cause performance bottlenecks
> > with IMA, as it heavily utilizes PCR extend operations.
The patch Subject line and problem description aren't quite right. In the case
of TPM pcr_extend, the session isn't being encrypted, only HMAC'ed. According
to James, it's the HMAC itself that is causing the performance degradation. I
would remove the word "encrypted" throughout.
> >
> > In order to address this performance issue, introduce disable_encrypt_pcrs
> > kernel command-line parameter to the TPM driver.
> >
> > Cc: James Bottomley <James.Bottomley@...senPartnership.com>
> > Link: https://lore.kernel.org/linux-integrity/20241015193916.59964-1-zohar@linux.ibm.com/
> > Fixes: 6519fea6fd37 ("tpm: add hmac checks to tpm2_pcr_extend()")
> > Co-developed-by: Roberto Sassu <roberto.sassu@...wei.com>
> > Signed-off-by: Roberto Sassu <roberto.sassu@...wei.com>
> > Signed-off-by: Mimi Zohar <zohar@...ux.ibm.com>
> > Co-developed-by: Jarkko Sakkinen <jarkko@...nel.org>
> > Signed-off-by: Jarkko Sakkinen <jarkko@...nel.org>
> > ---
> > v1:
> > - Derived from the earlier RFC patch with a different parameter scope,
> > cleaner commit message and some other tweaks. I decided to create
> > something because I did not noticed any progress. Note only compile
> > tested as I wanted to get something quickly out.
Thanks, Jarkko. Does "parameter scope" refer to using module_param instead of
__setup?
> > ---
>
> Noticed a couple of things I missed after sending this (see below).
>
> > .../admin-guide/kernel-parameters.txt | 10 ++++
> > drivers/char/tpm/tpm2-cmd.c | 33 ++++++++---
> > drivers/char/tpm/tpm2-sessions.c | 59 +++++++++++--------
> > include/linux/tpm.h | 4 ++
> > 4 files changed, 74 insertions(+), 32 deletions(-)
> >
> > diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
> > index 1518343bbe22..e27517e1a26f 100644
> > --- a/Documentation/admin-guide/kernel-parameters.txt
> > +++ b/Documentation/admin-guide/kernel-parameters.txt
> > @@ -6727,6 +6727,16 @@
> > torture.verbose_sleep_duration= [KNL]
> > Duration of each verbose-printk() sleep in jiffies.
> >
> > + tpm.disable_encrypt_pcrs= [HW,TPM]
> > + Do not protect PCR registers from unintended physical
> > + access, or interposers in the bus by the means of
> > + having an encrypted and integrity protected session
> > + wrapped around TPM2_PCR_Extend command. Consider this
> > + in a situation where TPM is heavily utilized by
> > + IMA, thus protection causing a major performance hit,
> > + and the space where machines are deployed is by other
> > + means guarded.
Remove the word "encrypted".
> > +
> > tpm_suspend_pcr=[HW,TPM]
> > Format: integer pcr id
> > Specify that at suspend time, the tpm driver
> > diff --git a/drivers/char/tpm/tpm2-cmd.c b/drivers/char/tpm/tpm2-cmd.c
> > index 1e856259219e..6ec307b1cb99 100644
> > --- a/drivers/char/tpm/tpm2-cmd.c
> > +++ b/drivers/char/tpm/tpm2-cmd.c
> > @@ -14,6 +14,10 @@
> > #include "tpm.h"
> > #include <crypto/hash_info.h>
> >
> > +static bool disable_encrypt_pcrs;
> > +module_param(disable_encrypt_pcrs, bool, 0444);
The variable should probably be named something like disable_pcr_hmac_session.
> > +MODULE_PARM_DESC(disable_encrypt_pcrs, "Disable TPM2_PCR_Extend encryption");
> > +
> > static struct tpm2_hash tpm2_hash_map[] = {
> > {HASH_ALGO_SHA1, TPM_ALG_SHA1},
> > {HASH_ALGO_SHA256, TPM_ALG_SHA256},
> > @@ -232,18 +236,26 @@ int tpm2_pcr_extend(struct tpm_chip *chip, u32 pcr_idx,
> > int rc;
> > int i;
> >
> > - rc = tpm2_start_auth_session(chip);
> > - if (rc)
> > - return rc;
> > + if (!disable_encrypt_pcrs) {
> > + rc = tpm2_start_auth_session(chip);
> > + if (rc)
> > + return rc;
> > + }
> >
> > rc = tpm_buf_init(&buf, TPM2_ST_SESSIONS, TPM2_CC_PCR_EXTEND);
> > if (rc) {
> > - tpm2_end_auth_session(chip);
> > + if (!disable_encrypt_pcrs)
> > + tpm2_end_auth_session(chip);
> > return rc;
> > }
> >
> > - tpm_buf_append_name(chip, &buf, pcr_idx, NULL);
> > - tpm_buf_append_hmac_session(chip, &buf, 0, NULL, 0);
> > + if (!disable_encrypt_pcrs) {
> > + tpm_buf_append_name(chip, &buf, pcr_idx, NULL);
> > + tpm_buf_append_hmac_session(chip, &buf, 0, NULL, 0);
> > + } else {
> > + tpm_buf_append_handle(chip, &buf, pcr_idx, NULL);
> > + tpm_buf_append_auth(chip, &buf, 0, NULL, 0);
> > + }
> >
> > tpm_buf_append_u32(&buf, chip->nr_allocated_banks);
> >
> > @@ -253,9 +265,12 @@ int tpm2_pcr_extend(struct tpm_chip *chip, u32 pcr_idx,
> > chip->allocated_banks[i].digest_size);
> > }
> >
> > - tpm_buf_fill_hmac_session(chip, &buf);
> > - rc = tpm_transmit_cmd(chip, &buf, 0, "attempting extend a PCR value");
> > - rc = tpm_buf_check_hmac_response(chip, &buf, rc);
> > + if (!disable_encrypt_pcrs)
> > + tpm_buf_fill_hmac_session(chip, &buf);
> > + rc = tpm_transmit_cmd(chip, &buf, 0,
> > + "attempting extend a PCR value");
>
> Should be in a single line in order to minimize the diff.
>
> > + if (!disable_encrypt_pcrs)
> > + rc = tpm_buf_check_hmac_response(chip, &buf, rc);
> >
> > tpm_buf_destroy(&buf);
> >
> > diff --git a/drivers/char/tpm/tpm2-sessions.c b/drivers/char/tpm/tpm2-sessions.c
> > index 42df980168b6..02897debc3fa 100644
> > --- a/drivers/char/tpm/tpm2-sessions.c
> > +++ b/drivers/char/tpm/tpm2-sessions.c
> > @@ -205,6 +205,14 @@ static int tpm2_read_public(struct tpm_chip *chip, u32 handle, char *name)
> > }
> > #endif /* CONFIG_TCG_TPM2_HMAC */
> >
> > +void tpm_buf_append_handle(struct tpm_chip *chip, struct tpm_buf *buf,
> > + u32 handle, u8 *name)
> > +{
> > + tpm_buf_append_u32(buf, handle);
> > + /* count the number of handles in the upper bits of flags */
> > + buf->handles++;
> > +}
> > +
> > /**
> > * tpm_buf_append_name() - add a handle area to the buffer
> > * @chip: the TPM chip structure
> > @@ -237,9 +245,7 @@ void tpm_buf_append_name(struct tpm_chip *chip, struct tpm_buf *buf,
> > #endif
> >
> > if (!tpm2_chip_auth(chip)) {
> > - tpm_buf_append_u32(buf, handle);
> > - /* count the number of handles in the upper bits of flags */
> > - buf->handles++;
> > + tpm_buf_append_handle(chip, buf, handle, name);
> > return;
> > }
> >
> > @@ -272,6 +278,31 @@ void tpm_buf_append_name(struct tpm_chip *chip, struct tpm_buf *buf,
> > }
> > EXPORT_SYMBOL_GPL(tpm_buf_append_name);
> >
> > +void tpm_buf_append_auth(struct tpm_chip *chip, struct tpm_buf *buf,
> > + u8 attributes, u8 *passphrase, int passphrase_len)
> > +{
> > + /* offset tells us where the sessions area begins */
> > + int offset = buf->handles * 4 + TPM_HEADER_SIZE;
> > + u32 len = 9 + passphrase_len;
> > +
> > + if (tpm_buf_length(buf) != offset) {
> > + /* not the first session so update the existing length */
> > + len += get_unaligned_be32(&buf->data[offset]);
> > + put_unaligned_be32(len, &buf->data[offset]);
> > + } else {
> > + tpm_buf_append_u32(buf, len);
> > + }
> > + /* auth handle */
> > + tpm_buf_append_u32(buf, TPM2_RS_PW);
> > + /* nonce */
> > + tpm_buf_append_u16(buf, 0);
> > + /* attributes */
> > + tpm_buf_append_u8(buf, 0);
> > + /* passphrase */
> > + tpm_buf_append_u16(buf, passphrase_len);
> > + tpm_buf_append(buf, passphrase, passphrase_len);
> > +}
> > +
> > /**
> > * tpm_buf_append_hmac_session() - Append a TPM session element
> > * @chip: the TPM chip structure
> > @@ -309,26 +340,8 @@ void tpm_buf_append_hmac_session(struct tpm_chip *chip, struct tpm_buf *buf,
> > #endif
> >
> > if (!tpm2_chip_auth(chip)) {
> > - /* offset tells us where the sessions area begins */
> > - int offset = buf->handles * 4 + TPM_HEADER_SIZE;
> > - u32 len = 9 + passphrase_len;
> > -
> > - if (tpm_buf_length(buf) != offset) {
> > - /* not the first session so update the existing length */
> > - len += get_unaligned_be32(&buf->data[offset]);
> > - put_unaligned_be32(len, &buf->data[offset]);
> > - } else {
> > - tpm_buf_append_u32(buf, len);
> > - }
> > - /* auth handle */
> > - tpm_buf_append_u32(buf, TPM2_RS_PW);
> > - /* nonce */
> > - tpm_buf_append_u16(buf, 0);
> > - /* attributes */
> > - tpm_buf_append_u8(buf, 0);
> > - /* passphrase */
> > - tpm_buf_append_u16(buf, passphrase_len);
> > - tpm_buf_append(buf, passphrase, passphrase_len);
> > + tpm_buf_append_auth(chip, buf, attributes, passphrase,
> > + passphrase_len);
> > return;
> > }
> >
> > diff --git a/include/linux/tpm.h b/include/linux/tpm.h
> > index 587b96b4418e..4892cd004530 100644
> > --- a/include/linux/tpm.h
> > +++ b/include/linux/tpm.h
> > @@ -502,9 +502,13 @@ static inline struct tpm2_auth *tpm2_chip_auth(struct tpm_chip *chip)
> >
> > void tpm_buf_append_name(struct tpm_chip *chip, struct tpm_buf *buf,
> > u32 handle, u8 *name);
> > +void tpm_buf_append_handle(struct tpm_chip *chip, struct tpm_buf *buf,
> > + u32 handle, u8 *name);
> > void tpm_buf_append_hmac_session(struct tpm_chip *chip, struct tpm_buf *buf,
> > u8 attributes, u8 *passphrase,
> > int passphraselen);
> > +void tpm_buf_append_auth(struct tpm_chip *chip, struct tpm_buf *buf,
> > + u8 attributes, u8 *passphrase, int passphraselen);
>
> This is declared in wrong place as it has no outside callers. So I will
> move it to drivers/char/tpm/tpm.h instead. Please correct if I'm
> overlooking something.
Sure
>
> > static inline void tpm_buf_append_hmac_session_opt(struct tpm_chip *chip,
> > struct tpm_buf *buf,
> > u8 attributes,
>
>
> BR, Jarkko
>
Powered by blists - more mailing lists