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@xxxxxxxxxxxxxxxxxxx>
> ---
>
> * 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@xxxxxxxxxxxxxxx>
/Jarkko
