Re: [PATCH v18 14/25] tss2: Add TPM2 types and Marshal/Unmarshal functions

[Daniel Kiper]

On Fri, Jun 28, 2024 at 04:18:57PM +0800, Gary Lin via Grub-devel wrote:
> This commit adds the necessary TPM2 types and structs as the preparation
> for the TPM2 Software Stack (TSS2) support. The Marshal/Unmarshal
> functions are also added to handle the data structure to be submitted to
> TPM2 commands and to be received 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/tss2_mu.c      | 1170 +++++++++++++++++++++++++++++
>  grub-core/lib/tss2/tss2_mu.h      |  397 ++++++++++
>  grub-core/lib/tss2/tss2_structs.h |  768 +++++++++++++++++++
>  grub-core/lib/tss2/tss2_types.h   |  404 ++++++++++
>  4 files changed, 2739 insertions(+)
>  create mode 100644 grub-core/lib/tss2/tss2_mu.c
>  create mode 100644 grub-core/lib/tss2/tss2_mu.h
>  create mode 100644 grub-core/lib/tss2/tss2_structs.h
>  create mode 100644 grub-core/lib/tss2/tss2_types.h
>
> diff --git a/grub-core/lib/tss2/tss2_mu.c b/grub-core/lib/tss2/tss2_mu.c
> new file mode 100644
> index 000000000..af7a75266
> --- /dev/null
> +++ b/grub-core/lib/tss2/tss2_mu.c
> @@ -0,0 +1,1170 @@
> +/*
> + *  GRUB  --  GRand Unified Bootloader
> + *  Copyright (C) 2024 Free Software Foundation, Inc.
> + *  Copyright (C) 2022 Microsoft Corporation

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/>.
> + */
> +
> +#include <grub/misc.h>
> +
> +#include <tss2_mu.h>
> +
> +void
> +grub_Tss2_MU_TPMS_AUTH_COMMAND_Marshal (grub_tpm2_buffer_t buffer,
> +                                     const TPMS_AUTH_COMMAND* authCommand)

s/const TPMS_AUTH_COMMAND*/ const TPMS_AUTH_COMMAND *authCommand/

As I can see this can be wrong in the whole patch set. Please fix it all over
the place...

> +{
> +  grub_uint32_t start;
> +  grub_uint32_t tmp;
> +
> +  grub_tpm2_buffer_pack_u32 (buffer, 0);
> +  start = buffer->size;
> +
> +  grub_tpm2_buffer_pack_u32 (buffer, authCommand->sessionHandle);
> +
> +  grub_tpm2_buffer_pack_u16 (buffer, authCommand->nonce.size);
> +  grub_tpm2_buffer_pack (buffer, authCommand->nonce.buffer,
> +                      authCommand->nonce.size);
> +
> +  grub_tpm2_buffer_pack_u8 (buffer,
> +                         *((const grub_uint8_t*) 
> &authCommand->sessionAttributes));

s/const grub_uint8_t*/const grub_uint8_t */

Similar problem to the above. It has to be fixes...

> +  grub_tpm2_buffer_pack_u16 (buffer, authCommand->hmac.size);
> +  grub_tpm2_buffer_pack (buffer, authCommand->hmac.buffer,
> +                      authCommand->hmac.size);
> +
> +  tmp = grub_cpu_to_be32 (buffer->size - start);
> +  grub_memcpy (&buffer->data[start - sizeof (grub_uint32_t)], &tmp,
> +            sizeof (tmp));
> +}
> +
> +void
> +grub_Tss2_MU_TPM2B_Marshal (grub_tpm2_buffer_t buffer,
> +                         const grub_uint16_t size,
> +                         const grub_uint8_t* b)
> +{
> +  grub_tpm2_buffer_pack_u16 (buffer, size);
> +
> +  for (grub_uint16_t i = 0; i < size; i++)
> +    grub_tpm2_buffer_pack_u8 (buffer, b[i]);
> +}
> +
> +void
> +grub_Tss2_MU_TPMU_SYM_KEY_BITS_Marshal (grub_tpm2_buffer_t buffer,
> +                                     const TPMI_ALG_SYM_OBJECT algorithm,
> +                                     const TPMU_SYM_KEY_BITS *p)

But here it is correct. Please be consistent...

