diff mbox series

[v4,05/13] tpm: add cursor based buffer functions for response parsing

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

Commit Message

James Bottomley April 3, 2023, 9:39 p.m. UTC 
Extracting values from returned TPM buffers can be hard.  Add cursor
based (moving poiner) functions that make it easier to extract TPM
returned values from a buffer.

Signed-off-by: James Bottomley <James.Bottomley@HansenPartnership.com>

---
v4: add kernel doc and reword commit
---
 drivers/char/tpm/tpm-buf.c | 48 ++++++++++++++++++++++++++++++++++++++
 include/linux/tpm.h        |  3 +++
 2 files changed, 51 insertions(+)

Comments

Jarkko Sakkinen April 23, 2023, 4:14 a.m. UTC | #1 
On Tue Apr 4, 2023 at 12:39 AM EEST, James Bottomley wrote:
> Extracting values from returned TPM buffers can be hard.  Add cursor
> based (moving poiner) functions that make it easier to extract TPM
> returned values from a buffer.
>
> Signed-off-by: James Bottomley <James.Bottomley@HansenPartnership.com>
>
> ---
> v4: add kernel doc and reword commit
> ---
>  drivers/char/tpm/tpm-buf.c | 48 ++++++++++++++++++++++++++++++++++++++
>  include/linux/tpm.h        |  3 +++
>  2 files changed, 51 insertions(+)
>
> diff --git a/drivers/char/tpm/tpm-buf.c b/drivers/char/tpm/tpm-buf.c
> index b7e42fb6266c..da0f6e725c3f 100644
> --- a/drivers/char/tpm/tpm-buf.c
> +++ b/drivers/char/tpm/tpm-buf.c
> @@ -6,6 +6,8 @@
>  #include <linux/module.h>
>  #include <linux/tpm.h>
>  
> +#include <asm/unaligned.h>
> +
>  static int __tpm_buf_init(struct tpm_buf *buf)
>  {
>  	buf->data = (u8 *)__get_free_page(GFP_KERNEL);
> @@ -229,3 +231,49 @@ void tpm_buf_append_2b(struct tpm_buf *buf, struct tpm_buf *tpm2b)
>  	tpm_buf_reset_int(tpm2b);
>  }
>  EXPORT_SYMBOL_GPL(tpm_buf_append_2b);
> +
> +/* functions for unmarshalling data and moving the cursor */
> +
> +/**
> + * tpm_get_inc_u8 - read a u8 and move pointer beyond it
> + * @ptr: pointer to pointer
> + *
> + * @return: value read
> + */
> +u8 tpm_get_inc_u8(const u8 **ptr)
> +{
> +	return *((*ptr)++);
> +}
> +EXPORT_SYMBOL_GPL(tpm_get_inc_u8);
> +
> +/**
> + * tpm_get_inc_u16 - read a u16 and move pointer beyond it
> + * @ptr: pointer to pointer
> + *
> + * @return: value read (converted from big endian)
> + */
> +u16 tpm_get_inc_u16(const u8 **ptr)
> +{
> +	u16 val;
> +
> +	val = get_unaligned_be16(*ptr);
> +	*ptr += sizeof(val);
> +	return val;
> +}
> +EXPORT_SYMBOL_GPL(tpm_get_inc_u16);
> +
> +/**
> + * tpm_get_inc_u32 - read a u32 and move pointer beyond it
> + * @ptr: pointer to pointer
> + *
> + * @return: value read (converted from big endian)
> + */
> +u32 tpm_get_inc_u32(const u8 **ptr)
> +{
> +	u32 val;
> +
> +	val = get_unaligned_be32(*ptr);
> +	*ptr += sizeof(val);
> +	return val;
> +}
> +EXPORT_SYMBOL_GPL(tpm_get_inc_u32);
> diff --git a/include/linux/tpm.h b/include/linux/tpm.h
> index 76d495cb5b08..845eadfed715 100644
> --- a/include/linux/tpm.h
> +++ b/include/linux/tpm.h
> @@ -336,6 +336,9 @@ 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);
> +u8 tpm_get_inc_u8(const u8 **ptr);
> +u16 tpm_get_inc_u16(const u8 **ptr);
> +u32 tpm_get_inc_u32(const u8 **ptr);
>  
>  /*
>   * Check if TPM device is in the firmware upgrade mode.
> -- 
> 2.35.3

Why not just inline functions?

BR, Jarkko
Stefan Berger May 2, 2023, 1:54 p.m. UTC | #2 
On 4/3/23 17:39, James Bottomley wrote:
> Extracting values from returned TPM buffers can be hard.  Add cursor
> based (moving poiner) functions that make it easier to extract TPM

s/poiner/pointer

> returned values from a buffer.
> 
> Signed-off-by: James Bottomley <James.Bottomley@HansenPartnership.com>

Reviewed-by: Stefan Berger <stefanb@linux.ibm.com>

> 
> ---
> v4: add kernel doc and reword commit
> ---
>   drivers/char/tpm/tpm-buf.c | 48 ++++++++++++++++++++++++++++++++++++++
>   include/linux/tpm.h        |  3 +++
>   2 files changed, 51 insertions(+)
> 
> diff --git a/drivers/char/tpm/tpm-buf.c b/drivers/char/tpm/tpm-buf.c
> index b7e42fb6266c..da0f6e725c3f 100644
> --- a/drivers/char/tpm/tpm-buf.c
> +++ b/drivers/char/tpm/tpm-buf.c
> @@ -6,6 +6,8 @@
>   #include <linux/module.h>
>   #include <linux/tpm.h>
>   
> +#include <asm/unaligned.h>
> +
>   static int __tpm_buf_init(struct tpm_buf *buf)
>   {
>   	buf->data = (u8 *)__get_free_page(GFP_KERNEL);
> @@ -229,3 +231,49 @@ void tpm_buf_append_2b(struct tpm_buf *buf, struct tpm_buf *tpm2b)
>   	tpm_buf_reset_int(tpm2b);
>   }
>   EXPORT_SYMBOL_GPL(tpm_buf_append_2b);
> +
> +/* functions for unmarshalling data and moving the cursor */
> +
> +/**
> + * tpm_get_inc_u8 - read a u8 and move pointer beyond it
> + * @ptr: pointer to pointer
> + *
> + * @return: value read
> + */
> +u8 tpm_get_inc_u8(const u8 **ptr)
> +{
> +	return *((*ptr)++);
> +}
> +EXPORT_SYMBOL_GPL(tpm_get_inc_u8);
> +
> +/**
> + * tpm_get_inc_u16 - read a u16 and move pointer beyond it
> + * @ptr: pointer to pointer
> + *
> + * @return: value read (converted from big endian)
> + */
> +u16 tpm_get_inc_u16(const u8 **ptr)
> +{
> +	u16 val;
> +
> +	val = get_unaligned_be16(*ptr);
> +	*ptr += sizeof(val);
> +	return val;
> +}
> +EXPORT_SYMBOL_GPL(tpm_get_inc_u16);
> +
> +/**
> + * tpm_get_inc_u32 - read a u32 and move pointer beyond it
> + * @ptr: pointer to pointer
> + *
> + * @return: value read (converted from big endian)
> + */
> +u32 tpm_get_inc_u32(const u8 **ptr)
> +{
> +	u32 val;
> +
> +	val = get_unaligned_be32(*ptr);
> +	*ptr += sizeof(val);
> +	return val;
> +}
> +EXPORT_SYMBOL_GPL(tpm_get_inc_u32);
> diff --git a/include/linux/tpm.h b/include/linux/tpm.h
> index 76d495cb5b08..845eadfed715 100644
> --- a/include/linux/tpm.h
> +++ b/include/linux/tpm.h
> @@ -336,6 +336,9 @@ 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);
> +u8 tpm_get_inc_u8(const u8 **ptr);
> +u16 tpm_get_inc_u16(const u8 **ptr);
> +u32 tpm_get_inc_u32(const u8 **ptr);
>   
>   /*
>    * Check if TPM device is in the firmware upgrade mode.
Jarkko Sakkinen Aug. 22, 2023, 11:15 a.m. UTC | #3 
On Tue Apr 4, 2023 at 12:39 AM EEST, James Bottomley wrote:
> Extracting values from returned TPM buffers can be hard.  Add cursor
> based (moving poiner) functions that make it easier to extract TPM
> returned values from a buffer.
>
> Signed-off-by: James Bottomley <James.Bottomley@HansenPartnership.com>
>
> ---
> v4: add kernel doc and reword commit
> ---
>  drivers/char/tpm/tpm-buf.c | 48 ++++++++++++++++++++++++++++++++++++++
>  include/linux/tpm.h        |  3 +++
>  2 files changed, 51 insertions(+)
>
> diff --git a/drivers/char/tpm/tpm-buf.c b/drivers/char/tpm/tpm-buf.c
> index b7e42fb6266c..da0f6e725c3f 100644
> --- a/drivers/char/tpm/tpm-buf.c
> +++ b/drivers/char/tpm/tpm-buf.c
> @@ -6,6 +6,8 @@
>  #include <linux/module.h>
>  #include <linux/tpm.h>
>  
> +#include <asm/unaligned.h>
> +
>  static int __tpm_buf_init(struct tpm_buf *buf)
>  {
>  	buf->data = (u8 *)__get_free_page(GFP_KERNEL);
> @@ -229,3 +231,49 @@ void tpm_buf_append_2b(struct tpm_buf *buf, struct tpm_buf *tpm2b)
>  	tpm_buf_reset_int(tpm2b);
>  }
>  EXPORT_SYMBOL_GPL(tpm_buf_append_2b);
> +
> +/* functions for unmarshalling data and moving the cursor */
> +
> +/**
> + * tpm_get_inc_u8 - read a u8 and move pointer beyond it
> + * @ptr: pointer to pointer
> + *
> + * @return: value read
> + */
> +u8 tpm_get_inc_u8(const u8 **ptr)
> +{
> +	return *((*ptr)++);
> +}
> +EXPORT_SYMBOL_GPL(tpm_get_inc_u8);

