Message-ID: <20200715155053.GA3673@sequoia>
Date:   Wed, 15 Jul 2020 10:50:53 -0500
From:   Tyler Hicks <tyhicks@...ux.microsoft.com>
To:     Jarkko Sakkinen <jarkko.sakkinen@...ux.intel.com>
Cc:     Peter Huewe <peterhuewe@....de>, Ard Biesheuvel <ardb@...nel.org>,
        Jason Gunthorpe <jgg@...pe.ca>,
        Petr Vandrovec <petr@...are.com>,
        Nayna Jain <nayna@...ux.ibm.com>,
        Thirupathaiah Annapureddy <thiruan@...rosoft.com>,
        linux-integrity@...r.kernel.org, linux-efi@...r.kernel.org,
        linux-kernel@...r.kernel.org, Peter Jones <pjones@...hat.com>
Subject: Re: [PATCH v2] tpm: Require that all digests are present in
 TCG_PCR_EVENT2 structures

On 2020-07-13 23:57:19, Jarkko Sakkinen wrote:
> On Fri, Jul 10, 2020 at 02:29:55PM -0500, Tyler Hicks wrote:
> > Require that the TCG_PCR_EVENT2.digests.count value strictly matches the
> > value of TCG_EfiSpecIdEvent.numberOfAlgorithms in the event field of the
> > TCG_PCClientPCREvent event log header. Also require that
> > TCG_EfiSpecIdEvent.numberOfAlgorithms is non-zero.
> > 
> > The TCG PC Client Platform Firmware Profile Specification section 9.1
> > (Family "2.0", Level 00 Revision 1.04) states:
> > 
> >  For each Hash algorithm enumerated in the TCG_PCClientPCREvent entry,
> >  there SHALL be a corresponding digest in all TCG_PCR_EVENT2 structures.
> >  Note: This includes EV_NO_ACTION events which do not extend the PCR.
> > 
> > Section 9.4.5.1 provides this description of
> > TCG_EfiSpecIdEvent.numberOfAlgorithms:
> > 
> >  The number of Hash algorithms in the digestSizes field. This field MUST
> >  be set to a value of 0x01 or greater.
> > 
> > Enforce these restrictions, as required by the above specification, in
> > order to better identify and ignore invalid sequences of bytes at the
> > end of an otherwise valid TPM2 event log. Firmware doesn't always have
> > the means necessary to inform the kernel of the actual event log size so
> > the kernel's event log parsing code should be stringent when parsing the
> > event log for resiliency against firmware bugs. This is true, for
> > example, when firmware passes the event log to the kernel via a reserved
> > memory region described in device tree.
> > 
> > POWER and some ARM systems use the "linux,sml-base" and "linux,sml-size"
> > device tree properties to describe the memory region used to pass the
> > event log from firmware to the kernel. Unfortunately, the
> > "linux,sml-size" property describes the size of the entire reserved
> > memory region rather than the size of the event long within the memory
> > region and the event log format does not include information describing
> > the size of the event log.
> > 
> > tpm_read_log_of(), in drivers/char/tpm/eventlog/of.c, is where the
> > "linux,sml-size" property is used. At the end of that function,
> > log->bios_event_log_end is pointing at the end of the reserved memory
> > region. That's typically 0x10000 bytes offset from "linux,sml-base",
> > depending on what's defined in the device tree source.
> > 
> > The firmware event log only fills a portion of those 0x10000 bytes and
> > the rest of the memory region should be zeroed out by firmware. Even in
> > the case of a properly zeroed bytes in the remainder of the memory
> > region, the only thing allowing the kernel's event log parser to detect
> > the end of the event log is the following conditional in
> > __calc_tpm2_event_size():
> > 
> >         if (event_type == 0 && event_field->event_size == 0)
> >                 size = 0;
> > 
> > If that wasn't there, __calc_tpm2_event_size() would think that a 16
> > byte sequence of zeroes, following an otherwise valid event log, was
> > a valid event.
> > 
> > However, problems can occur if a single bit is set in the offset
> > corresponding to either the TCG_PCR_EVENT2.eventType or
> > TCG_PCR_EVENT2.eventSize fields, after the last valid event log entry.
> > This could confuse the parser into thinking that an additional entry is
> > present in the event log and exposing this invalid entry to userspace in
> > the /sys/kernel/security/tpm0/binary_bios_measurements file. Such
> > problems have been seen if firmware does not fully zero the memory
> > region upon a warm reboot.
> > 
> > This patch significantly raises the bar on how difficult it is for
> > stale/invalid memory to confuse the kernel's event log parser but
> > there's still, ultimately, a reliance on firmware to properly initialize
> > the remainder of the memory region reserved for the event log as the
> > parser cannot be expected to detect a stale but otherwise properly
> > formatted firmware event log entry.
> > 
> > Fixes: fd5c78694f3f ("tpm: fix handling of the TPM 2.0 event logs")
> > Signed-off-by: Tyler Hicks <tyhicks@...ux.microsoft.com>
> > ---
> > 
> > * v2
> >   - Rebase the patch on top of the TPM next branch, commit 786a2aa281f4
> >     ("Revert commit e918e570415c ("tpm_tis: Remove the HID IFX0102")")
> >   - Expand on the technical reasoning for needing strict event
> >     validation in the commit message
> >   - Improve the inline comment explaining the need for detecting
> >     malformed events
> > 
> >  include/linux/tpm_eventlog.h | 11 +++++++++--
> >  1 file changed, 9 insertions(+), 2 deletions(-)
> > 
> > diff --git a/include/linux/tpm_eventlog.h b/include/linux/tpm_eventlog.h
> > index 64356b199e94..739ba9a03ec1 100644
> > --- a/include/linux/tpm_eventlog.h
> > +++ b/include/linux/tpm_eventlog.h
> > @@ -211,9 +211,16 @@ static inline int __calc_tpm2_event_size(struct tcg_pcr_event2_head *event,
> >  
> >  	efispecid = (struct tcg_efi_specid_event_head *)event_header->event;
> >  
> > -	/* Check if event is malformed. */
> > +	/*
> > +	 * Perform validation of the event in order to identify malformed
> > +	 * events. This function may be asked to parse arbitrary byte sequences
> > +	 * immediately following a valid event log. The caller expects this
> > +	 * function to recognize that the byte sequence is not a valid event
> > +	 * and to return an event size of 0.
> > +	 */
> >  	if (memcmp(efispecid->signature, TCG_SPECID_SIG,
> > -		   sizeof(TCG_SPECID_SIG)) || count > efispecid->num_algs) {
> > +		   sizeof(TCG_SPECID_SIG)) ||
> > +	    !efispecid->num_algs || count != efispecid->num_algs) {
> >  		size = 0;
> >  		goto out;
> >  	}
> > -- 
> > 2.25.1
> > 
> 
> 
> Reviewed-by: Jarkko Sakkinen <jarkko.sakkinen@...ux.intel.com>

Thanks for the review. Do you plan to take this through the linux-tpmdd
tree or are you expecting someone else to pick up this patch?

Tyler

> 
> /Jarkko

Powered by blists - more mailing lists