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

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

[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

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

[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

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

[Jarkko Sakkinen]

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

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

[Jarkko Sakkinen]

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

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

[Matthew Garrett]

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).

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

[Matthew Garrett]

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>

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

[Jarkko Sakkinen]

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

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

[Jarkko Sakkinen]

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

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

[Matthew Garrett]

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.

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

[Peter Jones]

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

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

[Jarkko Sakkinen]

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

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

[Ard Biesheuvel]

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
>

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

[Ard Biesheuvel]

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
>