[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH v18 14/25] tss2: Add TPM2 types and Marshal/Unmarshal functio
|
From: |
Gary Lin |
|
Subject: |
Re: [PATCH v18 14/25] tss2: Add TPM2 types and Marshal/Unmarshal functions |
|
Date: |
Tue, 27 Aug 2024 14:22:02 +0800 |
On Thu, Aug 22, 2024 at 04:03:35PM +0200, Daniel Kiper wrote:
> 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...
>
Sorry for that. I've revised the whole patch set to fix the 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...
>
Sure.
> > +{
> > + 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...
>
Got it.
> > + 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.
>
Ok. I've spotted the similar usage in several MU functions. Will fix
them in the next version.
> > + 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...
>
Ok.
> > + }
> > + 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/
>
Ok.
> > + 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...
>
> [...]
>
Will add externs to all tss2 functions in the headers.
> > 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.
>
I overlooked the comment. It's all renamed in my WIP branch.
> [...]
>
> > +/* _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...
>
That name is actually from the TPM2 SPEC and the intel-tss2 library
follows the naming. Anyway, the '_PRIVATE' struct is not used by any
TPM2 command, so it's totally fine to rename the struct without leading
to any confusion.
> > +
> > +/* 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.
>
Thanks.
Gary Lin