No overflow check, and these should be static inlines.

Please consider this:

/**
 * tpm_buf_read_u8() - Read a byte from a TPM buffer
 * @buf:	&tpm_buf instance
 * @offset:	offset within the consumed part of the buffer
 */
static inline int tpm_buf_read_u8(const struct tpm_buf *buf, offs_t *offset)
{
	if (*offset++ >= buf->length)
		return -EINVAL;
	
	return buf->data[*offset - 1];		
}

Depends on:

https://lore.kernel.org/linux-integrity/20230821033630.1039527-1-jarkko@kernel.org/

No motivation for weird cursor concept, when the reality is just
a read from a buffer.

BR, Jarkko
Jarkko Sakkinen Aug. 22, 2023, 1:51 p.m. UTC | #4 
On Tue Aug 22, 2023 at 2:15 PM EEST, Jarkko Sakkinen wrote:
> On Tue Apr 4, 2023 at 12:39 AM EEST, James Bottomley wrote:
> > Extracting values from returned TPM buffers can be hard.  Add cursor
> > based (moving poiner) functions that make it easier to extract TPM
> > returned values from a buffer.
> >
> > Signed-off-by: James Bottomley <James.Bottomley@HansenPartnership.com>
> >
> > ---
> > v4: add kernel doc and reword commit
> > ---
> >  drivers/char/tpm/tpm-buf.c | 48 ++++++++++++++++++++++++++++++++++++++
> >  include/linux/tpm.h        |  3 +++
> >  2 files changed, 51 insertions(+)
> >
> > diff --git a/drivers/char/tpm/tpm-buf.c b/drivers/char/tpm/tpm-buf.c
> > index b7e42fb6266c..da0f6e725c3f 100644
> > --- a/drivers/char/tpm/tpm-buf.c
> > +++ b/drivers/char/tpm/tpm-buf.c
> > @@ -6,6 +6,8 @@
> >  #include <linux/module.h>
> >  #include <linux/tpm.h>
> >  
> > +#include <asm/unaligned.h>
> > +
> >  static int __tpm_buf_init(struct tpm_buf *buf)
> >  {
> >  	buf->data = (u8 *)__get_free_page(GFP_KERNEL);
> > @@ -229,3 +231,49 @@ void tpm_buf_append_2b(struct tpm_buf *buf, struct tpm_buf *tpm2b)
> >  	tpm_buf_reset_int(tpm2b);
> >  }
> >  EXPORT_SYMBOL_GPL(tpm_buf_append_2b);
> > +
> > +/* functions for unmarshalling data and moving the cursor */
> > +
> > +/**
> > + * tpm_get_inc_u8 - read a u8 and move pointer beyond it
> > + * @ptr: pointer to pointer
> > + *
> > + * @return: value read
> > + */
> > +u8 tpm_get_inc_u8(const u8 **ptr)
> > +{
> > +	return *((*ptr)++);
> > +}
> > +EXPORT_SYMBOL_GPL(tpm_get_inc_u8);
>
> No overflow check, and these should be static inlines.
>
> Please consider this:
>
> /**
>  * tpm_buf_read_u8() - Read a byte from a TPM buffer
>  * @buf:	&tpm_buf instance
>  * @offset:	offset within the consumed part of the buffer
>  */
> static inline int tpm_buf_read_u8(const struct tpm_buf *buf, offs_t *offset)
> {
> 	if (*offset++ >= buf->length)

Oops, this increases pointer location, not value, sorry (should be (*offset)++).

> 		return -EINVAL;
> 	
> 	return buf->data[*offset - 1];		
> }

