Re: [PATCH 1/2] tpm2-sessions: Add full HMAC and encrypt/decrypt session handling
On Mon, Mar 05, 2018 at 06:58:32AM -0800, James Bottomley wrote: > On Mon, 2018-03-05 at 13:35 +0200, Jarkko Sakkinen wrote: > > On Fri, Mar 02, 2018 at 10:06:15PM -0800, James Bottomley wrote: > > > > > > diff --git a/drivers/char/tpm/tpm2b.h b/drivers/char/tpm/tpm2b.h > > > new file mode 100644 > > > index ..c7726f2895aa > > > --- /dev/null > > > +++ b/drivers/char/tpm/tpm2b.h > > > @@ -0,0 +1,82 @@ > > > +/* SPDX-License-Identifier: GPL-2.0 */ > > > +#ifndef _TPM2_TPM2B_H > > > +#define _TPM2_TPM2B_H > > > +/* > > > + * Handing for tpm2b structures to facilitate the building of > > > commands > > > + */ > > > + > > > +#include "tpm.h" > > > + > > > +#include > > > + > > > +struct tpm2b { > > > + struct tpm_buf buf; > > > +}; > > > + > > > +/* opaque structure, holds auth session parameters like the > > > session key */ > > > +struct tpm2_auth; > > > + > > > +static inline int tpm2b_init(struct tpm2b *buf) > > > +{ > > > + return tpm_buf_init(&buf->buf, 0, 0); > > > +} > > > + > > > +static inline void tpm2b_reset(struct tpm2b *buf) > > > +{ > > > + struct tpm_input_header *head; > > > + > > > + head = (struct tpm_input_header *)buf->buf.data; > > > + head->length = cpu_to_be32(sizeof(*head)); > > > +} > > > + > > > +static inline void tpm2b_append(struct tpm2b *buf, const unsigned > > > char *data, > > > + unsigned int len) > > > +{ > > > + tpm_buf_append(&buf->buf, data, len); > > > +} > > > + > > > +#define TPM2B_APPEND(type) \ > > > + static inline void tpm2b_append_##type(struct tpm2b *buf, > > > const type value) { tpm_buf_append_##type(&buf->buf, value); } > > > + > > > +TPM2B_APPEND(u8) > > > +TPM2B_APPEND(u16) > > > +TPM2B_APPEND(u32) > > > + > > > +static inline void *tpm2b_buffer(const struct tpm2b *buf) > > > +{ > > > + return buf->buf.data + sizeof(struct tpm_input_header); > > > +} > > > + > > > +static inline u16 tpm2b_len(struct tpm2b *buf) > > > +{ > > > + return tpm_buf_length(&buf->buf) - sizeof(struct > > > tpm_input_header); > > > +} > > > + > > > +static inline void tpm2b_destroy(struct tpm2b *buf) > > > +{ > > > + tpm_buf_destroy(&buf->buf); > > > +} > > > + > > > +static inline void tpm_buf_append_2b(struct tpm_buf *buf, struct > > > tpm2b *tpm2b) > > > +{ > > > + u16 len = tpm2b_len(tpm2b); > > > + > > > + tpm_buf_append_u16(buf, len); > > > + tpm_buf_append(buf, tpm2b_buffer(tpm2b), len); > > > + /* clear the buf for reuse */ > > > + tpm2b_reset(tpm2b); > > > +} > > > + > > > +/* Macros for unmarshalling known size BE data */ > > > +#define GET_INC(type)\ > > > +static inline u##type get_inc_##type(const u8 **ptr) { \ > > > + u##type val;\ > > > + val = get_unaligned_be##type(*ptr); \ > > > + *ptr += sizeof(val);\ > > > + return val; \ > > > +} > > > + > > > +GET_INC(16) > > > +GET_INC(32) > > > + > > > +#endif > > > -- > > > 2.12.3 > > > > > > > Some meta stuff: > > > > * Add me to TO-field because I should probably review and eventually > > test these, right? > > Eventually; they're an RFC because we need to get the API right first > (I've already got a couple of fixes to it). For me the big picture looks good. You saw my comments about details. Refine those and I think this would already be digestable change. > > * CC to linux-security-module > > There's no change to anything in security module, so why? All security > module people who care about the TPM should be on linux-integrity and > those who don't likely don't want to see the changes. The reason > linux-crypto is on the cc is because I want them to make sure I'm using > their crypto system correctly. See: https://kernsec.org/wiki/index.php/Linux_Kernel_Integrity The big changes that affect the whole security tree in direct or indirect ways should go through that list. This was a wish from James Morris. > > > * Why there is no RFC tag given that these are so quite large > > changes? > > There is an RFC tag on 0/2 Ah, sorry, so it is. > > * Why in hell tpm2b.h? > > Because all sized TPM structures are in 2B form and manipulating them > can be made a lot easier with helper routines. I see it now that I looked the code in more detail. Suggestions to move forward: * Add enum tpm_buf_type { TPM_BUF_COMMAND, TPM_BUF_2B } and use struct tpm_buf for both. * Move tpm_buf_* stuff from tpm.h and tpm2-cmd.c to tpm_buf_*.[ch]. I would even suggest to move current inline functions to tpm_buf.c so that they can be traced. Performance impact is neglible but tracing is an useful asset for testing. For get_inc* I would just roll out two separate functions as the redudancy is quite neglibe. I just want to keep things as simple and trivial as possible. > James /Jarkko
Re: [PATCH 1/2] tpm2-sessions: Add full HMAC and encrypt/decrypt session handling
On Mon, 2018-03-05 at 13:35 +0200, Jarkko Sakkinen wrote: > On Fri, Mar 02, 2018 at 10:06:15PM -0800, James Bottomley wrote: > > > > diff --git a/drivers/char/tpm/tpm2b.h b/drivers/char/tpm/tpm2b.h > > new file mode 100644 > > index ..c7726f2895aa > > --- /dev/null > > +++ b/drivers/char/tpm/tpm2b.h > > @@ -0,0 +1,82 @@ > > +/* SPDX-License-Identifier: GPL-2.0 */ > > +#ifndef _TPM2_TPM2B_H > > +#define _TPM2_TPM2B_H > > +/* > > + * Handing for tpm2b structures to facilitate the building of > > commands > > + */ > > + > > +#include "tpm.h" > > + > > +#include > > + > > +struct tpm2b { > > + struct tpm_buf buf; > > +}; > > + > > +/* opaque structure, holds auth session parameters like the > > session key */ > > +struct tpm2_auth; > > + > > +static inline int tpm2b_init(struct tpm2b *buf) > > +{ > > + return tpm_buf_init(&buf->buf, 0, 0); > > +} > > + > > +static inline void tpm2b_reset(struct tpm2b *buf) > > +{ > > + struct tpm_input_header *head; > > + > > + head = (struct tpm_input_header *)buf->buf.data; > > + head->length = cpu_to_be32(sizeof(*head)); > > +} > > + > > +static inline void tpm2b_append(struct tpm2b *buf, const unsigned > > char *data, > > + unsigned int len) > > +{ > > + tpm_buf_append(&buf->buf, data, len); > > +} > > + > > +#define TPM2B_APPEND(type) \ > > + static inline void tpm2b_append_##type(struct tpm2b *buf, > > const type value) { tpm_buf_append_##type(&buf->buf, value); } > > + > > +TPM2B_APPEND(u8) > > +TPM2B_APPEND(u16) > > +TPM2B_APPEND(u32) > > + > > +static inline void *tpm2b_buffer(const struct tpm2b *buf) > > +{ > > + return buf->buf.data + sizeof(struct tpm_input_header); > > +} > > + > > +static inline u16 tpm2b_len(struct tpm2b *buf) > > +{ > > + return tpm_buf_length(&buf->buf) - sizeof(struct > > tpm_input_header); > > +} > > + > > +static inline void tpm2b_destroy(struct tpm2b *buf) > > +{ > > + tpm_buf_destroy(&buf->buf); > > +} > > + > > +static inline void tpm_buf_append_2b(struct tpm_buf *buf, struct > > tpm2b *tpm2b) > > +{ > > + u16 len = tpm2b_len(tpm2b); > > + > > + tpm_buf_append_u16(buf, len); > > + tpm_buf_append(buf, tpm2b_buffer(tpm2b), len); > > + /* clear the buf for reuse */ > > + tpm2b_reset(tpm2b); > > +} > > + > > +/* Macros for unmarshalling known size BE data */ > > +#define GET_INC(type) \ > > +static inline u##type get_inc_##type(const u8 **ptr) { \ > > + u##type val;\ > > + val = get_unaligned_be##type(*ptr); \ > > + *ptr += sizeof(val);\ > > + return val; \ > > +} > > + > > +GET_INC(16) > > +GET_INC(32) > > + > > +#endif > > -- > > 2.12.3 > > > > Some meta stuff: > > * Add me to TO-field because I should probably review and eventually > test these, right? Eventually; they're an RFC because we need to get the API right first (I've already got a couple of fixes to it). > * CC to linux-security-module There's no change to anything in security module, so why? All security module people who care about the TPM should be on linux-integrity and those who don't likely don't want to see the changes. The reason linux-crypto is on the cc is because I want them to make sure I'm using their crypto system correctly. > * Why there is no RFC tag given that these are so quite large > changes? There is an RFC tag on 0/2 > * Why in hell tpm2b.h? Because all sized TPM structures are in 2B form and manipulating them can be made a lot easier with helper routines. James
Re: [PATCH 1/2] tpm2-sessions: Add full HMAC and encrypt/decrypt session handling
On Mon, Mar 05, 2018 at 01:35:33PM +0200, Jarkko Sakkinen wrote: > On Fri, Mar 02, 2018 at 10:06:15PM -0800, James Bottomley wrote: > > diff --git a/drivers/char/tpm/tpm2b.h b/drivers/char/tpm/tpm2b.h > > new file mode 100644 > > index ..c7726f2895aa > > --- /dev/null > > +++ b/drivers/char/tpm/tpm2b.h > > @@ -0,0 +1,82 @@ > > +/* SPDX-License-Identifier: GPL-2.0 */ > > +#ifndef _TPM2_TPM2B_H > > +#define _TPM2_TPM2B_H > > +/* > > + * Handing for tpm2b structures to facilitate the building of commands > > + */ > > + > > +#include "tpm.h" > > + > > +#include > > + > > +struct tpm2b { > > + struct tpm_buf buf; > > +}; > > + > > +/* opaque structure, holds auth session parameters like the session key */ > > +struct tpm2_auth; > > + > > +static inline int tpm2b_init(struct tpm2b *buf) > > +{ > > + return tpm_buf_init(&buf->buf, 0, 0); > > +} > > + > > +static inline void tpm2b_reset(struct tpm2b *buf) > > +{ > > + struct tpm_input_header *head; > > + > > + head = (struct tpm_input_header *)buf->buf.data; > > + head->length = cpu_to_be32(sizeof(*head)); > > +} > > + > > +static inline void tpm2b_append(struct tpm2b *buf, const unsigned char > > *data, > > + unsigned int len) > > +{ > > + tpm_buf_append(&buf->buf, data, len); > > +} > > + > > +#define TPM2B_APPEND(type) \ > > + static inline void tpm2b_append_##type(struct tpm2b *buf, const type > > value) { tpm_buf_append_##type(&buf->buf, value); } > > + > > +TPM2B_APPEND(u8) > > +TPM2B_APPEND(u16) > > +TPM2B_APPEND(u32) > > + > > +static inline void *tpm2b_buffer(const struct tpm2b *buf) > > +{ > > + return buf->buf.data + sizeof(struct tpm_input_header); > > +} > > + > > +static inline u16 tpm2b_len(struct tpm2b *buf) > > +{ > > + return tpm_buf_length(&buf->buf) - sizeof(struct tpm_input_header); > > +} > > + > > +static inline void tpm2b_destroy(struct tpm2b *buf) > > +{ > > + tpm_buf_destroy(&buf->buf); > > +} > > + > > +static inline void tpm_buf_append_2b(struct tpm_buf *buf, struct tpm2b > > *tpm2b) > > +{ > > + u16 len = tpm2b_len(tpm2b); > > + > > + tpm_buf_append_u16(buf, len); > > + tpm_buf_append(buf, tpm2b_buffer(tpm2b), len); > > + /* clear the buf for reuse */ > > + tpm2b_reset(tpm2b); > > +} > > + > > +/* Macros for unmarshalling known size BE data */ > > +#define GET_INC(type) \ > > +static inline u##type get_inc_##type(const u8 **ptr) { \ > > + u##type val;\ > > + val = get_unaligned_be##type(*ptr); \ > > + *ptr += sizeof(val);\ > > + return val; \ > > +} > > + > > +GET_INC(16) > > +GET_INC(32) > > + > > +#endif > > -- > > 2.12.3 > > > > Some meta stuff: > > * Add me to TO-field because I should probably review and eventually > test these, right? > * CC to linux-security-module > * Why there is no RFC tag given that these are so quite large changes? > * Why in hell tpm2b.h? > > /Jarkko Some other large scale level stuff that I saw: * Do not document functions in the file header. * Make the stuff in the header (expect functio descriptions) as documentation block: https://www.kernel.org/doc/Documentation/kernel-doc-nano-HOWTO.txt If you don't want to do this, then just move the documentation to the commit message. I rather do not have the documentation at all in the files than have it in unmaintanable form. * Create structs for complex things that you add inside TPM commands and declare these helpers right before the function and add them with tpm_buf_append(). A good metric for this is when you see your self adding field names as a comment. This should make these patches a factor cleaner. We use this approach in some places such as tpm2_get_pcr_allocation(). * Please, oh please get rid of this tpm2b.h. It is a definitive NAK (OK I said it already above but cannot really over emphasize it). * Short summary should be "tpm: add encrypted and integrity protected sessions" or something along the lines. I guess this is enough for first review round? /Jarkko
Re: [PATCH 1/2] tpm2-sessions: Add full HMAC and encrypt/decrypt session handling
On Fri, Mar 02, 2018 at 10:06:15PM -0800, James Bottomley wrote: > diff --git a/drivers/char/tpm/tpm2b.h b/drivers/char/tpm/tpm2b.h > new file mode 100644 > index ..c7726f2895aa > --- /dev/null > +++ b/drivers/char/tpm/tpm2b.h > @@ -0,0 +1,82 @@ > +/* SPDX-License-Identifier: GPL-2.0 */ > +#ifndef _TPM2_TPM2B_H > +#define _TPM2_TPM2B_H > +/* > + * Handing for tpm2b structures to facilitate the building of commands > + */ > + > +#include "tpm.h" > + > +#include > + > +struct tpm2b { > + struct tpm_buf buf; > +}; > + > +/* opaque structure, holds auth session parameters like the session key */ > +struct tpm2_auth; > + > +static inline int tpm2b_init(struct tpm2b *buf) > +{ > + return tpm_buf_init(&buf->buf, 0, 0); > +} > + > +static inline void tpm2b_reset(struct tpm2b *buf) > +{ > + struct tpm_input_header *head; > + > + head = (struct tpm_input_header *)buf->buf.data; > + head->length = cpu_to_be32(sizeof(*head)); > +} > + > +static inline void tpm2b_append(struct tpm2b *buf, const unsigned char *data, > + unsigned int len) > +{ > + tpm_buf_append(&buf->buf, data, len); > +} > + > +#define TPM2B_APPEND(type) \ > + static inline void tpm2b_append_##type(struct tpm2b *buf, const type > value) { tpm_buf_append_##type(&buf->buf, value); } > + > +TPM2B_APPEND(u8) > +TPM2B_APPEND(u16) > +TPM2B_APPEND(u32) > + > +static inline void *tpm2b_buffer(const struct tpm2b *buf) > +{ > + return buf->buf.data + sizeof(struct tpm_input_header); > +} > + > +static inline u16 tpm2b_len(struct tpm2b *buf) > +{ > + return tpm_buf_length(&buf->buf) - sizeof(struct tpm_input_header); > +} > + > +static inline void tpm2b_destroy(struct tpm2b *buf) > +{ > + tpm_buf_destroy(&buf->buf); > +} > + > +static inline void tpm_buf_append_2b(struct tpm_buf *buf, struct tpm2b > *tpm2b) > +{ > + u16 len = tpm2b_len(tpm2b); > + > + tpm_buf_append_u16(buf, len); > + tpm_buf_append(buf, tpm2b_buffer(tpm2b), len); > + /* clear the buf for reuse */ > + tpm2b_reset(tpm2b); > +} > + > +/* Macros for unmarshalling known size BE data */ > +#define GET_INC(type)\ > +static inline u##type get_inc_##type(const u8 **ptr) { \ > + u##type val;\ > + val = get_unaligned_be##type(*ptr); \ > + *ptr += sizeof(val);\ > + return val; \ > +} > + > +GET_INC(16) > +GET_INC(32) > + > +#endif > -- > 2.12.3 > Some meta stuff: * Add me to TO-field because I should probably review and eventually test these, right? * CC to linux-security-module * Why there is no RFC tag given that these are so quite large changes? * Why in hell tpm2b.h? /Jarkko
[PATCH 1/2] tpm2-sessions: Add full HMAC and encrypt/decrypt session handling
This code adds true session based HMAC authentication plus parameter decryption and response encryption using AES. The basic design of this code 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 now needs a primary key to encrypt the session salt to, so we derive an EC key from the NULL seed and store it in the tpm_chip structure. We also make sure that this seed remains for the kernel by using a kernel space to take it out of the TPM when userspace wants to use it. Signed-off-by: James Bottomley --- drivers/char/tpm/Kconfig | 3 + drivers/char/tpm/Makefile| 2 +- drivers/char/tpm/tpm.h | 22 + drivers/char/tpm/tpm2-cmd.c | 22 +- drivers/char/tpm/tpm2-sessions.c | 907 +++ drivers/char/tpm/tpm2-sessions.h | 55 +++ drivers/char/tpm/tpm2b.h | 82 7 files changed, 1080 insertions(+), 13 deletions(-) create mode 100644 drivers/char/tpm/tpm2-sessions.c create mode 100644 drivers/char/tpm/tpm2-sessions.h create mode 100644 drivers/char/tpm/tpm2b.h diff --git a/drivers/char/tpm/Kconfig b/drivers/char/tpm/Kconfig index 0aee88df98d1..8c714d8550c4 100644 --- a/drivers/char/tpm/Kconfig +++ b/drivers/char/tpm/Kconfig @@ -8,6 +8,9 @@ menuconfig TCG_TPM select SECURITYFS select CRYPTO select CRYPTO_HASH_INFO + select CRYPTO_ECDH + select CRYPTO_AES + select CRYPTO_CFB ---help--- If you have a TPM security chip in your system, which implements the Trusted Computing Group's specification, diff --git a/drivers/char/tpm/Makefile b/drivers/char/tpm/Makefile index d37c4a1748f5..95ef2b10cc8d 100644 --- a/drivers/char/tpm/Makefile +++ b/drivers/char/tpm/Makefile @@ -5,7 +5,7 @@ obj-$(CONFIG_TCG_TPM) += tpm.o tpm-y := tpm-interface.o tpm-dev.o tpm-sysfs.o tpm-chip.o tpm2-cmd.o \ tpm-dev-common.o tpmrm-dev.o tpm1_eventlog.o tpm2_eventlog.o \ - tpm2-space.o + tpm2-space.o tpm2-sessions.o tpm-$(CONFIG_ACPI) += tpm_ppi.o tpm_eventlog_acpi.o tpm-$(CONFIG_EFI) += tpm_eventlog_efi.o tpm-$(CONFIG_OF) += tpm_eventlog_of.o diff --git a/drivers/char/tpm/tpm.h b/drivers/char/tpm/tpm.h index 3e083a30a108..95a0d5288d6a 100644 --- a/drivers/char/tpm/tpm.h +++ b/drivers/char/tpm/tpm.h @@ -42,6 +42,9 @@ #include #endif +/* fixed define for the curve we use which is NIST_P256 */ +#define EC_PT_SZ 32 + enum tpm_const { TPM_MINOR = 224,/* officially assigned */ TPM_BUFSIZE = 4096, @@ -114,16 +117,25 @@ enum tpm2_return_codes { enum tpm2_algorithms { TPM2_ALG_ERROR = 0x, TPM2_ALG_SHA1 = 0x0004, + TPM2_ALG_AES= 0x0006, TPM2_ALG_KEYEDHASH = 0x0008, TPM2_ALG_SHA256 = 0x000B, TPM2_ALG_SHA384 = 0x000C, TPM2_ALG_SHA512 = 0x000D, TPM2_ALG_NULL = 0x0010, TPM2_ALG_SM3_256= 0x0012, + TPM2_ALG_ECC= 0x0023, + TPM2_ALG_CFB= 0x0043, +}; + +enum tpm2_curves { + TPM2_ECC_NONE = 0x, + TPM2_ECC_NIST_P256 = 0x0003, }; enum tpm2_command_codes { TPM2_CC_FIRST = 0x011F, + TPM2_CC_CREATE_PRIMARY = 0x0131, TPM2_CC_SELF_TEST = 0x0143, TPM2_CC_STARTUP = 0x0144, TPM2_CC_SHUTDOWN= 0x0145, @@ -133,6 +145,7 @@ enum tpm2_command_codes { TPM2_CC_CONTEXT_LOAD= 0x0161, TPM2_CC_CONTEXT_SAVE= 0x0162, TPM2_CC_FLUSH_CONTEXT = 0x016