> +{
> +  switch (algorithm)
> +    {
> +    case TPM_ALG_AES:
> +    case TPM_ALG_SM4:
> +    case TPM_ALG_CAMELLIA:
> +    case TPM_ALG_XOR:
> +      grub_tpm2_buffer_pack_u16 (buffer, *((const grub_uint16_t*) p));
> +      break;
> +    case TPM_ALG_NULL:
> +      break;
> +    default:
> +      buffer->error = 1;
> +      break;
> +    }
> +}
> +
> +void
> +grub_Tss2_MU_TPMU_SYM_MODE_Marshal (grub_tpm2_buffer_t buffer,
> +                                 const TPMI_ALG_SYM_OBJECT algorithm,
> +                                 const TPMU_SYM_MODE *p)
> +{
> +  switch (algorithm)
> +    {
> +    case TPM_ALG_AES:
> +    case TPM_ALG_SM4:
> +    case TPM_ALG_CAMELLIA:
> +      grub_tpm2_buffer_pack_u16 (buffer, *((const grub_uint16_t*) p));
> +      break;
> +    case TPM_ALG_XOR:
> +    case TPM_ALG_NULL:
> +      break;
> +    default:
> +      buffer->error = 1;
> +      break;
> +    }
> +}
> +
> +void
> +grub_Tss2_MU_TPMT_SYM_DEF_Marshal (grub_tpm2_buffer_t buffer,
> +                                const TPMT_SYM_DEF *p)
> +{
> +  grub_tpm2_buffer_pack_u16 (buffer, p->algorithm);
> +  grub_Tss2_MU_TPMU_SYM_KEY_BITS_Marshal (buffer, p->algorithm, &p->keyBits);
> +  grub_Tss2_MU_TPMU_SYM_MODE_Marshal (buffer, p->algorithm, &p->mode);
> +}
> +
> +void
> +grub_Tss2_MU_TPMS_PCR_SELECTION_Marshal (grub_tpm2_buffer_t buffer,
> +                                      const TPMS_PCR_SELECTION* pcrSelection)
> +{
> +  grub_tpm2_buffer_pack_u16 (buffer, pcrSelection->hash);
> +  grub_tpm2_buffer_pack_u8 (buffer, pcrSelection->sizeOfSelect);
> +
> +  for (grub_uint32_t i = 0; i < pcrSelection->sizeOfSelect; i++)
> +    grub_tpm2_buffer_pack_u8 (buffer, pcrSelection->pcrSelect[i]);
> +}
> +
> +void
> +grub_Tss2_MU_TPML_PCR_SELECTION_Marshal (grub_tpm2_buffer_t buffer,
> +                                      const TPML_PCR_SELECTION* pcrSelection)
> +{
> +  grub_tpm2_buffer_pack_u32 (buffer, pcrSelection->count);
> +
> +  for (grub_uint32_t i = 0; i < pcrSelection->count; i++)

Even if it works do not define variables in that way. Please do that at
the beginning of a function.

> +    grub_Tss2_MU_TPMS_PCR_SELECTION_Marshal (buffer,
> +                                          &pcrSelection->pcrSelections[i]);
> +}

[...]

> +void
> +grub_Tss2_MU_TPM2B_PUBLIC_Marshal (grub_tpm2_buffer_t buffer,
> +                                const TPM2B_PUBLIC *p)
> +{
> +  grub_uint32_t start;
> +  grub_uint16_t size;
> +
> +  if (p)
> +    {
> +      grub_tpm2_buffer_pack_u16 (buffer, p->size);
> +
> +      start = buffer->size;
> +      grub_Tss2_MU_TPMT_PUBLIC_Marshal (buffer, &p->publicArea);
> +      size = grub_cpu_to_be16 (buffer->size - start);
> +      grub_memcpy (&buffer->data[start - sizeof (grub_uint16_t)], &size,
> +                sizeof (size));

Please reduce wrapping further where possible. And it is still possible
in many places...

> +    }
> +  else
> +    grub_tpm2_buffer_pack_u16 (buffer, 0);
> +}

[...]