Actually probably best would be if you would add first (in order to
have all the logic in a single location):

/**
 * tpm_buf_read() - Read from a TPM buffer
 * @buf:	&tpm_buf instance
 * @count:	the number of bytes to read
 * @offset:	offset within the buffer
 * @output:	the output buffer
 */
int tpm_buf_read(const struct tpm_buf *buf, size_t count, offs_t *offset, void *output)
{
	if (*(offset + count) >= buf->length)
		return -EINVAL;

	memcpy(output, &buf->data[*offset], count);
	*offset += count;

]	return 0;
}
EXPORT_SYMBOL_GPL(tpm_buf_read);

For instance:

static inline static int tpm_buf_read_u16(const struct tpm_buf *buf, offs_t *offset)
{
	u16 value;
	int ret;

	ret = tpm_buf_read(buf, sizeof(u16), offset, &value);	

	return ret ? ret : be16_to_cpu(value);
}
 
BR, Jarkko
diff mbox series

Patch

diff --git a/drivers/char/tpm/tpm-buf.c b/drivers/char/tpm/tpm-buf.c
index b7e42fb6266c..da0f6e725c3f 100644
--- a/drivers/char/tpm/tpm-buf.c
+++ b/drivers/char/tpm/tpm-buf.c
@@ -6,6 +6,8 @@ 
 #include <linux/module.h>
 #include <linux/tpm.h>
 
