diff mbox series

[03/12] tpm: add buffer handling for TPM2B types

Message ID 20230216201410.15010-4-James.Bottomley@HansenPartnership.com (mailing list archive)
State New
Headers show
Series add integrity and security to TPM2 transactions | expand

Commit Message

James Bottomley Feb. 16, 2023, 8:14 p.m. UTC 
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 | 71 +++++++++++++++++++++++++++++++++++---
 include/linux/tpm.h        |  3 ++
 2 files changed, 69 insertions(+), 5 deletions(-)

Comments

Jarkko Sakkinen Feb. 27, 2023, 8:31 a.m. UTC | #1 
On Thu, Feb 16, 2023 at 03:14:01PM -0500, 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>
> ---
>  drivers/char/tpm/tpm-buf.c | 71 +++++++++++++++++++++++++++++++++++---
>  include/linux/tpm.h        |  3 ++
>  2 files changed, 69 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/char/tpm/tpm-buf.c b/drivers/char/tpm/tpm-buf.c
> index ca59b92e0f95..292c6f14f72c 100644
> --- a/drivers/char/tpm/tpm-buf.c
> +++ b/drivers/char/tpm/tpm-buf.c
> @@ -7,17 +7,16 @@
>  #include <linux/module.h>
>  #include <linux/tpm.h>
>  
> -int tpm_buf_init(struct tpm_buf *buf, u16 tag, u32 ordinal)
> +static int __tpm_buf_init(struct tpm_buf *buf)
>  {
>  	buf->data = (u8 *)__get_free_page(GFP_KERNEL);
>  	if (!buf->data)
>  		return -ENOMEM;
>  
>  	buf->flags = 0;
> -	tpm_buf_reset(buf, tag, ordinal);
> +
>  	return 0;
>  }
> -EXPORT_SYMBOL_GPL(tpm_buf_init);
>  
>  void tpm_buf_reset(struct tpm_buf *buf, u16 tag, u32 ordinal)
>  {
> @@ -29,17 +28,60 @@ void tpm_buf_reset(struct tpm_buf *buf, u16 tag, u32 ordinal)
>  }
>  EXPORT_SYMBOL_GPL(tpm_buf_reset);
>  
> +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(buf, tag, ordinal);
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(tpm_buf_init);
> +
> +int tpm_buf_init_2b(struct tpm_buf *buf)

kdoc

