Re: [SeaBIOS] [PATCH 3/3] tpm: Extend tpm20_extend to support extending to multiple PCR banks

[Kevin O'Connor]

On Tue, Jul 19, 2016 at 01:41:56PM -0400, Stefan Berger wrote:
> Extend the tpm20_extend function to support extending a hash to
> multiple PCR banks. The sha1 hash that's being extended into the
> sha256 bank for example, will be filled with zero-bytes to the
> size of a sha256 hash.
> Signed-off-by: Stefan Berger
>
> src/std/tcg.h | 14 +++++++--
>  src/tcgbios.c | 99 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++---
>  2 files changed, 105 insertions(+), 8 deletions(-)
>
> diff --git a/src/std/tcg.h b/src/std/tcg.h
> index 1644684..9e4ef57 100644
> --- a/src/std/tcg.h
> +++ b/src/std/tcg.h
> @@ -100,6 +100,10 @@ enum irq_ids {
>  #define EV_IPL_PARTITION_DATA   14
>
> #define SHA1_BUFSIZE                20
> +#define SHA256_BUFSIZE              32
> +#define SHA384_BUFSIZE              48
> +#define SHA512_BUFSIZE              64
> +#define SM3_256_BUFSIZE             32
>
> /* Input and Output blocks for the TCG BIOS commands */
>
> @@ -381,6 +385,10 @@ struct tpm_res_sha1complete {
>  #define TPM2_RH_PLATFORM            0x4000000c
>
> #define TPM2_ALG_SHA1               0x0004
> +#define TPM2_ALG_SHA256             0x000b
> +#define TPM2_ALG_SHA384             0x000c
> +#define TPM2_ALG_SHA512             0x000d
> +#define TPM2_ALG_SM3_256            0x0012
>
> /* TPM 2 command tags */
>  #define TPM2_ST_NO_SESSIONS         0x8001
> @@ -442,8 +450,8 @@ struct tpm2_req_hierarchychangeauth {
>  } PACKED;
>
> struct tpm2_digest_value {
> u16 hashalg; /* TPM2_ALG_SHA1 */
> u8 sha1[SHA1_BUFSIZE];
> u16 hashAlg; /* TPM2_ALG_SHA1 */
> u8 hash[0]; /* size depends on hashAlg */
>
> } PACKED;
>
> struct tpm2_req_extend {
> @@ -452,7 +460,7 @@ struct tpm2_req_extend {
>      u32 authblocksize;
>      struct tpm2_authblock authblock;
>      u32 count;
> struct tpm2_digest_value digest;
> struct tpm2_digest_value digest[0];
>
> } PACKED;
>
> struct tpm2_req_clearcontrol {
> diff --git a/src/tcgbios.c b/src/tcgbios.c
> index 72ae3c6..acd0b71 100644
> --- a/src/tcgbios.c
> +++ b/src/tcgbios.c
> @@ -446,11 +446,94 @@ tpm12_extend(u32 pcrindex, const u8 *digest)
>      return 0;
>  }
>
> +static int
> +tpm20_get_hash_buffersize(u16 hashAlg)
> +{
> switch (hashAlg) {
> case TPM2_ALG_SHA1:
>    return SHA1_BUFSIZE;
> case TPM2_ALG_SHA256:
>    return SHA256_BUFSIZE;
> case TPM2_ALG_SHA384:
>    return SHA384_BUFSIZE;
> case TPM2_ALG_SHA512:
>    return SHA512_BUFSIZE;
> case TPM2_ALG_SM3_256:
>    return SM3_256_BUFSIZE;
> default:
>    return -1;
> }
>
> +}
>
> +/*
> Write the TPM2 PCR Extend 'body' containing the hashes. Follow
> the PCR bank configuration of the TPM.
> dest: destinatiobn buffer to write into; if NULL, nothing is written

Spelling typo.
> pcrindex: the PCR index
> hash: the hash value to write
> hashAlg: the hash alogorithm determining the size of the hash
> count: pointer to a counter for how many entries were writte
> Returns the number of bytes written into the buffer; -1 on fatal error
> */
>
> +static int
> +tpm20_write_extend_body(char *dest, u32 pcrindex, const u8 *hash,
>                    u16 hashAlg, u32 *count)
>
> +{
> int offset = 0, hsize;
> struct tpms_pcr_selection *sel = tpm20_pcr_selection->selections;
> void *nsel, *end = (void *)tpm20_pcr_selection + tpm20_pcr_selection_size;
> int hash_size = tpm20_get_hash_buffersize(hashAlg);
> for (*count = 0;
>     *count < be32_to_cpu(tpm20_pcr_selection->count);
>     (*count)++) {
>    u8 sizeOfSelect = sel->sizeOfSelect;
>    nsel = (void *)sel +
>           offsetof(struct tpms_pcr_selection, pcrSelect) +
>           sizeOfSelect;
>    if (nsel > end)
>        break;
>    u8 idx = pcrindex >> 3;
>    u8 mask = (1 << (pcrindex & 7));
>    /* skip if PCR not used in this bank */
>    if (idx > sizeOfSelect || !(sel->pcrSelect[idx] & mask))
>        continue;
>    hsize = tpm20_get_hash_buffersize(be16_to_cpu(sel->hashAlg));
>    if (hsize < 0) {
>        dprintf(DEBUG_tcg, "TPM is using an unsupported hash: %d\n",
>                be16_to_cpu(sel->hashAlg));
>        return -1;
>    }
>    if (dest) {
>        struct tpm2_digest_value *v =
>          (struct tpm2_digest_value *)&dest[offset];
>        v->hashAlg = sel->hashAlg;
>        memset(v->hash, 0, hsize);
>        memcpy(v->hash, hash, hash_size);
>    }
>    offset += sizeof(sel->hashAlg) + hsize;
>    sel = nsel;
> }
> if (sel != end) {
>    dprintf(DEBUG_tcg, "Malformed pcr selection structure fron TPM\n");
>    return -1;
> }
> return offset;
>
> +}
>
> static int tpm20_extend(u32 pcrindex, const u8 *digest, u16 hashAlg)
>  {
>      struct tpm2_req_extend tmp_tre = {
>          .hdr.tag     = cpu_to_be16(TPM2_ST_SESSIONS),
>    .hdr.totlen  = cpu_to_be32(sizeof(tmp_tre)),
>    .hdr.totlen  = cpu_to_be32(0),
>    .hdr.ordinal = cpu_to_be32(TPM2_CC_PCR_Extend),
>    .pcrindex    = cpu_to_be32(pcrindex),
>    .authblocksize = cpu_to_be32(sizeof(tmp_tre.authblock)),
>
> @@ -461,14 +544,20 @@ static int tpm20_extend(u32 pcrindex, const u8 *digest, u16 hashAlg)
>              .pwdsize = cpu_to_be16(0),
>          },
>      };
> u32 count = 1;
> u8 buffer[sizeof(tmp_tre) + sizeof(struct tpm2_digest_value)];
> u32 count = 0;
> int bsize = tpm20_write_extend_body(NULL, pcrindex, digest, hashAlg,
>                                    &count);
> if (bsize < 0)
>    return bsize;
> u8 buffer[sizeof(tmp_tre) + bsize];

Is it possible to allocate buffer to be some suitably large size (like
512 bytes) instead of trying to calculate an exact size?  SeaBIOS
uses static stack sizes (typically 2K) and it's harder to analyze
stack usage when buffers are dynamically sized.
>  struct tpm2_req_extend *tre = (struct tpm2_req_extend *)buffer;
>
>  memcpy(tre, &tmp_tre, sizeof(tmp_tre));
> tre->hdr.totlen = cpu_to_be32(sizeof(buffer));
> tpm20_write_extend_body((char *)&tre->digest, pcrindex, digest, hashAlg,
>                        &count);
> tre->count = cpu_to_be32(count);

Isn't count always equal to tpm20_pcr_selection->count?  If so, why
pass it to/from tpm20_write_extend_body()?

Series looks good to me.

Thanks,
-Kevin