Hi Eddie,
On Thu, 2 Mar 2023 at 16:17, Ilias Apalodimas
<ilias.apalodi...@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
