The initial TPM2 HMAC session capability added HMAC authentication to
each and every TPM communication making the pcr_extend performance
abysmal for HW TPMs. Further, the new CONFIG_TCG_TPM2_HMAC option was
configured by default on x86_64.

The decision to use the TPM2 HMAC session capability feature doesn't
differentiate between the critical encrypted and the non-encrypted
communication, but when configured is required for all TPM communication.

In addition, the reason to HMAC the tpm2_pcr_extend() as provided in commit
6519fea6fd37 ("tpm: add hmac checks to tpm2_pcr_extend()") was to protect
tpm2_pcr_extend() when used by "trusted keys" to lock the PCR.  However,
locking the PCR is currently limited to TPM 1.2.

We can revert the commit which adds the HMAC sessions for
tpm2_pcr_extend, allow just the TPM2 pcr_extend HMAC capability to be
disabled on boot for better IMA performance, or define a generic boot
command line option to disable HMAC in general.  This patch allows
disabling the HMAC for just the TPM2_pcr_extend.

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>
---
Comment: applied and tested with/without patches in Jarkko's hmac-v5 branch -
commit 92999f9cd11f ("tpm: flush the auth session only when /dev/tpm0 is open")

 .../admin-guide/kernel-parameters.txt         |  5 ++
 drivers/char/tpm/tpm2-cmd.c                   | 41 ++++++++++---
 drivers/char/tpm/tpm2-sessions.c              | 59 +++++++++++--------
 include/linux/tpm.h                           |  4 ++
 4 files changed, 77 insertions(+), 32 deletions(-)

diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
index 1518343bbe22..c7811f32ba28 100644
--- a/Documentation/admin-guide/kernel-parameters.txt
+++ b/Documentation/admin-guide/kernel-parameters.txt
@@ -6727,6 +6727,11 @@
 	torture.verbose_sleep_duration= [KNL]
 			Duration of each verbose-printk() sleep in jiffies.
 
+	tpm_pcr_extend_hmac_disable [HW,TPM]
+			Disable TPM2 pcr_extend HMAC for better IMA
+			performance. By default is set to true (1).
+			Mainly needed when using a HW TPM2.
+
 	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 aba024cbe7c5..bac409520a72 100644
--- a/drivers/char/tpm/tpm2-cmd.c
+++ b/drivers/char/tpm/tpm2-cmd.c
@@ -14,6 +14,14 @@
 #include "tpm.h"
 #include <crypto/hash_info.h>
 
