| Message ID | 1568031515.6613.31.camel@HansenPartnership.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | add integrity and security to TPM2 transactions | expand |
On Mon, Sep 09, 2019 at 01:18:35PM +0100, James Bottomley wrote: > 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 <James.Bottomley@HansenPartnership.com> I think a better idea would be to have headerless TPM buffers and also it makes sense to have a separate length field in the struct to keep the code sane given that sometimes the buffer does not store the length. E.g. enum tpm_buf_flags { TPM_BUF_OVERFLOW = BIT(0), TPM_BUF_HEADERLESS = BIT(1), }; struct tpm_buf { unsigned int length; struct page *data_page; unsigned int flags; u8 *data; }; /Jarkko
On Fri, 2019-09-20 at 17:18 +0300, Jarkko Sakkinen wrote: > On Mon, Sep 09, 2019 at 01:18:35PM +0100, James Bottomley wrote: > > 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 <James.Bottomley@HansenPartnership.c > > om> > > I think a better idea would be to have headerless TPM buffers I thought about that. The main problem is that most of the construct/append functions use the header, and these are the functions most useful to the TPM2B operation. The other thing that argues against this is that the TPM2B case would save nothing if we eliminated the header, because we allocate a page for all the data regardless. > and also it makes sense to have a separate length field in the > struct to keep the code sane given that sometimes the buffer does not > store the length. I'm really not sure about that one. The header length has to be filled in for the non-TPM2B case but right at the moment we have no finish function for the buf where it could be, so we'd end up having to maintain two lengths in every update operation on non-TPM2B buffers. That seems inefficient and the only slight efficiency we get in the TPM2B case is not having to do the big endian conversion from the header which doesn't seem to be worth the added complexity. James > E.g. > > enum tpm_buf_flags { > TPM_BUF_OVERFLOW = BIT(0), > TPM_BUF_HEADERLESS = BIT(1), > }; > > struct tpm_buf { > unsigned int length; > struct page *data_page; > unsigned int flags; > u8 *data; > }; > > /Jarkko >
On Tue, Sep 24, 2019 at 07:12:40AM -0400, James Bottomley wrote: > I thought about that. The main problem is that most of the > construct/append functions use the header, and these are the functions > most useful to the TPM2B operation. > > The other thing that argues against this is that the TPM2B case would > save nothing if we eliminated the header, because we allocate a page > for all the data regardless. It would be way more clean. There is absolutely nothing TPM2B specific. > > and also it makes sense to have a separate length field in the > > struct to keep the code sane given that sometimes the buffer does not > > store the length. > > I'm really not sure about that one. The header length has to be filled > in for the non-TPM2B case but right at the moment we have no finish > function for the buf where it could be, so we'd end up having to > maintain two lengths in every update operation on non-TPM2B buffers. > That seems inefficient and the only slight efficiency we get in the > TPM2B case is not having to do the big endian conversion from the > header which doesn't seem to be worth the added complexity. It would be way more clean and an insignificant concern when it comes to performance. I don't see any problem updating two lengths. /Jarkko
On Wed, Sep 25, 2019 at 03:34:01PM +0300, Jarkko Sakkinen wrote: > On Tue, Sep 24, 2019 at 07:12:40AM -0400, James Bottomley wrote: > > I thought about that. The main problem is that most of the > > construct/append functions use the header, and these are the functions > > most useful to the TPM2B operation. > > > > The other thing that argues against this is that the TPM2B case would > > save nothing if we eliminated the header, because we allocate a page > > for all the data regardless. > > It would be way more clean. There is absolutely nothing TPM2B specific. Given the recent regression I'm detaching allocation from tpm_buf and make it purely a decorator (sending patch today). /Jarkko
diff --git a/drivers/char/tpm/tpm-buf.c b/drivers/char/tpm/tpm-buf.c index 9fa8a9cb0fdf..8c1ed8a14e01 100644 --- a/drivers/char/tpm/tpm-buf.c +++ b/drivers/char/tpm/tpm-buf.c @@ -8,6 +8,8 @@ #include <linux/module.h> +#include <asm/unaligned.h> + static int __tpm_buf_init(struct tpm_buf *buf) { buf->data_page = alloc_page(GFP_HIGHUSER); @@ -46,6 +48,24 @@ int tpm_buf_init(struct tpm_buf *buf, u16 tag, u32 ordinal) } 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) { kunmap(buf->data_page); @@ -53,6 +73,13 @@ void tpm_buf_destroy(struct tpm_buf *buf) } 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; @@ -116,3 +143,23 @@ 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); + diff --git a/drivers/char/tpm/tpm.h b/drivers/char/tpm/tpm.h index 8c5b8bba60d2..7627917db345 100644 --- a/drivers/char/tpm/tpm.h +++ b/drivers/char/tpm/tpm.h @@ -292,6 +292,7 @@ struct tpm_buf { int tpm_buf_init(struct tpm_buf *buf, u16 tag, u32 ordinal); void tpm_buf_reset(struct tpm_buf *buf, u16 tag, u32 ordinal); +int tpm_buf_init_2b(struct tpm_buf *buf); 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, @@ -299,6 +300,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); extern struct class *tpm_class; extern struct class *tpmrm_class;
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 <James.Bottomley@HansenPartnership.com> --- drivers/char/tpm/tpm-buf.c | 47 ++++++++++++++++++++++++++++++++++++++++++++++ drivers/char/tpm/tpm.h | 2 ++ 2 files changed, 49 insertions(+)