Linux-EFI Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH 1/2] efi+tpm: Don't access event->count when it isn't mapped.
@ 2019-08-26 15:30 Peter Jones
  2019-08-26 15:30 ` [PATCH 2/2] efi+tpm: don't traverse an event log with no events Peter Jones
                   ` (2 more replies)
  0 siblings, 3 replies; 13+ messages in thread
From: Peter Jones @ 2019-08-26 15:30 UTC (permalink / raw)
  To: Ard Biesheuvel
  Cc: Jarkko Sakkinen, Roberto Sassu, Matthew Garrett,
	Bartosz Szczepanek, Lyude Paul, linux-efi, linux-kernel,
	Peter Jones

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.

Signed-off-by: Peter Jones <pjones@redhat.com>
Tested-by: Lyude Paul <lyude@redhat.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 63238c84dc0..549dab0b56b 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 = event->count;
+	event_type = 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.23.0.rc2


^ permalink raw reply	[flat|nested] 13+ messages in thread

* [PATCH 2/2] efi+tpm: don't traverse an event log with no events
  2019-08-26 15:30 [PATCH 1/2] efi+tpm: Don't access event->count when it isn't mapped Peter Jones
@ 2019-08-26 15:30 ` Peter Jones
  2019-08-26 16:30   ` Jarkko Sakkinen
  2019-08-31 16:46   ` Ard Biesheuvel
  2019-08-26 16:28 ` [PATCH 1/2] efi+tpm: Don't access event->count when it isn't mapped Jarkko Sakkinen
  2019-08-31 16:11 ` Ard Biesheuvel
  2 siblings, 2 replies; 13+ messages in thread
From: Peter Jones @ 2019-08-26 15:30 UTC (permalink / raw)
  To: Ard Biesheuvel
  Cc: Jarkko Sakkinen, Roberto Sassu, Matthew Garrett,
	Bartosz Szczepanek, Lyude Paul, linux-efi, linux-kernel,
	Peter Jones

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.

Signed-off-by: Peter Jones <pjones@redhat.com>
Tested-by: Lyude Paul <lyude@redhat.com>
---
 drivers/firmware/efi/tpm.c | 14 +++++++++-----
 1 file changed, 9 insertions(+), 5 deletions(-)

diff --git a/drivers/firmware/efi/tpm.c b/drivers/firmware/efi/tpm.c
index 1d3f5ca3eaa..be51ed17c6e 100644
--- a/drivers/firmware/efi/tpm.c
+++ b/drivers/firmware/efi/tpm.c
@@ -75,11 +75,15 @@ 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.23.0.rc2


^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 1/2] efi+tpm: Don't access event->count when it isn't mapped.
  2019-08-26 15:30 [PATCH 1/2] efi+tpm: Don't access event->count when it isn't mapped Peter Jones
  2019-08-26 15:30 ` [PATCH 2/2] efi+tpm: don't traverse an event log with no events Peter Jones
@ 2019-08-26 16:28 ` Jarkko Sakkinen
  2019-08-26 17:44   ` Matthew Garrett
  2019-08-31 16:11 ` Ard Biesheuvel
  2 siblings, 1 reply; 13+ messages in thread
From: Jarkko Sakkinen @ 2019-08-26 16:28 UTC (permalink / raw)
  To: Peter Jones
  Cc: Ard Biesheuvel, Roberto Sassu, Matthew Garrett,
	Bartosz Szczepanek, Lyude Paul, linux-efi, linux-kernel

On Mon, Aug 26, 2019 at 11:30:27AM -0400, Peter Jones wrote:
> 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.
> 
> Signed-off-by: Peter Jones <pjones@redhat.com>
> Tested-by: Lyude Paul <lyude@redhat.com>

Reviewed-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>

/Jarkko

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 2/2] efi+tpm: don't traverse an event log with no events
  2019-08-26 15:30 ` [PATCH 2/2] efi+tpm: don't traverse an event log with no events Peter Jones