+static int __ro_after_init tpm2_pcr_extend_hmac = 1;
+static int __init tpm2_pcr_extend_hmac_setup(char *str)
+{
+	tpm2_pcr_extend_hmac = 0;
+	return 0;
+}
+__setup("tpm2_pcr_extend_hmac_disable", tpm2_pcr_extend_hmac_setup);
+
 static struct tpm2_hash tpm2_hash_map[] = {
 	{HASH_ALGO_SHA1, TPM_ALG_SHA1},
 	{HASH_ALGO_SHA256, TPM_ALG_SHA256},
@@ -232,18 +240,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 (tpm2_pcr_extend_hmac) {
+		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 (tpm2_pcr_extend_hmac)
+			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 (tpm2_pcr_extend_hmac) {
+		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 +269,16 @@ 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 (tpm2_pcr_extend_hmac) {
+		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);
+	} else {
+		rc = tpm_transmit_cmd(chip, &buf, 0,
+				      "attempting extend a PCR value");
+	}
+
 
 	tpm_buf_destroy(&buf);
 
diff --git a/drivers/char/tpm/tpm2-sessions.c b/drivers/char/tpm/tpm2-sessions.c
index de860773eead..fae56dfe0d92 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 28a932aa0416..d30bb1c114f1 100644
--- a/include/linux/tpm.h
+++ b/include/linux/tpm.h
@@ -504,9 +504,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.0

On Tue Oct 15, 2024 at 10:39 PM EEST, Mimi Zohar wrote:
> The initial TPM2 HMAC session capability added HMAC authentication to
> each and every TPM communication making the pcr_extend performance
> abysmal for HW TPMs. Further, the new CONFIG_TCG_TPM2_HMAC option was
> configured by default on x86_64.
>
> The decision to use the TPM2 HMAC session capability feature doesn't
> differentiate between the critical encrypted and the non-encrypted
> communication, but when configured is required for all TPM communication.
>
> In addition, the reason to HMAC the tpm2_pcr_extend() as provided in commit
> 6519fea6fd37 ("tpm: add hmac checks to tpm2_pcr_extend()") was to protect
> tpm2_pcr_extend() when used by "trusted keys" to lock the PCR.  However,
> locking the PCR is currently limited to TPM 1.2.
>
> We can revert the commit which adds the HMAC sessions for
> tpm2_pcr_extend, allow just the TPM2 pcr_extend HMAC capability to be
> disabled on boot for better IMA performance, or define a generic boot
> command line option to disable HMAC in general.  This patch allows
> disabling the HMAC for just the TPM2_pcr_extend.
>
> 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>

I have alternative proposal that hit me today.

First an observation: I think this issue shows that we also stress
beyond limits desktop configurations with encrypted bus, even tho it is
not in the same way visible. This affects bunch of things, including
e.g. power consumption. Not a lot but best possible situation would be
if callers could be served without any additional stress.

A second observation is in [1]: 

"It is recommended that a TPM implement the RNG in a manner that would
allow it to return RNG octets such that, as long as the value of
bytesRequested is not greater than the maximum digest size, the
frequency of bytesRequested being more than the number of octets
available is an infrequent occurrence."

I think from this we can derive a fair assumption that with any possible
TPM2 chip we can pull a 32 byte value within a single transcation (i.e.
matching SHA256 digest size).

So based on these facts I think this might be a sweet spot in making a
compromise between performance and security:

1. Generate a 32 byte seed every N iterations (calls of
   tpm2_get_random(). Store it to chip->random_seed.
2. In-between iterations use PRNG to generate the values
   starting form chip->random_seed.

I think N could be fairly large without causing any major difference
(even when analyzed through numerical error analysis) between calling
TPM2_GetRandom for each and every iteration. And this way bus encryption
never has to be disabled.

I'd see this as win-win approach.

PS. I have no idea what kind of PRNG's kernel provides (never used
such).

[1] 16.1.TPM2_GetRandom
    https://trustedcomputinggroup.org/wp-content/uploads/TPM-2.0-1.83-Part-3-Commands.pdf

BR, Jarkko

On Thu, 2024-11-07 at 00:26 +0200, Jarkko Sakkinen wrote:
> On Tue Oct 15, 2024 at 10:39 PM EEST, Mimi Zohar wrote:
> > The initial TPM2 HMAC session capability added HMAC authentication
> > to each and every TPM communication making the pcr_extend
> > performance abysmal for HW TPMs. Further, the new
> > CONFIG_TCG_TPM2_HMAC option was configured by default on x86_64.
> > 
> > The decision to use the TPM2 HMAC session capability feature
> > doesn't differentiate between the critical encrypted and the non-
> > encrypted communication, but when configured is required for all
> > TPM communication.
> > 
> > In addition, the reason to HMAC the tpm2_pcr_extend() as provided
> > in commit 6519fea6fd37 ("tpm: add hmac checks to
> > tpm2_pcr_extend()") was to protect tpm2_pcr_extend() when used by
> > "trusted keys" to lock the PCR.  However, locking the PCR is
> > currently limited to TPM 1.2.
> > 
> > We can revert the commit which adds the HMAC sessions for
> > tpm2_pcr_extend, allow just the TPM2 pcr_extend HMAC capability to
> > be disabled on boot for better IMA performance, or define a generic
> > boot command line option to disable HMAC in general.  This patch
> > allows disabling the HMAC for just the TPM2_pcr_extend.
> > 
> > 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>
> 
> I have alternative proposal that hit me today.
> 
> First an observation: I think this issue shows that we also stress
> beyond limits desktop configurations with encrypted bus, even tho it
> is
> not in the same way visible. This affects bunch of things, including
> e.g. power consumption. Not a lot but best possible situation would
> be
> if callers could be served without any additional stress.
> 
> A second observation is in [1]: 
> 
> "It is recommended that a TPM implement the RNG in a manner that
> would
> allow it to return RNG octets such that, as long as the value of
> bytesRequested is not greater than the maximum digest size, the
> frequency of bytesRequested being more than the number of octets
> available is an infrequent occurrence."
> 
> I think from this we can derive a fair assumption that with any
> possible
> TPM2 chip we can pull a 32 byte value within a single transcation
> (i.e.
> matching SHA256 digest size).
> 
> So based on these facts I think this might be a sweet spot in making
> a
> compromise between performance and security:
> 
> 1. Generate a 32 byte seed every N iterations (calls of
>    tpm2_get_random(). Store it to chip->random_seed.
> 2. In-between iterations use PRNG to generate the values
>    starting form chip->random_seed.
> 
> I think N could be fairly large without causing any major difference
> (even when analyzed through numerical error analysis) between calling
> TPM2_GetRandom for each and every iteration. And this way bus
> encryption
> never has to be disabled.
> 
> I'd see this as win-win approach.
> 
> PS. I have no idea what kind of PRNG's kernel provides (never used
> such).
> 
> [1] 16.1.TPM2_GetRandom
>    
> https://trustedcomputinggroup.org/wp-content/uploads/TPM-2.0-1.83-Part-3-Commands.pdf

I'm a bit confused here.  It's TPM2_PCR_Extend we have the trouble with
(as Mimi says in her email that you quoted) not TPM2_GetRandom.

The random number generator reseed occurs in a kernel thread that fires
about once a minute, so it doesn't show up in really any of the boot
timings.  Plus even with sessions added, what there now isn't a
significant overhead even to the running kernel given it's asynchronous
and called infrequently.

James

On Thu Nov 7, 2024 at 12:52 AM EET, James Bottomley wrote:
> On Thu, 2024-11-07 at 00:26 +0200, Jarkko Sakkinen wrote:
> > On Tue Oct 15, 2024 at 10:39 PM EEST, Mimi Zohar wrote:
> > > The initial TPM2 HMAC session capability added HMAC authentication
> > > to each and every TPM communication making the pcr_extend
> > > performance abysmal for HW TPMs. Further, the new
> > > CONFIG_TCG_TPM2_HMAC option was configured by default on x86_64.
> > > 
> > > The decision to use the TPM2 HMAC session capability feature
> > > doesn't differentiate between the critical encrypted and the non-
> > > encrypted communication, but when configured is required for all
> > > TPM communication.
> > > 
> > > In addition, the reason to HMAC the tpm2_pcr_extend() as provided
> > > in commit 6519fea6fd37 ("tpm: add hmac checks to
> > > tpm2_pcr_extend()") was to protect tpm2_pcr_extend() when used by
> > > "trusted keys" to lock the PCR.  However, locking the PCR is
> > > currently limited to TPM 1.2.
> > > 
> > > We can revert the commit which adds the HMAC sessions for
> > > tpm2_pcr_extend, allow just the TPM2 pcr_extend HMAC capability to
> > > be disabled on boot for better IMA performance, or define a generic
> > > boot command line option to disable HMAC in general.  This patch
> > > allows disabling the HMAC for just the TPM2_pcr_extend.
> > > 
> > > 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>
> > 
> > I have alternative proposal that hit me today.
> > 
> > First an observation: I think this issue shows that we also stress
> > beyond limits desktop configurations with encrypted bus, even tho it
> > is
> > not in the same way visible. This affects bunch of things, including
> > e.g. power consumption. Not a lot but best possible situation would
> > be
> > if callers could be served without any additional stress.
> > 
> > A second observation is in [1]: 
> > 
> > "It is recommended that a TPM implement the RNG in a manner that
> > would
> > allow it to return RNG octets such that, as long as the value of
> > bytesRequested is not greater than the maximum digest size, the
> > frequency of bytesRequested being more than the number of octets
> > available is an infrequent occurrence."
> > 
> > I think from this we can derive a fair assumption that with any
> > possible
> > TPM2 chip we can pull a 32 byte value within a single transcation
> > (i.e.
> > matching SHA256 digest size).
> > 
> > So based on these facts I think this might be a sweet spot in making
> > a
> > compromise between performance and security:
> > 
> > 1. Generate a 32 byte seed every N iterations (calls of
> >    tpm2_get_random(). Store it to chip->random_seed.
> > 2. In-between iterations use PRNG to generate the values
> >    starting form chip->random_seed.
> > 
> > I think N could be fairly large without causing any major difference
> > (even when analyzed through numerical error analysis) between calling
> > TPM2_GetRandom for each and every iteration. And this way bus
> > encryption
> > never has to be disabled.
> > 
> > I'd see this as win-win approach.
> > 
> > PS. I have no idea what kind of PRNG's kernel provides (never used
> > such).
> > 
> > [1] 16.1.TPM2_GetRandom
> >    
> > https://trustedcomputinggroup.org/wp-content/uploads/TPM-2.0-1.83-Part-3-Commands.pdf
>
> I'm a bit confused here.  It's TPM2_PCR_Extend we have the trouble with
> (as Mimi says in her email that you quoted) not TPM2_GetRandom.
>
> The random number generator reseed occurs in a kernel thread that fires
> about once a minute, so it doesn't show up in really any of the boot
> timings.  Plus even with sessions added, what there now isn't a
> significant overhead even to the running kernel given it's asynchronous
> and called infrequently.

Ah, right then we need the boot flag, and my earlier comments to the
parameter apply. I've never used IMA so I don't actually even know in
detail how it is using TPM.

Now that I did some seek I mixed this up with the report:

https://chaos.social/@gromit/113345582873908273

Anyway concerning this issue and patch, my earlier comments still apply.

BR, Jarkko

On Thu, 2024-11-07 at 01:22 +0200, Jarkko Sakkinen wrote:
> On Thu Nov 7, 2024 at 12:52 AM EET, James Bottomley wrote:
> > 
> > I'm a bit confused here.  It's TPM2_PCR_Extend we have the trouble with
> > (as Mimi says in her email that you quoted) not TPM2_GetRandom.
> > 
> > The random number generator reseed occurs in a kernel thread that fires
> > about once a minute, so it doesn't show up in really any of the boot
> > timings.  Plus even with sessions added, what there now isn't a
> > significant overhead even to the running kernel given it's asynchronous
> > and called infrequently.
> 
> Ah, right then we need the boot flag, and my earlier comments to the
> parameter apply. I've never used IMA so I don't actually even know in
> detail how it is using TPM.

Huh?  A simple explanation is that IMA-measurement maintains a measurement list,
similar to the pre-boot event log.  Each IMA-measurement record extends the TPM
PCR (default PCR 10).

Assuming IMA is enabled in the kernel, then just add "ima_policy=tcb" or
"ima_policy=critical_data" on the boot command line.  To view the measurement
records, cat <securityfs>/integrity/ima/ascii_runtime_measurements.  Normally
the IMA policy specified on the boot command line is replaced with a finer
grained custom policy.

Mimi

On Thu Nov 7, 2024 at 1:52 AM EET, Mimi Zohar wrote:
> On Thu, 2024-11-07 at 01:22 +0200, Jarkko Sakkinen wrote:
> > On Thu Nov 7, 2024 at 12:52 AM EET, James Bottomley wrote:
> > > 
> > > I'm a bit confused here.  It's TPM2_PCR_Extend we have the trouble with
> > > (as Mimi says in her email that you quoted) not TPM2_GetRandom.
> > > 
> > > The random number generator reseed occurs in a kernel thread that fires
> > > about once a minute, so it doesn't show up in really any of the boot
> > > timings.  Plus even with sessions added, what there now isn't a
> > > significant overhead even to the running kernel given it's asynchronous
> > > and called infrequently.
> > 
> > Ah, right then we need the boot flag, and my earlier comments to the
> > parameter apply. I've never used IMA so I don't actually even know in
> > detail how it is using TPM.
>
> Huh?  A simple explanation is that IMA-measurement maintains a measurement list,
> similar to the pre-boot event log.  Each IMA-measurement record extends the TPM
> PCR (default PCR 10).
>
> Assuming IMA is enabled in the kernel, then just add "ima_policy=tcb" or
> "ima_policy=critical_data" on the boot command line.  To view the measurement
> records, cat <securityfs>/integrity/ima/ascii_runtime_measurements.  Normally
> the IMA policy specified on the boot command line is replaced with a finer
> grained custom policy.

I'll try to figure out how to test it regularly. And yeah we need the
flag obviously.

I have my (CI compatible) framework that I run regularly with upstream
that I've mentioned a few times earlier.

https://codeberg.org/jarkko/linux-tpmdd-test

How would I would make all files in /etc get to get the checksums, and
how can I generate legit and illegit change to some file in that tree?

No need to address how to implement that to my framework, I can figure
that out. I just would love throw something so that any performance
regressions will be catched right at the get go, i.e. before they
end up to the mainline.

> Mimi

BR, Jarkko

On Thu, 2024-11-07 at 02:03 +0200, Jarkko Sakkinen wrote:
> On Thu Nov 7, 2024 at 1:52 AM EET, Mimi Zohar wrote:
> > On Thu, 2024-11-07 at 01:22 +0200, Jarkko Sakkinen wrote:
> > > On Thu Nov 7, 2024 at 12:52 AM EET, James Bottomley wrote:
> > > > 
> > > > I'm a bit confused here.  It's TPM2_PCR_Extend we have the trouble with
> > > > (as Mimi says in her email that you quoted) not TPM2_GetRandom.
> > > > 
> > > > The random number generator reseed occurs in a kernel thread that fires
> > > > about once a minute, so it doesn't show up in really any of the boot
> > > > timings.  Plus even with sessions added, what there now isn't a
> > > > significant overhead even to the running kernel given it's asynchronous
> > > > and called infrequently.
> > > 
> > > Ah, right then we need the boot flag, and my earlier comments to the
> > > parameter apply. I've never used IMA so I don't actually even know in
> > > detail how it is using TPM.
> > 
> > Huh?  A simple explanation is that IMA-measurement maintains a measurement list,
> > similar to the pre-boot event log.  Each IMA-measurement record extends the TPM
> > PCR (default PCR 10).
> > 
> > Assuming IMA is enabled in the kernel, then just add "ima_policy=tcb" or
> > "ima_policy=critical_data" on the boot command line.  To view the measurement
> > records, cat <securityfs>/integrity/ima/ascii_runtime_measurements.  Normally
> > the IMA policy specified on the boot command line is replaced with a finer
> > grained custom policy.
> 
> I'll try to figure out how to test it regularly. And yeah we need the
> flag obviously.
> 
> I have my (CI compatible) framework that I run regularly with upstream
> that I've mentioned a few times earlier.
> 
> https://codeberg.org/jarkko/linux-tpmdd-test
> 
> How would I would make all files in /etc get to get the checksums, and
> how can I generate legit and illegit change to some file in that tree?
> 
> No need to address how to implement that to my framework, I can figure
> that out. I just would love throw something so that any performance
> regressions will be catched right at the get go, i.e. before they
> end up to the mainline.

Yes, I still need to look at it.  FYI, the IMA policy cannot be defined in terms
of pathnames.  For testing, we've been loopback mounting a filesystem and
defining policy rules based on the UUID of the filesystem.  If you're using
SELinux, then rules can be defined in terms of SELinux labels. There are other
methods of identifying files.  Ken's been working on new IMA documentation[1],
which can be viewed here
https://ima-doc.readthedocs.io/en/latest/ima-concepts.html .

Here are some examples as to how to locally verify the IMA measurement list and
the boot aggregate.

1. To locally verify the IMA measurement list matches TPM PCR-10, use evmctl
(ima-evm-utils).  For example,

a. An IMA measurement list without integrity violations
(/sys/kernel/security/ima/violations)

evmctl ima_measurement /sys/kernel/security/ima/binary_runtime_measurements

b. An IMA measurement list with integrity violations

evmctl ima_measurement --ignore-violations
/sys/kernel/security/ima/binary_runtime_measurements

2. To locally verify the 'boot_aggregate' record, the first record in the IMA
measurement list, use "evmctl ima_boot_aggregate -v" and compare the resulting
hash with the one in the boot_aggregate record.
  
[1] https://github.com/linux-integrity/ima-doc
[2] https://github.com/linux-integrity/ima-evm-utils/tree/next-testing/

Mimi

On Thu Nov 7, 2024 at 3:07 AM EET, Mimi Zohar wrote:
> On Thu, 2024-11-07 at 02:03 +0200, Jarkko Sakkinen wrote:
> > On Thu Nov 7, 2024 at 1:52 AM EET, Mimi Zohar wrote:
> > > On Thu, 2024-11-07 at 01:22 +0200, Jarkko Sakkinen wrote:
> > > > On Thu Nov 7, 2024 at 12:52 AM EET, James Bottomley wrote:
> > > > > 
> > > > > I'm a bit confused here.  It's TPM2_PCR_Extend we have the trouble with
> > > > > (as Mimi says in her email that you quoted) not TPM2_GetRandom.
> > > > > 
> > > > > The random number generator reseed occurs in a kernel thread that fires
> > > > > about once a minute, so it doesn't show up in really any of the boot
> > > > > timings.  Plus even with sessions added, what there now isn't a
> > > > > significant overhead even to the running kernel given it's asynchronous
> > > > > and called infrequently.
> > > > 
> > > > Ah, right then we need the boot flag, and my earlier comments to the
> > > > parameter apply. I've never used IMA so I don't actually even know in
> > > > detail how it is using TPM.
> > > 
> > > Huh?  A simple explanation is that IMA-measurement maintains a measurement list,
> > > similar to the pre-boot event log.  Each IMA-measurement record extends the TPM
> > > PCR (default PCR 10).
> > > 
> > > Assuming IMA is enabled in the kernel, then just add "ima_policy=tcb" or
> > > "ima_policy=critical_data" on the boot command line.  To view the measurement
> > > records, cat <securityfs>/integrity/ima/ascii_runtime_measurements.  Normally
> > > the IMA policy specified on the boot command line is replaced with a finer
> > > grained custom policy.
> > 
> > I'll try to figure out how to test it regularly. And yeah we need the
> > flag obviously.
> > 
> > I have my (CI compatible) framework that I run regularly with upstream
> > that I've mentioned a few times earlier.
> > 
> > https://codeberg.org/jarkko/linux-tpmdd-test
> > 
> > How would I would make all files in /etc get to get the checksums, and
> > how can I generate legit and illegit change to some file in that tree?
> > 
> > No need to address how to implement that to my framework, I can figure
> > that out. I just would love throw something so that any performance
> > regressions will be catched right at the get go, i.e. before they
> > end up to the mainline.
>
> Yes, I still need to look at it.  FYI, the IMA policy cannot be defined in terms
> of pathnames.  For testing, we've been loopback mounting a filesystem and
> defining policy rules based on the UUID of the filesystem.  If you're using
> SELinux, then rules can be defined in terms of SELinux labels. There are other
> methods of identifying files.  Ken's been working on new IMA documentation[1],
> which can be viewed here
> https://ima-doc.readthedocs.io/en/latest/ima-concepts.html .
>
> Here are some examples as to how to locally verify the IMA measurement list and
> the boot aggregate.
>
> 1. To locally verify the IMA measurement list matches TPM PCR-10, use evmctl
> (ima-evm-utils).  For example,
>
> a. An IMA measurement list without integrity violations
> (/sys/kernel/security/ima/violations)
>
> evmctl ima_measurement /sys/kernel/security/ima/binary_runtime_measurements
>
> b. An IMA measurement list with integrity violations
>
> evmctl ima_measurement --ignore-violations
> /sys/kernel/security/ima/binary_runtime_measurements
>
> 2. To locally verify the 'boot_aggregate' record, the first record in the IMA
> measurement list, use "evmctl ima_boot_aggregate -v" and compare the resulting
> hash with the one in the boot_aggregate record.

Thanks! I write an issue based on this to my Codeberg repository, and
purge it once the time. I'll start by that and later on formalize
some commits or perhaps IMA specific buildroot config...

As far as the patch goes, I thought that I refine the patch myself, and
save everyone's time and nervers from unnecessary reviews rounds. It
does not make any radical changes to the approach.

See https://lore.kernel.org/linux-integrity/20241107004708.108667-1-jarkko@kernel.org/

I cannot take reviewed/tested-by's from any of the authors but if you
can check that it works for you I can surely send it Linus without
further tags than three SOB's :-) That said happy to get at least
tested-by from someone.

I'll send a PR to Linus as soon as possible.

>   
> [1] https://github.com/linux-integrity/ima-doc
> [2] https://github.com/linux-integrity/ima-evm-utils/tree/next-testing/
>
> Mimi

BR, Jarkko

On Thu, 2024-11-07 at 03:55 +0200, Jarkko Sakkinen wrote:
> On Thu Nov 7, 2024 at 3:07 AM EET, Mimi Zohar wrote:
> > On Thu, 2024-11-07 at 02:03 +0200, Jarkko Sakkinen wrote:
> > > On Thu Nov 7, 2024 at 1:52 AM EET, Mimi Zohar wrote:
> > > > On Thu, 2024-11-07 at 01:22 +0200, Jarkko Sakkinen wrote:
> > > > > On Thu Nov 7, 2024 at 12:52 AM EET, James Bottomley wrote:
> > > > > > 
> > > > > > I'm a bit confused here.  It's TPM2_PCR_Extend we have the trouble with
> > > > > > (as Mimi says in her email that you quoted) not TPM2_GetRandom.
> > > > > > 
> > > > > > The random number generator reseed occurs in a kernel thread that fires
> > > > > > about once a minute, so it doesn't show up in really any of the boot
> > > > > > timings.  Plus even with sessions added, what there now isn't a
> > > > > > significant overhead even to the running kernel given it's asynchronous
> > > > > > and called infrequently.
> > > > > 
> > > > > Ah, right then we need the boot flag, and my earlier comments to the
> > > > > parameter apply. I've never used IMA so I don't actually even know in
> > > > > detail how it is using TPM.
> > > > 
> > > > Huh?  A simple explanation is that IMA-measurement maintains a measurement list,
> > > > similar to the pre-boot event log.  Each IMA-measurement record extends the TPM
> > > > PCR (default PCR 10).
> > > > 
> > > > Assuming IMA is enabled in the kernel, then just add "ima_policy=tcb" or
> > > > "ima_policy=critical_data" on the boot command line.  To view the measurement
> > > > records, cat <securityfs>/integrity/ima/ascii_runtime_measurements.  Normally
> > > > the IMA policy specified on the boot command line is replaced with a finer
> > > > grained custom policy.
> > > 
> > > I'll try to figure out how to test it regularly. And yeah we need the
> > > flag obviously.
> > > 
> > > I have my (CI compatible) framework that I run regularly with upstream
> > > that I've mentioned a few times earlier.
> > > 
> > > https://codeberg.org/jarkko/linux-tpmdd-test
> > > 
> > > How would I would make all files in /etc get to get the checksums, and
> > > how can I generate legit and illegit change to some file in that tree?
> > > 
> > > No need to address how to implement that to my framework, I can figure
> > > that out. I just would love throw something so that any performance
> > > regressions will be catched right at the get go, i.e. before they
> > > end up to the mainline.
> > 
> > Yes, I still need to look at it.  FYI, the IMA policy cannot be defined in terms
> > of pathnames.  For testing, we've been loopback mounting a filesystem and
> > defining policy rules based on the UUID of the filesystem.  If you're using
> > SELinux, then rules can be defined in terms of SELinux labels. There are other
> > methods of identifying files.  Ken's been working on new IMA documentation[1],
> > which can be viewed here
> > https://ima-doc.readthedocs.io/en/latest/ima-concepts.html .
> > 
> > Here are some examples as to how to locally verify the IMA measurement list and
> > the boot aggregate.
> > 
> > 1. To locally verify the IMA measurement list matches TPM PCR-10, use evmctl
> > (ima-evm-utils).  For example,
> > 
> > a. An IMA measurement list without integrity violations
> > (/sys/kernel/security/ima/violations)
> > 
> > evmctl ima_measurement /sys/kernel/security/ima/binary_runtime_measurements
> > 
> > b. An IMA measurement list with integrity violations
> > 
> > evmctl ima_measurement --ignore-violations
> > /sys/kernel/security/ima/binary_runtime_measurements
> > 
> > 2. To locally verify the 'boot_aggregate' record, the first record in the IMA
> > measurement list, use "evmctl ima_boot_aggregate -v" and compare the resulting
> > hash with the one in the boot_aggregate record.
> 
> Thanks! I write an issue based on this to my Codeberg repository, and
> purge it once the time. I'll start by that and later on formalize
> some commits or perhaps IMA specific buildroot config...

Another important test would to be to make sure that IMA doesn't go into "TPM-
bypass" mode, which happens when the TPM initialization is for some reason
delayed.

https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/security/integrity/ima/ima_init.c#n124
> As far as the patch goes, I thought that I refine the patch myself, and
> save everyone's time and nervers from unnecessary reviews rounds. It
> does not make any radical changes to the approach.

Thanks
> 
> See https://lore.kernel.org/linux-integrity/20241107004708.108667-1-jarkko@kernel.org/
> 
> I cannot take reviewed/tested-by's from any of the authors but if you
> can check that it works for you I can surely send it Linus without
> further tags than three SOB's :-) That said happy to get at least
> tested-by from someone.

Our emails crossed.  I suggested removing the word "encrypted" throughout the
patch, as pcr_extend isn't encrypted, just HMAC'ed.

I'll re-test first thing tomorrow morning. Does the module_param require a value
or is specifying the name on the boot command line enough?

> 
> I'll send a PR to Linus as soon as possible.

Ok
> 
> >   
> > [1] https://github.com/linux-integrity/ima-doc
> > [2] https://github.com/linux-integrity/ima-evm-utils/tree/next-testing/

thanks,

Mimi

On Thu Nov 7, 2024 at 5:14 AM EET, Mimi Zohar wrote:
> On Thu, 2024-11-07 at 03:55 +0200, Jarkko Sakkinen wrote:
> > On Thu Nov 7, 2024 at 3:07 AM EET, Mimi Zohar wrote:
> > > On Thu, 2024-11-07 at 02:03 +0200, Jarkko Sakkinen wrote:
> > > > On Thu Nov 7, 2024 at 1:52 AM EET, Mimi Zohar wrote:
> > > > > On Thu, 2024-11-07 at 01:22 +0200, Jarkko Sakkinen wrote:
> > > > > > On Thu Nov 7, 2024 at 12:52 AM EET, James Bottomley wrote:
> > > > > > > 
> > > > > > > I'm a bit confused here.  It's TPM2_PCR_Extend we have the trouble with
> > > > > > > (as Mimi says in her email that you quoted) not TPM2_GetRandom.
> > > > > > > 
> > > > > > > The random number generator reseed occurs in a kernel thread that fires
> > > > > > > about once a minute, so it doesn't show up in really any of the boot
> > > > > > > timings.  Plus even with sessions added, what there now isn't a
> > > > > > > significant overhead even to the running kernel given it's asynchronous
> > > > > > > and called infrequently.
> > > > > > 
> > > > > > Ah, right then we need the boot flag, and my earlier comments to the
> > > > > > parameter apply. I've never used IMA so I don't actually even know in
> > > > > > detail how it is using TPM.
> > > > > 
> > > > > Huh?  A simple explanation is that IMA-measurement maintains a measurement list,
> > > > > similar to the pre-boot event log.  Each IMA-measurement record extends the TPM
> > > > > PCR (default PCR 10).
> > > > > 
> > > > > Assuming IMA is enabled in the kernel, then just add "ima_policy=tcb" or
> > > > > "ima_policy=critical_data" on the boot command line.  To view the measurement
> > > > > records, cat <securityfs>/integrity/ima/ascii_runtime_measurements.  Normally
> > > > > the IMA policy specified on the boot command line is replaced with a finer
> > > > > grained custom policy.
> > > > 
> > > > I'll try to figure out how to test it regularly. And yeah we need the
> > > > flag obviously.
> > > > 
> > > > I have my (CI compatible) framework that I run regularly with upstream
> > > > that I've mentioned a few times earlier.
> > > > 
> > > > https://codeberg.org/jarkko/linux-tpmdd-test
> > > > 
> > > > How would I would make all files in /etc get to get the checksums, and
> > > > how can I generate legit and illegit change to some file in that tree?
> > > > 
> > > > No need to address how to implement that to my framework, I can figure
> > > > that out. I just would love throw something so that any performance
> > > > regressions will be catched right at the get go, i.e. before they
> > > > end up to the mainline.
> > > 
> > > Yes, I still need to look at it.  FYI, the IMA policy cannot be defined in terms
> > > of pathnames.  For testing, we've been loopback mounting a filesystem and
> > > defining policy rules based on the UUID of the filesystem.  If you're using
> > > SELinux, then rules can be defined in terms of SELinux labels. There are other
> > > methods of identifying files.  Ken's been working on new IMA documentation[1],
> > > which can be viewed here
> > > https://ima-doc.readthedocs.io/en/latest/ima-concepts.html .
> > > 
> > > Here are some examples as to how to locally verify the IMA measurement list and
> > > the boot aggregate.
> > > 
> > > 1. To locally verify the IMA measurement list matches TPM PCR-10, use evmctl
> > > (ima-evm-utils).  For example,
> > > 
> > > a. An IMA measurement list without integrity violations
> > > (/sys/kernel/security/ima/violations)
> > > 
> > > evmctl ima_measurement /sys/kernel/security/ima/binary_runtime_measurements
> > > 
> > > b. An IMA measurement list with integrity violations
> > > 
> > > evmctl ima_measurement --ignore-violations
> > > /sys/kernel/security/ima/binary_runtime_measurements
> > > 
> > > 2. To locally verify the 'boot_aggregate' record, the first record in the IMA
> > > measurement list, use "evmctl ima_boot_aggregate -v" and compare the resulting
> > > hash with the one in the boot_aggregate record.
> > 
> > Thanks! I write an issue based on this to my Codeberg repository, and
> > purge it once the time. I'll start by that and later on formalize
> > some commits or perhaps IMA specific buildroot config...
>
> Another important test would to be to make sure that IMA doesn't go into "TPM-
> bypass" mode, which happens when the TPM initialization is for some reason
> delayed.
>
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/security/integrity/ima/ima_init.c#n124
> > As far as the patch goes, I thought that I refine the patch myself, and
> > save everyone's time and nervers from unnecessary reviews rounds. It
> > does not make any radical changes to the approach.
>
> Thanks
> > 
> > See https://lore.kernel.org/linux-integrity/20241107004708.108667-1-jarkko@kernel.org/
> > 
> > I cannot take reviewed/tested-by's from any of the authors but if you
> > can check that it works for you I can surely send it Linus without
> > further tags than three SOB's :-) That said happy to get at least
> > tested-by from someone.
>
> Our emails crossed.  I suggested removing the word "encrypted" throughout the
> patch, as pcr_extend isn't encrypted, just HMAC'ed.

Well me getting this wrong, i.e. not noticing that in that case SA_ENCRYPT
is not passed, is a sign that hmac is wrong choice... I.e. my mistake
proves an argument here ;-)


>
> I'll re-test first thing tomorrow morning. Does the module_param require a value
> or is specifying the name on the boot command line enough?

Module params is what you should use in LKM's unless you have special
reason to use __setup(). It's a kernel-wide global where as this flag
is TPM driver only.

You need to just namespace it with "tpm." when passing throug kernel
command-line i.e. "tpm.pcr_integrity=1" (or whatever, open for naming
suggestions).

BR, Jarkko

On Thu Nov 7, 2024 at 1:22 AM EET, Jarkko Sakkinen wrote:
> > I'm a bit confused here.  It's TPM2_PCR_Extend we have the trouble with
> > (as Mimi says in her email that you quoted) not TPM2_GetRandom.
> >
> > The random number generator reseed occurs in a kernel thread that fires
> > about once a minute, so it doesn't show up in really any of the boot
> > timings.  Plus even with sessions added, what there now isn't a
> > significant overhead even to the running kernel given it's asynchronous
> > and called infrequently.
>
> Ah, right then we need the boot flag, and my earlier comments to the
> parameter apply. I've never used IMA so I don't actually even know in
> detail how it is using TPM.
>
> Now that I did some seek I mixed this up with the report:
>
> https://chaos.social/@gromit/113345582873908273
>
> Anyway concerning this issue and patch, my earlier comments still apply.

Makes me wonder tho why do we then export tpm_get_random() in the first
place? HWRNG does not needed that export, and the code does not have any
of the mentioned features.

BR, Jarkko

On Thu Nov 7, 2024 at 1:40 AM EET, Jarkko Sakkinen wrote:
> On Thu Nov 7, 2024 at 1:22 AM EET, Jarkko Sakkinen wrote:
> > > I'm a bit confused here.  It's TPM2_PCR_Extend we have the trouble with
> > > (as Mimi says in her email that you quoted) not TPM2_GetRandom.
> > >
> > > The random number generator reseed occurs in a kernel thread that fires
> > > about once a minute, so it doesn't show up in really any of the boot
> > > timings.  Plus even with sessions added, what there now isn't a
> > > significant overhead even to the running kernel given it's asynchronous
> > > and called infrequently.
> >
> > Ah, right then we need the boot flag, and my earlier comments to the
> > parameter apply. I've never used IMA so I don't actually even know in
> > detail how it is using TPM.
> >
> > Now that I did some seek I mixed this up with the report:
> >
> > https://chaos.social/@gromit/113345582873908273
> >
> > Anyway concerning this issue and patch, my earlier comments still apply.
>
> Makes me wonder tho why do we then export tpm_get_random() in the first
> place? HWRNG does not needed that export, and the code does not have any
> of the mentioned features.

I mean specifically the code for tpm_get_random() and tpm2_get_random().

BR, Jarkko

On Tue Oct 15, 2024 at 10:39 PM EEST, Mimi Zohar wrote:
> The initial TPM2 HMAC session capability added HMAC authentication to
> each and every TPM communication making the pcr_extend performance
> abysmal for HW TPMs. Further, the new CONFIG_TCG_TPM2_HMAC option was
> configured by default on x86_64.
>
> The decision to use the TPM2 HMAC session capability feature doesn't
> differentiate between the critical encrypted and the non-encrypted
> communication, but when configured is required for all TPM communication.
>
> In addition, the reason to HMAC the tpm2_pcr_extend() as provided in commit
> 6519fea6fd37 ("tpm: add hmac checks to tpm2_pcr_extend()") was to protect
> tpm2_pcr_extend() when used by "trusted keys" to lock the PCR.  However,
> locking the PCR is currently limited to TPM 1.2.
>
> We can revert the commit which adds the HMAC sessions for
> tpm2_pcr_extend, allow just the TPM2 pcr_extend HMAC capability to be
> disabled on boot for better IMA performance, or define a generic boot
> command line option to disable HMAC in general.  This patch allows
> disabling the HMAC for just the TPM2_pcr_extend.
>
> 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>
> ---
> Comment: applied and tested with/without patches in Jarkko's hmac-v5 branch -
> commit 92999f9cd11f ("tpm: flush the auth session only when /dev/tpm0 is open")
>
>  .../admin-guide/kernel-parameters.txt         |  5 ++
>  drivers/char/tpm/tpm2-cmd.c                   | 41 ++++++++++---
>  drivers/char/tpm/tpm2-sessions.c              | 59 +++++++++++--------
>  include/linux/tpm.h                           |  4 ++
>  4 files changed, 77 insertions(+), 32 deletions(-)
>
> diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
> index 1518343bbe22..c7811f32ba28 100644
> --- a/Documentation/admin-guide/kernel-parameters.txt
> +++ b/Documentation/admin-guide/kernel-parameters.txt
> @@ -6727,6 +6727,11 @@
>  	torture.verbose_sleep_duration= [KNL]
>  			Duration of each verbose-printk() sleep in jiffies.
>  
> +	tpm_pcr_extend_hmac_disable [HW,TPM]
> +			Disable TPM2 pcr_extend HMAC for better IMA
> +			performance. By default is set to true (1).
> +			Mainly needed when using a HW TPM2.

Thanks for doing this! I think the code change itself is pretty good but
maybe we should not emphasize HMAC per se (applies to config flag too but
it is what it is now) but instead that they are encrypted and integrity
protected.

I guess all these features intend to protect data from unintended and
physical access, like in common sense terms.

So like for any possible sysadmin and similar I think this would be something
that anyone could grab:

	tpm_disable_protect_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.

Perhaps a bit long but at least it is clear and helps to make the right choice.

BR, Jarkko

On Wed Oct 16, 2024 at 12:29 AM EEST, Jarkko Sakkinen wrote:
> On Tue Oct 15, 2024 at 10:39 PM EEST, Mimi Zohar wrote:
> > The initial TPM2 HMAC session capability added HMAC authentication to
> > each and every TPM communication making the pcr_extend performance
> > abysmal for HW TPMs. Further, the new CONFIG_TCG_TPM2_HMAC option was
> > configured by default on x86_64.
> >
> > The decision to use the TPM2 HMAC session capability feature doesn't
> > differentiate between the critical encrypted and the non-encrypted
> > communication, but when configured is required for all TPM communication.
> >
> > In addition, the reason to HMAC the tpm2_pcr_extend() as provided in commit
> > 6519fea6fd37 ("tpm: add hmac checks to tpm2_pcr_extend()") was to protect
> > tpm2_pcr_extend() when used by "trusted keys" to lock the PCR.  However,
> > locking the PCR is currently limited to TPM 1.2.
> >
> > We can revert the commit which adds the HMAC sessions for
> > tpm2_pcr_extend, allow just the TPM2 pcr_extend HMAC capability to be
> > disabled on boot for better IMA performance, or define a generic boot
> > command line option to disable HMAC in general.  This patch allows
> > disabling the HMAC for just the TPM2_pcr_extend.
> >
> > 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>
> > ---
> > Comment: applied and tested with/without patches in Jarkko's hmac-v5 branch -
> > commit 92999f9cd11f ("tpm: flush the auth session only when /dev/tpm0 is open")
> >
> >  .../admin-guide/kernel-parameters.txt         |  5 ++
> >  drivers/char/tpm/tpm2-cmd.c                   | 41 ++++++++++---
> >  drivers/char/tpm/tpm2-sessions.c              | 59 +++++++++++--------
> >  include/linux/tpm.h                           |  4 ++
> >  4 files changed, 77 insertions(+), 32 deletions(-)
> >
> > diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
> > index 1518343bbe22..c7811f32ba28 100644
> > --- a/Documentation/admin-guide/kernel-parameters.txt
> > +++ b/Documentation/admin-guide/kernel-parameters.txt
> > @@ -6727,6 +6727,11 @@
> >  	torture.verbose_sleep_duration= [KNL]
> >  			Duration of each verbose-printk() sleep in jiffies.
> >  
> > +	tpm_pcr_extend_hmac_disable [HW,TPM]
> > +			Disable TPM2 pcr_extend HMAC for better IMA
> > +			performance. By default is set to true (1).
> > +			Mainly needed when using a HW TPM2.
>
> Thanks for doing this! I think the code change itself is pretty good but
> maybe we should not emphasize HMAC per se (applies to config flag too but
> it is what it is now) but instead that they are encrypted and integrity
> protected.
>
> I guess all these features intend to protect data from unintended and
> physical access, like in common sense terms.
>
> So like for any possible sysadmin and similar I think this would be something
> that anyone could grab:
>
> 	tpm_disable_protect_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.
>
> Perhaps a bit long but at least it is clear and helps to make the right choice.

Back in 2018 at LA, I think it was LSS, there was BoF where this was
discussed I said that for me this feature does not necessarily make
sense since data centers tend to have armed guards, and not black hat
would ever take a even a minor risk of getting hole in the head :-)

After that the whole ecosystem has changed, especially thanks to what
Apple has done with their security chip and user friendly encrypted
boot process, and that has reflected to systemd and the use TPM2,
and thus as a feature bus protection has become relevant.

So also based on these old conclusions I had I fully agree that we
need such a flag to balance things between desktop/laptop and server
use cases, which are both quite relevant. E.g. just me personally
I really enjoy the experience of being able to boot my ThinkPad 
with encryption and without having to type a passphrase per
boot.

I.e. the buy-in part is totally addressed as far as I'm concerned :-)

BR, Jarkko