[PATCH v2 1/2] efi+tpm: Don't access event->count when it isn't mapped.

[PATCH v2 1/2] efi+tpm: Don't access event->count when it isn't mapped.

[Jarkko Sakkinen]

From: Peter Jones <pjones@redhat.com>

Some machines generate a lot of event log entries.  When we're
iterating over them, the code removes the old mapping and adds a
new one, so once we cross the page boundary we're unmapping the page
with the count on it.  Hilarity ensues.

This patch keeps the info from the header in local variables so we don't
need to access that page again or keep track of if it's mapped.

Fixes: 44038bc514a2 ("tpm: Abstract crypto agile event size calculations")
Cc: linux-efi@vger.kernel.org
Cc: linux-integrity@vger.kernel.org
Cc: stable@vger.kernel.org
Signed-off-by: Peter Jones <pjones@redhat.com>
Tested-by: Lyude Paul <lyude@redhat.com>
Reviewed-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
Acked-by: Matthew Garrett <mjg59@google.com>
Acked-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
---
 include/linux/tpm_eventlog.h | 14 +++++++++++---
 1 file changed, 11 insertions(+), 3 deletions(-)

diff --git a/include/linux/tpm_eventlog.h b/include/linux/tpm_eventlog.h
index 63238c84dc0b..12584b69a3f3 100644
--- a/include/linux/tpm_eventlog.h
+++ b/include/linux/tpm_eventlog.h
@@ -170,6 +170,7 @@ static inline int __calc_tpm2_event_size(struct tcg_pcr_event2_head *event,
 	u16 halg;
 	int i;
 	int j;
+	u32 count, event_type;
 
 	marker = event;
 	marker_start = marker;
@@ -190,16 +191,22 @@ static inline int __calc_tpm2_event_size(struct tcg_pcr_event2_head *event,
 	}
 
 	event = (struct tcg_pcr_event2_head *)mapping;
+	/*
+	 * the loop below will unmap these fields if the log is larger than
+	 * one page, so save them here for reference.
+	 */
+	count = READ_ONCE(event->count);
+	event_type = READ_ONCE(event->event_type);
 
 	efispecid = (struct tcg_efi_specid_event_head *)event_header->event;
 
 	/* Check if event is malformed. */
-	if (event->count > efispecid->num_algs) {
+	if (count > efispecid->num_algs) {
 		size = 0;
 		goto out;
 	}
 
-	for (i = 0; i < event->count; i++) {
+	for (i = 0; i < count; i++) {
 		halg_size = sizeof(event->digests[i].alg_id);
 
 		/* Map the digest's algorithm identifier */
@@ -256,8 +263,9 @@ static inline int __calc_tpm2_event_size(struct tcg_pcr_event2_head *event,
 		+ event_field->event_size;
 	size = marker - marker_start;
 
-	if ((event->event_type == 0) && (event_field->event_size == 0))
+	if (event_type == 0 && event_field->event_size == 0)
 		size = 0;
+
 out:
 	if (do_mapping)
 		TPM_MEMUNMAP(mapping, mapping_size);
-- 
2.20.1

[PATCH v2 2/2] efi+tpm: don't traverse an event log with no events

[Jarkko Sakkinen]

From: Peter Jones <pjones@redhat.com>

When there are no entries to put into the final event log, some machines
will return the template they would have populated anyway.  In this case
the nr_events field is 0, but the rest of the log is just garbage.

This patch stops us from trying to iterate the table with
__calc_tpm2_event_size() when the number of events in the table is 0.

Fixes: c46f3405692d ("tpm: Reserve the TPM final events table")
Cc: linux-efi@vger.kernel.org
Cc: linux-integrity@vger.kernel.org
Cc: stable@vger.kernel.org
Signed-off-by: Peter Jones <pjones@redhat.com>
Tested-by: Lyude Paul <lyude@redhat.com>
Reviewed-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
Acked-by: Matthew Garrett <mjg59@google.com>
Acked-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
---
 drivers/firmware/efi/tpm.c | 15 ++++++++++-----
 1 file changed, 10 insertions(+), 5 deletions(-)

diff --git a/drivers/firmware/efi/tpm.c b/drivers/firmware/efi/tpm.c
index 1d3f5ca3eaaf..b9ae5c6f9b9c 100644
--- a/drivers/firmware/efi/tpm.c
+++ b/drivers/firmware/efi/tpm.c
@@ -75,11 +75,16 @@ int __init efi_tpm_eventlog_init(void)
 		goto out;
 	}
 
-	tbl_size = tpm2_calc_event_log_size((void *)efi.tpm_final_log
-					    + sizeof(final_tbl->version)
-					    + sizeof(final_tbl->nr_events),
-					    final_tbl->nr_events,
-					    log_tbl->log);
+	tbl_size = 0;
+	if (final_tbl->nr_events != 0) {
+		void *events = (void *)efi.tpm_final_log
+				+ sizeof(final_tbl->version)
+				+ sizeof(final_tbl->nr_events);
+
+		tbl_size = tpm2_calc_event_log_size(events,
+						    final_tbl->nr_events,
+						    log_tbl->log);
+	}
 	memblock_reserve((unsigned long)final_tbl,
 			 tbl_size + sizeof(*final_tbl));
 	early_memunmap(final_tbl, sizeof(*final_tbl));
-- 
2.20.1

Re: [PATCH v2 1/2] efi+tpm: Don't access event->count when it isn't mapped.

[Ard Biesheuvel]

On Wed, 25 Sep 2019 at 12:16, Jarkko Sakkinen
<jarkko.sakkinen@linux.intel.com> wrote:
>
> From: Peter Jones <pjones@redhat.com>
>
> Some machines generate a lot of event log entries.  When we're
> iterating over them, the code removes the old mapping and adds a
> new one, so once we cross the page boundary we're unmapping the page
> with the count on it.  Hilarity ensues.
>
> This patch keeps the info from the header in local variables so we don't
> need to access that page again or keep track of if it's mapped.
>
> Fixes: 44038bc514a2 ("tpm: Abstract crypto agile event size calculations")
> Cc: linux-efi@vger.kernel.org
> Cc: linux-integrity@vger.kernel.org
> Cc: stable@vger.kernel.org
> Signed-off-by: Peter Jones <pjones@redhat.com>
> Tested-by: Lyude Paul <lyude@redhat.com>
> Reviewed-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
> Acked-by: Matthew Garrett <mjg59@google.com>
> Acked-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>

Thanks Jarkko.

Shall I take these through the EFI tree?


> ---
>  include/linux/tpm_eventlog.h | 14 +++++++++++---
>  1 file changed, 11 insertions(+), 3 deletions(-)
>
> diff --git a/include/linux/tpm_eventlog.h b/include/linux/tpm_eventlog.h
> index 63238c84dc0b..12584b69a3f3 100644
> --- a/include/linux/tpm_eventlog.h
> +++ b/include/linux/tpm_eventlog.h
> @@ -170,6 +170,7 @@ static inline int __calc_tpm2_event_size(struct tcg_pcr_event2_head *event,
>         u16 halg;
>         int i;
>         int j;
> +       u32 count, event_type;
>
>         marker = event;
>         marker_start = marker;
> @@ -190,16 +191,22 @@ static inline int __calc_tpm2_event_size(struct tcg_pcr_event2_head *event,
>         }
>
>         event = (struct tcg_pcr_event2_head *)mapping;
> +       /*
> +        * the loop below will unmap these fields if the log is larger than
> +        * one page, so save them here for reference.
> +        */
> +       count = READ_ONCE(event->count);
> +       event_type = READ_ONCE(event->event_type);
>
>         efispecid = (struct tcg_efi_specid_event_head *)event_header->event;
>
>         /* Check if event is malformed. */
> -       if (event->count > efispecid->num_algs) {
> +       if (count > efispecid->num_algs) {
>                 size = 0;
>                 goto out;
>         }
>
> -       for (i = 0; i < event->count; i++) {
> +       for (i = 0; i < count; i++) {
>                 halg_size = sizeof(event->digests[i].alg_id);
>
>                 /* Map the digest's algorithm identifier */
> @@ -256,8 +263,9 @@ static inline int __calc_tpm2_event_size(struct tcg_pcr_event2_head *event,
>                 + event_field->event_size;
>         size = marker - marker_start;
>
> -       if ((event->event_type == 0) && (event_field->event_size == 0))
> +       if (event_type == 0 && event_field->event_size == 0)
>                 size = 0;
> +
>  out:
>         if (do_mapping)
>                 TPM_MEMUNMAP(mapping, mapping_size);
> --
> 2.20.1
>

Re: [PATCH v2 1/2] efi+tpm: Don't access event->count when it isn't mapped.

[Jarkko Sakkinen]

On Wed, Sep 25, 2019 at 12:25:05PM +0200, Ard Biesheuvel wrote:
> On Wed, 25 Sep 2019 at 12:16, Jarkko Sakkinen
> <jarkko.sakkinen@linux.intel.com> wrote:
> >
> > From: Peter Jones <pjones@redhat.com>
> >
> > Some machines generate a lot of event log entries.  When we're
> > iterating over them, the code removes the old mapping and adds a
> > new one, so once we cross the page boundary we're unmapping the page
> > with the count on it.  Hilarity ensues.
> >
> > This patch keeps the info from the header in local variables so we don't
> > need to access that page again or keep track of if it's mapped.
> >
> > Fixes: 44038bc514a2 ("tpm: Abstract crypto agile event size calculations")
> > Cc: linux-efi@vger.kernel.org
> > Cc: linux-integrity@vger.kernel.org
> > Cc: stable@vger.kernel.org
> > Signed-off-by: Peter Jones <pjones@redhat.com>
> > Tested-by: Lyude Paul <lyude@redhat.com>
> > Reviewed-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
> > Acked-by: Matthew Garrett <mjg59@google.com>
> > Acked-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> > Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
> 
> Thanks Jarkko.
> 
> Shall I take these through the EFI tree?

Would be great, if you could because I already sent one PR with fixes for
v5.4-rc1 yesterday.

/Jarkko

Re: [PATCH v2 1/2] efi+tpm: Don't access event->count when it isn't mapped.

[Jerry Snitselaar]

On Wed Sep 25 19, Jarkko Sakkinen wrote:
>On Wed, Sep 25, 2019 at 12:25:05PM +0200, Ard Biesheuvel wrote:
>> On Wed, 25 Sep 2019 at 12:16, Jarkko Sakkinen
>> <jarkko.sakkinen@linux.intel.com> wrote:
>> >
>> > From: Peter Jones <pjones@redhat.com>
>> >
>> > Some machines generate a lot of event log entries.  When we're
>> > iterating over them, the code removes the old mapping and adds a
>> > new one, so once we cross the page boundary we're unmapping the page
>> > with the count on it.  Hilarity ensues.
>> >
>> > This patch keeps the info from the header in local variables so we don't
>> > need to access that page again or keep track of if it's mapped.
>> >
>> > Fixes: 44038bc514a2 ("tpm: Abstract crypto agile event size calculations")
>> > Cc: linux-efi@vger.kernel.org
>> > Cc: linux-integrity@vger.kernel.org
>> > Cc: stable@vger.kernel.org
>> > Signed-off-by: Peter Jones <pjones@redhat.com>
>> > Tested-by: Lyude Paul <lyude@redhat.com>
>> > Reviewed-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
>> > Acked-by: Matthew Garrett <mjg59@google.com>
>> > Acked-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>> > Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
>>
>> Thanks Jarkko.
>>
>> Shall I take these through the EFI tree?
>
>Would be great, if you could because I already sent one PR with fixes for
>v5.4-rc1 yesterday.
>
>/Jarkko

My patch collides with this, so I will submit a v3 that applies on top of
these once I've run a test with all 3 applied on this t480s.

Re: [PATCH v2 1/2] efi+tpm: Don't access event->count when it isn't mapped.

[Jerry Snitselaar]

On Wed Sep 25 19, Jerry Snitselaar wrote:
>On Wed Sep 25 19, Jarkko Sakkinen wrote:
>>On Wed, Sep 25, 2019 at 12:25:05PM +0200, Ard Biesheuvel wrote:
>>>On Wed, 25 Sep 2019 at 12:16, Jarkko Sakkinen
>>><jarkko.sakkinen@linux.intel.com> wrote:
>>>>
>>>> From: Peter Jones <pjones@redhat.com>
>>>>
>>>> Some machines generate a lot of event log entries.  When we're
>>>> iterating over them, the code removes the old mapping and adds a
>>>> new one, so once we cross the page boundary we're unmapping the page
>>>> with the count on it.  Hilarity ensues.
>>>>
>>>> This patch keeps the info from the header in local variables so we don't
>>>> need to access that page again or keep track of if it's mapped.
>>>>
>>>> Fixes: 44038bc514a2 ("tpm: Abstract crypto agile event size calculations")
>>>> Cc: linux-efi@vger.kernel.org
>>>> Cc: linux-integrity@vger.kernel.org
>>>> Cc: stable@vger.kernel.org
>>>> Signed-off-by: Peter Jones <pjones@redhat.com>
>>>> Tested-by: Lyude Paul <lyude@redhat.com>
>>>> Reviewed-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
>>>> Acked-by: Matthew Garrett <mjg59@google.com>
>>>> Acked-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>>>> Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
>>>
>>>Thanks Jarkko.
>>>
>>>Shall I take these through the EFI tree?
>>
>>Would be great, if you could because I already sent one PR with fixes for
>>v5.4-rc1 yesterday.
>>
>>/Jarkko
>
>My patch collides with this, so I will submit a v3 that applies on top of
>these once I've run a test with all 3 applied on this t480s.

Tested with Peter's patches, and that was the root cause on this 480s.

I think there should still be a check for tbl_size to make sure we
aren't sticking -1 into efi_tpm_final_log_size though, which will be
the case right now if it fails to parse an event.

Re: [PATCH v2 1/2] efi+tpm: Don't access event->count when it isn't mapped.

[Jarkko Sakkinen]

On Wed, Sep 25, 2019 at 08:16:16AM -0700, Jerry Snitselaar wrote:
> On Wed Sep 25 19, Jarkko Sakkinen wrote:
> > On Wed, Sep 25, 2019 at 12:25:05PM +0200, Ard Biesheuvel wrote:
> > > On Wed, 25 Sep 2019 at 12:16, Jarkko Sakkinen
> > > <jarkko.sakkinen@linux.intel.com> wrote:
> > > >
> > > > From: Peter Jones <pjones@redhat.com>
> > > >
> > > > Some machines generate a lot of event log entries.  When we're
> > > > iterating over them, the code removes the old mapping and adds a
> > > > new one, so once we cross the page boundary we're unmapping the page
> > > > with the count on it.  Hilarity ensues.
> > > >
> > > > This patch keeps the info from the header in local variables so we don't
> > > > need to access that page again or keep track of if it's mapped.
> > > >
> > > > Fixes: 44038bc514a2 ("tpm: Abstract crypto agile event size calculations")
> > > > Cc: linux-efi@vger.kernel.org
> > > > Cc: linux-integrity@vger.kernel.org
> > > > Cc: stable@vger.kernel.org
> > > > Signed-off-by: Peter Jones <pjones@redhat.com>
> > > > Tested-by: Lyude Paul <lyude@redhat.com>
> > > > Reviewed-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
> > > > Acked-by: Matthew Garrett <mjg59@google.com>
> > > > Acked-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> > > > Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
> > > 
> > > Thanks Jarkko.
> > > 
> > > Shall I take these through the EFI tree?
> > 
> > Would be great, if you could because I already sent one PR with fixes for
> > v5.4-rc1 yesterday.
> > 
> > /Jarkko
> 
> My patch collides with this, so I will submit a v3 that applies on top of
> these once I've run a test with all 3 applied on this t480s.

Great, thanks.

/Jarkko

Re: [PATCH v2 1/2] efi+tpm: Don't access event->count when it isn't mapped.

[Jarkko Sakkinen]

On Wed, Sep 25, 2019 at 09:41:33AM -0700, Jerry Snitselaar wrote:
> On Wed Sep 25 19, Jerry Snitselaar wrote:
> > On Wed Sep 25 19, Jarkko Sakkinen wrote:
> > > On Wed, Sep 25, 2019 at 12:25:05PM +0200, Ard Biesheuvel wrote:
> > > > On Wed, 25 Sep 2019 at 12:16, Jarkko Sakkinen
> > > > <jarkko.sakkinen@linux.intel.com> wrote:
> > > > > 
> > > > > From: Peter Jones <pjones@redhat.com>
> > > > > 
> > > > > Some machines generate a lot of event log entries.  When we're
> > > > > iterating over them, the code removes the old mapping and adds a
> > > > > new one, so once we cross the page boundary we're unmapping the page
> > > > > with the count on it.  Hilarity ensues.
> > > > > 
> > > > > This patch keeps the info from the header in local variables so we don't
> > > > > need to access that page again or keep track of if it's mapped.
> > > > > 
> > > > > Fixes: 44038bc514a2 ("tpm: Abstract crypto agile event size calculations")
> > > > > Cc: linux-efi@vger.kernel.org
> > > > > Cc: linux-integrity@vger.kernel.org
> > > > > Cc: stable@vger.kernel.org
> > > > > Signed-off-by: Peter Jones <pjones@redhat.com>
> > > > > Tested-by: Lyude Paul <lyude@redhat.com>
> > > > > Reviewed-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
> > > > > Acked-by: Matthew Garrett <mjg59@google.com>
> > > > > Acked-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> > > > > Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
> > > > 
> > > > Thanks Jarkko.
> > > > 
> > > > Shall I take these through the EFI tree?
> > > 
> > > Would be great, if you could because I already sent one PR with fixes for
> > > v5.4-rc1 yesterday.
> > > 
> > > /Jarkko
> > 
> > My patch collides with this, so I will submit a v3 that applies on top of
> > these once I've run a test with all 3 applied on this t480s.
> 
> Tested with Peter's patches, and that was the root cause on this 480s.
> 
> I think there should still be a check for tbl_size to make sure we
> aren't sticking -1 into efi_tpm_final_log_size though, which will be
> the case right now if it fails to parse an event.

You could sent a follow up patch for that I think. The current
ones are kind of already "went through the process" and do right
things but I do agree that a sanity check would make sense just
in case.

/Jarkko