[PATCH] tpm: Require that all digests are present in TCG_PCR_EVENT2 structures

[PATCH] tpm: Require that all digests are present in TCG_PCR_EVENT2 structures

[Tyler Hicks]

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.

Prior to this patch, a single bit 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, could confuse the parser into
thinking that an additional entry is present in the event log. This
patch raises the bar on how difficult it is for stale memory to confuse
the kernel's event log parser but there's still 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@linux.microsoft.com>
---
 include/linux/tpm_eventlog.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/include/linux/tpm_eventlog.h b/include/linux/tpm_eventlog.h
index 4f8c90c93c29..d83eb9fd5614 100644
--- a/include/linux/tpm_eventlog.h
+++ b/include/linux/tpm_eventlog.h
@@ -201,7 +201,7 @@ 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. */
-	if (count > efispecid->num_algs) {
+	if (!efispecid->num_algs || count != efispecid->num_algs) {
 		size = 0;
 		goto out;
 	}
-- 
2.25.1

Re: [PATCH] tpm: Require that all digests are present in TCG_PCR_EVENT2 structures

[Ard Biesheuvel]

(cc Matthew and Peter)

On Tue, 16 Jun 2020 at 01:28, Tyler Hicks <tyhicks@linux.microsoft.com> 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.
>

When does this happen? Do we have code in mainline that does this?

> Prior to this patch, a single bit 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, could confuse the parser into
> thinking that an additional entry is present in the event log. This
> patch raises the bar on how difficult it is for stale memory to confuse
> the kernel's event log parser but there's still 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@linux.microsoft.com>
> ---

I am all for stringent checks, but this could potentially break
measured boot on systems that are working fine today, right?

>  include/linux/tpm_eventlog.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/include/linux/tpm_eventlog.h b/include/linux/tpm_eventlog.h
> index 4f8c90c93c29..d83eb9fd5614 100644
> --- a/include/linux/tpm_eventlog.h
> +++ b/include/linux/tpm_eventlog.h
> @@ -201,7 +201,7 @@ 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. */
> -       if (count > efispecid->num_algs) {
> +       if (!efispecid->num_algs || count != efispecid->num_algs) {
>                 size = 0;
>                 goto out;
>         }
> --
> 2.25.1
>

Re: [PATCH] tpm: Require that all digests are present in TCG_PCR_EVENT2 structures

[Tyler Hicks]

On 2020-06-16 11:08:38, Ard Biesheuvel wrote:
> (cc Matthew and Peter)

Thanks!

> On Tue, 16 Jun 2020 at 01:28, Tyler Hicks <tyhicks@linux.microsoft.com> 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.
> >
> 
> When does this happen? Do we have code in mainline that does this?

We do. POWER and some ARM firmware that pass the firmware event log via
"linux,sml-base" and "linux,sml-size" properties:

 https://open-power.github.io/skiboot/doc/device-tree/tpm.html

The "linux,sml-size" property is the size of the memory region dedicated
to the firmware event log and not the size of the firmware event log
itself.

tpm_read_log_of() in drivers/char/tpm/eventlog/of.c is where this
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.

I suspect that ACPI event log support may be implemented similarly, from
skimming tpm_read_log_acpi() and the TCG ACPI Specification, but I don't
know for sure.


Anyways, you wouldn't know from reading __calc_tpm2_event_size() but the
only thing allowing the kernel's event log parser to work on these
systems that don't inform the kernel of the actual firmware event log
size is the following conditional and assignment 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 was a valid event.

> > Prior to this patch, a single bit 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, could confuse the parser into
> > thinking that an additional entry is present in the event log. This
> > patch raises the bar on how difficult it is for stale memory to confuse
> > the kernel's event log parser but there's still 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@linux.microsoft.com>
> > ---
> 
> I am all for stringent checks, but this could potentially break
> measured boot on systems that are working fine today, right?

Yes, I think there is some risk in breaking existing systems that aren't
conforming to the spec. I'm no expert in this area so I can't say how
high the risk is. I think __calc_tpm2_event_size() is only used for
exposing the TPM2 firmware event log to userspace and then attestation
services make use of it from there. Breakage would cause the kernel to
not fully expose the firmware event log to userspace via
/sys/kernel/security/tpm*/binary_bios_measurements and that could result
in attestation failures that cause these systems to be marked as
untrusted.

I'm not in a hurry to get this merged and welcome as much feedback as
possible on the risks involved as well as my understanding of the TCG PC
Client Platform Firmware Profile Specification.

Tyler

> 
> >  include/linux/tpm_eventlog.h | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/include/linux/tpm_eventlog.h b/include/linux/tpm_eventlog.h
> > index 4f8c90c93c29..d83eb9fd5614 100644
> > --- a/include/linux/tpm_eventlog.h
> > +++ b/include/linux/tpm_eventlog.h
> > @@ -201,7 +201,7 @@ 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. */
> > -       if (count > efispecid->num_algs) {
> > +       if (!efispecid->num_algs || count != efispecid->num_algs) {
> >                 size = 0;
> >                 goto out;
> >         }
> > --
> > 2.25.1
> >

Re: [PATCH] tpm: Require that all digests are present in TCG_PCR_EVENT2 structures

[Jarkko Sakkinen]

On Tue, Jun 16, 2020 at 11:08:38AM +0200, Ard Biesheuvel wrote:
> (cc Matthew and Peter)
> 
> On Tue, 16 Jun 2020 at 01:28, Tyler Hicks <tyhicks@linux.microsoft.com> 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.
> >
> 
> When does this happen? Do we have code in mainline that does this?
> 
> > Prior to this patch, a single bit 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, could confuse the parser into
> > thinking that an additional entry is present in the event log. This
> > patch raises the bar on how difficult it is for stale memory to confuse
> > the kernel's event log parser but there's still 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@linux.microsoft.com>
> > ---
> 
> I am all for stringent checks, but this could potentially break
> measured boot on systems that are working fine today, right?

There would not be any sane reason to implement a TPM chip like that.

/Jarkko

Re: [PATCH] tpm: Require that all digests are present in TCG_PCR_EVENT2 structures

[Tyler Hicks]

On 2020-06-18 02:09:58, Jarkko Sakkinen wrote:
> On Tue, Jun 16, 2020 at 11:08:38AM +0200, Ard Biesheuvel wrote:
> > (cc Matthew and Peter)
> > 
> > On Tue, 16 Jun 2020 at 01:28, Tyler Hicks <tyhicks@linux.microsoft.com> 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.
> > >
> > 
> > When does this happen? Do we have code in mainline that does this?
> > 
> > > Prior to this patch, a single bit 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, could confuse the parser into
> > > thinking that an additional entry is present in the event log. This
> > > patch raises the bar on how difficult it is for stale memory to confuse
> > > the kernel's event log parser but there's still 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@linux.microsoft.com>
> > > ---
> > 
> > I am all for stringent checks, but this could potentially break
> > measured boot on systems that are working fine today, right?
> 
> There would not be any sane reason to implement a TPM chip like that.

Jarkko, is this an ack from you?

Is there anything I can do to help along this fix?

I've spoke with two others that have poured through these specs to
implement firmware event log parsers and they thought the change made
sense.

Tyler

> 
> /Jarkko

Re: [PATCH] tpm: Require that all digests are present in TCG_PCR_EVENT2 structures

[Peter Jones]

On Tue, Jun 16, 2020 at 11:08:38AM +0200, Ard Biesheuvel wrote:
> (cc Matthew and Peter)
> 
> On Tue, 16 Jun 2020 at 01:28, Tyler Hicks <tyhicks@linux.microsoft.com> 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.
> >
> 
> When does this happen? Do we have code in mainline that does this?
> 
> > Prior to this patch, a single bit 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, could confuse the parser into
> > thinking that an additional entry is present in the event log. This
> > patch raises the bar on how difficult it is for stale memory to confuse
> > the kernel's event log parser but there's still 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@linux.microsoft.com>
> > ---
> 
> I am all for stringent checks, but this could potentially break
> measured boot on systems that are working fine today, right?

Seems like in that case our measurement is unreliable and can't really
be trusted.  That said, having things that were using the measurements
before this suddenly stop being able to access sealed secrets is not a
great experience for the user who unwittingly bought the junk hardware.
Same with the zero-supported-hashes case.  It would be nice to at log it
and have a way for them to opt-in to allowing the old measurement to go
through, so they can recover their data, though I don't know what that
method would be if the measured command line is one of their
dependencies.

-- 
        Peter

Re: [PATCH] tpm: Require that all digests are present in TCG_PCR_EVENT2 structures

[Ard Biesheuvel]

On Tue, 30 Jun 2020 at 20:53, Peter Jones <pjones@redhat.com> wrote:
>
> On Tue, Jun 16, 2020 at 11:08:38AM +0200, Ard Biesheuvel wrote:
> > (cc Matthew and Peter)
> >
> > On Tue, 16 Jun 2020 at 01:28, Tyler Hicks <tyhicks@linux.microsoft.com> 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.
> > >
> >
> > When does this happen? Do we have code in mainline that does this?
> >
> > > Prior to this patch, a single bit 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, could confuse the parser into
> > > thinking that an additional entry is present in the event log. This
> > > patch raises the bar on how difficult it is for stale memory to confuse
> > > the kernel's event log parser but there's still 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@linux.microsoft.com>
> > > ---
> >
> > I am all for stringent checks, but this could potentially break
> > measured boot on systems that are working fine today, right?
>
> Seems like in that case our measurement is unreliable and can't really
> be trusted.  That said, having things that were using the measurements
> before this suddenly stop being able to access sealed secrets is not a
> great experience for the user who unwittingly bought the junk hardware.
> Same with the zero-supported-hashes case.  It would be nice to at log it
> and have a way for them to opt-in to allowing the old measurement to go
> through, so they can recover their data, though I don't know what that
> method would be if the measured command line is one of their
> dependencies.
>

Maybe use a EFI variable?

Re: [PATCH] tpm: Require that all digests are present in TCG_PCR_EVENT2 structures

[Tyler Hicks]

On 2020-06-30 14:53:28, Peter Jones wrote:
> On Tue, Jun 16, 2020 at 11:08:38AM +0200, Ard Biesheuvel wrote:
> > (cc Matthew and Peter)
> > 
> > On Tue, 16 Jun 2020 at 01:28, Tyler Hicks <tyhicks@linux.microsoft.com> 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.
> > >
> > 
> > When does this happen? Do we have code in mainline that does this?
> > 
> > > Prior to this patch, a single bit 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, could confuse the parser into
> > > thinking that an additional entry is present in the event log. This
> > > patch raises the bar on how difficult it is for stale memory to confuse
> > > the kernel's event log parser but there's still 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@linux.microsoft.com>
> > > ---
> > 
> > I am all for stringent checks, but this could potentially break
> > measured boot on systems that are working fine today, right?
> 
> Seems like in that case our measurement is unreliable and can't really
> be trusted.  That said, having things that were using the measurements
> before this suddenly stop being able to access sealed secrets is not a
> great experience for the user who unwittingly bought the junk hardware.

I haven't seen where anyone has suggested that such junk hardware
exists. Do you know of or expect any firmware that has mismatched
TCG_PCR_EVENT2.digests.count and TCG_EfiSpecIdEvent.numberOfAlgorithms
values?

I would think that the userspace code that's parsing
/sys/kernel/security/tpm0/binary_bios_measurements would also have
issues with such an event log.

> Same with the zero-supported-hashes case.

Small but important correction: it is a zero-hashes case, not a
zero-supported-hashes case

There's no handshake involved or anything like that. This would only
cause problems if the firmware provided no hashes, which means the
firmware event log is unusable, anyways.

Tyler

> It would be nice to at log it and have a way for them to opt-in to
> allowing the old measurement to go through, so they can recover their
> data, though I don't know what that method would be if the measured
> command line is one of their dependencies.
> 
> -- 
>         Peter

Re: [PATCH] tpm: Require that all digests are present in TCG_PCR_EVENT2 structures

[Peter Jones]

On Tue, Jun 30, 2020 at 02:23:22PM -0500, Tyler Hicks wrote:
> > > I am all for stringent checks, but this could potentially break
> > > measured boot on systems that are working fine today, right?
> > 
> > Seems like in that case our measurement is unreliable and can't really
> > be trusted.  That said, having things that were using the measurements
> > before this suddenly stop being able to access sealed secrets is not a
> > great experience for the user who unwittingly bought the junk hardware.
> 
> I haven't seen where anyone has suggested that such junk hardware
> exists. Do you know of or expect any firmware that has mismatched
> TCG_PCR_EVENT2.digests.count and TCG_EfiSpecIdEvent.numberOfAlgorithms
> values?

If nobody has seen any hardware that actually produces the values you're
excluding, then I don't have a strong objection.
 
> I would think that the userspace code that's parsing
> /sys/kernel/security/tpm0/binary_bios_measurements would also have
> issues with such an event log.
> 
> > Same with the zero-supported-hashes case.
> 
> Small but important correction: it is a zero-hashes case, not a
> zero-supported-hashes case
> 
> There's no handshake involved or anything like that. This would only
> cause problems if the firmware provided no hashes, which means the
> firmware event log is unusable, anyways.

Indeed.

-- 
        Peter

Re: [PATCH] tpm: Require that all digests are present in TCG_PCR_EVENT2 structures

[Jarkko Sakkinen]

On Tue, Jun 30, 2020 at 01:33:21PM -0500, Tyler Hicks wrote:
> Jarkko, is this an ack from you?
> 
> Is there anything I can do to help along this fix?
> 
> I've spoke with two others that have poured through these specs to
> implement firmware event log parsers and they thought the change made
> sense.
> 
> Tyler

I revisited the original patch and this stroke into my eye:

"This is true, for example, when firmware passes the event log to the
kernel via a reserved memory region described in device tree."

During this discussion you gave an explanation what can trigger the bug
but in the commit message nothing anchors to anything.

Please give a concrete example what can trigger the issue directly in
the commit message instead. It's obviously needed.

In addition, you could also rewrite the existing inline comment to be
something more reasonable to the context.

/Jarkko

Re: [PATCH] tpm: Require that all digests are present in TCG_PCR_EVENT2 structures

[Tyler Hicks]

On 2020-07-03 02:57:18, Jarkko Sakkinen wrote:
> On Tue, Jun 30, 2020 at 01:33:21PM -0500, Tyler Hicks wrote:
> > Jarkko, is this an ack from you?
> > 
> > Is there anything I can do to help along this fix?
> > 
> > I've spoke with two others that have poured through these specs to
> > implement firmware event log parsers and they thought the change made
> > sense.
> > 
> > Tyler
> 
> I revisited the original patch and this stroke into my eye:
> 
> "This is true, for example, when firmware passes the event log to the
> kernel via a reserved memory region described in device tree."
> 
> During this discussion you gave an explanation what can trigger the bug
> but in the commit message nothing anchors to anything.
> 
> Please give a concrete example what can trigger the issue directly in
> the commit message instead. It's obviously needed.
> 
> In addition, you could also rewrite the existing inline comment to be
> something more reasonable to the context.

These are all fair points and I also see that there's a new conflict
with the TPM next branch. I'll rebase the patch on the current next
branch, expand on the commit message, and improve the comment in v2.

Tyler

> 
> /Jarkko

Re: [PATCH] tpm: Require that all digests are present in TCG_PCR_EVENT2 structures

[Jarkko Sakkinen]

On Thu, Jul 09, 2020 at 05:58:23PM -0500, Tyler Hicks wrote:
> On 2020-07-03 02:57:18, Jarkko Sakkinen wrote:
> > On Tue, Jun 30, 2020 at 01:33:21PM -0500, Tyler Hicks wrote:
> > > Jarkko, is this an ack from you?
> > > 
> > > Is there anything I can do to help along this fix?
> > > 
> > > I've spoke with two others that have poured through these specs to
> > > implement firmware event log parsers and they thought the change made
> > > sense.
> > > 
> > > Tyler
> > 
> > I revisited the original patch and this stroke into my eye:
> > 
> > "This is true, for example, when firmware passes the event log to the
> > kernel via a reserved memory region described in device tree."
> > 
> > During this discussion you gave an explanation what can trigger the bug
> > but in the commit message nothing anchors to anything.
> > 
> > Please give a concrete example what can trigger the issue directly in
> > the commit message instead. It's obviously needed.
> > 
> > In addition, you could also rewrite the existing inline comment to be
> > something more reasonable to the context.
> 
> These are all fair points and I also see that there's a new conflict
> with the TPM next branch. I'll rebase the patch on the current next
> branch, expand on the commit message, and improve the comment in v2.
> 
> Tyler

Thank you.

/Jarkko