Re: [PATCH 4/6] tpm: sanity check the log version before using it

[Gregory Price]

On Fri, Sep 13, 2024 at 09:40:30AM +0300, Ilias Apalodimas wrote:
> Hi Gregory,
> 
> On Fri, 6 Sept 2024 at 23:28, Gregory Price <gourry@xxxxxxxxxx> wrote:
> >
> > If the log version is not sane (0 or >2), don't attempt to use
> > the rest of the log values for anything to avoid potential corruption.
> >
> > Signed-off-by: Gregory Price <gourry@xxxxxxxxxx>
> > ---
> >  drivers/firmware/efi/tpm.c | 9 +++++++++
> >  1 file changed, 9 insertions(+)
> >
> > diff --git a/drivers/firmware/efi/tpm.c b/drivers/firmware/efi/tpm.c
> > index 6e03eed0dc6f..9a080887a3e0 100644
> > --- a/drivers/firmware/efi/tpm.c
> > +++ b/drivers/firmware/efi/tpm.c
> > @@ -60,6 +60,15 @@ int __init efi_tpm_eventlog_init(void)
> >                 return -ENOMEM;
> >         }
> >
> > +       if (!log_tbl->version ||
> > +           log_tbl->version > EFI_TCG2_EVENT_LOG_FORMAT_TCG_2) {
> > +               pr_err(FW_BUG "TPM Events table version invalid (%x)\n",
> > +                      log_tbl->version);
> > +               early_memunmap(log_tbl, sizeof(*log_tbl));
> > +               efi.tpm_log = EFI_INVALID_TABLE_ADDR;
> > +               return -EINVAL;
> 
> I don't think we need this check at all. Did you actually see this happening?
> efi_retrieve_eventlog() that runs during the efistub tries to retrieve
> the log and the EFI protocol itself explicitly says that the firmware
> *must* return EFI_INVALID_PARAMETER if the event log is not in 1.2 or
> 2.0 format. If the firmware does something wrong, we should report the
> FW BUG in that function, instead of installing the config tables Linux
> uses internally to handover the log and catching it late.
> 
> Thanks
> /Ilias
>

We saw this happen and discovered it was a disagreement between EFI/OS/kexec
causing the table to be overwritten during kexec.  We've since found a fix for
that.  So the result was that it appeared the firmware was doing something
wrong. The sanity check at least allowed us to boot without immediately
crashing - because the tables don't get reinstalled, they get re-used
(at least that's by best understanding of the whole interaction).

If the check seems superfluous, i can drop it.
 
> 
> 
> > +       }
> > +
> >         tbl_size = sizeof(*log_tbl) + log_tbl->size;
> >         if (memblock_reserve(efi.tpm_log, tbl_size)) {
> >                 pr_err("TPM Event Log memblock reserve fails (0x%lx, 0x%x)\n",
> > --
> > 2.43.0
> >