Re: [PATCH v18 13/25] tss2: Add TPM2 buffer handling functions

[Gary Lin]

On Thu, Aug 22, 2024 at 03:21:51PM +0200, Daniel Kiper wrote:
> On Fri, Jun 28, 2024 at 04:18:56PM +0800, Gary Lin via Grub-devel wrote:
> > As the prepartion to support TPM2 Software Stack (TSS2), this commit
> > implements the TPM2 buffer handling functions to pack data for the TPM2
> > commands and unpack the data from the response.
> >
> > Cc: Stefan Berger <stefanb@linux.ibm.com>
> > Signed-off-by: Hernan Gatta <hegatta@linux.microsoft.com>
> > Signed-off-by: Gary Lin <glin@suse.com>
> > ---
> >  grub-core/lib/tss2/buffer.c      | 149 +++++++++++++++++++++++++++++++
> >  grub-core/lib/tss2/tss2_buffer.h |  66 ++++++++++++++
> >  2 files changed, 215 insertions(+)
> >  create mode 100644 grub-core/lib/tss2/buffer.c
> >  create mode 100644 grub-core/lib/tss2/tss2_buffer.h
> >
> > diff --git a/grub-core/lib/tss2/buffer.c b/grub-core/lib/tss2/buffer.c
> > new file mode 100644
> > index 000000000..00a57b464
> > --- /dev/null
> > +++ b/grub-core/lib/tss2/buffer.c
> > @@ -0,0 +1,149 @@
> > +/*
> > + *  GRUB  --  GRand Unified Bootloader
> > + *  Copyright (C) 2024 Free Software Foundation, Inc.
> > + *  Copyright (C) 2022 Microsoft Corporation
> 
> Wrong order. The "Free Software Foundation" line should go behind the
> "Microsoft Corporation".
> 
Oops. I'll fix the order in all affected files.

> > + *  GRUB is free software: you can redistribute it and/or modify
> > + *  it under the terms of the GNU General Public License as published by
> > + *  the Free Software Foundation, either version 3 of the License, or
> > + *  (at your option) any later version.
> > + *
> > + *  GRUB is distributed in the hope that it will be useful,
> > + *  but WITHOUT ANY WARRANTY; without even the implied warranty of
> > + *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> > + *  GNU General Public License for more details.
> > + *
> > + *  You should have received a copy of the GNU General Public License
> > + *  along with GRUB.  If not, see <http://www.gnu.org/licenses/>.
> > + */
> > +
> > +#include <grub/misc.h>
> > +
> > +#include <tss2_buffer.h>
> > +
> > +void grub_tpm2_buffer_init (grub_tpm2_buffer_t buffer)
> > +{
> > +  grub_memset (buffer->data, 0, sizeof (buffer->data));
> > +  buffer->size = 0;
> > +  buffer->offset = 0;
> > +  buffer->cap = sizeof (buffer->data);
> > +  buffer->error = 0;
> > +}
> > +
> > +void
> > +grub_tpm2_buffer_pack (grub_tpm2_buffer_t buffer, const void* data,
> 
> s/void* data/void *data/
> 
Will fix it in the next version.

> > +                  grub_size_t size)
> > +{
> > +  grub_uint32_t r = buffer->cap - buffer->size;
> > +
> > +  if (buffer->error)
> > +    return;
> > +
> > +  if (size > r)
> > +    {
> > +      buffer->error = 1;
> > +      return;
> > +    }
> > +
> > +  grub_memcpy (&buffer->data[buffer->size], (void*) data, size);
> 
> s/void*/void */
> 
Will fix it.

> > +  buffer->size += size;
> > +}
> > +
> > +void
> > +grub_tpm2_buffer_pack_u8 (grub_tpm2_buffer_t buffer, grub_uint8_t value)
> > +{
> > +  grub_tpm2_buffer_pack (buffer, (const char*) &value, sizeof (value));
> 
> s/const char*/const char */
> 
> Wait, why do you cast to "const char*" if the grub_tpm2_buffer_pack()
> second argument is "const void *"? I think it should be changed. If yes
> please fix similar problems everywhere.
> 
Right, it's should be "const void *". I'll fix all those pack functions.

> > +}
> > +
> > +void
> > +grub_tpm2_buffer_pack_u16 (grub_tpm2_buffer_t buffer, grub_uint16_t value)
> > +{
> > +  grub_uint16_t tmp = grub_cpu_to_be16 (value);
> > +
> > +  grub_tpm2_buffer_pack (buffer, (const char*) &tmp, sizeof (tmp));
> 
> Ditto and below please...
> 
Got it.

> > +}
> > +
> > +void
> > +grub_tpm2_buffer_pack_u32 (grub_tpm2_buffer_t buffer, grub_uint32_t value)
> > +{
> > +  grub_uint32_t tmp = grub_cpu_to_be32 (value);
> > +
> > +  grub_tpm2_buffer_pack (buffer, (const char*) &tmp, sizeof (tmp));
> > +}
> > +
> > +void
> > +grub_tpm2_buffer_unpack (grub_tpm2_buffer_t buffer, void* data,
> 
> s/void* data/void *data/
> 
Okay.

> > +                    grub_size_t size)
> > +{
> > +  grub_uint32_t r = buffer->size - buffer->offset;
> > +
> > +  if (buffer->error)
> > +    return;
> > +
> > +  if (size > r)
> > +    {
> > +      buffer->error = 1;
> > +      return;
> > +    }
> > +
> > +  grub_memcpy (data, &buffer->data[buffer->offset], size);
> > +  buffer->offset += size;
> > +}
> > +
> > +void
> > +grub_tpm2_buffer_unpack_u8 (grub_tpm2_buffer_t buffer, grub_uint8_t* value)
> 
> Again, s/grub_uint8_t* value/grub_uint8_t *value/. Please fix similar issues 
> everywhere...
> 
I'll check all patchs again.

