Re: [PATCH] tpm: introduce struct tpm_buf

[Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>]

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