> +{
> +	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;

Please make tpm_buf_init() and tpm_buf_reset() to work for both cases.

This explodes the whole thing into an unmaintainable mess. It is better
to have a type as a parameter for tpm_buf_init() and have only single
flow instead of open coded and patched variation.

I'd simply just put it as:

struct tpm_buf *tpm_buf_init(u16 tag, u32 ordinal, bool tpm2b)

> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(tpm_buf_init_2b);
> +
>  void tpm_buf_destroy(struct tpm_buf *buf)
>  {
>  	free_page((unsigned long)buf->data);
>  }
>  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;
> +	u32 len;
>  
> -	return be32_to_cpu(head->length);
> +	len = be32_to_cpu(head->length);
> +	if (buf->flags & TPM_BUF_2B)
> +		len -= sizeof(*head);
> +	return len;
>  }
>  EXPORT_SYMBOL_GPL(tpm_buf_length);
>  
> @@ -55,7 +97,7 @@ void tpm_buf_append(struct tpm_buf *buf,
>  		    unsigned int new_len)
>  {
>  	struct tpm_header *head = (struct tpm_header *) buf->data;
> -	u32 len = tpm_buf_length(buf);
> +	u32 len = be32_to_cpu(head->length);
>  
>  	/* Return silently if overflow has already happened. */
>  	if (buf->flags & TPM_BUF_OVERFLOW)
> @@ -93,3 +135,22 @@ 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/include/linux/tpm.h b/include/linux/tpm.h
> index 150b39b6190e..f2d4dab6d832 100644
> --- a/include/linux/tpm.h
> +++ b/include/linux/tpm.h
> @@ -300,6 +300,7 @@ struct tpm_header {
>  
>  enum tpm_buf_flags {
>  	TPM_BUF_OVERFLOW	= BIT(0),
> +	TPM_BUF_2B		= BIT(1),
>  };


This is IMHO unnecessary complex.

I think we could just have two bools:

        bool overflow;
        bool tpm2b;

>  
>  struct tpm_buf {
> @@ -324,6 +325,7 @@ struct tpm2_hash {
>  
>  
>  int tpm_buf_init(struct tpm_buf *buf, u16 tag, u32 ordinal);
> +int tpm_buf_init_2b(struct tpm_buf *buf);
>  void tpm_buf_reset(struct tpm_buf *buf, u16 tag, u32 ordinal);
>  void tpm_buf_destroy(struct tpm_buf *buf);
>  u32 tpm_buf_length(struct tpm_buf *buf);
> @@ -332,6 +334,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);
>  
>  /*
>   * Check if TPM device is in the firmware upgrade mode.
> -- 
> 2.35.3
> 

BR, Jarkko
James Bottomley March 28, 2023, 7:42 p.m. UTC | #2 
On Mon, 2023-02-27 at 10:31 +0200, Jarkko Sakkinen wrote:
> On Thu, Feb 16, 2023 at 03:14:01PM -0500, 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>
> > ---
> >  drivers/char/tpm/tpm-buf.c | 71
> > +++++++++++++++++++++++++++++++++++---
> >  include/linux/tpm.h        |  3 ++
> >  2 files changed, 69 insertions(+), 5 deletions(-)
> > 
> > diff --git a/drivers/char/tpm/tpm-buf.c b/drivers/char/tpm/tpm-
> > buf.c
> > index ca59b92e0f95..292c6f14f72c 100644
> > --- a/drivers/char/tpm/tpm-buf.c
> > +++ b/drivers/char/tpm/tpm-buf.c
> > @@ -7,17 +7,16 @@
> >  #include <linux/module.h>
> >  #include <linux/tpm.h>
> >  
> > -int tpm_buf_init(struct tpm_buf *buf, u16 tag, u32 ordinal)
> > +static int __tpm_buf_init(struct tpm_buf *buf)
> >  {
> >         buf->data = (u8 *)__get_free_page(GFP_KERNEL);
> >         if (!buf->data)
> >                 return -ENOMEM;
> >  
> >         buf->flags = 0;
> > -       tpm_buf_reset(buf, tag, ordinal);
> > +
> >         return 0;
> >  }
> > -EXPORT_SYMBOL_GPL(tpm_buf_init);
> >  
> >  void tpm_buf_reset(struct tpm_buf *buf, u16 tag, u32 ordinal)
> >  {
> > @@ -29,17 +28,60 @@ void tpm_buf_reset(struct tpm_buf *buf, u16
> > tag, u32 ordinal)
> >  }
> >  EXPORT_SYMBOL_GPL(tpm_buf_reset);
> >  
> > +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(buf, tag, ordinal);
> > +
> > +       return 0;
> > +}
> > +EXPORT_SYMBOL_GPL(tpm_buf_init);
> > +
> > +int tpm_buf_init_2b(struct tpm_buf *buf)
> 
> kdoc

I'm currently working on adding kdoc to everything.  However:

> > +{
> > +       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;
> 
> Please make tpm_buf_init() and tpm_buf_reset() to work for both
> cases.

That's not a good idea: tpm_buf_init() and tpm_buf_reset() are used to
initialize *command* buffers.  tpm_buf_init_2b() is used for parameters
within commands and can't encompass whole commands, so the arguments
are different (that's why tpm_buf_init_2b() has no tag or ordinal).

> This explodes the whole thing into an unmaintainable mess. It is
> better to have a type as a parameter for tpm_buf_init() and have only
> single flow instead of open coded and patched variation.
> 
> I'd simply just put it as:
> 
> struct tpm_buf *tpm_buf_init(u16 tag, u32 ordinal, bool tpm2b)

The convention in Linux is that it's better to have named initializers
if we can rather than use less obvious booleans or flags ... think the
conversion from printk(KERN_ERR, ...) to pr_err(...)

Additionally tag and ordinal have no meaning for a tpm2b, so you're
really gluing two incompatible initializations into one which is bound
to cause confusion.

I've no objection in principle to doing a reset of a tpm2b (except,
again, it has no use for tag or ordinal) but I've just not got any code
that would use it, so I was leaving it out until someone had an actual
use case.

[...]
> > index 150b39b6190e..f2d4dab6d832 100644
> > --- a/include/linux/tpm.h
> > +++ b/include/linux/tpm.h
> > @@ -300,6 +300,7 @@ struct tpm_header {
> >  
> >  enum tpm_buf_flags {
> >         TPM_BUF_OVERFLOW        = BIT(0),
> > +       TPM_BUF_2B              = BIT(1),
> >  };
> 
> 
> This is IMHO unnecessary complex.
> 
> I think we could just have two bools:
> 
>         bool overflow;
>         bool tpm2b;

The advice (in the coding-style.rst bool section) is not to do this but
go the other way (so use flags instead of a string of bools).  The
reason is that even though bool represents a true/false value, it
usually takes one machine word (32 bits or sometimes more) to do it, so
bools tend to bloat structures over single bit fields.

James
diff mbox series

Patch

diff --git a/drivers/char/tpm/tpm-buf.c b/drivers/char/tpm/tpm-buf.c
index ca59b92e0f95..292c6f14f72c 100644
--- a/drivers/char/tpm/tpm-buf.c
+++ b/drivers/char/tpm/tpm-buf.c
@@ -7,17 +7,16 @@ 
 #include <linux/module.h>
 #include <linux/tpm.h>
 
-int tpm_buf_init(struct tpm_buf *buf, u16 tag, u32 ordinal)
+static int __tpm_buf_init(struct tpm_buf *buf)
 {
 	buf->data = (u8 *)__get_free_page(GFP_KERNEL);
 	if (!buf->data)
 		return -ENOMEM;
 
 	buf->flags = 0;
-	tpm_buf_reset(buf, tag, ordinal);
+
 	return 0;
 }
-EXPORT_SYMBOL_GPL(tpm_buf_init);
 
 void tpm_buf_reset(struct tpm_buf *buf, u16 tag, u32 ordinal)
 {
@@ -29,17 +28,60 @@  void tpm_buf_reset(struct tpm_buf *buf, u16 tag, u32 ordinal)
 }
 EXPORT_SYMBOL_GPL(tpm_buf_reset);
 
+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(buf, tag, ordinal);
+
+	return 0;
+}
+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)
 {
 	free_page((unsigned long)buf->data);
 }
 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;
+	u32 len;
 
-	return be32_to_cpu(head->length);
+	len = be32_to_cpu(head->length);
+	if (buf->flags & TPM_BUF_2B)
+		len -= sizeof(*head);
+	return len;
 }
 EXPORT_SYMBOL_GPL(tpm_buf_length);
 
@@ -55,7 +97,7 @@  void tpm_buf_append(struct tpm_buf *buf,
 		    unsigned int new_len)
 {
 	struct tpm_header *head = (struct tpm_header *) buf->data;
-	u32 len = tpm_buf_length(buf);
+	u32 len = be32_to_cpu(head->length);
 
 	/* Return silently if overflow has already happened. */
 	if (buf->flags & TPM_BUF_OVERFLOW)
@@ -93,3 +135,22 @@  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/include/linux/tpm.h b/include/linux/tpm.h
index 150b39b6190e..f2d4dab6d832 100644
--- a/include/linux/tpm.h
+++ b/include/linux/tpm.h
@@ -300,6 +300,7 @@  struct tpm_header {
 
 enum tpm_buf_flags {
 	TPM_BUF_OVERFLOW	= BIT(0),
+	TPM_BUF_2B		= BIT(1),
 };
 
 struct tpm_buf {
@@ -324,6 +325,7 @@  struct tpm2_hash {
 
 
 int tpm_buf_init(struct tpm_buf *buf, u16 tag, u32 ordinal);
+int tpm_buf_init_2b(struct tpm_buf *buf);
 void tpm_buf_reset(struct tpm_buf *buf, u16 tag, u32 ordinal);
 void tpm_buf_destroy(struct tpm_buf *buf);
 u32 tpm_buf_length(struct tpm_buf *buf);
@@ -332,6 +334,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);
 
 /*
  * Check if TPM device is in the firmware upgrade mode.