Re: [PATCH v3 11/22] x86/tpm.c: support extending PCRs of TPM2.0

[Sergii Dmytruk <sergii.dmytruk@xxxxxxxxx>]

On Wed, Oct 22, 2025 at 05:13:26PM +0200, Jan Beulich wrote:
> > -/****************************** TPM1.2 specific 
> > *******************************/
> > -#define TPM_ORD_Extend              0x00000014
> > -#define TPM_ORD_SHA1Start           0x000000A0
> > -#define TPM_ORD_SHA1Update          0x000000A1
> > -#define TPM_ORD_SHA1CompleteExtend  0x000000A3
> > +/****************************** TPM1.2 & TPM2.0 
> > *******************************/
> >
> > -#define TPM_TAG_RQU_COMMAND         0x00C1
> > -#define TPM_TAG_RSP_COMMAND         0x00C4
> > +/*
> > + * TPM1.2 is required to support commands of up to 1101 bytes, vendors 
> > rarely
> > + * go above that. Limit maximum size of block of data to be hashed to 1024.
> > + *
> > + * TPM2.0 should support hashing of at least 1024 bytes.
> > + */
> > +#define MAX_HASH_BLOCK      1024
> >
> >  /* All fields of following structs are big endian. */
> >  struct tpm_cmd_hdr {
> > @@ -168,6 +179,17 @@ struct tpm_rsp_hdr {
> >      uint32_t    returnCode;
> >  } __packed;
> >
> > +/****************************** TPM1.2 specific 
> > *******************************/
> > +
> > +#define TPM_ORD_Extend              0x00000014
> > +#define TPM_ORD_SHA1Start           0x000000A0
> > +#define TPM_ORD_SHA1Update          0x000000A1
> > +#define TPM_ORD_SHA1CompleteExtend  0x000000A3
> > +
> > +#define TPM_TAG_RQU_COMMAND         0x00C1
> > +#define TPM_TAG_RSP_COMMAND         0x00C4
> > +
> > +/* All fields of following structs are big endian. */
> >  struct extend_cmd {
> >      struct tpm_cmd_hdr h;
> >      uint32_t pcrNum;
>
> Can the previous patch please put these right in their final resting place?

Some earlier comment of yours requested separate headers for these
definitions, so they aren't moved anymore.

> > +#define PUT_BYTES(p, bytes, size)  do {  \
> > +        memcpy((p), (bytes), (size));    \
>
> Preferably without the excess parentheses, much like you have it ...
>
> > +        (p) += (size);                   \
> > +    } while ( 0 )
> > +
> > +#define PUT_16BIT(p, data) do {          \
> > +        *(uint16_t *)(p) = swap16(data); \
>
> ... e.g. in the function call here.
>
> > +        (p) += 2;                        \
> > +    } while ( 0 )

OK, just tend to always parenthesise parameters in macros.

> > +    cmd_rsp.finish_c = (struct tpm2_sequence_complete_cmd) {
> > +        .h.tag = swap16(TPM_ST_SESSIONS),
> > +        .h.paramSize = swap32(sizeof(cmd_rsp.finish_c) + size),
> > +        .h.ordinal = swap32(TPM2_PCR_EventSequenceComplete),
> > +        .pcrHandle = swap32(HR_PCR + pcr),
> > +        .sequenceHandle = swap32(seq_handle),
> > +        .sessionHdrSize = swap32(sizeof(struct tpm2_session_header)*2),
>
> Why *2? Where to the two session headers go? (Also nit: blanks missing around 
> *.)
>
> > +        .pcrSession.handle = swap32(TPM_RS_PW),
> > +        .sequenceSession.handle = swap32(TPM_RS_PW),
> > +        .dataSize = swap16(size),
> > +    };

Because TPM2_PCR_EventSequenceComplete command has two sessions filled directly
below in .pcrSession and .sequenceSession fields.  Will fix the spacing.

> > +static uint32_t tpm2_hash_extend(unsigned loc, const uint8_t *buf,
> > +                                 unsigned size, unsigned pcr,
> > +                                 const struct tpm2_log_hashes *log_hashes)
> > +{
> > +    uint32_t rc;
> > +    unsigned i;
> > +    struct tpm2_log_hashes supported_hashes = {0};
> > +
> > +    request_locality(loc);
> > +
> > +    for ( i = 0; i < log_hashes->count; ++i )
> > +    {
> > +        const struct tpm2_log_hash *hash = &log_hashes->hashes[i];
> > +        if ( !tpm_supports_hash(loc, hash) )
> > +        {
> > +            printk(XENLOG_WARNING "Skipped hash unsupported by TPM: %d\n",
> > +                   hash->alg);
> > +            continue;
> > +        }
> > +
> > +        if ( hash->alg == TPM_ALG_SHA1 )
> > +        {
> > +            sha1_hash(hash->data, buf, size);
> > +        }
> > +        else if ( hash->alg == TPM_ALG_SHA256 )
> > +        {
> > +            sha2_256_digest(hash->data, buf, size);
> > +        }
> > +        else
>
> Is this really just "else", not "else if ( ... )"?
>
> > +        {
> > +            /* This is called "OneDigest" in TXT Software Development 
> > Guide. */
> > +            memset(hash->data, 0, size);
> > +            hash->data[0] = 1;
> > +        }

Yes, only these two algorithms are supported, others are expected to
have some fake values (the next version won't do anything in the
else-branch, leaving that to the caller).

> > +        if ( supported_hashes.count == MAX_HASH_COUNT )
> > +        {
> > +            printk(XENLOG_ERR "Hit hash count implementation limit: %d\n",
> > +                   MAX_HASH_COUNT);
> > +            return -1;
>
> This is an odd return value for a function returning uint32_t. And it's also 
> ...

Will `#define TPM_INTERNAL_ERROR 0xffffffffU`.  TPM only uses the lower
12 bits of UINT32, so there is no ambiguity.

> > +
> > +        rc = tpm2_hash_extend(loc, buf, size, pcr, &log_hashes);
> > +        if ( rc != 0 )
> > +        {
> > +#ifndef __EARLY_SLAUNCH__
> > +            printk(XENLOG_ERR "Extending PCR%u failed with TPM error: 
> > 0x%08x\n",
> > +                   pcr, rc);
>
> ... not exactly a TPM error.

Will s/TPM/an/.

Regards,
Sergii