> +void
> +grub_Tss2_MU_TPMU_SIGNATURE_Marshal (grub_tpm2_buffer_t buffer,
> +                                     const TPMI_ALG_SIG_SCHEME sigAlg,
> +                                     const TPMU_SIGNATURE *p)
> +{
> +  switch (sigAlg)
> +    {
> +    case TPM_ALG_RSASSA:
> +      grub_Tss2_MU_TPMS_SIGNATURE_RSA_Marshal (buffer, (TPMS_SIGNATURE_RSA 
> *)&p->rsassa);

Missing space in cast...

s/(TPMS_SIGNATURE_RSA *)&p->rsassa/(TPMS_SIGNATURE_RSA *) &p->rsassa/

> +      break;
> +    case TPM_ALG_RSAPSS:
> +      grub_Tss2_MU_TPMS_SIGNATURE_RSA_Marshal (buffer, (TPMS_SIGNATURE_RSA 
> *)&p->rsapss);

Ditto and below...

> +      break;
> +    case TPM_ALG_ECDSA:
> +      grub_Tss2_MU_TPMS_SIGNATURE_ECC_Marshal (buffer, (TPMS_SIGNATURE_ECC 
> *)&p->ecdsa);
> +      break;
> +    case TPM_ALG_ECDAA:
> +      grub_Tss2_MU_TPMS_SIGNATURE_ECC_Marshal (buffer, (TPMS_SIGNATURE_ECC 
> *)&p->ecdaa);
> +      break;
> +    case TPM_ALG_SM2:
> +      grub_Tss2_MU_TPMS_SIGNATURE_ECC_Marshal (buffer, (TPMS_SIGNATURE_ECC 
> *)&p->sm2);
> +      break;
> +    case TPM_ALG_ECSCHNORR:
> +      grub_Tss2_MU_TPMS_SIGNATURE_ECC_Marshal (buffer, (TPMS_SIGNATURE_ECC 
> *)&p->ecschnorr);
> +      break;
> +    case TPM_ALG_HMAC:
> +      grub_Tss2_MU_TPMT_HA_Marshal (buffer, &p->hmac);
> +      break;
> +    case TPM_ALG_NULL:
> +      break;
> +    default:
> +      buffer->error = 1;
> +      break;
> +    }
> +}

[...]

> diff --git a/grub-core/lib/tss2/tss2_mu.h b/grub-core/lib/tss2/tss2_mu.h
> new file mode 100644
> index 000000000..041ee9956
> --- /dev/null
> +++ b/grub-core/lib/tss2/tss2_mu.h
> @@ -0,0 +1,397 @@
> +/*
> + *  GRUB  --  GRand Unified Bootloader
> + *  Copyright (C) 2024 Free Software Foundation, Inc.
> + *  Copyright (C) 2022 Microsoft Corporation

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_MU_HEADER
> +#define GRUB_TPM2_MU_HEADER 1
> +
> +#include <tss2_buffer.h>
> +#include <tss2_structs.h>
> +
> +void
> +grub_Tss2_MU_TPMS_AUTH_COMMAND_Marshal (grub_tpm2_buffer_t buffer,
> +                                     const TPMS_AUTH_COMMAND* authCommand);

Missing externs...

[...]

> diff --git a/grub-core/lib/tss2/tss2_structs.h 
> b/grub-core/lib/tss2/tss2_structs.h
> new file mode 100644
> index 000000000..9f9dc3d92
> --- /dev/null
> +++ b/grub-core/lib/tss2/tss2_structs.h
> @@ -0,0 +1,768 @@
> +/*
> + *  GRUB  --  GRand Unified Bootloader
> + *  Copyright (C) 2024 Free Software Foundation, Inc.
> + *  Copyright (C) 2022 Microsoft Corporation

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_INTERNAL_STRUCTS_HEADER
> +#define GRUB_TPM2_INTERNAL_STRUCTS_HEADER 1
> +
> +#include <tss2_types.h>
> +
> +/* TPMS_TAGGED_PROPERTY Structure */
> +struct TPMS_TAGGED_PROPERTY
> +{
> +  TPM_PT property;
> +  grub_uint32_t value;
> +};
> +typedef struct TPMS_TAGGED_PROPERTY TPMS_TAGGED_PROPERTY;

Please add "_t" to typedefs, i.e.
  typedef struct TPMS_TAGGED_PROPERTY TPMS_TAGGED_PROPERTY_t.

I think I asked for this last time.

[...]

> +/* _PRIVATE Structure */
> +struct _PRIVATE
> +{
> +  TPM2B_DIGEST integrityOuter;
> +  TPM2B_DIGEST integrityInner;
> +  TPM2B_SENSITIVE sensitive;
> +};
> +typedef struct _PRIVATE _PRIVATE;

I would not use so generic name...

s/_PRIVATE/__TPM2B_PRIVATE/? At least...

> +
> +/* TPM2B_PRIVATE Structure */
> +struct TPM2B_PRIVATE
> +{
> +  grub_uint16_t size;
> +  grub_uint8_t buffer[sizeof(_PRIVATE)];
> +};
> +typedef struct TPM2B_PRIVATE TPM2B_PRIVATE;

Feel free to add my RB when you fix all these minor issues.

Daniel