Skip to contentSkip to conversation

tpm: Opt-in in disable PCR integrity protection

PATCH Superseded Jarkko Sakkinen 4 participants 14 messages Nov 7, 2024 – Nov 12, 2024

Subsystems (3)
Documentation Keys-trusted TPM device driver

v2 of 3 Superseded Latest is v3 →

lore
Share & tools
lore

Jarkko Sakkinen

author maintainer
#0 lore raw

PATCH v2 1/1 tpm: Opt-in in disable PCR integrity protection

The initial HMAC session feature added TPM bus encryption and/or integrity protection to various in-kernel TPM operations. This can cause performance bottlenecks with IMA, as it heavily utilizes PCR extend operations.

In order to mitigate this performance issue, introduce a kernel command-line parameter to the TPM driver for disabling the integrity protection for PCR extension.

Cc James Bottomley <[email protected]>
Link https://lore.kernel.org/linux-integrity/[email protected]/ open here
Fixes 6519fea6fd37 ("tpm: add hmac checks to tpm2_pcr_extend()")
Co-developed-by Roberto Sassu <[email protected]>
Signed-off-by Roberto Sassu <[email protected]>
Co-developed-by Mimi Zohar <[email protected]>
Signed-off-by Mimi Zohar <[email protected]>
Signed-off-by Jarkko Sakkinen <[email protected]>

---
v2:
- Move tpm_buf_append_handle() to the correct file, remove spurious parameter (name), include error on TPM2B and add documentation. Keep the declaration in linux/tpm.h despite not exported as it is easiest to maintain tpm_buf_* in a single header.
- Rename kernel command-line option as "disable_pcr_integrity_protection", as Mimi pointed out it does not carry SA_ENCRYPT flag.
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/tpm-buf.c                    | 20 ++++++++
 drivers/char/tpm/tpm2-cmd.c                   | 30 ++++++++---
 drivers/char/tpm/tpm2-sessions.c              | 51 ++++++++++---------
 include/linux/tpm.h                           |  3 ++
 5 files changed, 83 insertions(+), 31 deletions(-)
Documentation/admin-guide/kernel-parameters.txt +10 -0
index 1518343bbe22..9fc406b20a74 100644
@@ -6727,6 +6727,16 @@
 	torture.verbose_sleep_duration= [KNL]
 			Duration of each verbose-printk() sleep in jiffies.
 
+	tpm.disable_pcr_integrity_protection= [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
drivers/char/tpm/tpm-buf.c +20 -0
index cad0048bcc3c..e49a19fea3bd 100644
@@ -146,6 +146,26 @@ void tpm_buf_append_u32(struct tpm_buf *buf, const u32 value)
 }
 EXPORT_SYMBOL_GPL(tpm_buf_append_u32);
 
