.../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(-)
From: Mimi Zohar <zohar@linux.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.
In order to address this performance issue, introduce disable_encrypt_pcrs
kernel command-line parameter to the TPM driver.
Cc: James Bottomley <James.Bottomley@HansenPartnership.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@huawei.com>
Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
Signed-off-by: Mimi Zohar <zohar@linux.ibm.com>
Co-developed-by: Jarkko Sakkinen <jarkko@kernel.org>
Signed-off-by: Jarkko Sakkinen <jarkko@kernel.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.
---
.../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.
+
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);
+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");
+ 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);
static inline void tpm_buf_append_hmac_session_opt(struct tpm_chip *chip,
struct tpm_buf *buf,
u8 attributes,
--
2.47.0On Thu Nov 7, 2024 at 2:47 AM EET, Jarkko Sakkinen wrote:
> From: Mimi Zohar <zohar@linux.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.
>
> In order to address this performance issue, introduce disable_encrypt_pcrs
> kernel command-line parameter to the TPM driver.
>
> Cc: James Bottomley <James.Bottomley@HansenPartnership.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@huawei.com>
> Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
> Signed-off-by: Mimi Zohar <zohar@linux.ibm.com>
> Co-developed-by: Jarkko Sakkinen <jarkko@kernel.org>
> Signed-off-by: Jarkko Sakkinen <jarkko@kernel.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.
> ---
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.
> +
> 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);
> +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.
> static inline void tpm_buf_append_hmac_session_opt(struct tpm_chip *chip,
> struct tpm_buf *buf,
> u8 attributes,
BR, Jarkko
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@linux.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@HansenPartnership.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@huawei.com>
> > Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
> > Signed-off-by: Mimi Zohar <zohar@linux.ibm.com>
> > Co-developed-by: Jarkko Sakkinen <jarkko@kernel.org>
> > Signed-off-by: Jarkko Sakkinen <jarkko@kernel.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
>
On Thu Nov 7, 2024 at 4:48 AM EET, Mimi Zohar wrote: > 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@linux.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. I have to say I disagree with that. Encryption is the feature we get with HMAC and is more understandable for most. HMAC is implemnetation detail. BR, Jarkko
On Thu Nov 7, 2024 at 8:24 AM EET, Jarkko Sakkinen wrote: > On Thu Nov 7, 2024 at 4:48 AM EET, Mimi Zohar wrote: > > 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@linux.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. > > I have to say I disagree with that. Encryption is the feature we get > with HMAC and is more understandable for most. HMAC is implemnetation > detail. Sorry my bad. In the case of PCR extend SA_ENCRYPT is not passed. Well, that underlines my point tbh :-) I cannot know from HMAC whether it is encrypte or not, can I? I.e. open for any other word than encrypted or HMAC because other is wrong and other provides zero information content. BR, Jarkko
On Thu Nov 7, 2024 at 2:51 AM EET, Jarkko Sakkinen wrote: > On Thu Nov 7, 2024 at 2:47 AM EET, Jarkko Sakkinen wrote: > > From: Mimi Zohar <zohar@linux.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. > > > > In order to address this performance issue, introduce disable_encrypt_pcrs > > kernel command-line parameter to the TPM driver. > > > > Cc: James Bottomley <James.Bottomley@HansenPartnership.com> We also probably want this: Cc: stable@vger.kernel.org # v6.10+ BR, Jarkko
© 2016 - 2024 Red Hat, Inc.