Re: [PATCH v3 1/6] tpm-buf: create new functions for handling TPM buffers

[Jarkko Sakkinen]

On Fri, 2018-03-16 at 13:58 +0200, Jarkko Sakkinen wrote:
> On Sat, 2018-03-10 at 14:14 -0800, James Bottomley wrote:
> > TPM_BUF_OVERFLOW= BIT(0),
> > +   TPM_BUF_2B  = BIT(1),
> 
> Instead of re-using this I would prefer to have another enum for
> buffer type. tpm_buf_init() could have the signature:
> 
> int tpm_buf_init(unsigned int type);
> 
> For commands there should be a function:
> 
> void tpm_buf_set_command_header(struct tpm_buf *buf, u16 tag, u32 ordinal);
> 
> And tpm_buf_append_2b() should not exist at all. It should be
> maintained automatically by other append commands.

Can you send the next version this patch as a separate entity? Once
I can land this we have kind of stable ground for the following
patches. After that it is easier test and review them.

/Jarkko

Re: [PATCH v3 1/6] tpm-buf: create new functions for handling TPM buffers

[Jarkko Sakkinen]

On Sat, 2018-03-10 at 14:14 -0800, James Bottomley wrote:
>   TPM_BUF_OVERFLOW= BIT(0),
> + TPM_BUF_2B  = BIT(1),

Instead of re-using this I would prefer to have another enum for
buffer type. tpm_buf_init() could have the signature:

int tpm_buf_init(unsigned int type);

For commands there should be a function:

void tpm_buf_set_command_header(struct tpm_buf *buf, u16 tag, u32 ordinal);

And tpm_buf_append_2b() should not exist at all. It should be
maintained automatically by other append commands.

/Jarkko

Re: [PATCH v3 1/6] tpm-buf: create new functions for handling TPM buffers

[J Freyensee]

On 3/12/18 10:59 AM, James Bottomley wrote:

On Mon, 2018-03-12 at 09:00 -0700, J Freyensee wrote:

