Re: [PATCH] tpm: check event log version before reading final events

From: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
To: Ard Biesheuvel <ardb@kernel.org>
Cc: "Loïc Yhuel" <loic.yhuel@gmail.com>,
	linux-integrity@vger.kernel.org,
	"Matthew Garrett" <matthewgarrett@google.com>,
	javierm@redhat.com
Subject: Re: [PATCH] tpm: check event log version before reading final events
Date: Fri, 15 May 2020 02:44:55 +0300	[thread overview]
Message-ID: <7f477bcd992f0927a25f8538663c7bad8efb73c1.camel@linux.intel.com> (raw)
In-Reply-To: <CAMj1kXHX2Hf_QeKF4m-4yfjzcM2=AFV-EzthQvDO1pOsyaSsZw@mail.gmail.com>

On Thu, 2020-05-14 at 10:10 +0200, Ard Biesheuvel wrote:
> On Thu, 14 May 2020 at 03:09, Jarkko Sakkinen
> <jarkko.sakkinen@linux.intel.com> wrote:
> > On Tue, 2020-05-12 at 08:44 +0200, Ard Biesheuvel wrote:
> > > Hi Loïc,
> > > 
> > > Thanks for the fix.
> > > 
> > > On Tue, 12 May 2020 at 06:01, Loïc Yhuel <loic.yhuel@gmail.com> wrote:
> > > > This fixes the boot issues since 5.3 on several Dell models when the TPM
> > > > is enabled. Depending on the exact grub binary, booting the kernel would
> > > > freeze early, or just report an error parsing the final events log.
> > > > 
> > > > We get an event log in the SHA-1 format, which doesn't have a
> > > > tcg_efi_specid_event_head in the first event, and there is a final events
> > > > table which doesn't match the crypto agile format.
> > > > __calc_tpm2_event_size reads bad "count" and "efispecid->num_algs", and
> > > > either fails, or loops long enough for the machine to be appear frozen.
> > > > 
> > > > So we now only parse the final events table, which is per the spec always
> > > > supposed to be in the crypto agile format, when we got a event log in this
> > > > format.
> > > > 
> > > 
> > > So what functionality do we lose here? Can we still make meaningful
> > > use of the event log without the final log? I thought one was
> > > incomplete without the other?
> > 
> > Nope it would be incomplete [*].
> > 
> > So probably would make sense to at least issue a warning in this case.
> > 
> > [*] https://www.kernel.org/doc/Documentation/security/tpm/tpm_event_log.rst
> > 
> 
> Right.
> 
> I'll take the current patch as a fix for now.
> 
> I agree it makes sense to add a warning, but I'd like to understand
> better which exact condition we should warn about.
> Currently, tpm_read_log_efi() simply disregards the final log if it is
> not in crypto agile format, but is there any point to exposing
> anything in that case?

IMHO just a brief warning that "tail is missing" would be sufficient,
no other info.

/Jarkko