@ 2019-08-26 16:30   ` Jarkko Sakkinen
  2019-08-26 17:45     ` Matthew Garrett
  2019-08-31 16:46   ` Ard Biesheuvel
  1 sibling, 1 reply; 13+ messages in thread
From: Jarkko Sakkinen @ 2019-08-26 16:30 UTC (permalink / raw)
  To: Peter Jones
  Cc: Ard Biesheuvel, Roberto Sassu, Matthew Garrett,
	Bartosz Szczepanek, Lyude Paul, linux-efi, linux-kernel

On Mon, Aug 26, 2019 at 11:30:28AM -0400, Peter Jones wrote:
> 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.
> 
> Signed-off-by: Peter Jones <pjones@redhat.com>
> Tested-by: Lyude Paul <lyude@redhat.com>
> ---
>  drivers/firmware/efi/tpm.c | 14 +++++++++-----
>  1 file changed, 9 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/firmware/efi/tpm.c b/drivers/firmware/efi/tpm.c
> index 1d3f5ca3eaa..be51ed17c6e 100644
> --- a/drivers/firmware/efi/tpm.c
> +++ b/drivers/firmware/efi/tpm.c
> @@ -75,11 +75,15 @@ 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);
> +	}

Reviewed-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>

/Jarkko

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 1/2] efi+tpm: Don't access event->count when it isn't mapped.
  2019-08-26 16:28 ` [PATCH 1/2] efi+tpm: Don't access event->count when it isn't mapped Jarkko Sakkinen
@ 2019-08-26 17:44   ` Matthew Garrett
  2019-08-27 11:03     ` Jarkko Sakkinen
  0 siblings, 1 reply; 13+ messages in thread
From: Matthew Garrett @ 2019-08-26 17:44 UTC (permalink / raw)
  To: Jarkko Sakkinen
  Cc: Peter Jones, Ard Biesheuvel, Roberto Sassu, Bartosz Szczepanek,
	Lyude Paul, linux-efi, Linux Kernel Mailing List

On Mon, Aug 26, 2019 at 9:28 AM Jarkko Sakkinen
<jarkko.sakkinen@linux.intel.com> wrote:
>
> On Mon, Aug 26, 2019 at 11:30:27AM -0400, Peter Jones wrote:
> > 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.
> >
> > 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>

Jarkko, these two should probably go to 5.3 if possible - I
independently had a report of a system hitting this issue last week
(Intel apparently put a surprising amount of data in the event logs on
the NUCs).

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 2/2] efi+tpm: don't traverse an event log with no events
  2019-08-26 16:30   ` Jarkko Sakkinen
@ 2019-08-26 17:45     ` Matthew Garrett
  0 siblings, 0 replies; 13+ messages in thread
From: Matthew Garrett @ 2019-08-26 17:45 UTC (permalink / raw)
  To: Jarkko Sakkinen
  Cc: Peter Jones, Ard Biesheuvel, Roberto Sassu, Bartosz Szczepanek,
	Lyude Paul, linux-efi, Linux Kernel Mailing List