+int tpm_buf_init(struct tpm_buf *buf, u16 tag, u32 ordinal)
+{
+   int rc;
+
+   rc = __tpm_buf_init(buf);


Assuming that functions like tpm_buf_init() are the top-level API
being defined in this patch, shouldn't it check if buf is valid
before passing into the internal functions like __tpm_buf_init(buf)
(maybe WARN()/BUG_ON()?).  Or does __tpm_buf_init(buf) do this check?

These are kernel internal APIs designed for on stack struct tpm_buf
usage,


ok.


so I can't think of a viable threat model that would require
this type of checking ... do you have one?


no, nothing particular in mind.  I just get a little nervous when I see 
variables being passed unchecked into internal functions starting with '__'.


Regards,
Jay



James

Re: [PATCH v3 1/6] tpm-buf: create new functions for handling TPM buffers

[James Bottomley]

On Mon, 2018-03-12 at 09:00 -0700, J Freyensee wrote:
> > 
> > +int tpm_buf_init(struct tpm_buf *buf, u16 tag, u32 ordinal)
> > +{
> > +   int rc;
> > +
> > +   rc = __tpm_buf_init(buf);
> 
> 
> Assuming that functions like tpm_buf_init() are the top-level API
> being defined in this patch, shouldn't it check if buf is valid
> before passing into the internal functions like __tpm_buf_init(buf)
> (maybe WARN()/BUG_ON()?).  Or does __tpm_buf_init(buf) do this check?

These are kernel internal APIs designed for on stack struct tpm_buf
usage, so I can't think of a viable threat model that would require
this type of checking ... do you have one?

James

Re: [PATCH v3 1/6] tpm-buf: create new functions for handling TPM buffers

[J Freyensee]

+int tpm_buf_init(struct tpm_buf *buf, u16 tag, u32 ordinal)
+{
+   int rc;
+
+   rc = __tpm_buf_init(buf);



Assuming that functions like tpm_buf_init() are the top-level API being 
defined in this patch, shouldn't it check if buf is valid before passing 
into the internal functions like __tpm_buf_init(buf) (maybe 
WARN()/BUG_ON()?).  Or does __tpm_buf_init(buf) do this check?


Thanks,
Jay

[PATCH v3 1/6] tpm-buf: create new functions for handling TPM buffers

[James Bottomley]

This separates out the old tpm_buf_... handling functions from static
inlines into tpm.h and makes them their own tpm-buf.c file.  It also
adds handling for tpm2b structures and also incremental pointer
advancing parsers.

Signed-off-by: James Bottomley 

---

v2: added this patch to separate out the API changes
v3: added tpm_buf_reset_cmd()
---
 drivers/char/tpm/Makefile  |   2 +-
 drivers/char/tpm/tpm-buf.c | 191 +
 drivers/char/tpm/tpm.h |  95 --
 3 files changed, 208 insertions(+), 80 deletions(-)
 create mode 100644 drivers/char/tpm/tpm-buf.c

diff --git a/drivers/char/tpm/Makefile b/drivers/char/tpm/Makefile
index d37c4a1748f5..41b2482b97c3 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 tpm-buf.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-buf.c b/drivers/char/tpm/tpm-buf.c
new file mode 100644
index ..146a71cec067
--- /dev/null
+++ b/drivers/char/tpm/tpm-buf.c
@@ -0,0 +1,191 @@
+// SPDX-License-Identifier: GPL-2.0
+
+/*
+ * Handing for tpm2b structures to facilitate the building of commands
+ */
+
+#include "tpm.h"
+
+#include 
+
+#include 
+
+static int __tpm_buf_init(struct tpm_buf *buf)
+{
+   buf->data_page = alloc_page(GFP_HIGHUSER);
+   if (!buf->data_page)
+   return -ENOMEM;
+
+   buf->flags = 0;
+   buf->data = kmap(buf->data_page);
+
+   return 0;
+}
+
+void tpm_buf_reset_cmd(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);
+}
+EXPORT_SYMBOL_GPL(tpm_buf_reset_cmd);
+
+int tpm_buf_init(struct tpm_buf *buf, u16 tag, u32 ordinal)
+{
+   int rc;
+
+   rc = __tpm_buf_init(buf);
+   if (rc)
+   return rc;
+
+   tpm_buf_reset_cmd(buf, tag, ordinal);
+
+   return 0;
+}
+EXPORT_SYMBOL_GPL(tpm_buf_init);
+
+int tpm_buf_init_2b(struct tpm_buf *buf)
+{
+   struct tpm_input_header *head;
+   int rc;
+
+   rc = __tpm_buf_init(buf);
+   if (rc)
+   return rc;
+
+   head = (struct tpm_input_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);
+   __free_page(buf->data_page);
+}
+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_input_header *head = (struct tpm_input_header *)buf->data;
+   u32 len;
+
+   len = be32_to_cpu(head->length);
+   if (buf->flags & TPM_BUF_2B)
+   len -= sizeof(*head);
+   return len;
+}
+EXPORT_SYMBOL_GPL(tpm_buf_length);
+
+u16 tpm_buf_tag(struct tpm_buf *buf)
+{
+   struct tpm_input_header *head = (struct tpm_input_header *)buf->data;
+
+   return be16_to_cpu(head->tag);
+}
+EXPORT_SYMBOL_GPL(tpm_buf_tag);
+
+void tpm_buf_append(struct tpm_buf *buf,
+   const unsigned char *new_data,
+   unsigned int new_len)
+{
+   struct tpm_input_header *head = (struct tpm_input_header *) buf->data;
+   u32 len = be32_to_cpu(head->length);
+
+   /* Return silently if overflow has already happened. */
+   if (buf->flags & TPM_BUF_OVERFLOW)
+   return;
+
+   if ((len + new_len) > PAGE_SIZE) {
+   WARN(1, "tpm_buf: overflow\n");
+   buf->flags |= TPM_BUF_OVERFLOW;
+   return;
+   }
+
+   memcpy(>data[len], new_data, new_len);
+   head->length = cpu_to_be32(len + new_len);
+}
+EXPORT_SYMBOL_GPL(tpm_buf_append);
+
+void tpm_buf_append_u8(struct tpm_buf *buf, const u8 value)
+{
+   tpm_buf_append(buf, , 1);
+}
+EXPORT_SYMBOL_GPL(tpm_buf_append_u8);
+
+void tpm_buf_append_u16(struct tpm_buf *buf, const u16 value)
+{
+   __be16 value2 = cpu_to_be16(value);
+
+   tpm_buf_append(buf, (u8 *) , 2);
+}
+EXPORT_SYMBOL_GPL(tpm_buf_append_u16);
+
+void tpm_buf_append_u32(struct tpm_buf *buf, const u32 value)
+{
+   __be32 value2 = cpu_to_be32(value);
+
+   tpm_buf_append(buf, (u8 *) , 4);
+}
+EXPORT_SYMBOL_GPL(tpm_buf_append_u32);
+
+static void tpm_buf_reset(struct tpm_buf *buf)
+{
+   struct tpm_input_header