linux-kernel - Re: [PATCH 6/6] libstub,tpm: do not ignore failure case when reading final event log

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAMj1kXHEH5PE=7swKMJUP-gTAuyoJH5E8NL+RuFnMceF=MpoWQ@mail.gmail.com>
Date: Fri, 13 Sep 2024 17:59:17 +0200
From: Ard Biesheuvel <ardb@...nel.org>
To: Gregory Price <gourry@...rry.net>
Cc: linux-efi@...r.kernel.org, linux-kernel@...r.kernel.org, leitao@...ian.org, 
	usamaarif642@...il.com, sathyanarayanan.kuppuswamy@...ux.intel.com, 
	ilias.apalodimas@...aro.org
Subject: Re: [PATCH 6/6] libstub,tpm: do not ignore failure case when reading
 final event log

On Fri, 13 Sept 2024 at 17:30, Gregory Price <gourry@...rry.net> wrote:
>
> On Fri, Sep 13, 2024 at 05:25:27PM +0200, Ard Biesheuvel wrote:
> > On Fri, 6 Sept 2024 at 22:28, Gregory Price <gourry@...rry.net> wrote:
> > >
> > > Current code fails to check for an error case when reading events from
> > > final event log to calculate offsets.  Check the error case, report the
> > > error, and break early because all subsequent calls will also fail.
> > >
> > > Signed-off-by: Gregory Price <gourry@...rry.net>
> > > ---
> > >  drivers/firmware/efi/libstub/tpm.c | 4 ++++
> > >  1 file changed, 4 insertions(+)
> > >
> > > diff --git a/drivers/firmware/efi/libstub/tpm.c b/drivers/firmware/efi/libstub/tpm.c
> > > index 4f9f0e049a7a..c71b0d3e66d2 100644
> > > --- a/drivers/firmware/efi/libstub/tpm.c
> > > +++ b/drivers/firmware/efi/libstub/tpm.c
> > > @@ -124,6 +124,10 @@ static void efi_retrieve_tcg2_eventlog(int version, efi_physical_addr_t log_loca
> > >                         event_size = __calc_tpm2_event_size(header,
> > >                                                    (void *)(long)log_location,
> > >                                                    false);
> > > +                       if (!event_size) {
> > > +                               efi_err("Invalid TPM Final Event Log Entry\n");
> > > +                               break;
> > > +                       }
> >
> > I don't object to this in principle, the only problem is that these
> > log prints are not recorded anywhere: they are printed to the EFI boot
> > console by the EFI stub, which may not even be visible, and is
> > definitely not captured by the kernel logging routines.
>
> Could simply drop the err and break if you think it's just going to get
> lost anyway.  Not sure there's a good way to generate a signal at this point.
>

Yeah. For the record, I absolutely detest the kludgy code there, how
we parse the map, parse and unmap the event header for every entry in
the log.

So while I highly appreciate the effort you are putting in to polish
this code, I wonder if it wouldn't be better to code this up properly
instead.