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

From: "Jarkko Sakkinen" <jarkko@kernel.org>
To: "Jarkko Sakkinen" <jarkko@kernel.org>,
	"James Bottomley" <James.Bottomley@HansenPartnership.com>,
	<linux-integrity@vger.kernel.org>
Cc: <keyrings@vger.kernel.org>, "Ard Biesheuvel" <ardb@kernel.org>
Subject: Re: [PATCH v4 05/13] tpm: add cursor based buffer functions for response parsing
Date: Tue, 22 Aug 2023 16:51:19 +0300	[thread overview]
Message-ID: <CUZ4F9XVTLQC.2Y0E44H0PZ3P3@suppilovahvero> (raw)
In-Reply-To: <CUZ143E6EP2G.2V9A6YWYBSXJX@suppilovahvero>

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