Re: [PATCH 1/2] tpm2-sessions: Add full HMAC and encrypt/decrypt session handling

[Jarkko Sakkinen]

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, 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, data, len);
> > > +}
> > > +
> > > +#define TPM2B_APPEND(type) \
> > > + static inline void tpm2b_append_##type(struct tpm2b *buf,
> > > const type value) { tpm_buf_append_##type(>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) - sizeof(struct
> > > tpm_input_header);
> > > +}
> > > +
> > > +static inline void tpm2b_destroy(struct tpm2b *buf)
> > > +{
> > > + tpm_buf_destroy(>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

[James Bottomley]

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, 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, data, len);
> > +}
> > +
> > +#define TPM2B_APPEND(type) \
> > +   static inline void tpm2b_append_##type(struct tpm2b *buf,
> > const type value) { tpm_buf_append_##type(>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) - sizeof(struct
> > tpm_input_header);
> > +}
> > +
> > +static inline void tpm2b_destroy(struct tpm2b *buf)
> > +{
> > +   tpm_buf_destroy(>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

[Jarkko Sakkinen]

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, 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, data, len);
> > +}
> > +
> > +#define TPM2B_APPEND(type) \
> > +   static inline void tpm2b_append_##type(struct tpm2b *buf, const type 
> > value) { tpm_buf_append_##type(>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) - sizeof(struct tpm_input_header);
> > +}
> > +
> > +static inline void tpm2b_destroy(struct tpm2b *buf)
> > +{
> > +   tpm_buf_destroy(>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

[Jarkko Sakkinen]

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, 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, data, len);
> +}
> +
> +#define TPM2B_APPEND(type) \
> + static inline void tpm2b_append_##type(struct tpm2b *buf, const type 
> value) { tpm_buf_append_##type(>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) - sizeof(struct tpm_input_header);
> +}
> +
> +static inline void tpm2b_destroy(struct tpm2b *buf)
> +{
> + tpm_buf_destroy(>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

[James Bottomley]

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,