+#include <asm/unaligned.h>
+
 static int __tpm_buf_init(struct tpm_buf *buf)
 {
 	buf->data = (u8 *)__get_free_page(GFP_KERNEL);
@@ -229,3 +231,49 @@  void tpm_buf_append_2b(struct tpm_buf *buf, struct tpm_buf *tpm2b)
 	tpm_buf_reset_int(tpm2b);
 }
 EXPORT_SYMBOL_GPL(tpm_buf_append_2b);
+
+/* functions for unmarshalling data and moving the cursor */
+
+/**
+ * tpm_get_inc_u8 - read a u8 and move pointer beyond it
+ * @ptr: pointer to pointer
+ *
+ * @return: value read
+ */
+u8 tpm_get_inc_u8(const u8 **ptr)
+{
+	return *((*ptr)++);
+}
+EXPORT_SYMBOL_GPL(tpm_get_inc_u8);
+
+/**
+ * tpm_get_inc_u16 - read a u16 and move pointer beyond it
+ * @ptr: pointer to pointer
+ *
+ * @return: value read (converted from big endian)
+ */
+u16 tpm_get_inc_u16(const u8 **ptr)
+{
+	u16 val;
+
+	val = get_unaligned_be16(*ptr);
+	*ptr += sizeof(val);
+	return val;
+}
+EXPORT_SYMBOL_GPL(tpm_get_inc_u16);
+
+/**
+ * tpm_get_inc_u32 - read a u32 and move pointer beyond it
+ * @ptr: pointer to pointer
+ *
+ * @return: value read (converted from big endian)
+ */
+u32 tpm_get_inc_u32(const u8 **ptr)
+{
+	u32 val;
+
+	val = get_unaligned_be32(*ptr);
+	*ptr += sizeof(val);
+	return val;
+}
+EXPORT_SYMBOL_GPL(tpm_get_inc_u32);
diff --git a/include/linux/tpm.h b/include/linux/tpm.h
index 76d495cb5b08..845eadfed715 100644
--- a/include/linux/tpm.h
+++ b/include/linux/tpm.h
@@ -336,6 +336,9 @@  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);
+u8 tpm_get_inc_u8(const u8 **ptr);
+u16 tpm_get_inc_u16(const u8 **ptr);
+u32 tpm_get_inc_u32(const u8 **ptr);
 
 /*
  * Check if TPM device is in the firmware upgrade mode.