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

[Ard Biesheuvel]

On Fri, 13 Sept 2024 at 15:44, Ard Biesheuvel <ardb@xxxxxxxxxx> wrote:
>
> On Fri, 13 Sept 2024 at 15:39, Ilias Apalodimas
> <ilias.apalodimas@xxxxxxxxxx> wrote:
> >
> > On Fri, 13 Sept 2024 at 15:57, Gregory Price <gourry@xxxxxxxxxx> wrote:
> > >
> > > 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.
> >
> > Ok, that explains why it wasn't caught earlier at least. I would
> > prefer dropping it tbh, but I am going to defer to Ard for that.
> >
> > If we agree that this needs to go in btw, I think you should refactor
> > it a bit. That function already defines an out: label, which unmaps
> > memory. So you can rewrite the above as
> >
> > If(....) {
> >     ret = -EINVAL;
> >     efi.tpm_log = EFI_INVALID_TABLE_ADDR;
> >    goto out;
> > }
> >
>
> Validating a table that was created by the EFI stub seems redundant.
> If the version check needs to be tightened, please do so in
> efi_retrieve_tcg2_eventlog() (in the stub).

... and actually, this version is set by the EFI stub based on which
flavor of the TCG protocols it found.

So i don't think we need this check to begin with.

If we need to detect corruption of these tables, I'd prefer to add a
checksum or something like that. But I don't think we should bother.