> > +{
> > +  grub_uint32_t r = buffer->size - buffer->offset;
> > +
> > +  if (buffer->error)
> > +    return;
> > +
> > +  if (sizeof (*value) > r)
> > +    {
> > +      buffer->error = 1;
> > +      return;
> > +    }
> > +
> > +  grub_memcpy (value, &buffer->data[buffer->offset], sizeof (*value));
> > +  buffer->offset += sizeof (*value);
> > +}
> > +
> > +void
> > +grub_tpm2_buffer_unpack_u16 (grub_tpm2_buffer_t buffer, grub_uint16_t* 
> > value)
> > +{
> > +  grub_uint16_t tmp;
> > +  grub_uint32_t r = buffer->size - buffer->offset;
> > +
> > +  if (buffer->error)
> > +    return;
> > +
> > +  if (sizeof (tmp) > r)
> > +    {
> > +      buffer->error = 1;
> > +      return;
> > +    }
> > +
> > +  grub_memcpy (&tmp, &buffer->data[buffer->offset], sizeof (tmp));
> > +  buffer->offset += sizeof (tmp);
> > +  *value = grub_be_to_cpu16 (tmp);
> > +}
> > +
> > +void
> > +grub_tpm2_buffer_unpack_u32 (grub_tpm2_buffer_t buffer, grub_uint32_t* 
> > value)
> > +{
> > +  grub_uint32_t tmp;
> > +  grub_uint32_t r = buffer->size - buffer->offset;
> > +
> > +  if (buffer->error)
> > +    return;
> > +
> > +  if (sizeof (tmp) > r)
> > +    {
> > +      buffer->error = 1;
> > +      return;
> > +    }
> > +
> > +  grub_memcpy (&tmp, &buffer->data[buffer->offset], sizeof (tmp));
> > +  buffer->offset += sizeof (tmp);
> > +  *value = grub_be_to_cpu32 (tmp);
> > +}
> > diff --git a/grub-core/lib/tss2/tss2_buffer.h 
> > b/grub-core/lib/tss2/tss2_buffer.h
> > new file mode 100644
> > index 000000000..92648a1cb
> > --- /dev/null
> > +++ b/grub-core/lib/tss2/tss2_buffer.h
> > @@ -0,0 +1,66 @@
> > +/*
> > + *  GRUB  --  GRand Unified Bootloader
> > + *  Copyright (C) 2024 Free Software Foundation, Inc.
> > + *  Copyright (C) 2022 Microsoft Corporation
> 
> Again, wrong order...
> 
> > + *  GRUB is free software: you can redistribute it and/or modify
> > + *  it under the terms of the GNU General Public License as published by
> > + *  the Free Software Foundation, either version 3 of the License, or
> > + *  (at your option) any later version.
> > + *
> > + *  GRUB is distributed in the hope that it will be useful,
> > + *  but WITHOUT ANY WARRANTY; without even the implied warranty of
> > + *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> > + *  GNU General Public License for more details.
> > + *
> > + *  You should have received a copy of the GNU General Public License
> > + *  along with GRUB.  If not, see <http://www.gnu.org/licenses/>.
> > + */
> > +
> > +#ifndef GRUB_TPM2_BUFFER_HEADER
> > +#define GRUB_TPM2_BUFFER_HEADER 1
> > +
> > +#include <grub/types.h>
> > +
> > +#define GRUB_TPM2_BUFFER_CAPACITY 4096
> > +
> > +struct grub_tpm2_buffer
> > +{
> > +  grub_uint8_t data[GRUB_TPM2_BUFFER_CAPACITY];
> > +  grub_size_t size;
> > +  grub_size_t offset;
> > +  grub_size_t cap;
> > +  grub_uint8_t error;
> > +};
> > +typedef struct grub_tpm2_buffer *grub_tpm2_buffer_t;
> > +
> > +void
> > +grub_tpm2_buffer_init (grub_tpm2_buffer_t buffer);
> > +
> > +void
> > +grub_tpm2_buffer_pack (grub_tpm2_buffer_t buffer, const void* data,
> > +                  grub_size_t size);
> > +
> > +void
> > +grub_tpm2_buffer_pack_u8 (grub_tpm2_buffer_t buffer, grub_uint8_t value);
> > +
> > +void
> > +grub_tpm2_buffer_pack_u16 (grub_tpm2_buffer_t buffer, grub_uint16_t value);
> > +
> > +void
> > +grub_tpm2_buffer_pack_u32 (grub_tpm2_buffer_t buffer, grub_uint32_t value);
> > +
> > +void
> > +grub_tpm2_buffer_unpack (grub_tpm2_buffer_t buffer, void* data,
> > +                    grub_size_t size);
> > +
> > +void
> > +grub_tpm2_buffer_unpack_u8 (grub_tpm2_buffer_t buffer, grub_uint8_t* 
> > value);
> > +
> > +void
> > +grub_tpm2_buffer_unpack_u16 (grub_tpm2_buffer_t buffer, grub_uint16_t* 
> > value);
> > +
> > +void
> > +grub_tpm2_buffer_unpack_u32 (grub_tpm2_buffer_t buffer, grub_uint32_t* 
> > value);
> 
> Missing extern for all functions. Please fix it (in all patches).
> 
Will add extern to the tss2 functions.

> If you fix all these minor issues you can add my RB to the patch.
> 
Thanks,

Gary Lin