+/**
+ * tpm_buf_append_handle() - Add a handle
+ * @chip:	&tpm_chip instance
+ * @buf:	&tpm_buf instance
+ * @handle:	a TPM object handle
+ *
+ * Add a handle to the buffer, and increase the count tracking the number of
+ * handles in the command buffer. Works only for command buffers.
+ */
+void tpm_buf_append_handle(struct tpm_chip *chip, struct tpm_buf *buf, u32 handle)
+{
+	if (buf->flags & TPM_BUF_TPM2B) {
+		dev_err(&chip->dev, "Invalid buffer type (TPM2B)\n");
+		return;
+	}
+
+	tpm_buf_append_u32(buf, handle);
+	buf->handles++;
+}
+
 /**
  * tpm_buf_read() - Read from a TPM buffer
  * @buf:	&tpm_buf instance
drivers/char/tpm/tpm2-cmd.c +22 -8
index 1e856259219e..cc443bcf15e8 100644
@@ -14,6 +14,10 @@
 #include "tpm.h"
 #include <crypto/hash_info.h>
 
+static bool disable_pcr_integrity_protection;
+module_param(disable_pcr_integrity_protection, bool, 0444);
+MODULE_PARM_DESC(disable_pcr_integrity_protection, "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_pcr_integrity_protection) {
+		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_pcr_integrity_protection)
+			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_pcr_integrity_protection) {
+		tpm_buf_append_name(chip, &buf, pcr_idx);
+		tpm_buf_append_hmac_session(chip, &buf, 0, NULL, 0);
+	} else {
+		tpm_buf_append_handle(chip, &buf, pcr_idx);
+		tpm_buf_append_auth(chip, &buf, 0, NULL, 0);
+	}
 
 	tpm_buf_append_u32(&buf, chip->nr_allocated_banks);
 

@@ -253,9 +265,11 @@ int tpm2_pcr_extend(struct tpm_chip *chip, u32 pcr_idx,
 			       chip->allocated_banks[i].digest_size);
 	}
 
-	tpm_buf_fill_hmac_session(chip, &buf);
+	if (!disable_pcr_integrity_protection)
+		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_pcr_integrity_protection)
+		rc = tpm_buf_check_hmac_response(chip, &buf, rc);
 
 	tpm_buf_destroy(&buf);
drivers/char/tpm/tpm2-sessions.c +28 -23

69 lines not on this page · raw message

include/linux/tpm.h +3 -0
index 587b96b4418e..20a40ade8030 100644
@@ -421,6 +421,7 @@ void tpm_buf_append_u32(struct tpm_buf *buf, const u32 value);
 u8 tpm_buf_read_u8(struct tpm_buf *buf, off_t *offset);
 u16 tpm_buf_read_u16(struct tpm_buf *buf, off_t *offset);
 u32 tpm_buf_read_u32(struct tpm_buf *buf, off_t *offset);
+void tpm_buf_append_handle(struct tpm_chip *chip, struct tpm_buf *buf, u32 handle);
 
 /*
  * Check if TPM device is in the firmware upgrade mode.

@@ -505,6 +506,8 @@ void tpm_buf_append_name(struct tpm_chip *chip, struct tpm_buf *buf,
 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,
Signature · 1 line
2.47.0
maintainer
#1 lore raw

On Thu, 2024-11-07 at 11:51 +0200, Jarkko Sakkinen wrote:
[...]

Quoted 25 lines

+void tpm_buf_append_auth(struct tpm_chip *chip, struct tpm_buf *buf, +                        u8 attributes, u8 *passphrase, int passphrase_len)

drivers/char/tpm/tpm2-sessions.c +7 -0
+{
+       /* 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);

Review

Quoted 2 lines
(quoted diff) +2 -0
+}
+

The rest of the code looks fine, but if you're going to extract this as a separate function instead of doing the open coded struct tpm2_null_auth that was there originally, you should probably extract and use the tpm2_buf_append_auth() function in trusted_tpm2.c

James

author maintainer
#2 lore raw
Quoted 26 lines · James Bottomley

On Thu, 2024-11-07 at 11:51 +0200, Jarkko Sakkinen wrote:
[...]

Quoted 24 lines

+void tpm_buf_append_auth(struct tpm_chip *chip, struct tpm_buf *buf, +                        u8 attributes, u8 *passphrase, int passphrase_len)

drivers/char/tpm/tpm2-sessions.c +7 -0
+{
+       /* 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);

Quoted 8 lines

Review

Quoted 3 lines

+       tpm_buf_append(buf, passphrase, passphrase_len);

(quoted diff) +2 -0
+}
+

The rest of the code looks fine, but if you're going to extract this as a separate function instead of doing the open coded struct tpm2_null_auth that was there originally, you should probably extract and use the tpm2_buf_append_auth() function in trusted_tpm2.c

So this was straight up from Mimi's original patch :-)

Hmm... was there duplicate use for this in the patch? I'll check this.

Quoted 2 lines

James

BR, Jarkko

maintainer
#3 lore raw
Quoted 38 lines · Jarkko Sakkinen
Quoted 28 lines · James Bottomley

On Thu, 2024-11-07 at 11:51 +0200, Jarkko Sakkinen wrote:
[...]

Quoted 26 lines

+void tpm_buf_append_auth(struct tpm_chip *chip, struct tpm_buf *buf,
+                        u8 attributes, u8 *passphrase, int passphrase_len)

drivers/char/tpm/tpm2-sessions.c +7 -0
+{
+       /* 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);

Quoted 8 lines

Review

Quoted 2 lines
(quoted diff) +2 -0
+}
+

The rest of the code looks fine, but if you're going to extract this as a separate function instead of doing the open coded struct tpm2_null_auth that was there originally, you should probably extract and use the tpm2_buf_append_auth() function in trusted_tpm2.c

Quoted 1 line

So this was straight up from Mimi's original patch :-)

Yes, I had the same comment prepped for that too.

Quoted 2 lines

Hmm... was there duplicate use for this in the patch? I'll check this.

The original open coded the empty auth append with struct tpm2_null_auth since it's the only user. However, since we do have another user in trusted keys, it might make sense to consolidate.

James

maintainer
#4 lore raw
Quoted 39 lines · James Bottomley
Quoted 38 lines · Jarkko Sakkinen
Quoted 36 lines · James Bottomley

On Thu, 2024-11-07 at 11:51 +0200, Jarkko Sakkinen wrote:
[...]

Quoted 26 lines

+void tpm_buf_append_auth(struct tpm_chip *chip, struct tpm_buf *buf,
+                        u8 attributes, u8 *passphrase, int passphrase_len)

drivers/char/tpm/tpm2-sessions.c +7 -0
+{
+       /* 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);

Review

Quoted 2 lines
(quoted diff) +2 -0
+}
+

The rest of the code looks fine, but if you're going to extract this as a separate function instead of doing the open coded struct tpm2_null_auth that was there originally, you should probably extract and use the tpm2_buf_append_auth() function in trusted_tpm2.c

Quoted 3 lines
Quoted 1 line

So this was straight up from Mimi's original patch :-)

Yes, I had the same comment prepped for that too.

But it wasn't sent ...

Quoted 7 lines
Quoted 3 lines

Hmm... was there duplicate use for this in the patch? I'll check this.

The original open coded the empty auth append with struct tpm2_null_auth since it's the only user. However, since we do have another user in trusted keys, it might make sense to consolidate.

Instead of delaying the current patch from being upstreamed, perhaps this change can be deferred?

thanks,

Mimi

maintainer
#5 lore raw
Quoted 44 lines · Mimi Zohar
Quoted 43 lines · James Bottomley
Quoted 42 lines · Jarkko Sakkinen
Quoted 40 lines · James Bottomley

On Thu, 2024-11-07 at 11:51 +0200, Jarkko Sakkinen wrote:
[...]

Quoted 29 lines

+void tpm_buf_append_auth(struct tpm_chip *chip, struct tpm_buf
*buf,
+                        u8 attributes, u8 *passphrase, int passphrase_len)

drivers/char/tpm/tpm2-sessions.c +7 -0
+{
+       /* 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-

Quoted 1 line

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);

Review

Quoted 2 lines
(quoted diff) +2 -0
+}
+

The rest of the code looks fine, but if you're going to extract this as a separate function instead of doing the open coded struct
tpm2_null_auth that was there originally, you should probably extract and use the tpm2_buf_append_auth() function in trusted_tpm2.c

Quoted 5 lines
Quoted 3 lines
Quoted 1 line

So this was straight up from Mimi's original patch :-)

Yes, I had the same comment prepped for that too.

But it wasn't sent ...

no.

Quoted 10 lines
Quoted 7 lines
Quoted 3 lines

Hmm... was there duplicate use for this in the patch? I'll check this.

The original open coded the empty auth append with struct tpm2_null_auth since it's the only user.  However, since we do have another user in trusted keys, it might make sense to consolidate.

Instead of delaying the current patch from being upstreamed, perhaps this change can be deferred?

I don't see why not; it was the introduction of the new function rather than going back to the struct tpm2_null_auth_area of the original that caught my attention.

James

author maintainer
#6 lore raw
In reply to Mimi Zohar
Quoted 6 lines · Mimi Zohar
Quoted 3 lines

The original open coded the empty auth append with struct tpm2_null_auth since it's the only user. However, since we do have another user in trusted keys, it might make sense to consolidate.

Instead of delaying the current patch from being upstreamed, perhaps this change can be deferred?

Yes.

BR, Jarkko

maintainer
#7 lore raw
Quoted 9 lines · Jarkko Sakkinen
Quoted 6 lines · Mimi Zohar
Quoted 3 lines

The original open coded the empty auth append with struct tpm2_null_auth since it's the only user. However, since we do have another user in trusted keys, it might make sense to consolidate.

Instead of delaying the current patch from being upstreamed, perhaps this change can be deferred?

Yes.

Thanks. As soon as you repost the patch, I'll re-test.

Mimi

maintainer
#8 lore raw
In reply to Jarkko Sakkinen
Quoted 42 lines · Jarkko Sakkinen

The initial HMAC session feature added TPM bus encryption and/or integrity protection to various in-kernel TPM operations. This can cause performance bottlenecks with IMA, as it heavily utilizes PCR extend operations.

In order to mitigate this performance issue, introduce a kernel command-line parameter to the TPM driver for disabling the integrity protection for PCR extension.

Cc James Bottomley <[email protected]>
Link https://lore.kernel.org/linux-integrity/[email protected]/ open here
Fixes 6519fea6fd37 ("tpm: add hmac checks to tpm2_pcr_extend()")
Co-developed-by Roberto Sassu <[email protected]>
Signed-off-by Roberto Sassu <[email protected]>
Co-developed-by Mimi Zohar <[email protected]>
Signed-off-by Mimi Zohar <[email protected]>
Signed-off-by Jarkko Sakkinen <[email protected]>

---
v2:
- Move tpm_buf_append_handle() to the correct file, remove spurious parameter (name), include error on TPM2B and add documentation. Keep the declaration in linux/tpm.h despite not exported as it is easiest to maintain tpm_buf_* in a single header.
- Rename kernel command-line option as "disable_pcr_integrity_protection", as Mimi pointed out it does not carry SA_ENCRYPT flag.
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/tpm-buf.c                    | 20 ++++++++
 drivers/char/tpm/tpm2-cmd.c                   | 30 ++++++++---
 drivers/char/tpm/tpm2-sessions.c              | 51 ++++++++++---------
 include/linux/tpm.h                           |  3 ++
 5 files changed, 83 insertions(+), 31 deletions(-)
Documentation/admin-guide/kernel-parameters.txt +0 -0
index 1518343bbe22..9fc406b20a74 100644

Documentation/admin-guide/kernel-parameters.txt line 6733

Quoted 7 lines
Documentation/admin-guide/kernel-parameters.txt +4 -0
@@ -6727,6 +6727,16 @@
 	torture.verbose_sleep_duration= [KNL]
 			Duration of each verbose-printk() sleep in jiffies.
 
+	tpm.disable_pcr_integrity_protection= [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

"encrypted" isn't needed here.

Quoted 45 lines
Documentation/admin-guide/kernel-parameters.txt +6 -0
+			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
drivers/char/tpm/tpm-buf.c +20 -0
index cad0048bcc3c..e49a19fea3bd 100644
@@ -146,6 +146,26 @@ void tpm_buf_append_u32(struct tpm_buf *buf, const u32 value)
 }
 EXPORT_SYMBOL_GPL(tpm_buf_append_u32);
 
+/**
+ * tpm_buf_append_handle() - Add a handle
+ * @chip:	&tpm_chip instance
+ * @buf:	&tpm_buf instance
+ * @handle:	a TPM object handle
+ *
+ * Add a handle to the buffer, and increase the count tracking the number of
+ * handles in the command buffer. Works only for command buffers.
+ */
+void tpm_buf_append_handle(struct tpm_chip *chip, struct tpm_buf *buf, u32 handle)
+{
+	if (buf->flags & TPM_BUF_TPM2B) {
+		dev_err(&chip->dev, "Invalid buffer type (TPM2B)\n");
+		return;
+	}
+
+	tpm_buf_append_u32(buf, handle);
+	buf->handles++;
+}
+
 /**
  * tpm_buf_read() - Read from a TPM buffer
  * @buf:	&tpm_buf instance
drivers/char/tpm/tpm2-cmd.c +0 -0
index 1e856259219e..cc443bcf15e8 100644

drivers/char/tpm/tpm2-cmd.c line 19

Quoted 6 lines
drivers/char/tpm/tpm2-cmd.c +3 -0
@@ -14,6 +14,10 @@
 #include "tpm.h"
 #include <crypto/hash_info.h>
 
+static bool disable_pcr_integrity_protection;
+module_param(disable_pcr_integrity_protection, bool, 0444);
+MODULE_PARM_DESC(disable_pcr_integrity_protection, "Disable TPM2_PCR_Extend encryption");

I like the name 'disable_pcr_integrity_protection. However, the name and description doesn't match. Replace 'encryption' with 'integrity protection'.

Quoted 19 lines
drivers/char/tpm/tpm2-cmd.c +6 -3
+
 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_pcr_integrity_protection) {
+		rc = tpm2_start_auth_session(chip);
+		if (rc)
+			return rc;
+	}
 
 	rc = tpm_buf_init(&buf, TPM2_ST_SESSIONS, TPM2_CC_PCR_EXTEND);
 	if (rc) {

drivers/char/tpm/tpm2-cmd.c int tpm2_pcr_extend(struct tpm_chip *chip, u32 pcr_idx, · line 253

Quoted 10 lines
drivers/char/tpm/tpm2-cmd.c +4 -3
-		tpm2_end_auth_session(chip);
+		if (!disable_pcr_integrity_protection)
+			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_pcr_integrity_protection) {
+		tpm_buf_append_name(chip, &buf, pcr_idx);

tpm_buf_append_name() parameters didn't change. Don't remove the 'name' field here.

drivers/char/tpm/tpm2-cmd.c int tpm2_pcr_extend(struct tpm_chip *chip, u32 pcr_idx, · line 256

Quoted 3 lines
drivers/char/tpm/tpm2-cmd.c +3 -0
+		tpm_buf_append_hmac_session(chip, &buf, 0, NULL, 0);
+	} else {
+		tpm_buf_append_handle(chip, &buf, pcr_idx);

Or here.

drivers/char/tpm/tpm2-cmd.c int tpm2_pcr_extend(struct tpm_chip *chip, u32 pcr_idx, · line 261

Quoted 6 lines
drivers/char/tpm/tpm2-cmd.c +2 -0
+		tpm_buf_append_auth(chip, &buf, 0, NULL, 0);
+	}
 
 	tpm_buf_append_u32(&buf, chip->nr_allocated_banks);

Mimi

author maintainer
#9 lore raw
Quoted 6 lines · Mimi Zohar

Documentation/admin-guide/kernel-parameters.txt line 6733

Quoted 4 lines
Documentation/admin-guide/kernel-parameters.txt +4 -0
+	tpm.disable_pcr_integrity_protection= [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

"encrypted" isn't needed here.

fixing

Quoted 49 lines
Quoted 49 lines
Documentation/admin-guide/kernel-parameters.txt +6 -0
+			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
drivers/char/tpm/tpm-buf.c +20 -0
index cad0048bcc3c..e49a19fea3bd 100644
@@ -146,6 +146,26 @@ void tpm_buf_append_u32(struct tpm_buf *buf, const u32 value)
 }
 EXPORT_SYMBOL_GPL(tpm_buf_append_u32);
 
+/**
+ * tpm_buf_append_handle() - Add a handle
+ * @chip:	&tpm_chip instance
+ * @buf:	&tpm_buf instance
+ * @handle:	a TPM object handle
+ *
+ * Add a handle to the buffer, and increase the count tracking the number of
+ * handles in the command buffer. Works only for command buffers.
+ */
+void tpm_buf_append_handle(struct tpm_chip *chip, struct tpm_buf *buf, u32 handle)
+{
+	if (buf->flags & TPM_BUF_TPM2B) {
+		dev_err(&chip->dev, "Invalid buffer type (TPM2B)\n");
+		return;
+	}
+
+	tpm_buf_append_u32(buf, handle);
+	buf->handles++;
+}
+
 /**
  * tpm_buf_read() - Read from a TPM buffer
  * @buf:	&tpm_buf instance
drivers/char/tpm/tpm2-cmd.c +0 -0
index 1e856259219e..cc443bcf15e8 100644
@@ -14,6 +14,10 @@
 #include "tpm.h"
 #include <crypto/hash_info.h>
Quoted 6 lines

drivers/char/tpm/tpm2-cmd.c line 19

Quoted 3 lines
drivers/char/tpm/tpm2-cmd.c +3 -0
+static bool disable_pcr_integrity_protection;
+module_param(disable_pcr_integrity_protection, bool, 0444);
+MODULE_PARM_DESC(disable_pcr_integrity_protection, "Disable TPM2_PCR_Extend encryption");

I like the name 'disable_pcr_integrity_protection. However, the name and description doesn't match. Replace 'encryption' with 'integrity protection'.

Weird, I changed that. I don't know how it ended up being like that.

Quoted 26 lines
Quoted 26 lines
drivers/char/tpm/tpm2-cmd.c +8 -4
+
 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_pcr_integrity_protection) {
+		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_pcr_integrity_protection)
+			tpm2_end_auth_session(chip);
 		return rc;
 	}
Quoted 7 lines

drivers/char/tpm/tpm2-cmd.c int tpm2_pcr_extend(struct tpm_chip *chip, u32 pcr_idx, · line 253

Quoted 4 lines
drivers/char/tpm/tpm2-cmd.c +2 -2
-	tpm_buf_append_name(chip, &buf, pcr_idx, NULL);
-	tpm_buf_append_hmac_session(chip, &buf, 0, NULL, 0);
+	if (!disable_pcr_integrity_protection) {
+		tpm_buf_append_name(chip, &buf, pcr_idx);

tpm_buf_append_name() parameters didn't change. Don't remove the 'name' field here.

Hmm... weird I'll check this. Maybe I had something left to staging...

Quoted 8 lines

drivers/char/tpm/tpm2-cmd.c int tpm2_pcr_extend(struct tpm_chip *chip, u32 pcr_idx, · line 256

Quoted 5 lines
drivers/char/tpm/tpm2-cmd.c +3 -0
+		tpm_buf_append_hmac_session(chip, &buf, 0, NULL, 0);
+	} else {
+		tpm_buf_append_handle(chip, &buf, pcr_idx);

Or here.

Here I think it is appropriate

Quoted 9 lines

drivers/char/tpm/tpm2-cmd.c int tpm2_pcr_extend(struct tpm_chip *chip, u32 pcr_idx, · line 261

Quoted 7 lines
drivers/char/tpm/tpm2-cmd.c +2 -0
+		tpm_buf_append_auth(chip, &buf, 0, NULL, 0);
+	}
 
 	tpm_buf_append_u32(&buf, chip->nr_allocated_banks);

Mimi

BR, Jarkko

maintainer
#10 lore raw
Quoted 35 lines · Jarkko Sakkinen
Quoted 22 lines · Mimi Zohar
Quoted 22 lines
drivers/char/tpm/tpm2-cmd.c +7 -4
@@ -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_pcr_integrity_protection) {
+		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_pcr_integrity_protection)
+			tpm2_end_auth_session(chip);
 		return rc;
 	}
Quoted 7 lines

drivers/char/tpm/tpm2-cmd.c int tpm2_pcr_extend(struct tpm_chip *chip, u32 pcr_idx, · line 253

Quoted 4 lines
drivers/char/tpm/tpm2-cmd.c +2 -2
-	tpm_buf_append_name(chip, &buf, pcr_idx, NULL);
-	tpm_buf_append_hmac_session(chip, &buf, 0, NULL, 0);
+	if (!disable_pcr_integrity_protection) {
+		tpm_buf_append_name(chip, &buf, pcr_idx);

tpm_buf_append_name() parameters didn't change. Don't remove the 'name' field here.

Hmm... weird I'll check this. Maybe I had something left to staging...

Quoted 2 lines
Quoted 8 lines
Quoted 6 lines

drivers/char/tpm/tpm2-cmd.c int tpm2_pcr_extend(struct tpm_chip *chip, u32 pcr_idx, · line 256

Quoted 3 lines
drivers/char/tpm/tpm2-cmd.c +3 -0
+		tpm_buf_append_hmac_session(chip, &buf, 0, NULL, 0);
+	} else {
+		tpm_buf_append_handle(chip, &buf, pcr_idx);

Or here.

Here I think it is appropriate

Agreed

Quoted 7 lines
Quoted 7 lines
Quoted 7 lines
drivers/char/tpm/tpm2-cmd.c +2 -0
+		tpm_buf_append_auth(chip, &buf, 0, NULL, 0);
+	}
 
 	tpm_buf_append_u32(&buf, chip->nr_allocated_banks);
author maintainer
#11 lore raw
Quoted 32 lines · Mimi Zohar
Quoted 31 lines · Jarkko Sakkinen
Quoted 29 lines · Mimi Zohar
Quoted 16 lines
drivers/char/tpm/tpm2-cmd.c +5 -3
@@ -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_pcr_integrity_protection) {
+		rc = tpm2_start_auth_session(chip);
+		if (rc)
+			return rc;
+	}
 
 	rc = tpm_buf_init(&buf, TPM2_ST_SESSIONS, TPM2_CC_PCR_EXTEND);
 	if (rc) {

drivers/char/tpm/tpm2-cmd.c int tpm2_pcr_extend(struct tpm_chip *chip, u32 pcr_idx, · line 253

Quoted 10 lines
drivers/char/tpm/tpm2-cmd.c +4 -3
-		tpm2_end_auth_session(chip);
+		if (!disable_pcr_integrity_protection)
+			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_pcr_integrity_protection) {
+		tpm_buf_append_name(chip, &buf, pcr_idx);

tpm_buf_append_name() parameters didn't change. Don't remove the 'name' field here.

Quoted 1 line
Quoted 1 line

Hmm... weird I'll check this. Maybe I had something left to staging...

Yes! This was correct in my clone but not in the patch.

Clearly a sign that I wait until next week before sending a new version :-)

Quoted 8 lines
Quoted 8 lines
Quoted 7 lines
Quoted 6 lines
drivers/char/tpm/tpm2-cmd.c +3 -0
+		tpm_buf_append_hmac_session(chip, &buf, 0, NULL, 0);
+	} else {
+		tpm_buf_append_handle(chip, &buf, pcr_idx);
Quoted 5 lines
Quoted 3 lines
Quoted 1 line

Or here.

Here I think it is appropriate

Agreed

Great

BR, Jarkko

build bot
#12 lore raw
In reply to Jarkko Sakkinen
kernel test robot noticed the following build errors:

Hi Jarkko,

kernel test robot noticed the following build errors:

[auto build test ERROR on char-misc/char-misc-testing]
[also build test ERROR on char-misc/char-misc-next char-misc/char-misc-linus linus/master v6.12-rc6 next-20241106] [If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Jarkko-Sakkinen/tpm-Opt-in-in-disable-PCR-integrity-protection/20241107-175515
base:   char-misc/char-misc-testing
patch link:    https://lore.kernel.org/r/20241107095138.78209-1-jarkko%40kernel.org
patch subject: [PATCH v2] tpm: Opt-in in disable PCR integrity protection
config: x86_64-rhel-8.3 (https://download.01.org/0day-ci/archive/20241107/[email protected]/config)
compiler: gcc-12 (Debian 12.2.0-14) 12.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20241107/[email protected]/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <[email protected]>
| Closes: https://lore.kernel.org/oe-kbuild-all/[email protected]/

All errors (new ones prefixed by >>):

drivers/char/tpm/tpm2-cmd.c: In function 'tpm2_pcr_extend':

>> drivers/char/tpm/tpm2-cmd.c:253:17: error: too few arguments to function 'tpm_buf_append_name'
     253 |                 tpm_buf_append_name(chip, &buf, pcr_idx);
         |                 ^~~~~~~~~~~~~~~~~~~
   In file included from drivers/char/tpm/tpm.h:27,
                    from drivers/char/tpm/tpm2-cmd.c:14:

include/linux/tpm.h:504:6: note: declared here
504 | void tpm_buf_append_name(struct tpm_chip *chip, struct tpm_buf *buf,
| ^~~~~~~~~~~~~~~~~~~

vim +/tpm_buf_append_name +253 drivers/char/tpm/tpm2-cmd.c

222
223 /**
224 * tpm2_pcr_extend() - extend a PCR value
225 *
226 * @chip: TPM chip to use.
227 * @pcr_idx: index of the PCR.
228 * @digests: list of pcr banks and corresponding digest values to extend.
229 *
230 * Return: Same as with tpm_transmit_cmd.
231 */
232 int tpm2_pcr_extend(struct tpm_chip *chip, u32 pcr_idx,
233 struct tpm_digest *digests)
234 {
235 struct tpm_buf buf;
236 int rc;
237 int i;
238
239 if (!disable_pcr_integrity_protection) {
240 rc = tpm2_start_auth_session(chip);
241 if (rc)
242 return rc;
243 }
244
245 rc = tpm_buf_init(&buf, TPM2_ST_SESSIONS, TPM2_CC_PCR_EXTEND);
246 if (rc) {
247 if (!disable_pcr_integrity_protection)
248 tpm2_end_auth_session(chip);
249 return rc;
250 }
251
252 if (!disable_pcr_integrity_protection) {

 > 253			tpm_buf_append_name(chip, &buf, pcr_idx);

254 tpm_buf_append_hmac_session(chip, &buf, 0, NULL, 0);
255 } else {
256 tpm_buf_append_handle(chip, &buf, pcr_idx);
257 tpm_buf_append_auth(chip, &buf, 0, NULL, 0);
258 }
259
260 tpm_buf_append_u32(&buf, chip->nr_allocated_banks);
261
262 for (i = 0; i < chip->nr_allocated_banks; i++) {
263 tpm_buf_append_u16(&buf, digests[i].alg_id);
264 tpm_buf_append(&buf, (const unsigned char *)&digests[i].digest,
265 chip->allocated_banks[i].digest_size);
266 }
267
268 if (!disable_pcr_integrity_protection)
269 tpm_buf_fill_hmac_session(chip, &buf);
270 rc = tpm_transmit_cmd(chip, &buf, 0, "attempting extend a PCR value");
271 if (!disable_pcr_integrity_protection)
272 rc = tpm_buf_check_hmac_response(chip, &buf, rc);
273
274 tpm_buf_destroy(&buf);
275
276 return rc;
277 }
278