On Mon, Aug 26, 2019 at 9:30 AM Jarkko Sakkinen
<jarkko.sakkinen@linux.intel.com> wrote:
>
> On Mon, Aug 26, 2019 at 11:30:28AM -0400, Peter Jones wrote:
> > 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.
> >
> > Signed-off-by: Peter Jones <pjones@redhat.com>
> > Tested-by: Lyude Paul <lyude@redhat.com>
> > ---
> >  drivers/firmware/efi/tpm.c | 14 +++++++++-----
> >  1 file changed, 9 insertions(+), 5 deletions(-)
> >
> > diff --git a/drivers/firmware/efi/tpm.c b/drivers/firmware/efi/tpm.c
> > index 1d3f5ca3eaa..be51ed17c6e 100644
> > --- a/drivers/firmware/efi/tpm.c
> > +++ b/drivers/firmware/efi/tpm.c
> > @@ -75,11 +75,15 @@ 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);
> > +     }
>
> Reviewed-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
Acked-by: Matthew Garrett <mjg59@google.com>

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 1/2] efi+tpm: Don't access event->count when it isn't mapped.
  2019-08-26 17:44   ` Matthew Garrett
@ 2019-08-27 11:03     ` Jarkko Sakkinen
  2019-08-27 13:41       ` Jarkko Sakkinen
  0 siblings, 1 reply; 13+ messages in thread
From: Jarkko Sakkinen @ 2019-08-27 11:03 UTC (permalink / raw)
  To: Matthew Garrett
  Cc: Peter Jones, Ard Biesheuvel, Roberto Sassu, Bartosz Szczepanek,
	Lyude Paul, linux-efi, Linux Kernel Mailing List

On Mon, Aug 26, 2019 at 10:44:31AM -0700, Matthew Garrett wrote:
> On Mon, Aug 26, 2019 at 9:28 AM Jarkko Sakkinen
> <jarkko.sakkinen@linux.intel.com> wrote:
> >
> > On Mon, Aug 26, 2019 at 11:30:27AM -0400, Peter Jones wrote:
> > > 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.
> > >
> > > 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>
> 
> Jarkko, these two should probably go to 5.3 if possible - I
> independently had a report of a system hitting this issue last week
> (Intel apparently put a surprising amount of data in the event logs on
> the NUCs).

OK, I can try to push them. I'll do PR today.

/Jarkko

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 1/2] efi+tpm: Don't access event->count when it isn't mapped.
  2019-08-27 11:03     ` Jarkko Sakkinen
@ 2019-08-27 13:41       ` Jarkko Sakkinen
  2019-08-27 16:00         ` Matthew Garrett
  2019-08-27 22:11         ` Peter Jones
  0 siblings, 2 replies; 13+ messages in thread
From: Jarkko Sakkinen @ 2019-08-27 13:41 UTC (permalink / raw)
  To: Matthew Garrett, ard.biesheuvel
  Cc: Peter Jones, Roberto Sassu, Bartosz Szczepanek, Lyude Paul,
	linux-efi, Linux Kernel Mailing List

On Tue, Aug 27, 2019 at 02:03:44PM +0300, Jarkko Sakkinen wrote:
> > Jarkko, these two should probably go to 5.3 if possible - I
> > independently had a report of a system hitting this issue last week
> > (Intel apparently put a surprising amount of data in the event logs on
> > the NUCs).
> 
> OK, I can try to push them. I'll do PR today.

Ard, how do you wish these to be managed?

I'm asking this because:

1. Neither patch was CC'd to linux-integrity.
2. Neither patch has your tags ATM.

/Jarkko

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 1/2] efi+tpm: Don't access event->count when it isn't mapped.
  2019-08-27 13:41       ` Jarkko Sakkinen
@ 2019-08-27 16:00         ` Matthew Garrett
  2019-08-27 22:11         ` Peter Jones
  1 sibling, 0 replies; 13+ messages in thread
From: Matthew Garrett @ 2019-08-27 16:00 UTC (permalink / raw)
  To: Jarkko Sakkinen
  Cc: Ard Biesheuvel, Peter Jones, Roberto Sassu, Bartosz Szczepanek,
	Lyude Paul, linux-efi, Linux Kernel Mailing List

On Tue, Aug 27, 2019 at 6:42 AM Jarkko Sakkinen
<jarkko.sakkinen@linux.intel.com> wrote:
>
> On Tue, Aug 27, 2019 at 02:03:44PM +0300, Jarkko Sakkinen wrote:
> > > Jarkko, these two should probably go to 5.3 if possible - I
> > > independently had a report of a system hitting this issue last week
> > > (Intel apparently put a surprising amount of data in the event logs on
> > > the NUCs).
> >
> > OK, I can try to push them. I'll do PR today.
>
> Ard, how do you wish these to be managed?
>
> I'm asking this because:
>
> 1. Neither patch was CC'd to linux-integrity.
> 2. Neither patch has your tags ATM.

