.../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(-)
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 <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>
Co-developed-by: Mimi Zohar <zohar@linux.ibm.com>
Signed-off-by: Mimi Zohar <zohar@linux.ibm.com>
Signed-off-by: Jarkko Sakkinen <jarkko@kernel.org>
---
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(-)
diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
index 1518343bbe22..9fc406b20a74 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_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
diff --git a/drivers/char/tpm/tpm-buf.c b/drivers/char/tpm/tpm-buf.c
index cad0048bcc3c..e49a19fea3bd 100644
--- a/drivers/char/tpm/tpm-buf.c
+++ b/drivers/char/tpm/tpm-buf.c
@@ -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
diff --git a/drivers/char/tpm/tpm2-cmd.c b/drivers/char/tpm/tpm2-cmd.c
index 1e856259219e..cc443bcf15e8 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_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);
diff --git a/drivers/char/tpm/tpm2-sessions.c b/drivers/char/tpm/tpm2-sessions.c
index 42df980168b6..a7c1b162251b 100644
--- a/drivers/char/tpm/tpm2-sessions.c
+++ b/drivers/char/tpm/tpm2-sessions.c
@@ -237,9 +237,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);
return;
}
@@ -272,6 +270,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 +332,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..20a40ade8030 100644
--- a/include/linux/tpm.h
+++ b/include/linux/tpm.h
@@ -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,
--
2.47.0Hi 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/202411080436.fb2O1apj-lkp@intel.com/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/202411080436.fb2O1apj-lkp@intel.com/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 <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202411080436.fb2O1apj-lkp@intel.com/
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
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
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/202411072138.bgOIL36O-lkp@intel.com/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/202411072138.bgOIL36O-lkp@intel.com/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 <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202411072138.bgOIL36O-lkp@intel.com/
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
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
On Thu, 2024-11-07 at 11:51 +0200, Jarkko Sakkinen wrote:
> 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 <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>
> Co-developed-by: Mimi Zohar <zohar@linux.ibm.com>
> Signed-off-by: Mimi Zohar <zohar@linux.ibm.com>
> Signed-off-by: Jarkko Sakkinen <jarkko@kernel.org>
> ---
> 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(-)
>
> diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
> index 1518343bbe22..9fc406b20a74 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_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.
> + 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/tpm-buf.c b/drivers/char/tpm/tpm-buf.c
> index cad0048bcc3c..e49a19fea3bd 100644
> --- a/drivers/char/tpm/tpm-buf.c
> +++ b/drivers/char/tpm/tpm-buf.c
> @@ -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
> diff --git a/drivers/char/tpm/tpm2-cmd.c b/drivers/char/tpm/tpm2-cmd.c
> index 1e856259219e..cc443bcf15e8 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_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'.
> +
> 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_name() parameters didn't change. Don't remove the 'name' field
here.
> + tpm_buf_append_hmac_session(chip, &buf, 0, NULL, 0);
> + } else {
> + tpm_buf_append_handle(chip, &buf, pcr_idx);
Or here.
> + tpm_buf_append_auth(chip, &buf, 0, NULL, 0);
> + }
>
> tpm_buf_append_u32(&buf, chip->nr_allocated_banks);
>
>
Mimi
On Thu Nov 7, 2024 at 3:44 PM EET, Mimi Zohar wrote:
> > + 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
>
> > + 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/tpm-buf.c b/drivers/char/tpm/tpm-buf.c
> > index cad0048bcc3c..e49a19fea3bd 100644
> > --- a/drivers/char/tpm/tpm-buf.c
> > +++ b/drivers/char/tpm/tpm-buf.c
> > @@ -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
> > diff --git a/drivers/char/tpm/tpm2-cmd.c b/drivers/char/tpm/tpm2-cmd.c
> > index 1e856259219e..cc443bcf15e8 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_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.
>
> > +
> > 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_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...
>
>
> > + 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
>
> > + tpm_buf_append_auth(chip, &buf, 0, NULL, 0);
> > + }
> >
> > tpm_buf_append_u32(&buf, chip->nr_allocated_banks);
> >
> >
>
> Mimi
BR, Jarkko
On Thu, 2024-11-07 at 15:47 +0200, Jarkko Sakkinen wrote:
> On Thu Nov 7, 2024 at 3:44 PM EET, Mimi Zohar wrote:
> > >
> > > @@ -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_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...
>
> >
> >
> > > + 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
>
> >
> > > + tpm_buf_append_auth(chip, &buf, 0, NULL, 0);
> > > + }
> > >
> > > tpm_buf_append_u32(&buf, chip->nr_allocated_banks);
> > >
On Thu Nov 7, 2024 at 4:00 PM EET, Mimi Zohar wrote:
> On Thu, 2024-11-07 at 15:47 +0200, Jarkko Sakkinen wrote:
> > On Thu Nov 7, 2024 at 3:44 PM EET, Mimi Zohar wrote:
> > > >
> > > > @@ -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_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...
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
:-)
> >
> > >
> > >
> > > > + 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
Great
BR, Jarkko
On Thu, 2024-11-07 at 11:51 +0200, Jarkko Sakkinen wrote:
[...]
> +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);
> +}
> +
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
On Thu Nov 7, 2024 at 3:20 PM EET, James Bottomley wrote:
> On Thu, 2024-11-07 at 11:51 +0200, Jarkko Sakkinen wrote:
> [...]
> > +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);
> > +}
> > +
>
> 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.
>
> James
BR, Jarkko
On Thu, 2024-11-07 at 15:49 +0200, Jarkko Sakkinen wrote:
> On Thu Nov 7, 2024 at 3:20 PM EET, James Bottomley wrote:
> > On Thu, 2024-11-07 at 11:51 +0200, Jarkko Sakkinen wrote:
> > [...]
> > > +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);
> > > +}
> > > +
> >
> > 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 :-)
Yes, I had the same comment prepped for that too.
> 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
On Thu, 2024-11-07 at 08:52 -0500, James Bottomley wrote:
> On Thu, 2024-11-07 at 15:49 +0200, Jarkko Sakkinen wrote:
> > On Thu Nov 7, 2024 at 3:20 PM EET, James Bottomley wrote:
> > > On Thu, 2024-11-07 at 11:51 +0200, Jarkko Sakkinen wrote:
> > > [...]
> > > > +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);
> > > > +}
> > > > +
> > >
> > > 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 :-)
>
> Yes, I had the same comment prepped for that too.
But it wasn't sent ...
>
> > 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
On Mon Nov 11, 2024 at 9:53 PM EET, Mimi Zohar wrote: > > 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
On Tue, 2024-11-12 at 19:57 +0200, Jarkko Sakkinen wrote: > On Mon Nov 11, 2024 at 9:53 PM EET, Mimi Zohar wrote: > > > 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
On Mon, 2024-11-11 at 14:53 -0500, Mimi Zohar wrote:
> On Thu, 2024-11-07 at 08:52 -0500, James Bottomley wrote:
> > On Thu, 2024-11-07 at 15:49 +0200, Jarkko Sakkinen wrote:
> > > On Thu Nov 7, 2024 at 3:20 PM EET, James Bottomley wrote:
> > > > On Thu, 2024-11-07 at 11:51 +0200, Jarkko Sakkinen wrote:
> > > > [...]
> > > > > +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);
> > > > > +}
> > > > > +
> > > >
> > > > 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 :-)
> >
> > Yes, I had the same comment prepped for that too.
>
> But it wasn't sent ...
no.
> >
> > > 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
© 2016 - 2024 Red Hat, Inc.