Signature · 2 lines
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
build bot
#13 lore raw
In reply to Jarkko Sakkinen
kernel test robot noticed the following build errors:

Hi Jarkko,

kernel test robot noticed the following build errors:

[auto build test ERROR on char-misc/char-misc-testing]
[also build test ERROR on char-misc/char-misc-next char-misc/char-misc-linus linus/master v6.12-rc6 next-20241107] [If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Jarkko-Sakkinen/tpm-Opt-in-in-disable-PCR-integrity-protection/20241107-175515
base:   char-misc/char-misc-testing
patch link:    https://lore.kernel.org/r/20241107095138.78209-1-jarkko%40kernel.org
patch subject: [PATCH v2] tpm: Opt-in in disable PCR integrity protection
config: arm-randconfig-003-20241108 (https://download.01.org/0day-ci/archive/20241108/[email protected]/config)
compiler: clang version 15.0.7 (https://github.com/llvm/llvm-project 8dfdcc7b7bf66834a761bd8de445840ef68e4d1a)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20241108/[email protected]/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <[email protected]>
| Closes: https://lore.kernel.org/oe-kbuild-all/[email protected]/

All errors (new ones prefixed by >>):

In file included from drivers/char/tpm/tpm2-cmd.c:14:
In file included from drivers/char/tpm/tpm.h:28:
include/linux/tpm_eventlog.h:167:6: warning: variable 'mapping_size' set but not used [-Wunused-but-set-variable]

           int mapping_size;
               ^
>> drivers/char/tpm/tpm2-cmd.c:253:42: error: too few arguments to function call, expected 4, have 3
                   tpm_buf_append_name(chip, &buf, pcr_idx);
                   ~~~~~~~~~~~~~~~~~~~                    ^
   include/linux/tpm.h:504:6: note: 'tpm_buf_append_name' declared here
   void tpm_buf_append_name(struct tpm_chip *chip, struct tpm_buf *buf,
        ^

1 warning and 1 error generated.

vim +253 drivers/char/tpm/tpm2-cmd.c

222
223 /**
224 * tpm2_pcr_extend() - extend a PCR value
225 *
226 * @chip: TPM chip to use.
227 * @pcr_idx: index of the PCR.
228 * @digests: list of pcr banks and corresponding digest values to extend.
229 *
230 * Return: Same as with tpm_transmit_cmd.
231 */
232 int tpm2_pcr_extend(struct tpm_chip *chip, u32 pcr_idx,
233 struct tpm_digest *digests)
234 {
235 struct tpm_buf buf;
236 int rc;
237 int i;
238
239 if (!disable_pcr_integrity_protection) {
240 rc = tpm2_start_auth_session(chip);
241 if (rc)
242 return rc;
243 }
244
245 rc = tpm_buf_init(&buf, TPM2_ST_SESSIONS, TPM2_CC_PCR_EXTEND);
246 if (rc) {
247 if (!disable_pcr_integrity_protection)
248 tpm2_end_auth_session(chip);
249 return rc;
250 }
251
252 if (!disable_pcr_integrity_protection) {

 > 253			tpm_buf_append_name(chip, &buf, pcr_idx);

254 tpm_buf_append_hmac_session(chip, &buf, 0, NULL, 0);
255 } else {
256 tpm_buf_append_handle(chip, &buf, pcr_idx);
257 tpm_buf_append_auth(chip, &buf, 0, NULL, 0);
258 }
259
260 tpm_buf_append_u32(&buf, chip->nr_allocated_banks);
261
262 for (i = 0; i < chip->nr_allocated_banks; i++) {
263 tpm_buf_append_u16(&buf, digests[i].alg_id);
264 tpm_buf_append(&buf, (const unsigned char *)&digests[i].digest,
265 chip->allocated_banks[i].digest_size);
266 }
267
268 if (!disable_pcr_integrity_protection)
269 tpm_buf_fill_hmac_session(chip, &buf);
270 rc = tpm_transmit_cmd(chip, &buf, 0, "attempting extend a PCR value");
271 if (!disable_pcr_integrity_protection)
272 rc = tpm_buf_check_hmac_response(chip, &buf, rc);
273
274 tpm_buf_destroy(&buf);
275
276 return rc;
277 }
278

Signature · 2 lines
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki