add integrity and security to TPM2 transactions
Subsystems (7)
James Bottomley
maintainercover add integrity and security to TPM2 transactions
The interest in securing the TPM against interposers, both active and passive has risen to fever pitch with the demonstration of key recovery against windows bitlocker:
https://dolosgroup.io/blog/2021/7/9/from-stolen-laptop-to-inside-the-company-network
And subsequently the same attack being successful against all the Linux TPM based security solutions:
https://www.secura.com/blog/tpm-sniffing-attacks-against-non-bitlocker-targets
The attacks fall into two categories:
1. Passive Interposers, which sit on the bus and merely observe
2. Active Interposers, which try to manipulate TPM transactions on the bus using man in the middle and packet stealing to create TPM state the interposer owner desires.
Our broadest interposer target is the use of TPM_RS_PW for password authorization which sends the actual password to the TPM without any obfuscation and effectively hands it to any interposer. The way to fix this is to use real sessions for HMAC capabilities to ensure integrity and to use parameter and response encryption to ensure confidentiality of the data flowing over the TPM bus. HMAC sessions by agreeing a challenge with the TPM and then giving a response which is a HMAC of the password and the challenge, so the application proves knowledge of the password to the TPM without ever transmitting the password itself. Using HMAC sessions when sending commands to the TPM also provides some measure of protection against active interposers, since the interposer can't interfere with or delete a HMAC'd command (because they can't manufacture a response with the correct HMAC).
To protect TPM transactions where there isn't a shared secret (i.e. the command is something like a PCR extension which doesn't involve a TPM object with a password) we have to do a bit more work to set up sessions with a passed in encrypted secret (called a salt) to act in place of the shared secret in the HMAC. This secret salt is effectively a random number encrypted to a public key of the TPM. The final piece of the puzzle is using parameter input and response return encryption, so any interposer can't see the data passing from the application to the TPM and vice versa.
The most insidious interposer attack of all is a reset attack: since the interposer has access to the TPM bus, it can assert the TPM reset line any time it wants. When a TPM resets it mostly comes back in the same state except that all the PCRs are reset to their initial values. Controlling the reset line allows the interposer to change the PCR state after the fact by resetting the TPM and then replaying PCR extends to get the PCRs into a valid state to release secrets, so even if an attack event was recorded, the record is erased. This reset attack violates the fundamental princible of non-repudiability of TPM logs. Defeating the reset attack involves tying all TPM operations within the kernel to a property which will change detectably if the TPM is reset. For that reason, we tie all TPM sessions to the null hierarchy we obtain at start of day and whose seed changes on every reset. If an active interposer asserts a TPM reset, the new null primary won't match the kernel's stored one and all TPM operations will start failing because of HMAC mismatches in the sessions. So if the kernel TPM code keeps operating, it guarantees that a reset hasn't occurred.
The final part of the puzzle is that the machine owner must have a fixed idea of the EK of their TPM and should have certified this with the TPM manufacturer. On every boot, the certified EK public key should be used to do a make credential/activate credential attestation key insertion and then the null key certified with the attestation key. We can follow a trust on first use model where an OS installation will extract and verify a public EK and save it to a read only file.
This patch series adds a simple API which can ensure the above properties as a layered addition to the existing TPM handling code. This series now includes protections for PCR extend, getting random numbers from the TPM and data sealing and unsealing. It therefore eliminates all uses of TPM2_RS_PW in the kernel and adds encryption protection to sensitive data flowing into and out of the TPM. The first four patches add more sophisticated buffer handling to the TPM which is needed to build the more complex encryption and authentication based commands. Patch 6 adds all the generic cryptography primitives and patches 7-9 use them in critical TPM operations where we want to avoid or detect interposers. Patch 10 exports the name of the null key we used for boot/run time verification and patch 11 documents the security guarantees and expectations.
This was originally sent over four years ago, with the last iteration being:
https://lore.kernel.org/linux-integrity/[email protected]/
I'm dusting it off now because various forces at Microsoft and Google via the Open Compute Platform are making a lot of noise about interposers and we in the linux kernel look critically lacking in that regard, particularly for TPM trusted keys.
---
v2 fixes the problems smatch reported and adds more explanation about the code motion in the first few patches
v3 rebases the encryption to be against Ard's new library function, the aescfb addition of which appears as patch 1.
James
---
Ard Biesheuvel (1):
crypto: lib - implement library version of AES in CFB mode
James Bottomley (11):
tpm: move buffer handling from static inlines to real functions
tpm: add buffer handling for TPM2B types
tpm: add cursor based buffer functions for response parsing
tpm: add buffer function to point to returned parameters
tpm: export the context save and load commands
tpm: Add full HMAC and encrypt/decrypt session handling code
tpm: add hmac checks to tpm2_pcr_extend()
tpm: add session encryption protection to tpm2_get_random()
KEYS: trusted: Add session encryption protection to the seal/unseal
path
tpm: add the null key name as a sysfs export
Documentation: add tpm-security.rst
Documentation/security/tpm/tpm-security.rst | 216 ++++ drivers/char/tpm/Kconfig | 13 + drivers/char/tpm/Makefile | 2 + drivers/char/tpm/tpm-buf.c | 196 ++++ drivers/char/tpm/tpm-chip.c | 3 + drivers/char/tpm/tpm-sysfs.c | 18 + drivers/char/tpm/tpm.h | 14 + drivers/char/tpm/tpm2-cmd.c | 52 +- drivers/char/tpm/tpm2-sessions.c | 1160 +++++++++++++++++++ drivers/char/tpm/tpm2-space.c | 8 +- include/crypto/aes.h | 5 + include/linux/tpm.h | 257 ++-- lib/crypto/Kconfig | 5 + lib/crypto/Makefile | 3 + lib/crypto/aescfb.c | 75 ++ security/keys/trusted-keys/trusted_tpm2.c | 82 +- 16 files changed, 1984 insertions(+), 125 deletions(-) create mode 100644 Documentation/security/tpm/tpm-security.rst create mode 100644 drivers/char/tpm/tpm-buf.c create mode 100644 drivers/char/tpm/tpm2-sessions.c create mode 100644 lib/crypto/aescfb.c
Signature · 1 line
2.35.3
James Bottomley
maintainerPATCH 1/12 crypto: lib - implement library version of AES in CFB mode
From: Ard Biesheuvel <[email protected]>
Implement AES in CFB mode using the existing, mostly constant-time generic AES library implementation.
--- include/crypto/aes.h | 5 +++ lib/crypto/Kconfig | 5 +++ lib/crypto/Makefile | 3 ++ lib/crypto/aescfb.c | 75 ++++++++++++++++++++++++++++++++++++++++++++ 4 files changed, 88 insertions(+) create mode 100644 lib/crypto/aescfb.c
include/crypto/aes.h +5 -0
@@ -87,4 +87,9 @@ void aes_decrypt(const struct crypto_aes_ctx *ctx, u8 *out, const u8 *in);extern const u8 crypto_aes_sbox[];extern const u8 crypto_aes_inv_sbox[]; +void aescfb_encrypt (const struct crypto_aes_ctx *ctx,u8 *dst,const u8 *src, +int len,const u8 *iv); +void aescfb_decrypt (const struct crypto_aes_ctx *ctx,u8 *dst,const u8 *src, +int len,const u8 *iv); +#endif
lib/crypto/Kconfig +5 -0
@@ -8,6 +8,11 @@ config CRYPTO_LIB_UTILS config CRYPTO_LIB_AES tristate +config CRYPTO_LIB_AESCFB + tristate + select CRYPTO_LIB_AES + select CRYPTO_LIB_UTILS + config CRYPTO_LIB_AESGCM tristate select CRYPTO_LIB_AES
lib/crypto/Makefile +3 -0
@@ -10,6 +10,9 @@ obj-$(CONFIG_CRYPTO_LIB_CHACHA_GENERIC) += libchacha.o obj-$(CONFIG_CRYPTO_LIB_AES) += libaes.o libaes-y := aes.o +obj-$(CONFIG_CRYPTO_LIB_AESCFB) += libaescfb.o +libaescfb-y := aescfb.o + obj-$(CONFIG_CRYPTO_LIB_AESGCM) += libaesgcm.o libaesgcm-y := aesgcm.o
lib/crypto/aescfb.c new file +75 -0
75 lines not on this page · raw message
Signature · 1 line
2.35.3
Re: [PATCH 01/12] crypto: lib - implement library version of AES in CFB mode
Quoted 68 lines · James Bottomley
From: Ard Biesheuvel <[email protected]>
Implement AES in CFB mode using the existing, mostly constant-time generic AES library implementation.
Signed-off-by Ard Biesheuvel <[email protected]>Signed-off-by James Bottomley <[email protected]>--- include/crypto/aes.h | 5 +++ lib/crypto/Kconfig | 5 +++ lib/crypto/Makefile | 3 ++ lib/crypto/aescfb.c | 75 ++++++++++++++++++++++++++++++++++++++++++++ 4 files changed, 88 insertions(+) create mode 100644 lib/crypto/aescfb.cinclude/crypto/aes.h +5
-0@@ -87,4 +87,9 @@ void aes_decrypt(const struct crypto_aes_ctx *ctx, u8 *out, const u8 *in);extern const u8 crypto_aes_sbox[];extern const u8 crypto_aes_inv_sbox[]; +void aescfb_encrypt (const struct crypto_aes_ctx *ctx,u8 *dst,const u8 *src, +int len,const u8 *iv); +void aescfb_decrypt (const struct crypto_aes_ctx *ctx,u8 *dst,const u8 *src, +int len,const u8 *iv); +#endif lib/crypto/Kconfig +5
-0@@ -8,6 +8,11 @@ config CRYPTO_LIB_UTILS config CRYPTO_LIB_AES tristate +config CRYPTO_LIB_AESCFB + tristate + select CRYPTO_LIB_AES + select CRYPTO_LIB_UTILS + config CRYPTO_LIB_AESGCM tristate select CRYPTO_LIB_AESlib/crypto/Makefile +3
-0@@ -10,6 +10,9 @@ obj-$(CONFIG_CRYPTO_LIB_CHACHA_GENERIC) += libchacha.o obj-$(CONFIG_CRYPTO_LIB_AES) += libaes.o libaes-y := aes.o +obj-$(CONFIG_CRYPTO_LIB_AESCFB) += libaescfb.o +libaescfb-y := aescfb.o + obj-$(CONFIG_CRYPTO_LIB_AESGCM) += libaesgcm.o libaesgcm-y := aesgcm.olib/crypto/aescfb.c new file +3
-0@@ -0,0 +1,75 @@ +// SPDX-License-Identifier: GPL-2.0 +/* +* Minimal library implementation of AES in CFB mode
lib/crypto/aescfb.c line 13
Quoted 10 lines
lib/crypto/aescfb.c new file +10
-0+* +* Copyright 2023 Google LLC +*/ + +#include <linux/module.h> + +#include <crypto/algapi.h> +#include <crypto/aes.h> + +#include <asm/irqflags.h>
I'd remove the newlines in-between.
Quoted 65 lines
lib/crypto/aescfb.c new file +62
-0+ +static void aescfb_encrypt_block (const struct crypto_aes_ctx *ctx,void *dst, +const void *src) +{ +unsigned long flags; + +/* +* In AES-CFB, the AES encryption operates on known 'plaintext' (the IV +* and ciphertext), making it susceptible to timing attacks on the +* encryption key. The AES library already mitigates this risk to some +* extent by pulling the entire S-box into the caches before doing any +* substitutions, but this strategy is more effective when running with +* interrupts disabled. +*/ +local_irq_save (flags); +aes_encrypt (ctx, dst, src); +local_irq_restore (flags); +} + +void aescfb_encrypt (const struct crypto_aes_ctx *ctx,u8 *dst,const u8 *src, +int len,const u8 *iv) +{ +while (len >0 ) { +u8 ks[AES_BLOCK_SIZE ]; + +aescfb_encrypt_block (ctx, ks, iv); +crypto_xor_cpy (dst, src, ks,min (len,AES_BLOCK_SIZE )); + iv = dst; + + dst +=AES_BLOCK_SIZE ; + src +=AES_BLOCK_SIZE ; + len -=AES_BLOCK_SIZE ; + } +} + +void aescfb_decrypt (const struct crypto_aes_ctx *ctx,u8 *dst,const u8 *src, +int len,const u8 *iv) +{ +u8 ks[2 ][AES_BLOCK_SIZE ]; + +aescfb_encrypt_block (ctx, ks[0 ], iv); + +for (int i =0 ; len >0 ; i ^=1 ) { +if (len >AES_BLOCK_SIZE ) +/* +* Generate the keystream for the next block before +* performing the XOR, as that may update in place and +* overwrite the ciphertext. +*/ +aescfb_encrypt_block (ctx, ks[!i], src); + +crypto_xor_cpy (dst, src, ks[i],min (len,AES_BLOCK_SIZE )); + + dst +=AES_BLOCK_SIZE ; + src +=AES_BLOCK_SIZE ; + len -=AES_BLOCK_SIZE ; + } +} + +MODULE_DESCRIPTION ("Generic AES-CFB library" ); +MODULE_AUTHOR ("Ard Biesheuvel <[email protected]>" ); +MODULE_LICENSE ("GPL" );Signature · 1 line
2.35.3
BR, Jarkko
James Bottomley
maintainerPATCH 2/12 tpm: move buffer handling from static inlines to real functions
In reply to James Bottomleyseparate out the tpm_buf_... handling functions from static inlines in tpm.h and move them to their own tpm-buf.c file. This is a precursor to adding new functions for other TPM type handling because the amount of code will grow from the current 70 lines in tpm.h to about 200 lines when the additions are done. 200 lines of inline functions is a bit too much to keep in a header file.
---
v3: make tpm_buf_tag static
--- drivers/char/tpm/Makefile | 1 + drivers/char/tpm/tpm-buf.c | 95 ++++++++++++++++++++++++++++++++++++++ include/linux/tpm.h | 86 ++++------------------------------ 3 files changed, 105 insertions(+), 77 deletions(-) create mode 100644 drivers/char/tpm/tpm-buf.c
drivers/char/tpm/Makefile +1 -0
@@ -15,6 +15,7 @@ tpm-y += tpm-sysfs.o tpm-y += eventlog/common.o tpm-y += eventlog/tpm1.o tpm-y += eventlog/tpm2.o +tpm-y += tpm-buf.o tpm-$(CONFIG_ACPI) += tpm_ppi.o eventlog/acpi.o tpm-$(CONFIG_EFI) += eventlog/efi.o
drivers/char/tpm/tpm-buf.c new file +95 -0
95 lines not on this page · raw message
include/linux/tpm.h +9 -77
93 lines not on this page · raw message
Signature · 1 line
2.35.3
Jarkko Sakkinen
maintainerRe: [PATCH 02/12] tpm: move buffer handling from static inlines to real functions
Quoted 36 lines · James Bottomley
separate out the
tpm_buf_... handling functions from static inlines intpm.hand move them to their owntpm-buf.cfile. This is a precursor to adding new functions for other TPM type handling because the amount of code will grow from the current 70 lines intpm.hto about 200 lines when the additions are done. 200 lines of inline functions is a bit too much to keep in a header file.Signed-off-by James Bottomley <[email protected]>---
v3: maketpm_buf_tagstatic--- drivers/char/tpm/Makefile | 1 + drivers/char/tpm/tpm-buf.c | 95 ++++++++++++++++++++++++++++++++++++++ include/linux/tpm.h | 86 ++++------------------------------ 3 files changed, 105 insertions(+), 77 deletions(-) create mode 100644 drivers/char/tpm/tpm-buf.cdrivers/char/tpm/Makefile +1
-0@@ -15,6 +15,7 @@ tpm-y += tpm-sysfs.o tpm-y += eventlog/common.o tpm-y += eventlog/tpm1.o tpm-y += eventlog/tpm2.o +tpm-y += tpm-buf.o tpm-$(CONFIG_ACPI) += tpm_ppi.o eventlog/acpi.o tpm-$(CONFIG_EFI) += eventlog/efi.odrivers/char/tpm/tpm-buf.c new file +0
-0
drivers/char/tpm/tpm-buf.c line 2
Quoted 2 lines
drivers/char/tpm/tpm-buf.c new file +2
-0@@ -0,0 +1,95 @@ +// SPDX-License-Identifier: GPL-2.0 +
This empty line should be removed.
drivers/char/tpm/tpm-buf.c line 9
Quoted 7 lines
drivers/char/tpm/tpm-buf.c new file +7
-0+/* +* Handing for tpm_buf structures to facilitate the building of commands +*/ + +#include <linux/module.h> +#include <linux/tpm.h> +
Now that symbols are compiled and also exported, it would be good to add also kdoc's while at it, e.g.
/*
* tpm_buf_init - Init a buffer for a TPM command
* @buf: uninitialized buffer * @tag: tag of a TPM command * @ordinal: number of a TPM command
*
* Allocate a page sized buffer from heap for a TPM command, and initialize
* the command header.
*/
It is probably better to have an additional patch for this, as this one should just move code around.
drivers/char/tpm/tpm-buf.c line 22
Quoted 13 lines
drivers/char/tpm/tpm-buf.c new file +13
-0+int tpm_buf_init (struct tpm_buf *buf,u16 tag,u32 ordinal) +{ + buf->data = (u8 *)__get_free_page (GFP_KERNEL ); +if (!buf->data) +return -ENOMEM ; + + buf->flags =0 ; +tpm_buf_reset (buf, tag, ordinal); +return 0 ; +} +EXPORT_SYMBOL_GPL (tpm_buf_init); + +void tpm_buf_reset (struct tpm_buf *buf,u16 tag,u32 ordinal)
Ditto.
I guess you can fill these out. I just put an example to give a rough idea what to put.
drivers/char/tpm/tpm-buf.c line 32
Quoted 10 lines
drivers/char/tpm/tpm-buf.c new file +10
-0+{ +struct tpm_header *head = (struct tpm_header *) buf->data; + + head->tag =cpu_to_be16 (tag); + head->length =cpu_to_be32 (sizeof (*head)); + head->ordinal =cpu_to_be32 (ordinal); +} +EXPORT_SYMBOL_GPL (tpm_buf_reset); + +void tpm_buf_destroy (struct tpm_buf *buf)
Ditto.
drivers/char/tpm/tpm-buf.c line 38
Quoted 6 lines
drivers/char/tpm/tpm-buf.c new file +6
-0+{ +free_page ((unsigned long )buf->data); +} +EXPORT_SYMBOL_GPL (tpm_buf_destroy); + +u32 tpm_buf_length (struct tpm_buf *buf)
Ditto.
drivers/char/tpm/tpm-buf.c line 46
Quoted 8 lines
drivers/char/tpm/tpm-buf.c new file +8
-0+{ +struct tpm_header *head = (struct tpm_header *)buf->data; + +return be32_to_cpu (head->length); +} +EXPORT_SYMBOL_GPL (tpm_buf_length); + +static u16 tpm_buf_tag (struct tpm_buf *buf)
Ditto.
Quoted 19 lines
drivers/char/tpm/tpm-buf.c new file +19
-0+{ +struct tpm_header *head = (struct tpm_header *)buf->data; + +return be16_to_cpu (head->tag); +} + +void tpm_buf_append (struct tpm_buf *buf, +const unsigned char *new_data, +unsigned int new_len) +{ +struct tpm_header *head = (struct tpm_header *) buf->data; +u32 len =tpm_buf_length (buf); + +/* Return silently if overflow has already happened. */ +if (buf->flags &TPM_BUF_OVERFLOW ) +return ; + +if ((len + new_len) >PAGE_SIZE ) { +WARN (1 ,"tpm_buf: overflow\n" );
drivers/char/tpm/tpm-buf.c line 75
Quoted 10 lines
drivers/char/tpm/tpm-buf.c new file +10
-0+ buf->flags |=TPM_BUF_OVERFLOW ; +return ; + } + +memcpy (&buf->data[len], new_data, new_len); + head->length =cpu_to_be32 (len + new_len); +} +EXPORT_SYMBOL_GPL (tpm_buf_append); + +void tpm_buf_append_u8 (struct tpm_buf *buf,const u8 value)
Ditto.
drivers/char/tpm/tpm-buf.c line 81
Quoted 6 lines
drivers/char/tpm/tpm-buf.c new file +6
-0+{ +tpm_buf_append (buf, &value,1 ); +} +EXPORT_SYMBOL_GPL (tpm_buf_append_u8); + +void tpm_buf_append_u16 (struct tpm_buf *buf,const u16 value)
Ditto.
drivers/char/tpm/tpm-buf.c line 89
Quoted 8 lines
drivers/char/tpm/tpm-buf.c new file +8
-0+{ +__be16 value2 =cpu_to_be16 (value); + +tpm_buf_append (buf, (u8 *) &value2,2 ); +} +EXPORT_SYMBOL_GPL (tpm_buf_append_u16); + +void tpm_buf_append_u32 (struct tpm_buf *buf,const u32 value)
Ditto.
Quoted 107 lines
drivers/char/tpm/tpm-buf.c new file +6
-0+{ +__be32 value2 =cpu_to_be32 (value); + +tpm_buf_append (buf, (u8 *) &value2,4 ); +} +EXPORT_SYMBOL_GPL (tpm_buf_append_u32);include/linux/tpm.h +9
-77@@ -322,84 +322,16 @@ struct tpm2_hash {unsigned int tpm_id; }; -static inline void tpm_buf_reset (struct tpm_buf *buf,u16 tag,u32 ordinal) -{ -struct tpm_header *head = (struct tpm_header *)buf->data; - - head->tag =cpu_to_be16 (tag); - head->length =cpu_to_be32 (sizeof (*head)); - head->ordinal =cpu_to_be32 (ordinal); -} - -static inline int tpm_buf_init (struct tpm_buf *buf,u16 tag,u32 ordinal) -{ - buf->data = (u8 *)__get_free_page (GFP_KERNEL ); -if (!buf->data) -return -ENOMEM ; - - buf->flags =0 ; -tpm_buf_reset (buf, tag, ordinal); -return 0 ; -} - -static inline void tpm_buf_destroy (struct tpm_buf *buf) -{ -free_page ((unsigned long )buf->data); -} - -static inline u32 tpm_buf_length (struct tpm_buf *buf) -{ -struct tpm_header *head = (struct tpm_header *)buf->data; - -return be32_to_cpu (head->length); -} - -static inline u16 tpm_buf_tag (struct tpm_buf *buf) -{ -struct tpm_header *head = (struct tpm_header *)buf->data; - -return be16_to_cpu (head->tag); -} - -static inline void tpm_buf_append (struct tpm_buf *buf, -const unsigned char *new_data, -unsigned int new_len) -{ -struct tpm_header *head = (struct tpm_header *)buf->data; -u32 len =tpm_buf_length (buf); - -/* Return silently if overflow has already happened. */ -if (buf->flags &TPM_BUF_OVERFLOW ) -return ; - -if ((len + new_len) >PAGE_SIZE ) { -WARN (1 ,"tpm_buf: overflow\n" ); - buf->flags |=TPM_BUF_OVERFLOW ; -return ; - } -memcpy (&buf->data[len], new_data, new_len); - head->length =cpu_to_be32 (len + new_len); -} - -static inline void tpm_buf_append_u8 (struct tpm_buf *buf,const u8 value) -{ -tpm_buf_append (buf, &value,1 ); -} - -static inline void tpm_buf_append_u16 (struct tpm_buf *buf,const u16 value) -{ -__be16 value2 =cpu_to_be16 (value); - -tpm_buf_append (buf, (u8 *) &value2,2 ); -} - -static inline void tpm_buf_append_u32 (struct tpm_buf *buf,const u32 value) -{ -__be32 value2 =cpu_to_be32 (value); - -tpm_buf_append (buf, (u8 *) &value2,4 ); -} +int tpm_buf_init (struct tpm_buf *buf,u16 tag,u32 ordinal); +void tpm_buf_reset (struct tpm_buf *buf,u16 tag,u32 ordinal); +void tpm_buf_destroy (struct tpm_buf *buf); +u32 tpm_buf_length (struct tpm_buf *buf); +void tpm_buf_append (struct tpm_buf *buf,const unsigned char *new_data, +unsigned int new_len); +void tpm_buf_append_u8 (struct tpm_buf *buf,const u8 value); +void tpm_buf_append_u16 (struct tpm_buf *buf,const u16 value); +void tpm_buf_append_u32 (struct tpm_buf *buf,const u32 value);/* * Check if TPM device is in the firmware upgrade mode. Signature · 1 line
2.35.3
Since you are now moving things around, I think we should make best of it.
I'd consider a subsequent patch after this to further clean up the flow:
struct tpm_buf *tpm_buf_init(u16 tag, u32 ordinal)
{
struct tpm_buf *buf;
buf = kzalloc(sizeof(*buf), GFP_KERNEL);
if (!buf)
goto err_alloc_buf;
buf->data = (u8 *)__get_free_page(GFP_KERNEL);
if (!buf->data)
goto err_alloc_page;
buf->flags = 0;
tpm_buf_reset(buf, tag, ordinal);
return 0;err_alloc_page:
kfree(buf);
err_alloc_buf:
return ERR_PTR(-ENOMEM); }
EXPORT_SYMBOL_GPL(tpm_buf_init);
void tpm_buf_destroy(struct tpm_buf *buf)
{
free_page((unsigned long)buf->data);
kfree(buf);
}There is 28 call sites for tpm_buf_init() buf the change is fairly trival as tpm_buf_destroy() calls can stay as they are.
BR, Jarkko
James Bottomley
maintainerPATCH 3/12 tpm: add buffer handling for TPM2B types
In reply to James BottomleyMost complex TPM commands require appending TPM2B buffers to the command body. Since TPM2B types are essentially variable size arrays, it makes it impossible to represent these complex command arguments as structures and we simply have to build them up using append primitives like these.
--- drivers/char/tpm/tpm-buf.c | 71 +++++++++++++++++++++++++++++++++++--- include/linux/tpm.h | 3 ++ 2 files changed, 69 insertions(+), 5 deletions(-)
drivers/char/tpm/tpm-buf.c +66 -5
@@ -7,17 +7,16 @@#include <linux/module.h> #include <linux/tpm.h> -int tpm_buf_init (struct tpm_buf *buf,u16 tag,u32 ordinal) +static int __tpm_buf_init (struct tpm_buf *buf) { buf->data = (u8 *)__get_free_page (GFP_KERNEL );if (!buf->data)return -ENOMEM ; buf->flags =0 ; -tpm_buf_reset (buf, tag, ordinal); +return 0 ; } -EXPORT_SYMBOL_GPL (tpm_buf_init);void tpm_buf_reset (struct tpm_buf *buf,u16 tag,u32 ordinal) {
@@ -29,17 +28,60 @@ void tpm_buf_reset(struct tpm_buf *buf, u16 tag, u32 ordinal) }EXPORT_SYMBOL_GPL (tpm_buf_reset); +int tpm_buf_init (struct tpm_buf *buf,u16 tag,u32 ordinal) +{ +int rc; + + rc =__tpm_buf_init (buf); +if (rc) +return rc; + +tpm_buf_reset (buf, tag, ordinal); + +return 0 ; +} +EXPORT_SYMBOL_GPL (tpm_buf_init); + +int tpm_buf_init_2b (struct tpm_buf *buf) +{ +struct tpm_header *head; +int rc; + + rc =__tpm_buf_init (buf); +if (rc) +return rc; + + head = (struct tpm_header *) buf->data; + + head->length =cpu_to_be32 (sizeof (*head)); + + buf->flags =TPM_BUF_2B ; +return 0 ; +} +EXPORT_SYMBOL_GPL (tpm_buf_init_2b); +void tpm_buf_destroy (struct tpm_buf *buf) {free_page ((unsigned long )buf->data); }EXPORT_SYMBOL_GPL (tpm_buf_destroy); +static void *tpm_buf_data (struct tpm_buf *buf) +{ +if (buf->flags &TPM_BUF_2B ) +return buf->data +TPM_HEADER_SIZE ; +return buf->data; +} +u32 tpm_buf_length (struct tpm_buf *buf) {struct tpm_header *head = (struct tpm_header *)buf->data; +u32 len; -return be32_to_cpu (head->length); + len =be32_to_cpu (head->length); +if (buf->flags &TPM_BUF_2B ) + len -=sizeof (*head); +return len; }EXPORT_SYMBOL_GPL (tpm_buf_length);
@@ -55,7 +97,7 @@ void tpm_buf_append(struct tpm_buf *buf, unsigned int new_len) {struct tpm_header *head = (struct tpm_header *) buf->data; -u32 len =tpm_buf_length (buf); +u32 len =be32_to_cpu (head->length);/* Return silently if overflow has already happened. */ if (buf->flags &TPM_BUF_OVERFLOW )
@@ -93,3 +135,22 @@ void tpm_buf_append_u32(struct tpm_buf *buf, const u32 value)tpm_buf_append (buf, (u8 *) &value2,4 ); }EXPORT_SYMBOL_GPL (tpm_buf_append_u32); + +static void tpm_buf_reset_int (struct tpm_buf *buf) +{ +struct tpm_header *head; + + head = (struct tpm_header *)buf->data; + head->length =cpu_to_be32 (sizeof (*head)); +} + +void tpm_buf_append_2b (struct tpm_buf *buf,struct tpm_buf *tpm2b) +{ +u16 len =tpm_buf_length (tpm2b); + +tpm_buf_append_u16 (buf, len); +tpm_buf_append (buf,tpm_buf_data (tpm2b), len); +/* clear the buf for reuse */ +tpm_buf_reset_int (tpm2b); +} +EXPORT_SYMBOL_GPL (tpm_buf_append_2b);
include/linux/tpm.h +3 -0
@@ -300,6 +300,7 @@ struct tpm_header {enum tpm_buf_flags {TPM_BUF_OVERFLOW =BIT (0 ), +TPM_BUF_2B =BIT (1 ), };struct tpm_buf {
@@ -324,6 +325,7 @@ struct tpm2_hash {int tpm_buf_init (struct tpm_buf *buf,u16 tag,u32 ordinal); +int tpm_buf_init_2b (struct tpm_buf *buf);void tpm_buf_reset (struct tpm_buf *buf,u16 tag,u32 ordinal);void tpm_buf_destroy (struct tpm_buf *buf);u32 tpm_buf_length (struct tpm_buf *buf);
@@ -332,6 +334,7 @@ void tpm_buf_append(struct tpm_buf *buf, const unsigned char *new_data,void tpm_buf_append_u8 (struct tpm_buf *buf,const u8 value);void tpm_buf_append_u16 (struct tpm_buf *buf,const u16 value);void tpm_buf_append_u32 (struct tpm_buf *buf,const u32 value); +void tpm_buf_append_2b (struct tpm_buf *buf,struct tpm_buf *tpm2b);/* * Check if TPM device is in the firmware upgrade mode.
Signature · 1 line
2.35.3
Jarkko Sakkinen
maintainerRe: [PATCH 03/12] tpm: add buffer handling for TPM2B types
Quoted 45 lines · James Bottomley
Most complex TPM commands require appending TPM2B buffers to the command body. Since TPM2B types are essentially variable size arrays, it makes it impossible to represent these complex command arguments as structures and we simply have to build them up using append primitives like these.
Signed-off-by James Bottomley <[email protected]>--- drivers/char/tpm/tpm-buf.c | 71 +++++++++++++++++++++++++++++++++++--- include/linux/tpm.h | 3 ++ 2 files changed, 69 insertions(+), 5 deletions(-)drivers/char/tpm/tpm-buf.c +7
-3@@ -7,17 +7,16 @@#include <linux/module.h> #include <linux/tpm.h> -int tpm_buf_init (struct tpm_buf *buf,u16 tag,u32 ordinal) +static int __tpm_buf_init (struct tpm_buf *buf) { buf->data = (u8 *)__get_free_page (GFP_KERNEL );if (!buf->data)return -ENOMEM ; buf->flags =0 ; -tpm_buf_reset (buf, tag, ordinal); +return 0 ; } -EXPORT_SYMBOL_GPL (tpm_buf_init);void tpm_buf_reset (struct tpm_buf *buf,u16 tag,u32 ordinal) {@@ -29,17 +28,60 @@ void tpm_buf_reset(struct tpm_buf *buf, u16 tag, u32 ordinal) }EXPORT_SYMBOL_GPL (tpm_buf_reset); +int tpm_buf_init (struct tpm_buf *buf,u16 tag,u32 ordinal) +{ +int rc; + + rc =__tpm_buf_init (buf);
drivers/char/tpm/tpm-buf.c void tpm_buf_reset(struct tpm_buf *buf, u16 tag, u32 ordinal) · line 45
Quoted 10 lines
drivers/char/tpm/tpm-buf.c +10
-0+if (rc) +return rc; + +tpm_buf_reset (buf, tag, ordinal); + +return 0 ; +} +EXPORT_SYMBOL_GPL (tpm_buf_init); + +int tpm_buf_init_2b (struct tpm_buf *buf)
kdoc
drivers/char/tpm/tpm-buf.c void tpm_buf_reset(struct tpm_buf *buf, u16 tag, u32 ordinal) · line 58
Quoted 13 lines
drivers/char/tpm/tpm-buf.c +13
-0+{ +struct tpm_header *head; +int rc; + + rc =__tpm_buf_init (buf); +if (rc) +return rc; + + head = (struct tpm_header *) buf->data; + + head->length =cpu_to_be32 (sizeof (*head)); + + buf->flags =TPM_BUF_2B ;
Please make tpm_buf_init() and tpm_buf_reset() to work for both cases.
This explodes the whole thing into an unmaintainable mess. It is better to have a type as a parameter for tpm_buf_init() and have only single flow instead of open coded and patched variation.
I'd simply just put it as:
struct tpm_buf *tpm_buf_init(u16 tag, u32 ordinal, bool tpm2b)
Quoted 67 lines
drivers/char/tpm/tpm-buf.c +36
-2+return 0 ; +} +EXPORT_SYMBOL_GPL (tpm_buf_init_2b); +void tpm_buf_destroy (struct tpm_buf *buf) {free_page ((unsigned long )buf->data); }EXPORT_SYMBOL_GPL (tpm_buf_destroy); +static void *tpm_buf_data (struct tpm_buf *buf) +{ +if (buf->flags &TPM_BUF_2B ) +return buf->data +TPM_HEADER_SIZE ; +return buf->data; +} +u32 tpm_buf_length (struct tpm_buf *buf) {struct tpm_header *head = (struct tpm_header *)buf->data; +u32 len; -return be32_to_cpu (head->length); + len =be32_to_cpu (head->length); +if (buf->flags &TPM_BUF_2B ) + len -=sizeof (*head); +return len; }EXPORT_SYMBOL_GPL (tpm_buf_length);@@ -55,7 +97,7 @@ void tpm_buf_append(struct tpm_buf *buf, unsigned int new_len) {struct tpm_header *head = (struct tpm_header *) buf->data; -u32 len =tpm_buf_length (buf); +u32 len =be32_to_cpu (head->length);/* Return silently if overflow has already happened. */ if (buf->flags &TPM_BUF_OVERFLOW )@@ -93,3 +135,22 @@ void tpm_buf_append_u32(struct tpm_buf *buf, const u32 value)tpm_buf_append (buf, (u8 *) &value2,4 ); }EXPORT_SYMBOL_GPL (tpm_buf_append_u32); + +static void tpm_buf_reset_int (struct tpm_buf *buf) +{ +struct tpm_header *head; + + head = (struct tpm_header *)buf->data; + head->length =cpu_to_be32 (sizeof (*head)); +} + +void tpm_buf_append_2b (struct tpm_buf *buf,struct tpm_buf *tpm2b) +{ +u16 len =tpm_buf_length (tpm2b); + +tpm_buf_append_u16 (buf, len); +tpm_buf_append (buf,tpm_buf_data (tpm2b), len); +/* clear the buf for reuse */ +tpm_buf_reset_int (tpm2b); +} +EXPORT_SYMBOL_GPL (tpm_buf_append_2b);include/linux/tpm.h +0
-0
include/linux/tpm.h struct tpm_header { · line 304
Quoted 5 lines
include/linux/tpm.h +1
-0@@ -300,6 +300,7 @@ struct tpm_header {enum tpm_buf_flags {TPM_BUF_OVERFLOW =BIT (0 ), +TPM_BUF_2B =BIT (1 ), };
This is IMHO unnecessary complex.
I think we could just have two bools:
bool overflow;
bool tpm2b;Quoted 21 lines
include/linux/tpm.h +2
-0struct tpm_buf {@@ -324,6 +325,7 @@ struct tpm2_hash {int tpm_buf_init (struct tpm_buf *buf,u16 tag,u32 ordinal); +int tpm_buf_init_2b (struct tpm_buf *buf);void tpm_buf_reset (struct tpm_buf *buf,u16 tag,u32 ordinal);void tpm_buf_destroy (struct tpm_buf *buf);u32 tpm_buf_length (struct tpm_buf *buf);@@ -332,6 +334,7 @@ void tpm_buf_append(struct tpm_buf *buf, const unsigned char *new_data,void tpm_buf_append_u8 (struct tpm_buf *buf,const u8 value);void tpm_buf_append_u16 (struct tpm_buf *buf,const u16 value);void tpm_buf_append_u32 (struct tpm_buf *buf,const u32 value); +void tpm_buf_append_2b (struct tpm_buf *buf,struct tpm_buf *tpm2b);/* * Check if TPM device is in the firmware upgrade mode. Signature · 1 line
2.35.3
BR, Jarkko
James Bottomley
maintainerRe: [PATCH 03/12] tpm: add buffer handling for TPM2B types
Quoted 54 lines · Jarkko Sakkinen
Quoted 53 lines · James Bottomley
Most complex TPM commands require appending TPM2B buffers to the command body. Since TPM2B types are essentially variable size arrays, it makes it impossible to represent these complex command arguments as structures and we simply have to build them up using append primitives like these.
Signed-off-by: James Bottomley
<[email protected]>
---
drivers/char/tpm/tpm-buf.c | 71
+++++++++++++++++++++++++++++++++++---
include/linux/tpm.h | 3 ++2 files changed, 69 insertions(+), 5 deletions(-)drivers/char/tpm/tpm- renamed from drivers/char/tpm/tpm-buf.c +0
-0
buf.c
indexca59b92e0f95..292c6f14f72c100644drivers/char/tpm/tpm-buf.c +0
-0@@ -7,17 +7,16 @@ #include <linux/module.h> #include <linux/tpm.h>drivers/char/tpm/tpm-buf.c +1
-1-int tpm_buf_init (struct tpm_buf *buf,u16 tag,u32 ordinal) +static int __tpm_buf_init (struct tpm_buf *buf){
buf->data = (u8 *)__get_free_page(GFP_KERNEL); if (!buf->data)
return-ENOMEM;
buf->flags = 0;
- tpm_buf_reset(buf, tag, ordinal);
+
return 0;
}
-EXPORT_SYMBOL_GPL(tpm_buf_init);
voidtpm_buf_reset(struct tpm_buf *buf, u16 tag, u32 ordinal){drivers/char/tpm/tpm-buf.c +0
-0@@ -29,17 +28,60 @@ void tpm_buf_reset(struct tpm_buf *buf, u16 tag, u32 ordinal) } EXPORT_SYMBOL_GPL(tpm_buf_reset);drivers/char/tpm/tpm-buf.c +9
-0+int tpm_buf_init (struct tpm_buf *buf,u16 tag,u32 ordinal) +{ + int rc; + + rc = __tpm_buf_init(buf); + if (rc) + return rc; + + tpm_buf_reset(buf, tag, ordinal);
Quoted 8 lines
drivers/char/tpm/tpm-buf.c void tpm_buf_reset(struct tpm_buf *buf, u16 tag, u32 ordinal) · line 45
Quoted 6 lines
drivers/char/tpm/tpm-buf.c +6
-0+ + return 0; +} +EXPORT_SYMBOL_GPL (tpm_buf_init); + +int tpm_buf_init_2b (struct tpm_buf *buf)kdoc
I'm currently working on adding kdoc to everything. However:
Quoted 8 lines
Quoted 8 lines
drivers/char/tpm/tpm-buf.c +8
-0+{ + struct tpm_header *head; + int rc; + + rc = __tpm_buf_init(buf); + if (rc) + return rc; +
Quoted 8 lines
drivers/char/tpm/tpm-buf.c void tpm_buf_reset(struct tpm_buf *buf, u16 tag, u32 ordinal) · line 58
Quoted 5 lines
+ head = (struct
tpm_header*)buf->data;drivers/char/tpm/tpm-buf.c +4
-0+ + head->length = cpu_to_be32(sizeof(*head)); + + buf->flags = TPM_BUF_2B;Please make
tpm_buf_init()andtpm_buf_reset()to work for both cases.
That's not a good idea: tpm_buf_init() and tpm_buf_reset() are used to initialize *command* buffers. tpm_buf_init_2b() is used for parameters within commands and can't encompass whole commands, so the arguments are different (that's why tpm_buf_init_2b() has no tag or ordinal).
Quoted 7 lines
This explodes the whole thing into an unmaintainable mess. It is better to have a type as a parameter for
tpm_buf_init()and have only single flow instead of open coded and patched variation.I'd simply just put it as:
struct
tpm_buf*tpm_buf_init(u16 tag, u32 ordinal, bool tpm2b)
The convention in Linux is that it's better to have named initializers if we can rather than use less obvious booleans or flags ... think the conversion from printk(KERN_ERR, ...) to pr_err(...)
Additionally tag and ordinal have no meaning for a tpm2b, so you're really gluing two incompatible initializations into one which is bound to cause confusion.
I've no objection in principle to doing a reset of a tpm2b (except, again, it has no use for tag or ordinal) but I've just not got any code that would use it, so I was leaving it out until someone had an actual use case.
[...]
Quoted 13 lines
Quoted 9 lines
index
150b39b6190e..f2d4dab6d832100644include/linux/tpm.h +0
-0@@ -300,6 +300,7 @@ struct tpm_header {
enumtpm_buf_flags{
TPM_BUF_OVERFLOW = BIT(0),
+ TPM_BUF_2B = BIT(1),
};This is IMHO unnecessary complex.
Quoted 4 lines
I think we could just have two bools:
bool overflow;
bool tpm2b;
The advice (in the coding-style.rst bool section) is not to do this but go the other way (so use flags instead of a string of bools). The reason is that even though bool represents a true/false value, it usually takes one machine word (32 bits or sometimes more) to do it, so bools tend to bloat structures over single bit fields.
James
James Bottomley
maintainerPATCH 4/12 tpm: add cursor based buffer functions for response parsing
In reply to James BottomleyOnce we have encryption and authentication, marshalling and unmarshalling sessions becomes hugely complex. Add cursor based functions which update the current pointer to make response parsing easier.
--- drivers/char/tpm/tpm-buf.c | 29 +++++++++++++++++++++++++++++ include/linux/tpm.h | 3 +++ 2 files changed, 32 insertions(+)
drivers/char/tpm/tpm-buf.c +29 -0
@@ -7,6 +7,8 @@#include <linux/module.h> #include <linux/tpm.h> +#include <asm/unaligned.h> +static int __tpm_buf_init (struct tpm_buf *buf) { buf->data = (u8 *)__get_free_page (GFP_KERNEL );
@@ -154,3 +156,30 @@ void tpm_buf_append_2b(struct tpm_buf *buf, struct tpm_buf *tpm2b)tpm_buf_reset_int (tpm2b); }EXPORT_SYMBOL_GPL (tpm_buf_append_2b); + +/* functions for unmarshalling data and moving the cursor */ +u8 tpm_get_inc_u8 (const u8 **ptr) +{ +return *((*ptr)++); +} +EXPORT_SYMBOL_GPL (tpm_get_inc_u8); + +u16 tpm_get_inc_u16 (const u8 **ptr) +{ +u16 val; + + val =get_unaligned_be16 (*ptr); + *ptr +=sizeof (val); +return val; +} +EXPORT_SYMBOL_GPL (tpm_get_inc_u16); + +u32 tpm_get_inc_u32 (const u8 **ptr) +{ +u32 val; + + val =get_unaligned_be32 (*ptr); + *ptr +=sizeof (val); +return val; +} +EXPORT_SYMBOL_GPL (tpm_get_inc_u32);
include/linux/tpm.h +3 -0
@@ -335,6 +335,9 @@ void tpm_buf_append_u8(struct tpm_buf *buf, const u8 value);void tpm_buf_append_u16 (struct tpm_buf *buf,const u16 value);void tpm_buf_append_u32 (struct tpm_buf *buf,const u32 value);void tpm_buf_append_2b (struct tpm_buf *buf,struct tpm_buf *tpm2b); +u8 tpm_get_inc_u8 (const u8 **ptr); +u16 tpm_get_inc_u16 (const u8 **ptr); +u32 tpm_get_inc_u32 (const u8 **ptr);/* * Check if TPM device is in the firmware upgrade mode.
Signature · 1 line
2.35.3
Jarkko Sakkinen
maintainerRe: [PATCH 04/12] tpm: add cursor based buffer functions for response parsing
Quoted 4 lines · James Bottomley
Once we have encryption and authentication, marshalling and unmarshalling sessions becomes hugely complex. Add cursor based functions which update the current pointer to make response parsing easier.
You need to properly introduce the concept of cursor based function, and how that maps into implementation.
I understand absolutely nothing of the change just by reading the commit message, it is pretty much useless peace of text.
Quoted 68 lines
Signed-off-by James Bottomley <[email protected]>--- drivers/char/tpm/tpm-buf.c | 29 +++++++++++++++++++++++++++++ include/linux/tpm.h | 3 +++ 2 files changed, 32 insertions(+)drivers/char/tpm/tpm-buf.c +29
-0@@ -7,6 +7,8 @@#include <linux/module.h> #include <linux/tpm.h> +#include <asm/unaligned.h> +static int __tpm_buf_init (struct tpm_buf *buf) { buf->data = (u8 *)__get_free_page (GFP_KERNEL );@@ -154,3 +156,30 @@ void tpm_buf_append_2b(struct tpm_buf *buf, struct tpm_buf *tpm2b)tpm_buf_reset_int (tpm2b); }EXPORT_SYMBOL_GPL (tpm_buf_append_2b); + +/* functions for unmarshalling data and moving the cursor */ +u8 tpm_get_inc_u8 (const u8 **ptr) +{ +return *((*ptr)++); +} +EXPORT_SYMBOL_GPL (tpm_get_inc_u8); + +u16 tpm_get_inc_u16 (const u8 **ptr) +{ +u16 val; + + val =get_unaligned_be16 (*ptr); + *ptr +=sizeof (val); +return val; +} +EXPORT_SYMBOL_GPL (tpm_get_inc_u16); + +u32 tpm_get_inc_u32 (const u8 **ptr) +{ +u32 val; + + val =get_unaligned_be32 (*ptr); + *ptr +=sizeof (val); +return val; +} +EXPORT_SYMBOL_GPL (tpm_get_inc_u32);include/linux/tpm.h +3
-0@@ -335,6 +335,9 @@ void tpm_buf_append_u8(struct tpm_buf *buf, const u8 value);void tpm_buf_append_u16 (struct tpm_buf *buf,const u16 value);void tpm_buf_append_u32 (struct tpm_buf *buf,const u32 value);void tpm_buf_append_2b (struct tpm_buf *buf,struct tpm_buf *tpm2b); +u8 tpm_get_inc_u8 (const u8 **ptr); +u16 tpm_get_inc_u16 (const u8 **ptr); +u32 tpm_get_inc_u32 (const u8 **ptr);/* * Check if TPM device is in the firmware upgrade mode. Signature · 1 line
2.35.3
BR, Jarkko
James Bottomley
maintainerPATCH 5/12 tpm: add buffer function to point to returned parameters
In reply to James BottomleyIntroducing encryption sessions changes where the return parameters are located in the buffer because if a return session is present they're 4 bytes beyond the header with those 4 bytes showing the parameter length. If there is no return session, then they're in the usual place immediately after the header. The tpm_buf_parameters() encapsulates this calculation and should be used everywhere &buf.data[TPM_HEADER_SIZE] is used now
--- drivers/char/tpm/tpm-buf.c | 10 ++++++++++ include/linux/tpm.h | 2 ++ 2 files changed, 12 insertions(+)
drivers/char/tpm/tpm-buf.c +10 -0
@@ -183,3 +183,13 @@ u32 tpm_get_inc_u32(const u8 **ptr)return val; }EXPORT_SYMBOL_GPL (tpm_get_inc_u32); + +u8 *tpm_buf_parameters (struct tpm_buf *buf) +{ +int offset =TPM_HEADER_SIZE ; + +if (tpm_buf_tag (buf) ==TPM2_ST_SESSIONS ) + offset +=4 ; + +return &buf->data[offset]; +}
include/linux/tpm.h +2 -0
@@ -339,6 +339,8 @@ u8 tpm_get_inc_u8(const u8 **ptr);u16 tpm_get_inc_u16 (const u8 **ptr);u32 tpm_get_inc_u32 (const u8 **ptr); +u8 *tpm_buf_parameters (struct tpm_buf *buf); +/* * Check if TPM device is in the firmware upgrade mode. */
Signature · 1 line
2.35.3
Jarkko Sakkinen
maintainerRe: [PATCH 05/12] tpm: add buffer function to point to returned parameters
Quoted 19 lines · James Bottomley
Introducing encryption sessions changes where the return parameters are located in the buffer because if a return session is present they're 4 bytes beyond the header with those 4 bytes showing the parameter length. If there is no return session, then they're in the usual place immediately after the header. The
tpm_buf_parameters()encapsulates this calculation and should be used everywhere &buf.data[TPM_HEADER_SIZE] is used nowSigned-off-by James Bottomley <[email protected]>--- drivers/char/tpm/tpm-buf.c | 10 ++++++++++ include/linux/tpm.h | 2 ++ 2 files changed, 12 insertions(+)drivers/char/tpm/tpm-buf.c +0
-0
drivers/char/tpm/tpm-buf.c u32 tpm_get_inc_u32(const u8 **ptr) · line 187
Quoted 5 lines
drivers/char/tpm/tpm-buf.c +2
-0@@ -183,3 +183,13 @@ u32 tpm_get_inc_u32(const u8 **ptr)return val; }EXPORT_SYMBOL_GPL (tpm_get_inc_u32); + +u8 *tpm_buf_parameters (struct tpm_buf *buf)
kdoc
Quoted 24 lines
drivers/char/tpm/tpm-buf.c +8
-0+{ +int offset =TPM_HEADER_SIZE ; + +if (tpm_buf_tag (buf) ==TPM2_ST_SESSIONS ) + offset +=4 ; + +return &buf->data[offset]; +}include/linux/tpm.h +2
-0@@ -339,6 +339,8 @@ u8 tpm_get_inc_u8(const u8 **ptr);u16 tpm_get_inc_u16 (const u8 **ptr);u32 tpm_get_inc_u32 (const u8 **ptr); +u8 *tpm_buf_parameters (struct tpm_buf *buf); +/* * Check if TPM device is in the firmware upgrade mode. */ Signature · 1 line
2.35.3
BR, Jarkko
James Bottomley
maintainerPATCH 6/12 tpm: export the context save and load commands
In reply to James BottomleyThe TPM2 session HMAC and encryption handling code needs to save and restore a single volatile context for the elliptic curve version of the NULL seed, so export the APIs which do this for internal use.
--- drivers/char/tpm/tpm.h | 4 ++++ drivers/char/tpm/tpm2-space.c | 8 ++++---- 2 files changed, 8 insertions(+), 4 deletions(-)
drivers/char/tpm/tpm.h +4 -0
@@ -237,6 +237,10 @@ int tpm2_commit_space(struct tpm_chip *chip, struct tpm_space *space, void *buf, size_t *bufsiz);int tpm_devs_add (struct tpm_chip *chip);void tpm_devs_remove (struct tpm_chip *chip); +int tpm2_save_context (struct tpm_chip *chip,u32 handle,u8 *buf, +unsigned int buf_size,unsigned int *offset); +int tpm2_load_context (struct tpm_chip *chip,u8 *buf, +unsigned int *offset,u32 *handle);void tpm_bios_log_setup (struct tpm_chip *chip);void tpm_bios_log_teardown (struct tpm_chip *chip);
drivers/char/tpm/tpm2-space.c +4 -4
@@ -68,8 +68,8 @@ void tpm2_del_space(struct tpm_chip *chip, struct tpm_space *space)kfree (space->session_buf); } -static int tpm2_load_context (struct tpm_chip *chip,u8 *buf, -unsigned int *offset,u32 *handle) +int tpm2_load_context (struct tpm_chip *chip,u8 *buf, +unsigned int *offset,u32 *handle) {struct tpm_buf tbuf;struct tpm2_context *ctx;
@@ -119,8 +119,8 @@ static int tpm2_load_context(struct tpm_chip *chip, u8 *buf,return 0 ; } -static int tpm2_save_context (struct tpm_chip *chip,u32 handle,u8 *buf, -unsigned int buf_size,unsigned int *offset) +int tpm2_save_context (struct tpm_chip *chip,u32 handle,u8 *buf, +unsigned int buf_size,unsigned int *offset) {struct tpm_buf tbuf;unsigned int body_size;
Signature · 1 line
2.35.3
Jarkko Sakkinen
maintainerRe: [PATCH 06/12] tpm: export the context save and load commands
Quoted 5 lines · James Bottomley
The TPM2 session HMAC and encryption handling code needs to save and restore a single volatile context for the elliptic curve version of the NULL seed, so export the APIs which do this for internal use.
Signed-off-by James Bottomley <[email protected]>
These did not need EXPORT_SYMBOL_GPL()?
Quoted 49 lines
--- drivers/char/tpm/tpm.h | 4 ++++ drivers/char/tpm/tpm2-space.c | 8 ++++---- 2 files changed, 8 insertions(+), 4 deletions(-)drivers/char/tpm/tpm.h +4
-0@@ -237,6 +237,10 @@ int tpm2_commit_space(struct tpm_chip *chip, struct tpm_space *space, void *buf, size_t *bufsiz);int tpm_devs_add (struct tpm_chip *chip);void tpm_devs_remove (struct tpm_chip *chip); +int tpm2_save_context (struct tpm_chip *chip,u32 handle,u8 *buf, +unsigned int buf_size,unsigned int *offset); +int tpm2_load_context (struct tpm_chip *chip,u8 *buf, +unsigned int *offset,u32 *handle);void tpm_bios_log_setup (struct tpm_chip *chip);void tpm_bios_log_teardown (struct tpm_chip *chip);drivers/char/tpm/tpm2-space.c +4
-4@@ -68,8 +68,8 @@ void tpm2_del_space(struct tpm_chip *chip, struct tpm_space *space)kfree (space->session_buf); } -static int tpm2_load_context (struct tpm_chip *chip,u8 *buf, -unsigned int *offset,u32 *handle) +int tpm2_load_context (struct tpm_chip *chip,u8 *buf, +unsigned int *offset,u32 *handle) {struct tpm_buf tbuf;struct tpm2_context *ctx;@@ -119,8 +119,8 @@ static int tpm2_load_context(struct tpm_chip *chip, u8 *buf,return 0 ; } -static int tpm2_save_context (struct tpm_chip *chip,u32 handle,u8 *buf, -unsigned int buf_size,unsigned int *offset) +int tpm2_save_context (struct tpm_chip *chip,u32 handle,u8 *buf, +unsigned int buf_size,unsigned int *offset) {struct tpm_buf tbuf;unsigned int body_size;Signature · 1 line
2.35.3
BR, Jarkko
James Bottomley
maintainerRe: [PATCH 06/12] tpm: export the context save and load commands
Quoted 10 lines · Jarkko Sakkinen
Quoted 7 lines · James Bottomley
The TPM2 session HMAC and encryption handling code needs to save and restore a single volatile context for the elliptic curve version of the NULL seed, so export the APIs which do this for internal use.
Signed-off-by: James Bottomley
<[email protected]>These did not need
EXPORT_SYMBOL_GPL()?
No, because they're still internal to the overall tpm.ko even if it is compiled as a module. I don't think there's a huge reason to keep them as an internal only to the tpm core API, but equally without a consumer outside the core module, there's no reason to export them.
James
James Bottomley
maintainerPATCH 7/12 tpm: Add full HMAC and encrypt/decrypt session handling code
In reply to James BottomleyAdd true session based HMAC authentication plus parameter decryption and response encryption using AES. The basic design is to segregate all the nasty crypto, hash and hmac code into tpm2-sessions.c and export a usable API. The API first of all starts off by gaining a session with
tpm2_start_auth_session()
Which initiates a session with the TPM and allocates an opaque tpm2_auth structure to handle the session parameters. Then the use is simply:
* tpm_buf_append_name() in place of the tpm_buf_append_u32 for the handles
* tpm_buf_append_hmac_session() where tpm2_append_auth() would go
* tpm_buf_fill_hmac_session() called after the entire command buffer is finished but before tpm_transmit_cmd() is called which computes the correct HMAC and places it in the command at the correct location.
Finally, after tpm_transmit_cmd() is called, tpm_buf_check_hmac_response() is called to check that the returned HMAC matched and collect the new state for the next use of the session, if any.
The features of the session is controlled by the session attributes set in tpm_buf_append_hmac_session(). If TPM2_SA_CONTINUE_SESSION is not specified, the session will be flushed and the tpm2_auth structure freed in tpm_buf_check_hmac_response(); otherwise the session may be used again. Parameter encryption is specified by or'ing the flag TPM2_SA_DECRYPT and response encryption by or'ing the flag TPM2_SA_ENCRYPT. the various encryptions will be taken care of by tpm_buf_fill_hmac_session() and tpm_buf_check_hmac_response() respectively.
To get all of this to work securely, the Kernel needs a primary key to encrypt the session salt to, so an EC key from the NULL seed is derived and its context saved in the tpm_chip structure. The context is loaded on demand into an available volatile handle when tpm_start_auth_session() is called, but is flushed before that function exits to conserve handles.
---
v2: fix memory leaks from smatch; adjust for name hash size
v3: make tpm2_make_null_primary static
v4: use crypto library functions
--- drivers/char/tpm/Kconfig | 13 + drivers/char/tpm/Makefile | 1 + drivers/char/tpm/tpm-buf.c | 1 + drivers/char/tpm/tpm-chip.c | 3 + drivers/char/tpm/tpm.h | 10 + drivers/char/tpm/tpm2-cmd.c | 5 + drivers/char/tpm/tpm2-sessions.c | 1160 ++++++++++++++++++++++++++++++ include/linux/tpm.h | 165 +++++ 8 files changed, 1358 insertions(+) create mode 100644 drivers/char/tpm/tpm2-sessions.c
drivers/char/tpm/Kconfig +13 -0
@@ -27,6 +27,19 @@ menuconfig TCG_TPM if TCG_TPM +config TPM_BUS_SECURITY + bool "Use secure transactions on the TPM bus" + default y + select CRYPTO_ECDH + select CRYPTO_LIB_AESCFB + select CRYPTO_LIB_SHA256 + help + Setting this causes us to deploy a tamper resistent scheme + for communicating with the TPM to prevent or detect bus + snooping and iterposer attacks like TPM Genie. Saying Y here + adds some encryption overhead to all kernel to TPM + transactions. + config HW_RANDOM_TPM bool "TPM HW Random Number Generator support" depends on TCG_TPM && HW_RANDOM && !(TCG_TPM=y && HW_RANDOM=m)
drivers/char/tpm/Makefile +1 -0
@@ -17,6 +17,7 @@ tpm-y += eventlog/tpm1.o tpm-y += eventlog/tpm2.o tpm-y += tpm-buf.o +tpm-$(CONFIG_TPM_BUS_SECURITY) += tpm2-sessions.o tpm-$(CONFIG_ACPI) += tpm_ppi.o eventlog/acpi.o tpm-$(CONFIG_EFI) += eventlog/efi.o tpm-$(CONFIG_OF) += eventlog/of.o
drivers/char/tpm/tpm-buf.c +1 -0
@@ -27,6 +27,7 @@ void tpm_buf_reset(struct tpm_buf *buf, u16 tag, u32 ordinal) head->tag =cpu_to_be16 (tag); head->length =cpu_to_be32 (sizeof (*head)); head->ordinal =cpu_to_be32 (ordinal); + buf->handles =0 ; }EXPORT_SYMBOL_GPL (tpm_buf_reset);
drivers/char/tpm/tpm-chip.c +3 -0
@@ -271,6 +271,9 @@ static void tpm_dev_release(struct device *dev)kfree (chip->work_space.context_buf);kfree (chip->work_space.session_buf);kfree (chip->allocated_banks); +#ifdef CONFIG_TPM_BUS_SECURITY +kfree (chip->auth); +#endif kfree (chip); }
drivers/char/tpm/tpm.h +10 -0
@@ -246,4 +246,14 @@ void tpm_bios_log_setup(struct tpm_chip *chip);void tpm_bios_log_teardown (struct tpm_chip *chip);int tpm_dev_common_init (void );void tpm_dev_common_exit (void ); + +#ifdef CONFIG_TPM_BUS_SECURITY +int tpm2_sessions_init (struct tpm_chip *chip); +#else +static inline int tpm2_sessions_init (struct tpm_chip *chip) +{ +return 0 ; +} +#endif +#endif
drivers/char/tpm/tpm2-cmd.c +5 -0
@@ -759,6 +759,11 @@ int tpm2_auto_startup(struct tpm_chip *chip) rc =0 ; } +if (rc) + goto out; + + rc =tpm2_sessions_init (chip); + out:/* * Infineon TPM in field upgrade mode will return no data for the number
drivers/char/tpm/tpm2-sessions.c new file +1160 -0
1,160 lines not on this page · raw message
include/linux/tpm.h +165 -0
245 lines not on this page · raw message
Signature · 1 line
2.35.3
kernel test robot
build botRe: [PATCH 07/12] tpm: Add full HMAC and encrypt/decrypt session handling code
I love your patch! Perhaps something to improve:
Hi James,
I love your patch! Perhaps something to improve:
[auto build test WARNING on char-misc/char-misc-testing]
[also build test WARNING on char-misc/char-misc-next char-misc/char-misc-linus herbert-cryptodev-2.6/master herbert-crypto-2.6/master linus/master v6.2-rc8 next-20230216] [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/James-Bottomley/crypto-lib-implement-library-version-of-AES-in-CFB-mode/20230217-044329 patch link: https://lore.kernel.org/r/20230216201410.15010-8-James.Bottomley%40HansenPartnership.com patch subject: [PATCH 07/12] tpm: Add full HMAC and encrypt/decrypt session handling code config: m68k-randconfig-c44-20230212 (https://download.01.org/0day-ci/archive/20230217/[email protected]/config) compiler: m68k-linux-gcc (GCC) 12.1.0
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross chmod +x ~/bin/make.cross
# https://github.com/intel-lab-lkp/linux/commit/1d4f519aa756ba5ed684fb2a2f9ad6e1fb91bdb5
git remote add linux-review https://github.com/intel-lab-lkp/linux
git fetch --no-tags linux-review James-Bottomley/crypto-lib-implement-library-version-of-AES-in-CFB-mode/20230217-044329 git checkout 1d4f519aa756ba5ed684fb2a2f9ad6e1fb91bdb5
# save the config file
mkdir build_dir && cp config build_dir/.config
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross W=1 O=build_dir ARCH=m68k olddefconfig COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross W=1 O=build_dir ARCH=m68k SHELL=/bin/bash drivers/char/tpm/
If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <[email protected]>
| Link: https://lore.kernel.org/oe-kbuild-all/[email protected]/
All warnings (new ones prefixed by >>):
>> drivers/char/tpm/tpm2-sessions.c:337: warning: This comment starts with '/**', but isn't a kernel-doc comment. Refer Documentation/doc-guide/kernel-doc.rst
* tpm_buf_append_hmac_session() append a TPM session element
drivers/char/tpm/tpm2-sessions.c:855: warning: Excess function parameter 'authp' description in 'tpm2_start_auth_session'
vim +337 drivers/char/tpm/tpm2-sessions.c
335
336 /**
> 337 * tpm_buf_append_hmac_session() append a TPM session element
338 * @chip: the TPM chip structure
339 * @buf: The buffer to be appended
340 * @attributes: The session attributes
341 * @passphrase: The session authority (NULL if none)
342 * @passphraselen: The length of the session authority (0 if none)
343 *
344 * This fills in a session structure in the TPM command buffer, except
345 * for the HMAC which cannot be computed until the command buffer is
346 * complete. The type of session is controlled by the @attributes,
347 * the main ones of which are TPM2_SA_CONTINUE_SESSION which means the
348 * session won't terminate after tpm_buf_check_hmac_response(),
349 * TPM2_SA_DECRYPT which means this buffers first parameter should be
350 * encrypted with a session key and TPM2_SA_ENCRYPT, which means the
351 * response buffer's first parameter needs to be decrypted (confusing,
352 * but the defines are written from the point of view of the TPM).
353 *
354 * Any session appended by this command must be finalized by calling
355 * tpm_buf_fill_hmac_session() otherwise the HMAC will be incorrect
356 * and the TPM will reject the command.
357 *
358 * As with most tpm_buf operations, success is assumed because failure
359 * will be caused by an incorrect programming model and indicated by a
360 * kernel message.
361 */
362 void tpm_buf_append_hmac_session(struct tpm_chip *chip, struct tpm_buf *buf,
363 u8 attributes, u8 *passphrase,
364 int passphraselen)
365 {
366 u8 nonce[SHA256_DIGEST_SIZE];
367 u32 len;
368 struct tpm2_auth *auth = chip->auth;
369
370 /*
371 * The Architecture Guide requires us to strip trailing zeros
372 * before computing the HMAC
373 */
374 while (passphrase && passphraselen > 0
375 && passphrase[passphraselen - 1] == '\0')
376 passphraselen--;
377
378 auth->attrs = attributes;
379 auth->passphraselen = passphraselen;
380 if (passphraselen)
381 memcpy(auth->passphrase, passphrase, passphraselen);
382
383 if (auth->session != tpm_buf_length(buf)) {
384 /* we're not the first session */
385 len = get_unaligned_be32(&buf->data[auth->session]);
386 if (4 + len + auth->session != tpm_buf_length(buf)) {
387 WARN(1, "session length mismatch, cannot append");
388 return;
389 }
390
391 /* add our new session */
392 len += 9 + 2 * SHA256_DIGEST_SIZE;
393 put_unaligned_be32(len, &buf->data[auth->session]);
394 } else {
395 tpm_buf_append_u32(buf, 9 + 2 * SHA256_DIGEST_SIZE);
396 }
397
398 /* random number for our nonce */
399 get_random_bytes(nonce, sizeof(nonce));
400 memcpy(auth->our_nonce, nonce, sizeof(nonce));
401 tpm_buf_append_u32(buf, auth->handle);
402 /* our new nonce */
403 tpm_buf_append_u16(buf, SHA256_DIGEST_SIZE);
404 tpm_buf_append(buf, nonce, SHA256_DIGEST_SIZE);
405 tpm_buf_append_u8(buf, auth->attrs);
406 /* and put a placeholder for the hmac */
407 tpm_buf_append_u16(buf, SHA256_DIGEST_SIZE);
408 tpm_buf_append(buf, nonce, SHA256_DIGEST_SIZE);
409 }
410 EXPORT_SYMBOL(tpm_buf_append_hmac_session);
411
Signature · 2 lines
0-DAY CI Kernel Test Service https://github.com/intel/lkp-tests
kernel test robot
build botRe: [PATCH 07/12] tpm: Add full HMAC and encrypt/decrypt session handling code
In reply to James BottomleyI love your patch! Yet something to improve:
Hi James,
I love your patch! Yet something to improve:
[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 herbert-cryptodev-2.6/master herbert-crypto-2.6/master linus/master v6.2-rc8 next-20230217] [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/James-Bottomley/crypto-lib-implement-library-version-of-AES-in-CFB-mode/20230217-044329 patch link: https://lore.kernel.org/r/20230216201410.15010-8-James.Bottomley%40HansenPartnership.com patch subject: [PATCH 07/12] tpm: Add full HMAC and encrypt/decrypt session handling code config: parisc64-defconfig (https://download.01.org/0day-ci/archive/20230217/[email protected]/config) compiler: hppa64-linux-gcc (GCC) 12.1.0
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross chmod +x ~/bin/make.cross
# https://github.com/intel-lab-lkp/linux/commit/1d4f519aa756ba5ed684fb2a2f9ad6e1fb91bdb5
git remote add linux-review https://github.com/intel-lab-lkp/linux
git fetch --no-tags linux-review James-Bottomley/crypto-lib-implement-library-version-of-AES-in-CFB-mode/20230217-044329 git checkout 1d4f519aa756ba5ed684fb2a2f9ad6e1fb91bdb5
# save the config file
mkdir build_dir && cp config build_dir/.config
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross W=1 O=build_dir ARCH=parisc64 olddefconfig COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross W=1 O=build_dir ARCH=parisc64 SHELL=/bin/bash
If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <[email protected]>
| Link: https://lore.kernel.org/oe-kbuild-all/[email protected]/
All errors (new ones prefixed by >>, old ones prefixed by <<):
>> ERROR: modpost: "aescfb_encrypt" [drivers/char/tpm/tpm.ko] undefined! >> ERROR: modpost: "aescfb_decrypt" [drivers/char/tpm/tpm.ko] undefined!
Signature · 2 lines
0-DAY CI Kernel Test Service https://github.com/intel/lkp-tests
James Bottomley
maintainerRe: [PATCH 07/12] tpm: Add full HMAC and encrypt/decrypt session handling code
On Fri, 2023-02-17 at 19:30 +0800, kernel test robot wrote:
[...]
Quoted 4 lines
Quoted 4 lines
Quoted 4 lines
ERROR: modpost: "
aescfb_encrypt" [drivers/char/tpm/tpm.ko] undefined!
ERROR: modpost: "aescfb_decrypt" [drivers/char/tpm/tpm.ko] undefined!
Ard,
This happens because your aescfb_encrypt/decrypt() routines are missing EXPORT_SYMBOL statements.
Regards,
James
Ard Biesheuvel
maintainerRe: [PATCH 07/12] tpm: Add full HMAC and encrypt/decrypt session handling code
Quoted 8 lines · James Bottomley
On Fri, 2023-02-17 at 19:30 +0800, kernel test robot wrote:
[...]Quoted 4 lines
Quoted 4 lines
Quoted 4 lines
ERROR: modpost: "
aescfb_encrypt" [drivers/char/tpm/tpm.ko] undefined!
ERROR: modpost: "aescfb_decrypt" [drivers/char/tpm/tpm.ko] undefined!
Quoted 5 lines
Ard,
This happens because your aescfb_encrypt/decrypt() routines are missing
EXPORT_SYMBOLstatements.
OK, i'll add those.
James Bottomley
maintainerPATCH 8/12 tpm: add hmac checks to tpm2_pcr_extend()
In reply to James BottomleyWe use tpm2_pcr_extend() in trusted keys to extend a PCR to prevent a key from being re-loaded until the next reboot. To use this functionality securely, that extend must be protected by a session hmac.
--- drivers/char/tpm/tpm2-cmd.c | 27 ++++++++++----------------- 1 file changed, 10 insertions(+), 17 deletions(-)
drivers/char/tpm/tpm2-cmd.c +10 -17
@@ -216,13 +216,6 @@ int tpm2_pcr_read(struct tpm_chip *chip, u32 pcr_idx,return rc; } -struct tpm2_null_auth_area { -__be32 handle; -__be16 nonce_size; -u8 attributes; -__be16 auth_size; -} __packed; -/** * tpm2_pcr_extend() - extend a PCR value *
@@ -236,24 +229,22 @@ int tpm2_pcr_extend(struct tpm_chip *chip, u32 pcr_idx, struct tpm_digest *digests) {struct tpm_buf buf; -struct tpm2_null_auth_area auth_area;int rc;int i; - rc =tpm_buf_init (&buf,TPM2_ST_SESSIONS ,TPM2_CC_PCR_EXTEND ); + rc =tpm2_start_auth_session (chip);if (rc)return rc; -tpm_buf_append_u32 (&buf, pcr_idx); + rc =tpm_buf_init (&buf,TPM2_ST_SESSIONS ,TPM2_CC_PCR_EXTEND ); +if (rc) { +tpm2_end_auth_session (chip); +return rc; + } - auth_area.handle =cpu_to_be32 (TPM2_RS_PW ); - auth_area.nonce_size =0 ; - auth_area.attributes =0 ; - auth_area.auth_size =0 ; +tpm_buf_append_name (chip, &buf, pcr_idx,NULL ); +tpm_buf_append_hmac_session (chip, &buf,0 ,NULL ,0 ); -tpm_buf_append_u32 (&buf,sizeof (struct tpm2_null_auth_area )); -tpm_buf_append (&buf, (const unsigned char *)&auth_area, -sizeof (auth_area));tpm_buf_append_u32 (&buf, chip->nr_allocated_banks);for (i =0 ; i < chip->nr_allocated_banks; i++) {
@@ -262,7 +253,9 @@ 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);tpm_buf_destroy (&buf);
Signature · 1 line
2.35.3
Jarkko Sakkinen
maintainerRe: [PATCH 08/12] tpm: add hmac checks to tpm2_pcr_extend()
Quoted 1 line · James Bottomley
We use
tpm2_pcr_extend()in trusted keys to extend a PCR to prevent a
"Use tpm2_pcr_extend()"
Quoted 3 lines
key from being re-loaded until the next reboot. To use this functionality securely, that extend must be protected by a session hmac.
There's no description of action taken. This is only motivation part.
Quoted 70 lines
Signed-off-by James Bottomley <[email protected]>--- drivers/char/tpm/tpm2-cmd.c | 27 ++++++++++----------------- 1 file changed, 10 insertions(+), 17 deletions(-)drivers/char/tpm/tpm2-cmd.c +10
-17@@ -216,13 +216,6 @@ int tpm2_pcr_read(struct tpm_chip *chip, u32 pcr_idx,return rc; } -struct tpm2_null_auth_area { -__be32 handle; -__be16 nonce_size; -u8 attributes; -__be16 auth_size; -} __packed; -/** * tpm2_pcr_extend() - extend a PCR value * @@ -236,24 +229,22 @@ int tpm2_pcr_extend(struct tpm_chip *chip, u32 pcr_idx, struct tpm_digest *digests) {struct tpm_buf buf; -struct tpm2_null_auth_area auth_area;int rc;int i; - rc =tpm_buf_init (&buf,TPM2_ST_SESSIONS ,TPM2_CC_PCR_EXTEND ); + rc =tpm2_start_auth_session (chip);if (rc)return rc; -tpm_buf_append_u32 (&buf, pcr_idx); + rc =tpm_buf_init (&buf,TPM2_ST_SESSIONS ,TPM2_CC_PCR_EXTEND ); +if (rc) { +tpm2_end_auth_session (chip); +return rc; + } - auth_area.handle =cpu_to_be32 (TPM2_RS_PW ); - auth_area.nonce_size =0 ; - auth_area.attributes =0 ; - auth_area.auth_size =0 ; +tpm_buf_append_name (chip, &buf, pcr_idx,NULL ); +tpm_buf_append_hmac_session (chip, &buf,0 ,NULL ,0 ); -tpm_buf_append_u32 (&buf,sizeof (struct tpm2_null_auth_area )); -tpm_buf_append (&buf, (const unsigned char *)&auth_area, -sizeof (auth_area));tpm_buf_append_u32 (&buf, chip->nr_allocated_banks);for (i =0 ; i < chip->nr_allocated_banks; i++) {@@ -262,7 +253,9 @@ 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);tpm_buf_destroy (&buf);Signature · 1 line
2.35.3
BR, Jarkko
James Bottomley
maintainerPATCH 9/12 tpm: add session encryption protection to tpm2_get_random()
In reply to James BottomleyIf some entity is snooping the TPM bus, they can see the random numbers we're extracting from the TPM and do prediction attacks against their consumers. Foil this attack by using response encryption to prevent the attacker from seeing the random sequence.
--- drivers/char/tpm/tpm2-cmd.c | 20 ++++++++++++++++---- 1 file changed, 16 insertions(+), 4 deletions(-)
drivers/char/tpm/tpm2-cmd.c +16 -4
@@ -292,25 +292,35 @@ int tpm2_get_random(struct tpm_chip *chip, u8 *dest, size_t max)if (!num_bytes || max >TPM_MAX_RNG_DATA )return -EINVAL ; - err =tpm_buf_init (&buf,0 ,0 ); + err =tpm2_start_auth_session (chip);if (err)return err; + err =tpm_buf_init (&buf,0 ,0 ); +if (err) { +tpm2_end_auth_session (chip); +return err; + } + do { - tpm_buf_reset(&buf, TPM2_ST_NO_SESSIONS, TPM2_CC_GET_RANDOM); + tpm_buf_reset(&buf, TPM2_ST_SESSIONS, TPM2_CC_GET_RANDOM); + tpm_buf_append_hmac_session_opt(chip, &buf, TPM2_SA_ENCRYPT + | TPM2_SA_CONTINUE_SESSION, + NULL, 0); tpm_buf_append_u16(&buf, num_bytes); + tpm_buf_fill_hmac_session(chip, &buf); err = tpm_transmit_cmd(chip, &buf, offsetof(struct tpm2_get_random_out, buffer), "attempting get random"); + err = tpm_buf_check_hmac_response(chip, &buf, err); if (err) { if (err > 0) err = -EIO; goto out; } - out = (struct tpm2_get_random_out *) - &buf.data[TPM_HEADER_SIZE]; + out = (struct tpm2_get_random_out *)tpm_buf_parameters(&buf); recd = min_t(u32, be16_to_cpu(out->size), num_bytes); if (tpm_buf_length(&buf) < TPM_HEADER_SIZE +
@@ -327,6 +337,8 @@ int tpm2_get_random(struct tpm_chip *chip, u8 *dest, size_t max) }while (retries-- && total < max);tpm_buf_destroy (&buf); +tpm2_end_auth_session (chip); +return total ? total : -EIO ; out:tpm_buf_destroy (&buf);
Signature · 1 line
2.35.3
James Bottomley
maintainerPATCH 10/12 KEYS: trusted: Add session encryption protection to the seal/unseal path
In reply to James BottomleyIf some entity is snooping the TPM bus, the can see the data going in to be sealed and the data coming out as it is unsealed. Add parameter and response encryption to these cases to ensure that no secrets are leaked even if the bus is snooped.
As part of doing this conversion it was discovered that policy sessions can't work with HMAC protected authority because of missing pieces (the tpm Nonce). I've added code to work the same way as before, which will result in potential authority exposure (while still adding security for the command and the returned blob), and a fixme to redo the API to get rid of this security hole.
---
v2: fix unseal with policy and password
v3: fix session memory leak
--- security/keys/trusted-keys/trusted_tpm2.c | 82 ++++++++++++++++------- 1 file changed, 58 insertions(+), 24 deletions(-)
security/keys/trusted-keys/trusted_tpm2.c +58 -24
@@ -252,18 +252,19 @@ int tpm2_seal_trusted(struct tpm_chip *chip,if (rc)return rc; + rc =tpm2_start_auth_session (chip); +if (rc) + goto out_put; + rc =tpm_buf_init (&buf,TPM2_ST_SESSIONS ,TPM2_CC_CREATE );if (rc) { -tpm_put_ops (chip); -return rc; +tpm2_end_auth_session (chip); + goto out_put; } -tpm_buf_append_u32 (&buf, options->keyhandle); -tpm2_buf_append_auth (&buf,TPM2_RS_PW , -NULL /* nonce */ ,0 , -0 /* session_attributes */ , - options->keyauth/* hmac */ , -TPM_DIGEST_SIZE ); +tpm_buf_append_name (chip, &buf, options->keyhandle,NULL ); +tpm_buf_append_hmac_session (chip, &buf,TPM2_SA_DECRYPT , + options->keyauth,TPM_DIGEST_SIZE );/* sensitive */ tpm_buf_append_u16 (&buf,4 + options->blobauth_len + payload->key_len);
@@ -305,10 +306,13 @@ int tpm2_seal_trusted(struct tpm_chip *chip,if (buf.flags &TPM_BUF_OVERFLOW ) { rc = -E2BIG ; +tpm2_end_auth_session (chip); goto out; } +tpm_buf_fill_hmac_session (chip, &buf); rc =tpm_transmit_cmd (chip, &buf,4 ,"sealing data" ); + rc =tpm_buf_check_hmac_response (chip, &buf, rc);if (rc) goto out;
@@ -340,6 +344,7 @@ int tpm2_seal_trusted(struct tpm_chip *chip,else payload->blob_len = blob_len; +out_put:tpm_put_ops (chip);return rc; }
@@ -409,25 +414,31 @@ static int tpm2_load_cmd(struct tpm_chip *chip,if (blob_len > payload->blob_len)return -E2BIG ; - rc =tpm_buf_init (&buf,TPM2_ST_SESSIONS ,TPM2_CC_LOAD ); + rc =tpm2_start_auth_session (chip);if (rc)return rc; -tpm_buf_append_u32 (&buf, options->keyhandle); -tpm2_buf_append_auth (&buf,TPM2_RS_PW , -NULL /* nonce */ ,0 , -0 /* session_attributes */ , - options->keyauth/* hmac */ , -TPM_DIGEST_SIZE ); + rc =tpm_buf_init (&buf,TPM2_ST_SESSIONS ,TPM2_CC_LOAD ); +if (rc) { +tpm2_end_auth_session (chip); +return rc; + } + +tpm_buf_append_name (chip, &buf, options->keyhandle,NULL ); +tpm_buf_append_hmac_session (chip, &buf,0 , options->keyauth, +TPM_DIGEST_SIZE );tpm_buf_append (&buf, blob, blob_len);if (buf.flags &TPM_BUF_OVERFLOW ) { rc = -E2BIG ; +tpm2_end_auth_session (chip); goto out; } +tpm_buf_fill_hmac_session (chip, &buf); rc =tpm_transmit_cmd (chip, &buf,4 ,"loading blob" ); + rc =tpm_buf_check_hmac_response (chip, &buf, rc);if (!rc) *blob_handle =be32_to_cpup ( (__be32 *) &buf.data[TPM_HEADER_SIZE ]);
@@ -465,20 +476,43 @@ static int tpm2_unseal_cmd(struct tpm_chip *chip,u8 *data;int rc; - rc =tpm_buf_init (&buf,TPM2_ST_SESSIONS ,TPM2_CC_UNSEAL ); + rc =tpm2_start_auth_session (chip);if (rc)return rc; -tpm_buf_append_u32 (&buf, blob_handle); -tpm2_buf_append_auth (&buf, - options->policyhandle ? - options->policyhandle :TPM2_RS_PW , -NULL /* nonce */ ,0 , -TPM2_SA_CONTINUE_SESSION , - options->blobauth/* hmac */ , - options->blobauth_len); + rc =tpm_buf_init (&buf,TPM2_ST_SESSIONS ,TPM2_CC_UNSEAL ); +if (rc) { +tpm2_end_auth_session (chip); +return rc; + } + +tpm_buf_append_name (chip, &buf, blob_handle,NULL ); + +if (!options->policyhandle) { +tpm_buf_append_hmac_session (chip, &buf,TPM2_SA_ENCRYPT , + options->blobauth,TPM_DIGEST_SIZE ); + }else { +/* +* FIXME: The policy session was generated outside the +* kernel so we don't known the nonce and thus can't +* calculate a HMAC on it. Therefore, the user can +* only really use TPM2_PolicyPassword and we must +* send down the plain text password, which could be +* intercepted. We can still encrypt the returned +* key, but that's small comfort since the interposer +* could repeat our actions with the exfiltrated +* password. +*/ +tpm2_buf_append_auth (&buf, options->policyhandle, +NULL /* nonce */ ,0 ,0 , + options->blobauth, options->blobauth_len); +tpm_buf_append_hmac_session_opt (chip, &buf,TPM2_SA_ENCRYPT , +NULL ,0 ); + } +tpm_buf_fill_hmac_session (chip, &buf); rc =tpm_transmit_cmd (chip, &buf,6 ,"unsealing" ); + rc =tpm_buf_check_hmac_response (chip, &buf, rc);if (rc >0 ) rc = -EPERM ;
Signature · 1 line
2.35.3
James Bottomley
maintainerPATCH 11/12 tpm: add the null key name as a sysfs export
In reply to James BottomleyThis is the last component of encrypted tpm2 session handling that allows us to verify from userspace that the key derived from the NULL seed genuinely belongs to the TPM and has not been spoofed.
The procedure for doing this involves creating an attestation identity key (which requires verification of the TPM EK certificate) and then using that AIK to sign a certification of the Elliptic Curve key over the NULL seed. Userspace must create this EC Key using the parameters prescribed in TCG TPM v2.0 Provisioning Guidance for the SRK ECC; if this is done correctly the names will match and the TPM can then run a TPM2_Certify operation on this derived primary key using the newly created AIK.
--- drivers/char/tpm/tpm-sysfs.c | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+)
drivers/char/tpm/tpm-sysfs.c +18 -0
@@ -309,6 +309,21 @@ static ssize_t tpm_version_major_show(struct device *dev, }static DEVICE_ATTR_RO (tpm_version_major); +#ifdef CONFIG_TPM_BUS_SECURITY +static ssize_t null_name_show (struct device *dev,struct device_attribute *attr, +char *buf) +{ +struct tpm_chip *chip =to_tpm_chip (dev); +int size =TPM2_NAME_SIZE ; + +bin2hex (buf, chip->tpmkeyname, size); + size *=2 ; + buf[size++] ='\n' ; +return size; +} +static DEVICE_ATTR_RO (null_name); +#endif +static struct attribute *tpm1_dev_attrs[] = { &dev_attr_pubek.attr, &dev_attr_pcrs.attr,
@@ -326,6 +341,9 @@ static struct attribute *tpm1_dev_attrs[] = {static struct attribute *tpm2_dev_attrs[] = { &dev_attr_tpm_version_major.attr, +#if CONFIG_TPM_BUS_SECURITY + &dev_attr_null_name.attr, +#endifNULL };
Signature · 1 line
2.35.3
kernel test robot
build botRe: [PATCH 11/12] tpm: add the null key name as a sysfs export
I love your patch! Perhaps something to improve:
Hi James,
I love your patch! Perhaps something to improve:
[auto build test WARNING on char-misc/char-misc-testing]
[also build test WARNING on char-misc/char-misc-next char-misc/char-misc-linus herbert-cryptodev-2.6/master linus/master v6.2-rc8 next-20230217] [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/James-Bottomley/crypto-lib-implement-library-version-of-AES-in-CFB-mode/20230217-044329 patch link: https://lore.kernel.org/r/93740b6b42e67caa2a1854520b7e68f0966e5024.camel%40HansenPartnership.com patch subject: [PATCH 11/12] tpm: add the null key name as a sysfs export config: nios2-randconfig-m041-20230217 (https://download.01.org/0day-ci/archive/20230217/[email protected]/config) compiler: nios2-linux-gcc (GCC) 12.1.0
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross chmod +x ~/bin/make.cross
# https://github.com/intel-lab-lkp/linux/commit/4c35ecc57dfd3fd82341cd20dfc30c5f665ba952
git remote add linux-review https://github.com/intel-lab-lkp/linux
git fetch --no-tags linux-review James-Bottomley/crypto-lib-implement-library-version-of-AES-in-CFB-mode/20230217-044329 git checkout 4c35ecc57dfd3fd82341cd20dfc30c5f665ba952
# save the config file
mkdir build_dir && cp config build_dir/.config
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross W=1 O=build_dir ARCH=nios2 olddefconfig COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross W=1 O=build_dir ARCH=nios2 SHELL=/bin/bash drivers/char/tpm/
If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <[email protected]>
| Link: https://lore.kernel.org/oe-kbuild-all/[email protected]/
All warnings (new ones prefixed by >>):
>> drivers/char/tpm/tpm-sysfs.c:344:5: warning: "CONFIG_TPM_BUS_SECURITY" is not defined, evaluates to 0 [-Wundef]
344 | #if CONFIG_TPM_BUS_SECURITY
| ^~~~~~~~~~~~~~~~~~~~~~~
vim +/CONFIG_TPM_BUS_SECURITY +344 drivers/char/tpm/tpm-sysfs.c
341
342 static struct attribute *tpm2_dev_attrs[] = {
343 &dev_attr_tpm_version_major.attr,
> 344 #if CONFIG_TPM_BUS_SECURITY
345 &dev_attr_null_name.attr,
346 #endif
347 NULL
348 };
349
Signature · 2 lines
0-DAY CI Kernel Test Service https://github.com/intel/lkp-tests
James Bottomley
maintainerPATCH 12/12 Documentation: add tpm-security.rst
In reply to James BottomleyDocument how the new encrypted secure interface for TPM2 works and how security can be assured after boot by certifying the NULL seed.
--- Documentation/security/tpm/tpm-security.rst | 216 ++++++++++++++++++++ 1 file changed, 216 insertions(+) create mode 100644 Documentation/security/tpm/tpm-security.rst
Documentation/security/tpm/tpm-security.rst new file +216 -0
@@ -0,0 +1,216 @@ +.. SPDX-License-Identifier: GPL-2.0-only + +TPM Security +============ + +The object of this document is to describe how we make the kernel's +use of the TPM reasonably robust in the face of external snooping and +packet alteration attacks (called passive and active interposer attack +in the literature). The current security document is for TPM 2.0. + +Introduction +------------ + +The TPM is usually a discrete chip attached to a PC via some type of +low bandwidth bus. There are exceptions to this such as the Intel +PTT, which is a software TPM running inside a software environment +close to the CPU, which are subject to different attacks, but right at +the moment, most hardened security environments require a discrete +hardware TPM, which is the use case discussed here. + +Snooping and Alteration Attacks against the bus +----------------------------------------------- + +The current state of the art for snooping the `TPM Genie`_ hardware +interposer which is a simple external device that can be installed in +a couple of seconds on any system or laptop. Recently attacks were +successfully demonstrated against the `Windows Bitlocker TPM`_ system. +Most recently the same `attack against TPM based Linux disk +encryption`_ schemes. The next phase of research seems to be hacking +existing devices on the bus to act as interposers, so the fact that +the attacker requires physical access for a few seconds might +evaporate. However, the goal of this document is to protect TPM +secrets and integrity as far as we are able in this environment and to +try to insure that if we can't prevent the attack then at least we can +detect it. + +Unfortunately, most of the TPM functionality, including the hardware +reset capability can be controlled by an attacker who has access to +the bus, so we'll discuss some of the disruption possibilities below. + +Measurement (PCR) Integrity +--------------------------- + +Since the attacker can send their own commands to the TPM, they can +send arbitrary PCR extends and thus disrupt the measurement system, +which would be an annoying denial of service attack. However, there +are two, more serious, classes of attack aimed at entities sealed to +trust measurements. + +1. The attacker could intercept all PCR extends coming from the system + and completely substitute their own values, producing a replay of + an untampered state that would cause PCR measurements to attest to + a trusted state and release secrets + +2. At some point in time the attacker could reset the TPM, clearing + the PCRs and then send down their own measurements which would + effectively overwrite the boot time measurements the TPM has + already done. + +The first can be thwarted by always doing HMAC protection of the PCR +extend and read command meaning measurement values cannot be +substituted without producing a detectable HMAC failure in the +response. However, the second can only really be detected by relying +on some sort of mechanism for protection which would change over TPM +reset. + +Secrets Guarding +---------------- + +Certain information passing in and out of the TPM, such as key sealing +and private key import and random number generation, is vulnerable to +interception which HMAC protection alone cannot protect against, so +for these types of command we must also employ request and response +encryption to prevent the loss of secret information. + +Establishing Initial Trust with the TPM +--------------------------------------- + +In order to provide security from the beginning, an initial shared or +asymmetric secret must be established which must also be unknown to +the attacker. The most obvious avenues for this are the endorsement +and storage seeds, which can be used to derive asymmetric keys. +However, using these keys is difficult because the only way to pass +them into the kernel would be on the command line, which requires +extensive support in the boot system, and there's no guarantee that +either hierarchy would not have some type of authorization. + +The mechanism chosen for the Linux Kernel is to derive the primary +elliptic curve key from the null seed using the standard storage seed +parameters. The null seed has two advantages: firstly the hierarchy +physically cannot have an authorization, so we are always able to use +it and secondly, the null seed changes across TPM resets, meaning if +we establish trust on the null seed at start of day, all sessions +salted with the derived key will fail if the TPM is reset and the seed +changes. + +Obviously using the null seed without any other prior shared secrets, +we have to create and read the initial public key which could, of +course, be intercepted and substituted by the bus interposer. +However, the TPM has a key certification mechanism (using the EK +endorsement certificate, creating an attestation identity key and +certifying the null seed primary with that key) which is too complex +to run within the kernel, so we keep a copy of the null primary key +name, which is what is exported via sysfs so user-space can run the +full certification when it boots. The definitive guarantee here is +that if the null primary key certifies correctly, you know all your +TPM transactions since start of day were secure and if it doesn't, you +know there's an interposer on your system (and that any secret used +during boot may have been leaked). + +Stacking Trust +-------------- + +In the current null primary scenario, the TPM must be completely +cleared before handing it on to the next consumer. However the kernel +hands to user-space the name of the derived null seed key which can +then be verified by certification in user-space. Therefore, this chain +of name handoff can be used between the various boot components as +well (via an unspecified mechanism). For instance, grub could use the +null seed scheme for security and hand the name off to the kernel in +the boot area. The kernel could make its own derivation of the key +and the name and know definitively that if they differ from the handed +off version that tampering has occurred. Thus it becomes possible to +chain arbitrary boot components together (UEFI to grub to kernel) via +the name handoff provided each successive component knows how to +collect the name and verifies it against its derived key. + +Session Properties +------------------ + +All TPM commands the kernel uses allow sessions. HMAC sessions may be +used to check the integrity of requests and responses and decrypt and +encrypt flags may be used to shield parameters and responses. The +HMAC and encryption keys are usually derived from the shared +authorization secret, but for a lot of kernel operations that is well +known (and usually empty). Thus, every HMAC session used by the +kernel must be created using the null primary key as the salt key +which thus provides a cryptographic input into the session key +derivation. Thus, the kernel creates the null primary key once (as a +volatile TPM handle) and keeps it around in a saved context stored in +tpm_chip for every in-kernel use of the TPM. Currently, because of a +lack of de-gapping in the in-kernel resource manager, the session must +be created and destroyed for each operation, but, in future, a single +session may also be reused for the in-kernel HMAC, encryption and +decryption sessions. + +Protection Types +---------------- + +For every in-kernel operation we use null primary salted HMAC to +protect the integrity. Additionally, we use parameter encryption to +protect key sealing and parameter decryption to protect key unsealing +and random number generation. + +Null Primary Key Certification in Userspace +=========================================== + +Every TPM comes shipped with a couple of X.509 certificates for the +primary endorsement key. This document assumes that the Elliptic +Curve version of the certificate exists at 01C00002, but will work +equally well with the RSA certificate (at 01C00001). + +The first step in the certification is primary creation using the +template from the `TCG EK Credential Profile`_ which allows comparison +of the generated primary key against the one in the certificate (the +public key must match). Note that generation of the EK primary +requires the EK hierarchy password, but a pre-generated version of the +EC primary should exist at 81010002 and a TPM2_ReadPublic() may be +performed on this without needing the key authority. Next, the +certificate itself must be verified to chain back to the manufacturer +root (which should be published on the manufacturer website). Once +this is done, an attestation key (AK) is generated within the TPM and +it's name and the EK public key can be used to encrypt a secret using +TPM2_MakeCredential. The TPM then runs TPM2_ActivateCredential which +will only recover the secret if the binding between the TPM, the EK +and the AK is true. the generated AK may now be used to run a +certification of the null primary key whose name the kernel has +exported. Since TPM2_MakeCredential/ActivateCredential are somewhat +complicated, a more simplified process involving an externally +generated private key is described below. + +This process is a simplified abbreviation of the usual privacy CA +based attestation process. The assumption here is that the +attestation is done by the TPM owner who thus has access to only the +owner hierarchy. The owner creates an external public/private key +pair (assume elliptic curve in this case) and wraps the private key +for import using an inner wrapping process and parented to the EC +derived storage primary. The TPM2_Import() is done using a parameter +decryption HMAC session salted to the EK primary (which also does not +require the EK key authority) meaning that the inner wrapping key is +the encrypted parameter and thus the TPM will not be able to perform +the import unless is possesses the certified EK so if the command +succeeds and the HMAC verifies on return we know we have a loadable +copy of the private key only for the certified TPM. This key is now +loaded into the TPM and the Storage primary flushed (to free up space +for the null key generation). + +The null EC primary is now generated using the Storage profile +outlined in the `TCG TPM v2.0 Provisioning Guidance`_; the name of +this key (the hash of the public area) is computed and compared to the +null seed name presented by the kernel in +/sys/class/tpm/tpm0/null_name. If the names do not match, the TPM is +compromised. If the names match, the user performs a TPM2_Certify() +using the null primary as the object handle and the loaded private key +as the sign handle and providing randomized qualifying data. The +signature of the returned certifyInfo is verified against the public +part of the loaded private key and the qualifying data checked to +prevent replay. If all of these tests pass, the user is now assured +that TPM integrity and privacy was preserved across the entire boot +sequence of this kernel. + +.. _TPM Genie: https://www.nccgroup.trust/globalassets/about-us/us/documents/tpm-genie.pdf +.. _Windows Bitlocker TPM: https://dolosgroup.io/blog/2021/7/9/from-stolen-laptop-to-inside-the-company-network +.. _attack against TPM based Linux disk encryption: https://www.secura.com/blog/tpm-sniffing-attacks-against-non-bitlocker-targets +.. _TCG EK Credential Profile: https://trustedcomputinggroup.org/resource/tcg-ek-credential-profile-for-tpm-family-2-0/ +.. _TCG TPM v2.0 Provisioning Guidance: https://trustedcomputinggroup.org/resource/tcg-tpm-v2-0-provisioning-guidance/
Signature · 1 line
2.35.3
Jarkko Sakkinen
maintainerQuoted 135 lines · James Bottomley
The interest in securing the TPM against interposers, both active and passive has risen to fever pitch with the demonstration of key recovery against windows bitlocker:
https://dolosgroup.io/blog/2021/7/9/from-stolen-laptop-to-inside-the-company-network
And subsequently the same attack being successful against all the Linux TPM based security solutions:
https://www.secura.com/blog/tpm-sniffing-attacks-against-non-bitlocker-targets
The attacks fall into two categories:
1. Passive Interposers, which sit on the bus and merely observe
2. Active Interposers, which try to manipulate TPM transactions on the bus using man in the middle and packet stealing to create TPM state the interposer owner desires.Our broadest interposer target is the use of
TPM_RS_PWfor password authorization which sends the actual password to the TPM without any obfuscation and effectively hands it to any interposer. The way to fix this is to use real sessions for HMAC capabilities to ensure integrity and to use parameter and response encryption to ensure confidentiality of the data flowing over the TPM bus. HMAC sessions by agreeing a challenge with the TPM and then giving a response which is a HMAC of the password and the challenge, so the application proves knowledge of the password to the TPM without ever transmitting the password itself. Using HMAC sessions when sending commands to the TPM also provides some measure of protection against active interposers, since the interposer can't interfere with or delete a HMAC'd command (because they can't manufacture a response with the correct HMAC).To protect TPM transactions where there isn't a shared secret (i.e. the command is something like a PCR extension which doesn't involve a TPM object with a password) we have to do a bit more work to set up sessions with a passed in encrypted secret (called a salt) to act in place of the shared secret in the HMAC. This secret salt is effectively a random number encrypted to a public key of the TPM. The final piece of the puzzle is using parameter input and response return encryption, so any interposer can't see the data passing from the application to the TPM and vice versa.
The most insidious interposer attack of all is a reset attack: since the interposer has access to the TPM bus, it can assert the TPM reset line any time it wants. When a TPM resets it mostly comes back in the same state except that all the PCRs are reset to their initial values. Controlling the reset line allows the interposer to change the PCR state after the fact by resetting the TPM and then replaying PCR extends to get the PCRs into a valid state to release secrets, so even if an attack event was recorded, the record is erased. This reset attack violates the fundamental princible of non-repudiability of TPM logs. Defeating the reset attack involves tying all TPM operations within the kernel to a property which will change detectably if the TPM is reset. For that reason, we tie all TPM sessions to the null hierarchy we obtain at start of day and whose seed changes on every reset. If an active interposer asserts a TPM reset, the new null primary won't match the kernel's stored one and all TPM operations will start failing because of HMAC mismatches in the sessions. So if the kernel TPM code keeps operating, it guarantees that a reset hasn't occurred.
The final part of the puzzle is that the machine owner must have a fixed idea of the EK of their TPM and should have certified this with the TPM manufacturer. On every boot, the certified EK public key should be used to do a make credential/activate credential attestation key insertion and then the null key certified with the attestation key. We can follow a trust on first use model where an OS installation will extract and verify a public EK and save it to a read only file.
This patch series adds a simple API which can ensure the above properties as a layered addition to the existing TPM handling code. This series now includes protections for PCR extend, getting random numbers from the TPM and data sealing and unsealing. It therefore eliminates all uses of
TPM2_RS_PWin the kernel and adds encryption protection to sensitive data flowing into and out of the TPM. The first four patches add more sophisticated buffer handling to the TPM which is needed to build the more complex encryption and authentication based commands. Patch 6 adds all the generic cryptography primitives and patches 7-9 use them in critical TPM operations where we want to avoid or detect interposers. Patch 10 exports the name of the null key we used for boot/run time verification and patch 11 documents the security guarantees and expectations.This was originally sent over four years ago, with the last iteration being:
https://lore.kernel.org/linux-integrity/[email protected]/
I'm dusting it off now because various forces at Microsoft and Google via the Open Compute Platform are making a lot of noise about interposers and we in the linux kernel look critically lacking in that regard, particularly for TPM trusted keys.
---
v2 fixes the problems smatch reported and adds more explanation about the code motion in the first few patches
v3 rebases the encryption to be against Ard's new library function, the aescfb addition of which appears as patch 1.James
---
Ard Biesheuvel (1):
crypto: lib - implement library version of AES in CFB modeJames Bottomley (11):
tpm: move buffer handling from static inlines to real functions
tpm: add buffer handling for TPM2B types
tpm: add cursor based buffer functions for response parsing
tpm: add buffer function to point to returned parameters
tpm: export the context save and load commands
tpm: Add full HMAC and encrypt/decrypt session handling code
tpm: add hmac checks totpm2_pcr_extend()
tpm: add session encryption protection totpm2_get_random()
KEYS: trusted: Add session encryption protection to the seal/unseal
path
tpm: add the null key name as a sysfs export
Documentation: addtpm-security.rstDocumentation/security/tpm/tpm-security.rst | 216 ++++ drivers/char/tpm/Kconfig | 13 + drivers/char/tpm/Makefile | 2 + drivers/char/tpm/tpm-buf.c | 196 ++++ drivers/char/tpm/tpm-chip.c | 3 + drivers/char/tpm/tpm-sysfs.c | 18 + drivers/char/tpm/tpm.h | 14 + drivers/char/tpm/tpm2-cmd.c | 52 +- drivers/char/tpm/tpm2-sessions.c | 1160 +++++++++++++++++++ drivers/char/tpm/tpm2-space.c | 8 +- include/crypto/aes.h | 5 + include/linux/tpm.h | 257 ++-- lib/crypto/Kconfig | 5 +
Quoted 12 lines
lib/crypto/Makefile | 3 + lib/crypto/aescfb.c | 75 ++ security/keys/trusted-keys/trusted_tpm2.c | 82 +- 16 files changed, 1984 insertions(+), 125 deletions(-) create mode 100644 Documentation/security/tpm/tpm-security.rst create mode 100644 drivers/char/tpm/tpm-buf.c create mode 100644 drivers/char/tpm/tpm2-sessions.c create mode 100644 lib/crypto/aescfb.cSignature · 1 line
2.35.3
Overally looks much better than earlier version! I'll aim to test this next week [*].
[*] The startup I was in went out of business, meaning that I'm ATM independent contributor doing this on my free time until I find a new job. That said, I spend 4 hours a day with kernel every working day so should not affect much as far reviewing and testing goes.
BR, Jarkko