Feel free to add my tags, but I don't think it's important.

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 1/2] efi+tpm: Don't access event->count when it isn't mapped.
  2019-08-27 13:41       ` Jarkko Sakkinen
  2019-08-27 16:00         ` Matthew Garrett
@ 2019-08-27 22:11         ` Peter Jones
  2019-08-29 13:21           ` Jarkko Sakkinen
  1 sibling, 1 reply; 13+ messages in thread
From: Peter Jones @ 2019-08-27 22:11 UTC (permalink / raw)
  To: Jarkko Sakkinen
  Cc: Matthew Garrett, ard.biesheuvel, Roberto Sassu,
	Bartosz Szczepanek, Lyude Paul, linux-efi,
	Linux Kernel Mailing List

On Tue, Aug 27, 2019 at 04:41:55PM +0300, Jarkko Sakkinen wrote:
> On Tue, Aug 27, 2019 at 02:03:44PM +0300, Jarkko Sakkinen wrote:
> > > Jarkko, these two should probably go to 5.3 if possible - I
> > > independently had a report of a system hitting this issue last week
> > > (Intel apparently put a surprising amount of data in the event logs on
> > > the NUCs).
> > 
> > OK, I can try to push them. I'll do PR today.
> 
> Ard, how do you wish these to be managed?
> 
> I'm asking this because:
> 
> 1. Neither patch was CC'd to linux-integrity.
> 2. Neither patch has your tags ATM.

I think Ard's not back until September.  I can just to re-send them with
the accumulated tags and Cc linux-integrity, if you think that would
help?

-- 
  Peter

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 1/2] efi+tpm: Don't access event->count when it isn't mapped.
  2019-08-27 22:11         ` Peter Jones
@ 2019-08-29 13:21           ` Jarkko Sakkinen
  0 siblings, 0 replies; 13+ messages in thread
From: Jarkko Sakkinen @ 2019-08-29 13:21 UTC (permalink / raw)
  To: Peter Jones
  Cc: Matthew Garrett, ard.biesheuvel, Roberto Sassu,
	Bartosz Szczepanek, Lyude Paul, linux-efi,
	Linux Kernel Mailing List

On Tue, Aug 27, 2019 at 06:11:58PM -0400, Peter Jones wrote:
> On Tue, Aug 27, 2019 at 04:41:55PM +0300, Jarkko Sakkinen wrote:
> > On Tue, Aug 27, 2019 at 02:03:44PM +0300, Jarkko Sakkinen wrote:
> > > > Jarkko, these two should probably go to 5.3 if possible - I
> > > > independently had a report of a system hitting this issue last week
> > > > (Intel apparently put a surprising amount of data in the event logs on
> > > > the NUCs).
> > > 
> > > OK, I can try to push them. I'll do PR today.
> > 
> > Ard, how do you wish these to be managed?
> > 
> > I'm asking this because:
> > 
> > 1. Neither patch was CC'd to linux-integrity.
> > 2. Neither patch has your tags ATM.
> 
> I think Ard's not back until September.  I can just to re-send them with
> the accumulated tags and Cc linux-integrity, if you think that would
> help?

I take the risk. If possible, add all the cumulated tags to those
patches...

/Jarkko

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 1/2] efi+tpm: Don't access event->count when it isn't mapped.
  2019-08-26 15:30 [PATCH 1/2] efi+tpm: Don't access event->count when it isn't mapped Peter Jones
  2019-08-26 15:30 ` [PATCH 2/2] efi+tpm: don't traverse an event log with no events Peter Jones
  2019-08-26 16:28 ` [PATCH 1/2] efi+tpm: Don't access event->count when it isn't mapped Jarkko Sakkinen
