diff mbox

[tpmdd-devel] tpm: introduce struct tpm_buf

Message ID 1433250262-17200-1-git-send-email-jarkko.sakkinen@linux.intel.com
State Superseded
Headers show

Commit Message

Jarkko Sakkinen June 2, 2015, 1:04 p.m. UTC 
This patch introduces struct tpm_buf that provides a string buffer for
constructing TPM commands. This allows to construct variable sized TPM
commands. This feature is needed for TPM 2.0 commands in order to allow
policy authentication and algorithmic agility.

The commands in the tpm2-cmd.c have been updated to use struct tpm_buf.
Lots of awkward length calculations could be dropped because the buffer
knows its length.

The code is is along the lines of the string buffer code in
security/trusted/trusted.h.

Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
---
 drivers/char/tpm/tpm.h      |  87 ++++++++++++
 drivers/char/tpm/tpm2-cmd.c | 321 ++++++++++++++++----------------------------
 2 files changed, 200 insertions(+), 208 deletions(-)

Comments

Peter Hüwe June 2, 2015, 1:18 p.m. UTC | #1 
Hi,
> Betreff: [PATCH] tpm: introduce struct tpm_buf
> This patch introduces struct tpm_buf that provides a string buffer for
> constructing TPM commands. This allows to construct variable sized TPM
> commands. This feature is needed for TPM 2.0 commands in order to allow
> policy authentication and algorithmic agility.
> 
> The commands in the tpm2-cmd.c have been updated to use struct tpm_buf.
> Lots of awkward length calculations could be dropped because the buffer
> knows its length.
> 
> The code is is along the lines of the string buffer code in
> security/trusted/trusted.h.
> 

> --- a/drivers/char/tpm/tpm.h
> +++ b/drivers/char/tpm/tpm.h
> @@ -382,6 +382,93 @@ struct tpm_cmd_t {
> tpm_cmd_params params;
> } __packed;
> 
> +/* A string buffer type for constructing TPM commands. This is based on the
> + * code in security/keys/trusted.h.
> + */
> +
> +#define TPM_BUF_SIZE 512
Where does 512 come from? What about longer commands? Isn't TPM_BUF_SIZE defined elsewhere as 4096?
> 
> +
> +struct tpm_buf {
> + u8 data[TPM_BUF_SIZE];
> +};
> +

> +static inline void tpm_buf_append(struct tpm_buf *buf,
> + const unsigned char *data,
> + unsigned int len)
> +{
> + struct tpm_input_header *head = (struct tpm_input_header *) buf->data;
> +
> + BUG_ON((len + tpm_buf_length(buf)) > TPM_BUF_SIZE);
> +
> + memcpy(&buf->data[tpm_buf_length(buf)], data, len);
> + head->length = cpu_to_be32(tpm_buf_length(buf) + len);
> +}
> +
> +static inline void tpm_buf_store(struct tpm_buf *buf,
> + unsigned int pos,
> + const unsigned char *data,
> + unsigned int len)
> +{
> + BUG_ON((pos + len) > TPM_BUF_SIZE);
> +
> + memcpy(&buf->data[pos], data, len);
Isn't the updating of the length missing?
> +}

Thanks,
Peter


------------------------------------------------------------------------------
Jarkko Sakkinen June 2, 2015, 2:36 p.m. UTC | #2 
On Tue, Jun 02, 2015 at 03:18:54PM +0200, Peter Huewe wrote:
> Hi,
> > Betreff: [PATCH] tpm: introduce struct tpm_buf
> > This patch introduces struct tpm_buf that provides a string buffer for
> > constructing TPM commands. This allows to construct variable sized TPM
> > commands. This feature is needed for TPM 2.0 commands in order to allow
> > policy authentication and algorithmic agility.
> > 
> > The commands in the tpm2-cmd.c have been updated to use struct tpm_buf.
> > Lots of awkward length calculations could be dropped because the buffer
> > knows its length.
> > 
> > The code is is along the lines of the string buffer code in
> > security/trusted/trusted.h.
> > 
> 
> > --- a/drivers/char/tpm/tpm.h
> > +++ b/drivers/char/tpm/tpm.h
> > @@ -382,6 +382,93 @@ struct tpm_cmd_t {
> > tpm_cmd_params params;
> > } __packed;
> > 
> > +/* A string buffer type for constructing TPM commands. This is based on the
> > + * code in security/keys/trusted.h.
> > + */
> > +
> > +#define TPM_BUF_SIZE 512
> Where does 512 come from? What about longer commands? Isn't TPM_BUF_SIZE defined elsewhere as 4096?

No matter which algorithm we use for trusted keys, 512 bytes is enough
for command and response and trusted keys is the most demanding use case
for TPM at the moment.

I chose this buffer size because I can still get away without using
heap, which is nice for some use cases that should be resistant to
failure (suspend/resume mainly).

This was the buffer size also used in security/keys/trusted.c that
contains TPM 1.x based trusted keys (that I hope will migrate to TPM
driver soon).

However, if there is rationale to use a larger buffer size and heap,
I'm welcome for counter opinions.

