[PATCH v7 3/6] tpm: Support boot measurements

[Ilias Apalodimas]

Hi Eddie,

On Thu, 2 Mar 2023 at 16:17, Ilias Apalodimas
<ilias.apalodimas at linaro.org> wrote:
>
> Hi Eddie,
>
> The good news, is that this generally seems to be working and is really
> close to what I had in mind on code re-usage.  Thanks for the patience!
>
> The bad new now is that I think I found one last (famous last words)
> problem
>
> [...]
>
> > +     }
> > +
> > +     /* Read PCR0 to check if previous firmware extended the PCRs or not. */
> > +     rc = tcg2_pcr_read(dev, 0, &digest_list);
> > +     if (rc)
> > +             return rc;
> > +
>
> This is changing how the code used to work and I think the new way of doing
> it is wrong.
> First of all the check above doesn't check that PCR0 is indeed 0.  It
> simply checks we can *read* that PCR.
>
> > +     for (i = 0; i < digest_list.count; ++i) {
> > +             len = tpm2_algorithm_to_len(digest_list.digests[i].hash_alg);
> > +             for (j = 0; j < len; ++j) {
> > +                     if (digest_list.digests[i].digest.sha512[j])
> > +                             break;
> > +             }
> > +
> > +             /* PCR is non-zero; it has been extended, so skip extending. */
>
> I don't think we need this tbh.  The previous bootloader would have either
> extended some of the PCRs along with the EventLog construction or he hasn't.
> If it did indeed extend the PCRs then PCR0 should be != 0 since it must
> contain a measurement of EV_S_CRTM_VERSION. So looking at PCR0 should be
> sufficient to trigger replaying the EventLog or not.
> If the previous loader managed to mess up somehow,  I don't think it should
> be U-Boot's job to fix the mess :)

I actually misread the patch here.  This is indeed only checking PCR0.
I'll go check why one event is missing and get back to you.

>
> > +             if (j != len) {
> > +                     digest_list.count = 0;
> > +                     break;
> > +             }
> > +     }
> > +
> > +     elog->log_position = offsetof(struct tcg_pcr_event, event) + evsz;
> > +     rc = tcg2_log_find_end(elog, dev, &digest_list);
> > +     if (rc)
> > +             return rc;
> > +
> > +     elog->found = true;
> > +     return 0;
> > +}
> > +
>
> P.S: I did test this using TF-A and re-using the 'forwarded' EventLog.  I
> can see all the events replayed correctly apart from the last one, so i'll
> keep looking in case something else is missing
>
> Regards
> /Ilias