Re: [PATCH v3 10/22] x86/tpm.c: code for early hashing and extending PCRs (for TPM1.2)

[Sergii Dmytruk <sergii.dmytruk@xxxxxxxxx>]

On Wed, Oct 22, 2025 at 04:07:35PM +0200, Jan Beulich wrote:
> > +void tpm_hash_extend(unsigned loc, unsigned pcr, const uint8_t *buf,
> > +                     unsigned size, uint32_t type, const uint8_t *log_data,
> > +                     unsigned log_data_size);
>
> Here and elsewhere: We prefer "unsigned int" over just "unsigned".

Will update.

> > +    while ( (tis_read8(TPM_ACCESS_(loc)) & ACCESS_ACTIVE_LOCALITY) == 0 );
>
> Here and below - please put such semicolons on a separate line, to make more
> recognizable that no loop body follows. Also generally we prefer ! over == 0
> for such checks.

OK.

> > +static void send_cmd(unsigned loc, uint8_t *buf, unsigned i_size,
> > +                     unsigned *o_size)
> > +{
> > +    /*
> > +     * Value of "data available" bit counts only when "valid" field is set 
> > as
> > +     * well.
> > +     */
> > +    const unsigned data_avail = STS_VALID | STS_DATA_AVAIL;
> > +
> > +    unsigned i;
> > +
> > +    /* Make sure TPM can accept a command. */
> > +    if ( (tis_read8(TPM_STS_(loc)) & STS_COMMAND_READY) == 0 )
> > +    {
> > +        /* Abort current command. */
> > +        tis_write8(TPM_STS_(loc), STS_COMMAND_READY);
> > +        /* Wait until TPM is ready for a new one. */
> > +        while ( (tis_read8(TPM_STS_(loc)) & STS_COMMAND_READY) == 0 );
> > +    }
> > +
> > +    for ( i = 0; i < i_size; i++ )
> > +        tis_write8(TPM_DATA_FIFO_(loc), buf[i]);
> > +
> > +    tis_write8(TPM_STS_(loc), STS_TPM_GO);
> > +
> > +    /* Wait for the first byte of response. */
> > +    while ( (tis_read8(TPM_STS_(loc)) & data_avail) != data_avail);
>
> Here you properly follow the comment at the top of the function, while ...
>
> > +    for ( i = 0; i < *o_size && tis_read8(TPM_STS_(loc)) & data_avail; i++ 
> > )
>
> ... here you don't. Why?

Right, `== data_avail` was missing here.  Likely a remnant from an
earlier version.

> > +        buf[i] = tis_read8(TPM_DATA_FIFO_(loc));
> > +
> > +    if ( i < *o_size )
> > +        *o_size = i;
>
> Is there any real need for this assignment to be conditional?

There isn't, as far as I can tell, will be unconditional.

> > +    uint32_t intf_version = tis_read32(TPM_INTF_CAPABILITY_(0))
> > +                          & INTF_VERSION_MASK;
>
> Nit: The & wants to move to the previous line and indentation of the
> continuation line wants to increase by 2.

OK.

> > +    cmd_rsp.start_c = (struct sha1_start_cmd) {
> > +        .h.tag = swap16(TPM_TAG_RQU_COMMAND),
> > +        .h.paramSize = swap32(sizeof(struct sha1_start_cmd)),
>
> While here it may be viewed as on the edge, ...
>
> > +        .h.ordinal = swap32(TPM_ORD_SHA1Start),
> > +    };
> > +
> > +    send_cmd(loc, cmd_rsp.buf, sizeof(struct sha1_start_cmd), &o_size);
>
> ... here and ...
>
> > +    if ( o_size < sizeof(struct sha1_start_rsp) )
>
> ... here (and elsewhere) you would better use sizeof(<expression>), to make
> the connection there more clear.

OK and I'll update send_cmd() to be like in TPM2.0 case which gets rid
of duplicated expressions.

> > +            max_bytes = size & ~(64 - 1);
>
> ROUNDDOWN(size, 64)

Right, didn't see this macro.

> > +        o_size = sizeof(cmd_rsp);
> > +
> > +        cmd_rsp.update_c = (struct sha1_update_cmd){
>
> Please, at least within a single patch, be consistent about whether there is
> a blank between ) and {.

Sure.

> > +error:
>
> Nit: Labels indented by at least one blank please.

OK, didn't see that note about `diff -p` before.

> > +    new_entry = (void *)(((uint8_t *)evt_log) + evt_log->NextEventOffset);
>
>    new_entry = (void *)evt_log + evt_log->NextEventOffset;
>

OK.

> > +    if ( evt_log->NextEventOffset + sizeof(struct TPM12_PCREvent) + 
> > data_size
> > +         > evt_log_size )
>
> Nit: Operator placement again.

OK.

> > +        return NULL;
> > +
> > +    evt_log->NextEventOffset += sizeof(struct TPM12_PCREvent) + data_size;
> > +
> > +    new_entry->PCRIndex = pcr;
> > +    new_entry->Type = type;
> > +    new_entry->Size = data_size;
> > +
> > +    if ( data && data_size > 0 )
> > +        memcpy(new_entry->Data, data, data_size);
>
> What about "data && !data_size" or "!data && data_size"? Are these legal
> inputs that are fine to ignore? Otherwise - why the if()?

Not every event has event data, so it can be NULL and/or of zero size.
Will state that in a comment.

> > +void tpm_hash_extend(unsigned loc, unsigned pcr, const uint8_t *buf,
> > +                     unsigned size, uint32_t type, const uint8_t *log_data,
> > +                     unsigned log_data_size)
> > +{
> > +    void *evt_log_addr;
> > +    uint32_t evt_log_size;
> > +
> > +    find_evt_log(slaunch_get_slrt(), &evt_log_addr, &evt_log_size);
> > +    evt_log_addr = __va((uintptr_t)evt_log_addr);
> > +
> > +    if ( is_tpm12() )
> > +    {
> > +        uint8_t sha1_digest[SHA1_DIGEST_SIZE];
> > +
> > +        struct txt_ev_log_container_12 *evt_log = evt_log_addr;
> > +        void *entry_digest = create_log_event12(evt_log, evt_log_size, pcr,
> > +                                                type, log_data, 
> > log_data_size);
> > +
> > +        /* We still need to write computed hash somewhere. */
> > +        if ( entry_digest == NULL )
> > +            entry_digest = sha1_digest;
> > +
> > +        if ( !tpm12_hash_extend(loc, buf, size, pcr, entry_digest) )
> > +        {
> > +#ifndef __EARLY_SLAUNCH__
> > +            printk(XENLOG_ERR "Extending PCR%u failed\n", pcr);
> > +#endif
> > +        }
> > +    }
>
> And implicitly "else { ignore everything }"?

Yes, but the next version of the patch will return an error instead.

> > +#ifdef __EARLY_SLAUNCH__
> > +void asmlinkage tpm_extend_mbi(uint32_t *mbi, uint32_t slrt_pa)
>
> Pointer to const please, and then make sure ...
>
> > +{
> > +    /* Need this to implement slaunch_get_slrt() for early TPM code. */
> > +    slrt_location = slrt_pa;
> > +
> > +    /* MBI starts with uint32_t total_size. */
> > +    tpm_hash_extend(DRTM_LOC, DRTM_DATA_PCR, (uint8_t *)mbi, *mbi,
>
> ... not to cast away const-ness here.
>
> Jan

Will do.

Regards,
Sergii