Re: [PATCH 1/2] efi+tpm: Don't access event->count when it isn't mapped.
On Mon, 26 Aug 2019 at 18:30, 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 > Tested-by: Lyude Paul > --- > 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 > 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 1/2] efi+tpm: Don't access event->count when it isn't mapped.
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.
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.
On Tue, Aug 27, 2019 at 6:42 AM 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. 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.
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.
On Mon, Aug 26, 2019 at 10:44:31AM -0700, Matthew Garrett wrote: > On Mon, Aug 26, 2019 at 9:28 AM Jarkko Sakkinen > 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 > > > Tested-by: Lyude Paul > > > > Reviewed-by: Jarkko Sakkinen > Acked-by: Matthew Garrett > > 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.
On Mon, Aug 26, 2019 at 9:28 AM Jarkko Sakkinen 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 > > Tested-by: Lyude Paul > > Reviewed-by: Jarkko Sakkinen Acked-by: Matthew Garrett 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 1/2] efi+tpm: Don't access event->count when it isn't mapped.
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 > Tested-by: Lyude Paul Reviewed-by: Jarkko Sakkinen /Jarkko