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 <[email protected]>
Tested-by: Lyude Paul <[email protected]>
---
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
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 <[email protected]>
Tested-by: Lyude Paul <[email protected]>
---
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
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 <[email protected]>
> Tested-by: Lyude Paul <[email protected]>
Reviewed-by: Jarkko Sakkinen <[email protected]>
/Jarkko
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 <[email protected]>
> Tested-by: Lyude Paul <[email protected]>
> ---
> 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 <[email protected]>
/Jarkko
On Mon, Aug 26, 2019 at 9:28 AM Jarkko Sakkinen
<[email protected]> 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 <[email protected]>
> > Tested-by: Lyude Paul <[email protected]>
>
> Reviewed-by: Jarkko Sakkinen <[email protected]>
Acked-by: Matthew Garrett <[email protected]>
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).
On Mon, Aug 26, 2019 at 9:30 AM Jarkko Sakkinen
<[email protected]> 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 <[email protected]>
> > Tested-by: Lyude Paul <[email protected]>
> > ---
> > 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 <[email protected]>
Acked-by: Matthew Garrett <[email protected]>
On Mon, Aug 26, 2019 at 10:44:31AM -0700, Matthew Garrett wrote:
> On Mon, Aug 26, 2019 at 9:28 AM Jarkko Sakkinen
> <[email protected]> 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 <[email protected]>
> > > Tested-by: Lyude Paul <[email protected]>
> >
> > Reviewed-by: Jarkko Sakkinen <[email protected]>
> Acked-by: Matthew Garrett <[email protected]>
>
> 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
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
On Tue, Aug 27, 2019 at 6:42 AM Jarkko Sakkinen
<[email protected]> 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.
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
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
On Mon, 26 Aug 2019 at 18:30, Peter Jones <[email protected]> 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 <[email protected]>
> Tested-by: Lyude Paul <[email protected]>
> ---
> 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 <[email protected]>
> 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
>
On Mon, 26 Aug 2019 at 18:30, Peter Jones <[email protected]> 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 <[email protected]>
> Tested-by: Lyude Paul <[email protected]>
> ---
> 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 <[email protected]>
> + 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
>