@ 2019-08-31 16:11 ` Ard Biesheuvel
  2 siblings, 0 replies; 13+ messages in thread
From: Ard Biesheuvel @ 2019-08-31 16:11 UTC (permalink / raw)
  To: Peter Jones
  Cc: Jarkko Sakkinen, Roberto Sassu, Matthew Garrett,
	Bartosz Szczepanek, Lyude Paul, linux-efi,
	Linux Kernel Mailing List

On Mon, 26 Aug 2019 at 18:30, Peter Jones <pjones@redhat.com> wrote:
>
> 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.
>
> Signed-off-by: Peter Jones <pjones@redhat.com>
> Tested-by: Lyude Paul <lyude@redhat.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 63238c84dc0..549dab0b56b 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 = event->count;
> +       event_type = event->event_type;
>

These assignments should be using READ_ONCE(), since otherwise, the
compiler may reload these quantities from memory anyway.

With that fixed,

Acked-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>

>         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.23.0.rc2
>

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 2/2] efi+tpm: don't traverse an event log with no events
  2019-08-26 15:30 ` [PATCH 2/2] efi+tpm: don't traverse an event log with no events Peter Jones
  2019-08-26 16:30   ` Jarkko Sakkinen
@ 2019-08-31 16:46   ` Ard Biesheuvel
  1 sibling, 0 replies; 13+ messages in thread
From: Ard Biesheuvel @ 2019-08-31 16:46 UTC (permalink / raw)
  To: Peter Jones
  Cc: Jarkko Sakkinen, Roberto Sassu, Matthew Garrett,
	Bartosz Szczepanek, Lyude Paul, linux-efi,
	Linux Kernel Mailing List

On Mon, 26 Aug 2019 at 18:30, Peter Jones <pjones@redhat.com> wrote:
>
> 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.
>
> Signed-off-by: Peter Jones <pjones@redhat.com>
> Tested-by: Lyude Paul <lyude@redhat.com>
> ---
>  drivers/firmware/efi/tpm.c | 14 +++++++++-----
>  1 file changed, 9 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/firmware/efi/tpm.c b/drivers/firmware/efi/tpm.c
> index 1d3f5ca3eaa..be51ed17c6e 100644
> --- a/drivers/firmware/efi/tpm.c
> +++ b/drivers/firmware/efi/tpm.c
> @@ -75,11 +75,15 @@ 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);

Please put a newline here

With that fixed,

Acked-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>

> +               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.23.0.rc2
>

^ permalink raw reply	[flat|nested] 13+ messages in thread

end of thread, back to index

Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-08-26 15:30 [PATCH 1/2] efi+tpm: Don't access event->count when it isn't mapped Peter Jones
2019-08-26 15:30 ` [PATCH 2/2] efi+tpm: don't traverse an event log with no events Peter Jones
2019-08-26 16:30   ` Jarkko Sakkinen
2019-08-26 17:45     ` Matthew Garrett
2019-08-31 16:46   ` Ard Biesheuvel
2019-08-26 16:28 ` [PATCH 1/2] efi+tpm: Don't access event->count when it isn't mapped Jarkko Sakkinen
2019-08-26 17:44   ` Matthew Garrett
2019-08-27 11:03     ` Jarkko Sakkinen
2019-08-27 13:41       ` Jarkko Sakkinen
2019-08-27 16:00         ` Matthew Garrett
2019-08-27 22:11         ` Peter Jones
2019-08-29 13:21           ` Jarkko Sakkinen
2019-08-31 16:11 ` Ard Biesheuvel

Linux-EFI Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-efi/0 linux-efi/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-efi linux-efi/ https://lore.kernel.org/linux-efi \
		linux-efi@vger.kernel.org linux-efi@archiver.kernel.org
	public-inbox-index linux-efi


Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-efi


AGPL code for this site: git clone https://public-inbox.org/ public-inbox