> > 
> > +
> > +struct tpm_buf {
> > + u8 data[TPM_BUF_SIZE];
> > +};
> > +
> 
> > +static inline void tpm_buf_append(struct tpm_buf *buf,
> > + const unsigned char *data,
> > + unsigned int len)
> > +{
> > + struct tpm_input_header *head = (struct tpm_input_header *) buf->data;
> > +
> > + BUG_ON((len + tpm_buf_length(buf)) > TPM_BUF_SIZE);
> > +
> > + memcpy(&buf->data[tpm_buf_length(buf)], data, len);
> > + head->length = cpu_to_be32(tpm_buf_length(buf) + len);
> > +}
> > +
> > +static inline void tpm_buf_store(struct tpm_buf *buf,
> > + unsigned int pos,
> > + const unsigned char *data,
> > + unsigned int len)
> > +{
> > + BUG_ON((pos + len) > TPM_BUF_SIZE);
> > +
> > + memcpy(&buf->data[pos], data, len);
> Isn't the updating of the length missing?
> > +}
> 
> Thanks,
> Peter

/Jarkko

------------------------------------------------------------------------------
Jason Gunthorpe June 2, 2015, 6:13 p.m. UTC | #3 
On Tue, Jun 02, 2015 at 04:04:22PM +0300, Jarkko Sakkinen wrote:
> +/* A string buffer type for constructing TPM commands. This is based on the
> + * code in security/keys/trusted.h.
> + */
> +
> +#define TPM_BUF_SIZE 512
> +
> +struct tpm_buf {
> +	u8 data[TPM_BUF_SIZE];

This should be u32 or u64 to guarentee correct alignment for the
casting.


> +};
> +
> +static inline void tpm_buf_init(struct tpm_buf *buf, u16 tag, u32 ordinal)
> +{
> +	struct tpm_input_header *head;
> +
> +	head = (struct tpm_input_header *) buf->data;
> +
> +	head->tag = cpu_to_be16(tag);
> +	head->length = cpu_to_be32(sizeof(*head));
> +	head->ordinal = cpu_to_be32(ordinal);
> +}
> +
> +static inline u32 tpm_buf_length(struct tpm_buf *buf)
> +{
> +	struct tpm_input_header *head = (struct tpm_input_header *) buf->data;
> +
> +	return be32_to_cpu(head->length);
> +}
> +
> +static inline u16 tpm_buf_tag(struct tpm_buf *buf)
> +{
> +	return be16_to_cpu(*(__be16 *) &buf->data[0]);

be16_to_cpup ?

Any thought on someday using this for tpm1 as well?

Jason

------------------------------------------------------------------------------
Jarkko Sakkinen June 3, 2015, 12:23 p.m. UTC | #4 
On Tue, Jun 02, 2015 at 12:13:15PM -0600, Jason Gunthorpe wrote:
> On Tue, Jun 02, 2015 at 04:04:22PM +0300, Jarkko Sakkinen wrote:
> > +/* A string buffer type for constructing TPM commands. This is based on the
> > + * code in security/keys/trusted.h.
> > + */
> > +
> > +#define TPM_BUF_SIZE 512
> > +
> > +struct tpm_buf {
> > +	u8 data[TPM_BUF_SIZE];
> This should be u32 or u64 to guarentee correct alignment for the
> casting.

Good catch. The functions where this might cause trouble are *_length()
and *_tag().

In other places misalignment should not cause any regressions since data
is not directly assigned to the buffer with pointer casting.

I would prefer to fix by changing *_length() and *_tag() to copy the
value to a local variable and return that. It's a fail safe way and here
the performance is not an issue.

> > +};
> > +
> > +static inline void tpm_buf_init(struct tpm_buf *buf, u16 tag, u32 ordinal)
> > +{
> > +	struct tpm_input_header *head;
> > +
> > +	head = (struct tpm_input_header *) buf->data;
> > +
> > +	head->tag = cpu_to_be16(tag);
> > +	head->length = cpu_to_be32(sizeof(*head));
> > +	head->ordinal = cpu_to_be32(ordinal);
> > +}
> > +
> > +static inline u32 tpm_buf_length(struct tpm_buf *buf)
> > +{
> > +	struct tpm_input_header *head = (struct tpm_input_header *) buf->data;
> > +
> > +	return be32_to_cpu(head->length);
> > +}
> > +
> > +static inline u16 tpm_buf_tag(struct tpm_buf *buf)
> > +{
> > +	return be16_to_cpu(*(__be16 *) &buf->data[0]);
> 
> be16_to_cpup ?

Thanks, I'll change this.

> Any thought on someday using this for tpm1 as well?

Yes, I think this could form the baseline so that the sealing code that
currently resides is security/keys/trusted.c could be eventually moved
to drivers/char/tpm.

I will be contributing API to include/linux/tpm.h that can be then
called by trusted keys. I will also contribute the implementation for
TPM 2.0 sealing but it is up to those who implemented TPM 1.x sealing
code to do the migration. I put all the enablers in place so that doing
that transition should be relatively painless.

I have PoC for TPM 2.0 trusted keys written in Python available here:

https://github.com/jsakkine/tpm2-scripts/blob/master/tpm2.py

(In kernel version the default hash algorithm should be probably SHA-256
as SHA-1 is not considered secure by NIST anymore)

Doing also all the TPM 1.x migration on top of this would make
oversubscribed.

> Jason

/Jarkko

------------------------------------------------------------------------------
Jason Gunthorpe June 3, 2015, 4:11 p.m. UTC | #5 
On Wed, Jun 03, 2015 at 03:23:31PM +0300, Jarkko Sakkinen wrote:
> On Tue, Jun 02, 2015 at 12:13:15PM -0600, Jason Gunthorpe wrote:
> > On Tue, Jun 02, 2015 at 04:04:22PM +0300, Jarkko Sakkinen wrote:
> > > +/* A string buffer type for constructing TPM commands. This is based on the
> > > + * code in security/keys/trusted.h.
> > > + */
> > > +
> > > +#define TPM_BUF_SIZE 512
> > > +
> > > +struct tpm_buf {
> > > +	u8 data[TPM_BUF_SIZE];
> > This should be u32 or u64 to guarentee correct alignment for the
> > casting.
> 
> Good catch. The functions where this might cause trouble are *_length()
> and *_tag().
> 
> In other places misalignment should not cause any regressions since data
> is not directly assigned to the buffer with pointer casting.
> 
> I would prefer to fix by changing *_length() and *_tag() to copy the
> value to a local variable and return that. It's a fail safe way and here
> the performance is not an issue.

I would change the type, that is very simple and will improve
performance of the memcpy as well.

It actually isn't a problem for the casts to tpm_input_header - any
structure marked __packed will cause the compiler to assume that the
entire structure is unaligned and code gen accordingly, so the cast
will always work, but on x86 it will be more efficient if the array
is aligned.

Ideally we could change to something like:
 __attribute__((packed,aligned(4))) 

Instead of __packed which will help the compiler minimize unaligned
load instructions..

Jason

------------------------------------------------------------------------------
Jarkko Sakkinen June 3, 2015, 4:43 p.m. UTC | #6 
On Wed, Jun 03, 2015 at 10:11:28AM -0600, Jason Gunthorpe wrote:
> On Wed, Jun 03, 2015 at 03:23:31PM +0300, Jarkko Sakkinen wrote:
> > On Tue, Jun 02, 2015 at 12:13:15PM -0600, Jason Gunthorpe wrote:
> > > On Tue, Jun 02, 2015 at 04:04:22PM +0300, Jarkko Sakkinen wrote:
> > > > +/* A string buffer type for constructing TPM commands. This is based on the
> > > > + * code in security/keys/trusted.h.
> > > > + */
> > > > +
> > > > +#define TPM_BUF_SIZE 512
> > > > +
> > > > +struct tpm_buf {
> > > > +	u8 data[TPM_BUF_SIZE];
> > > This should be u32 or u64 to guarentee correct alignment for the
> > > casting.
> > 
> > Good catch. The functions where this might cause trouble are *_length()
> > and *_tag().
> > 
> > In other places misalignment should not cause any regressions since data
> > is not directly assigned to the buffer with pointer casting.
> > 
> > I would prefer to fix by changing *_length() and *_tag() to copy the
> > value to a local variable and return that. It's a fail safe way and here
> > the performance is not an issue.
> 
> I would change the type, that is very simple and will improve
> performance of the memcpy as well.
> 
> It actually isn't a problem for the casts to tpm_input_header - any
> structure marked __packed will cause the compiler to assume that the
> entire structure is unaligned and code gen accordingly, so the cast
> will always work, but on x86 it will be more efficient if the array
> is aligned.
> 
> Ideally we could change to something like:
>  __attribute__((packed,aligned(4))) 
> 
> Instead of __packed which will help the compiler minimize unaligned
> load instructions..

I realized basically the same what you said here when I refined the
patch (read this email after sending v2) :) I decided that align to 8
bytes.

I don't think the packed attribute is needed here because now I cast
the beginning to tpm_input_header, which is a packed struct.

> Jason

/Jarkko

------------------------------------------------------------------------------
Jason Gunthorpe June 3, 2015, 4:48 p.m. UTC | #7 
On Wed, Jun 03, 2015 at 07:43:34PM +0300, Jarkko Sakkinen wrote:
> I realized basically the same what you said here when I refined the
> patch (read this email after sending v2) :) I decided that align to 8
> bytes.

Don't use the __attribute__((align)), use just a u64, the effect is
identical, and the latter doesn't rely on compiler specific features.

> I don't think the packed attribute is needed here because now I cast
> the beginning to tpm_input_header, which is a packed struct.

I ment the attributs could be changed on the tpm_input_header, the buf
should just be a u64 array..

Jason

------------------------------------------------------------------------------
Jarkko Sakkinen June 3, 2015, 5:06 p.m. UTC | #8 
On Wed, Jun 03, 2015 at 10:48:51AM -0600, Jason Gunthorpe wrote:
> On Wed, Jun 03, 2015 at 07:43:34PM +0300, Jarkko Sakkinen wrote:
> > I realized basically the same what you said here when I refined the
> > patch (read this email after sending v2) :) I decided that align to 8
> > bytes.
> 
> Don't use the __attribute__((align)), use just a u64, the effect is
> identical, and the latter doesn't rely on compiler specific features.

Byte array is otherwise more convenient and this feature is widely used:

$ git grep "__attribute__((aligned" | wc -l
222

I don't see your point here.

> > I don't think the packed attribute is needed here because now I cast
> > the beginning to tpm_input_header, which is a packed struct.
> 
> I ment the attributs could be changed on the tpm_input_header, the buf
> should just be a u64 array..
> 
> Jason

/Jarkko

------------------------------------------------------------------------------
diff mbox

Patch

diff --git a/drivers/char/tpm/tpm.h b/drivers/char/tpm/tpm.h
index f8319a0..f1a1273 100644
--- a/drivers/char/tpm/tpm.h
+++ b/drivers/char/tpm/tpm.h
@@ -382,6 +382,93 @@  struct tpm_cmd_t {
 	tpm_cmd_params	params;
 } __packed;
 
+/* A string buffer type for constructing TPM commands. This is based on the
+ * code in security/keys/trusted.h.
+ */
+
+#define TPM_BUF_SIZE 512
+
+struct tpm_buf {
+	u8 data[TPM_BUF_SIZE];
+};
+
+static inline void tpm_buf_init(struct tpm_buf *buf, u16 tag, u32 ordinal)
+{
+	struct tpm_input_header *head;
+
+	head = (struct tpm_input_header *) buf->data;
+
+	head->tag = cpu_to_be16(tag);
+	head->length = cpu_to_be32(sizeof(*head));
+	head->ordinal = cpu_to_be32(ordinal);
+}
+
+static inline u32 tpm_buf_length(struct tpm_buf *buf)
+{
+	struct tpm_input_header *head = (struct tpm_input_header *) buf->data;
+
+	return be32_to_cpu(head->length);
+}
+
+static inline u16 tpm_buf_tag(struct tpm_buf *buf)
+{
+	return be16_to_cpu(*(__be16 *) &buf->data[0]);
+}
+
+static inline u8 *tpm_buf_out(struct tpm_buf *buf)
+{
+	return &buf->data[TPM_HEADER_SIZE];
+}
+
+static inline void tpm_buf_append(struct tpm_buf *buf,
+				  const unsigned char *data,
+				  unsigned int len)
+{
+	struct tpm_input_header *head = (struct tpm_input_header *) buf->data;
+
+	BUG_ON((len + tpm_buf_length(buf)) > TPM_BUF_SIZE);
+
+	memcpy(&buf->data[tpm_buf_length(buf)], data, len);
+	head->length = cpu_to_be32(tpm_buf_length(buf) + len);
+}
+
+static inline void tpm_buf_store(struct tpm_buf *buf,
+				 unsigned int pos,
+				 const unsigned char *data,
+				 unsigned int len)
+{
+	BUG_ON((pos + len) > TPM_BUF_SIZE);
+
+	memcpy(&buf->data[pos], data, 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);
+}
+
+static inline void tpm_buf_store_u32(struct tpm_buf *buf, unsigned int pos,
+				     const u32 value)
+{
+	__be32 value2 = cpu_to_be32(value);
+
+	tpm_buf_store(buf, pos, (u8 *) &value2, 4);
+}
+
 extern struct class *tpm_class;
 extern dev_t tpm_devt;
 extern const struct file_operations tpm_fops;
diff --git a/drivers/char/tpm/tpm2-cmd.c b/drivers/char/tpm/tpm2-cmd.c
index 011909a..a274cef 100644
--- a/drivers/char/tpm/tpm2-cmd.c
+++ b/drivers/char/tpm/tpm2-cmd.c
@@ -17,14 +17,6 @@ 
 
 #include "tpm.h"
 
-struct tpm2_startup_in {
-	__be16	startup_type;
-} __packed;
-
-struct tpm2_self_test_in {
-	u8	full_test;
-} __packed;
-
 struct tpm2_pcr_read_in {
 	__be32	pcr_selects_cnt;
 	__be16	hash_alg;
@@ -43,17 +35,7 @@  struct tpm2_pcr_read_out {
 	u8	digest[TPM_DIGEST_SIZE];
 } __packed;
 
-struct tpm2_null_auth_area {
-	__be32			handle;
-	__be16			nonce_size;
-	u8			attributes;
-	__be16			auth_size;
-} __packed;
-
 struct tpm2_pcr_extend_in {
-	__be32				pcr_idx;
-	__be32				auth_area_size;
-	struct tpm2_null_auth_area	auth_area;
 	__be32				digest_cnt;
 	__be16				hash_alg;
 	u8				digest[TPM_DIGEST_SIZE];
@@ -73,32 +55,11 @@  struct tpm2_get_tpm_pt_out {
 	__be32	value;
 } __packed;
 
-struct tpm2_get_random_in {
-	__be16	size;
-} __packed;
-
 struct tpm2_get_random_out {
 	__be16	size;
 	u8	buffer[TPM_MAX_RNG_DATA];
 } __packed;
 
-union tpm2_cmd_params {
-	struct	tpm2_startup_in		startup_in;
-	struct	tpm2_self_test_in	selftest_in;
-	struct	tpm2_pcr_read_in	pcrread_in;
-	struct	tpm2_pcr_read_out	pcrread_out;
-	struct	tpm2_pcr_extend_in	pcrextend_in;
-	struct	tpm2_get_tpm_pt_in	get_tpm_pt_in;
-	struct	tpm2_get_tpm_pt_out	get_tpm_pt_out;
-	struct	tpm2_get_random_in	getrandom_in;
-	struct	tpm2_get_random_out	getrandom_out;
-};
-
-struct tpm2_cmd {
-	tpm_cmd_header		header;
-	union tpm2_cmd_params	params;
-} __packed;
-
 /*
  * Array with one entry per ordinal defining the maximum amount
  * of time the chip could take to return the result. The values
@@ -221,15 +182,28 @@  static const u8 tpm2_ordinal_duration[TPM2_CC_LAST - TPM2_CC_FIRST + 1] = {
 	TPM_UNDEFINED		/* 18f */
 };
 
-#define TPM2_PCR_READ_IN_SIZE \
-	(sizeof(struct tpm_input_header) + \
-	 sizeof(struct tpm2_pcr_read_in))
+static void tpm2_buf_append_auth(struct tpm_buf *buf, u32 session_handle,
+				 const u8 *nonce, u16 nonce_len,
+				 u8 session_attributes,
+				 const u8 *hmac, u16 hmac_len)
+{
+	u32 buf_len = tpm_buf_length(buf);
+
+	tpm_buf_append_u32(buf, 0);
+	tpm_buf_append_u32(buf, session_handle);
+	tpm_buf_append_u16(buf, nonce_len);
 
-static const struct tpm_input_header tpm2_pcrread_header = {
-	.tag = cpu_to_be16(TPM2_ST_NO_SESSIONS),
-	.length = cpu_to_be32(TPM2_PCR_READ_IN_SIZE),
-	.ordinal = cpu_to_be32(TPM2_CC_PCR_READ)
-};
+	if (nonce)
+		tpm_buf_append(buf, nonce, nonce_len);
+
+	tpm_buf_append_u8(buf, session_attributes);
+	tpm_buf_append_u16(buf, hmac_len);
+
+	if (hmac)
+		tpm_buf_append(buf, hmac, hmac_len);
+
+	tpm_buf_store_u32(buf, buf_len, tpm_buf_length(buf) - buf_len);
+}
 
 /**
  * tpm2_pcr_read() - read a PCR value
@@ -243,42 +217,36 @@  static const struct tpm_input_header tpm2_pcrread_header = {
  */
 int tpm2_pcr_read(struct tpm_chip *chip, int pcr_idx, u8 *res_buf)
 {
+	struct tpm_buf buf;
+	struct tpm2_pcr_read_in in;
+	struct tpm2_pcr_read_out *out;
 	int rc;
-	struct tpm2_cmd cmd;
-	u8 *buf;
 
 	if (pcr_idx >= TPM2_PLATFORM_PCR)
 		return -EINVAL;
 
-	cmd.header.in = tpm2_pcrread_header;
-	cmd.params.pcrread_in.pcr_selects_cnt = cpu_to_be32(1);
-	cmd.params.pcrread_in.hash_alg = cpu_to_be16(TPM2_ALG_SHA1);
-	cmd.params.pcrread_in.pcr_select_size = TPM2_PCR_SELECT_MIN;
+	tpm_buf_init(&buf, TPM2_ST_NO_SESSIONS, TPM2_CC_PCR_READ);
 
-	memset(cmd.params.pcrread_in.pcr_select, 0,
-	       sizeof(cmd.params.pcrread_in.pcr_select));
-	cmd.params.pcrread_in.pcr_select[pcr_idx >> 3] = 1 << (pcr_idx & 0x7);
+	in.pcr_selects_cnt = cpu_to_be32(1);
+	in.hash_alg = cpu_to_be16(TPM2_ALG_SHA1);
+	in.pcr_select_size = TPM2_PCR_SELECT_MIN;
+	in.pcr_select[0] = 0x00;
+	in.pcr_select[1] = 0x00;
+	in.pcr_select[2] = 0x00;
+	in.pcr_select[pcr_idx >> 3] = 1 << (pcr_idx & 0x7);
 
-	rc = tpm_transmit_cmd(chip, &cmd, sizeof(cmd),
+	tpm_buf_append(&buf, (u8 *) &in, sizeof(in));
+
+	rc = tpm_transmit_cmd(chip, buf.data, TPM_BUF_SIZE,
 			      "attempting to read a pcr value");
-	if (rc == 0) {
-		buf = cmd.params.pcrread_out.digest;
-		memcpy(res_buf, buf, TPM_DIGEST_SIZE);
-	}
+	if (rc)
+		return rc;
 
-	return rc;
+	out = (struct tpm2_pcr_read_out *) &buf.data[TPM_HEADER_SIZE];
+	memcpy(res_buf, out->digest, TPM_DIGEST_SIZE);
+	return 0;
 }
 
-#define TPM2_GET_PCREXTEND_IN_SIZE \
-	(sizeof(struct tpm_input_header) + \
-	 sizeof(struct tpm2_pcr_extend_in))
-
-static const struct tpm_input_header tpm2_pcrextend_header = {
-	.tag = cpu_to_be16(TPM2_ST_SESSIONS),
-	.length = cpu_to_be32(TPM2_GET_PCREXTEND_IN_SIZE),
-	.ordinal = cpu_to_be32(TPM2_CC_PCR_EXTEND)
-};
-
 /**
  * tpm2_pcr_extend() - extend a PCR value
  * @chip:	TPM chip to use.
@@ -291,76 +259,63 @@  static const struct tpm_input_header tpm2_pcrextend_header = {
  */
 int tpm2_pcr_extend(struct tpm_chip *chip, int pcr_idx, const u8 *hash)
 {
-	struct tpm2_cmd cmd;
-	int rc;
-
-	cmd.header.in = tpm2_pcrextend_header;
-	cmd.params.pcrextend_in.pcr_idx = cpu_to_be32(pcr_idx);
-	cmd.params.pcrextend_in.auth_area_size =
-		cpu_to_be32(sizeof(struct tpm2_null_auth_area));
-	cmd.params.pcrextend_in.auth_area.handle =
-		cpu_to_be32(TPM2_RS_PW);
-	cmd.params.pcrextend_in.auth_area.nonce_size = 0;
-	cmd.params.pcrextend_in.auth_area.attributes = 0;
-	cmd.params.pcrextend_in.auth_area.auth_size = 0;
-	cmd.params.pcrextend_in.digest_cnt = cpu_to_be32(1);
-	cmd.params.pcrextend_in.hash_alg = cpu_to_be16(TPM2_ALG_SHA1);
-	memcpy(cmd.params.pcrextend_in.digest, hash, TPM_DIGEST_SIZE);
-
-	rc = tpm_transmit_cmd(chip, &cmd, sizeof(cmd),
-			      "attempting extend a PCR value");
+	struct tpm_buf buf;
+	struct tpm2_pcr_extend_in in;
 
-	return rc;
-}
+	tpm_buf_init(&buf, TPM2_ST_SESSIONS, TPM2_CC_PCR_EXTEND);
+	tpm_buf_append_u32(&buf, pcr_idx);
+	tpm2_buf_append_auth(&buf, TPM2_RS_PW, NULL /* nonce */, 0,
+			     0 /* session_attributes */, NULL /* hmac */, 0);
 
-#define TPM2_GETRANDOM_IN_SIZE \
-	(sizeof(struct tpm_input_header) + \
-	 sizeof(struct tpm2_get_random_in))
+	in.digest_cnt = cpu_to_be32(1);
+	in.hash_alg = cpu_to_be16(TPM2_ALG_SHA1);
+	memcpy(in.digest, hash, TPM_DIGEST_SIZE);
+	tpm_buf_append(&buf, (u8 *) &in, sizeof(in));
 
-static const struct tpm_input_header tpm2_getrandom_header = {
-	.tag = cpu_to_be16(TPM2_ST_NO_SESSIONS),
-	.length = cpu_to_be32(TPM2_GETRANDOM_IN_SIZE),
-	.ordinal = cpu_to_be32(TPM2_CC_GET_RANDOM)
-};
+	return tpm_transmit_cmd(chip, buf.data, TPM_BUF_SIZE,
+				"attempting extend a PCR value");
+}
 
 /**
  * tpm2_get_random() - get random bytes from the TPM RNG
  * @chip: TPM chip to use
- * @out: destination buffer for the random bytes
+ * @res_buf: destination buffer for the random bytes
  * @max: the max number of bytes to write to @out
  *
  * 0 is returned when the operation is successful. If a negative number is
  * returned it remarks a POSIX error code. If a positive number is returned
  * it remarks a TPM error.
  */
-int tpm2_get_random(struct tpm_chip *chip, u8 *out, size_t max)
+int tpm2_get_random(struct tpm_chip *chip, u8 *res_buf, size_t max)
 {
-	struct tpm2_cmd cmd;
+	struct tpm_buf buf;
+	struct tpm2_get_random_out *out;
 	u32 recd;
 	u32 num_bytes;
 	int err;
 	int total = 0;
 	int retries = 5;
-	u8 *dest = out;
+	u8 *dest = res_buf;
 
-	num_bytes = min_t(u32, max, sizeof(cmd.params.getrandom_out.buffer));
+	num_bytes = min_t(u32, max, TPM_MAX_RNG_DATA);
 
-	if (!out || !num_bytes ||
-	    max > sizeof(cmd.params.getrandom_out.buffer))
+	if (!res_buf || !num_bytes || max > TPM_MAX_RNG_DATA)
 		return -EINVAL;
 
 	do {
-		cmd.header.in = tpm2_getrandom_header;
-		cmd.params.getrandom_in.size = cpu_to_be16(num_bytes);
 
-		err = tpm_transmit_cmd(chip, &cmd, sizeof(cmd),
+		tpm_buf_init(&buf, TPM2_ST_SESSIONS, TPM2_CC_GET_RANDOM);
+		tpm_buf_append_u16(&buf, num_bytes);
+
+		err = tpm_transmit_cmd(chip, buf.data, TPM_BUF_SIZE,
 				       "attempting get random");
 		if (err)
 			break;
 
-		recd = min_t(u32, be16_to_cpu(cmd.params.getrandom_out.size),
-			     num_bytes);
-		memcpy(dest, cmd.params.getrandom_out.buffer, recd);
+		out = (struct tpm2_get_random_out *) tpm_buf_out(&buf);
+
+		recd = min_t(u32, be16_to_cpu(out->size), num_bytes);
+		memcpy(dest, out->buffer, recd);
 
 		dest += recd;
 		total += recd;
@@ -370,16 +325,6 @@  int tpm2_get_random(struct tpm_chip *chip, u8 *out, size_t max)
 	return total ? total : -EIO;
 }
 
-#define TPM2_GET_TPM_PT_IN_SIZE \
-	(sizeof(struct tpm_input_header) + \
-	 sizeof(struct tpm2_get_tpm_pt_in))
-
-static const struct tpm_input_header tpm2_get_tpm_pt_header = {
-	.tag = cpu_to_be16(TPM2_ST_NO_SESSIONS),
-	.length = cpu_to_be32(TPM2_GET_TPM_PT_IN_SIZE),
-	.ordinal = cpu_to_be32(TPM2_CC_GET_CAPABILITY)
-};
-
 /**
  * tpm2_get_tpm_pt() - get value of a TPM_CAP_TPM_PROPERTIES type property
  * @chip:		TPM chip to use.
@@ -391,33 +336,31 @@  static const struct tpm_input_header tpm2_get_tpm_pt_header = {
  * returned it remarks a POSIX error code. If a positive number is returned
  * it remarks a TPM error.
  */
-ssize_t tpm2_get_tpm_pt(struct tpm_chip *chip, u32 property_id,  u32 *value,
+ssize_t tpm2_get_tpm_pt(struct tpm_chip *chip, u32 property_id, u32 *value,
 			const char *desc)
 {
-	struct tpm2_cmd cmd;
+	struct tpm_buf buf;
+	struct tpm2_get_tpm_pt_in in;
+	struct tpm2_get_tpm_pt_out *out;
 	int rc;
 
-	cmd.header.in = tpm2_get_tpm_pt_header;
-	cmd.params.get_tpm_pt_in.cap_id = cpu_to_be32(TPM2_CAP_TPM_PROPERTIES);
-	cmd.params.get_tpm_pt_in.property_id = cpu_to_be32(property_id);
-	cmd.params.get_tpm_pt_in.property_cnt = cpu_to_be32(1);
+	tpm_buf_init(&buf, TPM2_ST_SESSIONS, TPM2_CC_GET_CAPABILITY);
 
-	rc = tpm_transmit_cmd(chip, &cmd, sizeof(cmd), desc);
-	if (!rc)
-		*value = cmd.params.get_tpm_pt_out.value;
+	in.cap_id = cpu_to_be32(TPM2_CAP_TPM_PROPERTIES);
+	in.property_id = cpu_to_be32(property_id);
+	in.property_cnt = cpu_to_be32(1);
 
-	return rc;
-}
+	tpm_buf_append(&buf, (u8 *) &in, sizeof(in));
 
-#define TPM2_STARTUP_IN_SIZE \
-	(sizeof(struct tpm_input_header) + \
-	 sizeof(struct tpm2_startup_in))
+	rc = tpm_transmit_cmd(chip, buf.data, TPM_BUF_SIZE, desc);
+	if (!rc) {
+		out = (struct tpm2_get_tpm_pt_out *)
+			&buf.data[TPM_HEADER_SIZE];
+		*value = be32_to_cpu(out->value);
+	}
 
-static const struct tpm_input_header tpm2_startup_header = {
-	.tag = cpu_to_be16(TPM2_ST_NO_SESSIONS),
-	.length = cpu_to_be32(TPM2_STARTUP_IN_SIZE),
-	.ordinal = cpu_to_be32(TPM2_CC_STARTUP)
-};
+	return rc;
+}
 
 /**
  * tpm2_startup() - send startup command to the TPM chip
@@ -431,26 +374,18 @@  static const struct tpm_input_header tpm2_startup_header = {
  */
 int tpm2_startup(struct tpm_chip *chip, u16 startup_type)
 {
-	struct tpm2_cmd cmd;
+	struct tpm_buf buf;
+	int rc;
 
-	cmd.header.in = tpm2_startup_header;
+	tpm_buf_init(&buf, TPM2_ST_NO_SESSIONS, TPM2_CC_STARTUP);
+	tpm_buf_append_u16(&buf, startup_type);
+	rc = tpm_transmit_cmd(chip, buf.data, TPM_BUF_SIZE,
+			      "attempting to start the TPM");
 
-	cmd.params.startup_in.startup_type = cpu_to_be16(startup_type);
-	return tpm_transmit_cmd(chip, &cmd, sizeof(cmd),
-				"attempting to start the TPM");
+	return rc;
 }
 EXPORT_SYMBOL_GPL(tpm2_startup);
 
-#define TPM2_SHUTDOWN_IN_SIZE \
-	(sizeof(struct tpm_input_header) + \
-	 sizeof(struct tpm2_startup_in))
-
-static const struct tpm_input_header tpm2_shutdown_header = {
-	.tag = cpu_to_be16(TPM2_ST_NO_SESSIONS),
-	.length = cpu_to_be32(TPM2_SHUTDOWN_IN_SIZE),
-	.ordinal = cpu_to_be32(TPM2_CC_SHUTDOWN)
-};
-
 /**
  * tpm2_shutdown() - send shutdown command to the TPM chip
  * @chip:		TPM chip to use.
@@ -459,20 +394,11 @@  static const struct tpm_input_header tpm2_shutdown_header = {
  */
 void tpm2_shutdown(struct tpm_chip *chip, u16 shutdown_type)
 {
-	struct tpm2_cmd cmd;
-	int rc;
-
-	cmd.header.in = tpm2_shutdown_header;
-	cmd.params.startup_in.startup_type = cpu_to_be16(shutdown_type);
+	struct tpm_buf buf;
 
-	rc = tpm_transmit_cmd(chip, &cmd, sizeof(cmd), "stopping the TPM");
-
-	/* In places where shutdown command is sent there's no much we can do
-	 * except print the error code on a system failure.
-	 */
-	if (rc < 0)
-		dev_warn(chip->pdev, "transmit returned %d while stopping the TPM",
-			 rc);
+	tpm_buf_init(&buf, TPM2_ST_NO_SESSIONS, TPM2_CC_SHUTDOWN);
+	tpm_buf_append_u16(&buf, shutdown_type);
+	tpm_transmit_cmd(chip, buf.data, TPM_BUF_SIZE, "stopping the TPM");
 }
 EXPORT_SYMBOL_GPL(tpm2_shutdown);
 
@@ -503,16 +429,6 @@  unsigned long tpm2_calc_ordinal_duration(struct tpm_chip *chip, u32 ordinal)
 }
 EXPORT_SYMBOL_GPL(tpm2_calc_ordinal_duration);
 
-#define TPM2_SELF_TEST_IN_SIZE \
-	(sizeof(struct tpm_input_header) + \
-	 sizeof(struct tpm2_self_test_in))
-
-static const struct tpm_input_header tpm2_selftest_header = {
-	.tag = cpu_to_be16(TPM2_ST_NO_SESSIONS),
-	.length = cpu_to_be32(TPM2_SELF_TEST_IN_SIZE),
-	.ordinal = cpu_to_be32(TPM2_CC_SELF_TEST)
-};
-
 /**
  * tpm2_continue_selftest() - start a self test
  * @chip: TPM chip to use
@@ -525,13 +441,13 @@  static const struct tpm_input_header tpm2_selftest_header = {
  */
 static int tpm2_start_selftest(struct tpm_chip *chip, bool full)
 {
+	struct tpm_buf buf;
 	int rc;
-	struct tpm2_cmd cmd;
 
-	cmd.header.in = tpm2_selftest_header;
-	cmd.params.selftest_in.full_test = full;
+	tpm_buf_init(&buf, TPM2_ST_NO_SESSIONS, TPM2_CC_SELF_TEST);
+	tpm_buf_append_u8(&buf, full);
 
-	rc = tpm_transmit_cmd(chip, &cmd, TPM2_SELF_TEST_IN_SIZE,
+	rc = tpm_transmit_cmd(chip, buf.data, TPM_BUF_SIZE,
 			      "continue selftest");
 
 	/* At least some prototype chips seem to give RC_TESTING error
@@ -562,11 +478,10 @@  int tpm2_do_selftest(struct tpm_chip *chip)
 	unsigned int loops;
 	unsigned int delay_msec = 100;
 	unsigned long duration;
-	struct tpm2_cmd cmd;
+	u8 dig[TPM_DIGEST_SIZE];
 	int i;
 
 	duration = tpm2_calc_ordinal_duration(chip, TPM2_CC_SELF_TEST);
-
 	loops = jiffies_to_msecs(duration) / delay_msec;
 
 	rc = tpm2_start_selftest(chip, true);
@@ -574,21 +489,8 @@  int tpm2_do_selftest(struct tpm_chip *chip)
 		return rc;
 
 	for (i = 0; i < loops; i++) {
-		/* Attempt to read a PCR value */
-		cmd.header.in = tpm2_pcrread_header;
-		cmd.params.pcrread_in.pcr_selects_cnt = cpu_to_be32(1);
-		cmd.params.pcrread_in.hash_alg = cpu_to_be16(TPM2_ALG_SHA1);
-		cmd.params.pcrread_in.pcr_select_size = TPM2_PCR_SELECT_MIN;
-		cmd.params.pcrread_in.pcr_select[0] = 0x01;
-		cmd.params.pcrread_in.pcr_select[1] = 0x00;
-		cmd.params.pcrread_in.pcr_select[2] = 0x00;
-
-		rc = tpm_transmit_cmd(chip, (u8 *) &cmd, sizeof(cmd), NULL);
-		if (rc < 0)
-			break;
-
-		rc = be32_to_cpu(cmd.header.out.return_code);
-		if (rc != TPM2_RC_TESTING)
+		rc = tpm2_pcr_read(chip, 1, dig);
+		if (rc < 0 || rc != TPM2_RC_TESTING)
 			break;
 
 		msleep(delay_msec);
@@ -624,21 +526,24 @@  EXPORT_SYMBOL_GPL(tpm2_gen_interrupt);
  */
 int tpm2_probe(struct tpm_chip *chip)
 {
-	struct tpm2_cmd cmd;
+	struct tpm_buf buf;
+	struct tpm2_get_tpm_pt_in in;
 	int rc;
 
-	cmd.header.in = tpm2_get_tpm_pt_header;
-	cmd.params.get_tpm_pt_in.cap_id = cpu_to_be32(TPM2_CAP_TPM_PROPERTIES);
-	cmd.params.get_tpm_pt_in.property_id = cpu_to_be32(0x100);
-	cmd.params.get_tpm_pt_in.property_cnt = cpu_to_be32(1);
+	in.cap_id = cpu_to_be32(TPM2_CAP_TPM_PROPERTIES);
+	in.property_id = cpu_to_be32(0x100);
+	in.property_cnt = cpu_to_be32(1);
+
+	tpm_buf_init(&buf, TPM2_ST_SESSIONS, TPM2_CC_GET_CAPABILITY);
+	tpm_buf_append(&buf, (u8 *) &in, sizeof(in));
 
-	rc = tpm_transmit(chip, (const char *) &cmd, sizeof(cmd));
+	rc = tpm_transmit(chip, buf.data, TPM_BUF_SIZE);
 	if (rc <  0)
 		return rc;
 	else if (rc < TPM_HEADER_SIZE)
 		return -EFAULT;
 
-	if (be16_to_cpu(cmd.header.out.tag) == TPM2_ST_NO_SESSIONS)
+	if (tpm_buf_tag(&buf) == TPM2_ST_NO_SESSIONS)
 		chip->flags |= TPM_CHIP_FLAG_TPM2;
 
 	return 0;