Re: [PATCH V5 2/4] tpm: Reserve the TPM final events table
Nope, it doesn't work. It compiled (after correcting one more leftover
mapping), but panicked the same way.
I've came up with a set of changes that make it working in my setup,
see attached patch. There was a problem with passing already remapped
address to tpm2_calc_event_log_size(), which tried to remap it for
second time. Few more adjustments were needed in remap-as-you-go code
so that new address is used consequently. Also, I've removed remapping
digest itself because it was never used.
I'm not certain these changes make it 100% correct, but it worked on
35 events I had in final log. Its ending seems fine now:
> [root@localhost ~]# hexdump -C
> /sys/kernel/security/tpm0/binary_bios_measurements | tail
> 3720 42 6f 6f 74 20 53 65 72 76 69 63 65 73 20 49 6e |Boot Services In|
> 3730 76 6f 63 61 74 69 6f 6e 05 00 00 00 07 00 00 80 |vocation|
> 3740 02 00 00 00 04 00 47 55 45 dd c9 78 d7 bf d0 36 |..GUE..x...6|
> 3750 fa cc 7e 2e 98 7f 48 18 9f 0d 0b 00 b5 4f 75 42 |..~...H..OuB|
> 3760 cb d8 72 a8 1a 9d 9d ea 83 9b 2b 8d 74 7c 7e bd |..r...+.t|~.|
> 3770 5e a6 61 5c 40 f4 2f 44 a6 db eb a0 28 00 00 00 |^.a\@./D(...|
> 3780 45 78 69 74 20 42 6f 6f 74 20 53 65 72 76 69 63 |Exit Boot Servic|
> 3790 65 73 20 52 65 74 75 72 6e 65 64 20 77 69 74 68 |es Returned with|
> 37a0 20 53 75 63 63 65 73 73 | Success|
> 37a8
Still, some refactoring could help here as __calc_tpm2_event_size has
grown and its logic became hard to follow. IMO it's far too complex
for inline function.
Attached patch should be applied on top of jjs/master.
Bartosz
diff --git a/drivers/firmware/efi/tpm.c b/drivers/firmware/efi/tpm.c
index fe48150f06d1..2c912ea08166 100644
--- a/drivers/firmware/efi/tpm.c
+++ b/drivers/firmware/efi/tpm.c
@@ -28,6 +28,7 @@ static int tpm2_calc_event_log_size(void *data, int count, void *size_info)
if (event_size == 0)
return -1;
size += event_size;
+ count--;
}
return size;
@@ -41,6 +42,7 @@ int __init efi_tpm_eventlog_init(void)
struct linux_efi_tpm_eventlog *log_tbl;
struct efi_tcg2_final_events_table *final_tbl;
unsigned int tbl_size;
+ int ret = 0;
if (efi.tpm_log == EFI_INVALID_TABLE_ADDR) {
/*
@@ -60,10 +62,9 @@ int __init efi_tpm_eventlog_init(void)
tbl_size = sizeof(*log_tbl) + log_tbl->size;
memblock_reserve(efi.tpm_log, tbl_size);
- early_memunmap(log_tbl, sizeof(*log_tbl));
if (efi.tpm_final_log == EFI_INVALID_TABLE_ADDR)
- return 0;
+ goto out;
final_tbl = early_memremap(efi.tpm_final_log, sizeof(*final_tbl));
@@ -71,17 +72,22 @@ int __init efi_tpm_eventlog_init(void)
pr_err("Failed to map TPM Final Event Log table @ 0x%lx\n",
efi.tpm_final_log);
efi.tpm_final_log = EFI_INVALID_TABLE_ADDR;
- return -ENOMEM;
+ ret = -ENOMEM;
+ goto out;
}
- tbl_size = tpm2_calc_event_log_size(final_tbl->events,
+ tbl_size = tpm2_calc_event_log_size(efi.tpm_final_log
+ + sizeof(final_tbl->version)
+ + sizeof(final_tbl->nr_events),
final_tbl->nr_events,
- (void *)efi.tpm_log);
+ log_tbl->log);
memblock_reserve((unsigned long)final_tbl,
tbl_size + sizeof(*final_tbl));
early_memunmap(final_tbl, sizeof(*final_tbl));
efi_tpm_final_log_size = tbl_size;
- return 0;
+out:
+ early_memunmap(log_tbl, sizeof(*log_tbl));
+ return ret;
}
diff --git a/include/linux/tpm_eventlog.h b/include/linux/tpm_eventlog.h
index 0ca27bc053af..63238c84dc0b 100644
--- a/include/linux/tpm_eventlog.h
+++ b/include/linux/tpm_eventlog.h
@@ -185,8 +185,12 @@ static inline int __calc_tpm2_event_size(struct tcg_pcr_event2_head *event,
size = 0;
goto out;
}
+ } else {
+ mapping = marker_start;
}
+ event = (struct tcg_pcr_event2_head *)mapping;
+
efispecid = (struct tcg_efi_specid_event_head *)event_header->event;
/* Check if event is malformed. */
@@ -201,34 +205,24 @@ static inline int __calc_tpm2_event_size(struct tcg_pcr_event2_head *event,
/* Map the digest's algorithm identifier */
if (do_mapping) {
TPM_MEMUNMAP(mapping, mapping_size);
- mapping_size = marker - marker_start + halg_size;
- mapping = TPM_MEMREMAP((unsigned long)marker_start,
- mapping_size);
+ mapping_size = halg_size;
+ mapping = TPM_MEMREMAP((unsigned long)marker,
+ mapping_size);
if (!mapping) {
size = 0;
goto out;
}
+ } else {
+ mapping = marker;
}
- memcpy(, marker, halg_size);
+ memcpy(, mapping, halg_size);
marker = marker + halg_size;
for (j = 0; j < efispecid->num_algs; j++) {
if (halg == efispecid->digest_sizes[j].alg_id) {
marker +=
efispecid->digest_sizes[j].digest_size;
-
-/* Map the digest content itself */
-if (do_mapping) {
- TPM_MEMUNMAP(mapping, mapping_size);
- mapping_size = marker - marker_start;
- mapping = TPM_MEMREMAP((unsigned long)marker_start,
- mapping_size);
- if
Re: [PATCH V5 2/4] tpm: Reserve the TPM final events table
On Fri, May 03, 2019 at 08:02:18AM +0200, Ingo Molnar wrote:
>
> * Matthew Garrett wrote:
>
> > On Thu, May 2, 2019 at 12:15 AM Ard Biesheuvel
> > wrote:
> > >
> > > (+ Ingo)
> > >
> > > On Tue, 30 Apr 2019 at 21:52, Matthew Garrett wrote:
> > > >
> > > > On Tue, Apr 30, 2019 at 6:07 AM Bartosz Szczepanek
> > > > wrote:
> > > > >
> > > > > I may be a little late with this comment, but I've just tested these
> > > > > patches on aarch64 platform (from the top of jjs/master) and got
> > > > > kernel panic ("Unable to handle kernel read", full log at the end of
> > > > > mail). I think there's problem with below call to
> > > > > tpm2_calc_event_log_size(), where physical address of efi.tpm_log is
> > > > > passed as (void *) and never remapped:
> > > >
> > > > Yes, it looks like this is just broken. Can you try with the attached
> > > > patch?
> > >
> > > I'm a bit uncomfortable with EFI code that is obviously broken and
> > > untested being queued for the next merge window in another tree.
> >
> > The patchset was Cc:ed to linux-efi@. Is there anything else I should
> > have done to ensure you picked it up rather than Jarkko?
>
> That's not the workflow rule the Linux kernel is using, if Cc:-ing a
> patchset was the only condition for upstream inclusion then we'd have a
> *LOT* of crap in the Linux kernel.
>
> Just applying those EFI changes without even as much as an Acked-by from
> the EFI maintainers is a *totally* unacceptable workflow.
>
> Please revert/rebase and re-try this on the proper submission channels.
>
> Meanwhile the broken code is NAK-ed by me:
>
>Nacked-by: Ingo Molnar
There must be some kind of misconception here. None of the changes have
been submitted so far. They are only in my master branch. They briefly
went to linux-next through my next branch but as soon as issues were
reported I wiped them off from there (which happened like 2-3 weeks
ago). They haven't been part off any of my PR's.
There is nothing to revert.
/Jarkko
Re: [PATCH V5 2/4] tpm: Reserve the TPM final events table
* Matthew Garrett wrote:
> On Thu, May 2, 2019 at 12:15 AM Ard Biesheuvel
> wrote:
> >
> > (+ Ingo)
> >
> > On Tue, 30 Apr 2019 at 21:52, Matthew Garrett wrote:
> > >
> > > On Tue, Apr 30, 2019 at 6:07 AM Bartosz Szczepanek
> > > wrote:
> > > >
> > > > I may be a little late with this comment, but I've just tested these
> > > > patches on aarch64 platform (from the top of jjs/master) and got
> > > > kernel panic ("Unable to handle kernel read", full log at the end of
> > > > mail). I think there's problem with below call to
> > > > tpm2_calc_event_log_size(), where physical address of efi.tpm_log is
> > > > passed as (void *) and never remapped:
> > >
> > > Yes, it looks like this is just broken. Can you try with the attached
> > > patch?
> >
> > I'm a bit uncomfortable with EFI code that is obviously broken and
> > untested being queued for the next merge window in another tree.
>
> The patchset was Cc:ed to linux-efi@. Is there anything else I should
> have done to ensure you picked it up rather than Jarkko?
That's not the workflow rule the Linux kernel is using, if Cc:-ing a
patchset was the only condition for upstream inclusion then we'd have a
*LOT* of crap in the Linux kernel.
Just applying those EFI changes without even as much as an Acked-by from
the EFI maintainers is a *totally* unacceptable workflow.
Please revert/rebase and re-try this on the proper submission channels.
Meanwhile the broken code is NAK-ed by me:
Nacked-by: Ingo Molnar
Thanks,
Ingo
Re: [PATCH V5 2/4] tpm: Reserve the TPM final events table
On Thu, May 02, 2019 at 11:03:08AM -0700, Matthew Garrett wrote:
> On Thu, May 2, 2019 at 1:32 AM Jarkko Sakkinen
> wrote:
> >
> > On Tue, Apr 30, 2019 at 03:07:09PM +0200, Bartosz Szczepanek wrote:
> > > I may be a little late with this comment, but I've just tested these
> > > patches on aarch64 platform (from the top of jjs/master) and got
> > > kernel panic ("Unable to handle kernel read", full log at the end of
> > > mail). I think there's problem with below call to
> > > tpm2_calc_event_log_size(), where physical address of efi.tpm_log is
> > > passed as (void *) and never remapped:
> >
> > Not late. This is not part of any PR yet. Thank you for the
> > feedback!
> >
> > Matthew, can you send an updated version of the whole patch set
> > with fixes to this issue and also reordering of the includes?
>
> Yes, I'll resend and let's do this again for 5.3.
Agreed, better not rush but get it right once.
/Jarkko
Re: [PATCH V5 2/4] tpm: Reserve the TPM final events table
On Thu, May 02, 2019 at 09:14:49AM +0200, Ard Biesheuvel wrote:
> (+ Ingo)
>
> On Tue, 30 Apr 2019 at 21:52, Matthew Garrett wrote:
> >
> > On Tue, Apr 30, 2019 at 6:07 AM Bartosz Szczepanek
> > wrote:
> > >
> > > I may be a little late with this comment, but I've just tested these
> > > patches on aarch64 platform (from the top of jjs/master) and got
> > > kernel panic ("Unable to handle kernel read", full log at the end of
> > > mail). I think there's problem with below call to
> > > tpm2_calc_event_log_size(), where physical address of efi.tpm_log is
> > > passed as (void *) and never remapped:
> >
> > Yes, it looks like this is just broken. Can you try with the attached patch?
>
> I'm a bit uncomfortable with EFI code that is obviously broken and
> untested being queued for the next merge window in another tree.
>
> What is currently queued there? Can we revert this change for the time
> being, and resubmit it via the EFI tree for v5.3?
Nothing ATM. The broken code is in my master branch ATM. Nothing
in my next branch nor have I included anything to my PRs. Should
be nothing to worry about in that sense :-)
/Jarkko
Re: [PATCH V5 2/4] tpm: Reserve the TPM final events table
Sorry, how about this one? I was confused by why I wasn't hitting
this, but on closer examination it turns out that my system populates
the final event log with 0 events which means we never hit this
codepath :(
diff --git a/drivers/firmware/efi/tpm.c b/drivers/firmware/efi/tpm.c
index 2ccaa6661aaf..db0fdaa9c666 100644
--- a/drivers/firmware/efi/tpm.c
+++ b/drivers/firmware/efi/tpm.c
@@ -28,6 +28,7 @@ static int tpm2_calc_event_log_size(void *data, int count, void *size_info)
if (event_size == 0)
return -1;
size += event_size;
+ count--;
}
return size;
@@ -41,6 +42,7 @@ int __init efi_tpm_eventlog_init(void)
struct linux_efi_tpm_eventlog *log_tbl;
struct efi_tcg2_final_events_table *final_tbl;
unsigned int tbl_size;
+ int ret = 0;
if (efi.tpm_log == EFI_INVALID_TABLE_ADDR) {
/*
@@ -60,10 +62,9 @@ int __init efi_tpm_eventlog_init(void)
tbl_size = sizeof(*log_tbl) + log_tbl->size;
memblock_reserve(efi.tpm_log, tbl_size);
- early_memunmap(log_tbl, sizeof(*log_tbl));
if (efi.tpm_final_log == EFI_INVALID_TABLE_ADDR)
- return 0;
+ goto out;
final_tbl = early_memremap(efi.tpm_final_log, sizeof(*final_tbl));
@@ -71,17 +72,20 @@ int __init efi_tpm_eventlog_init(void)
pr_err("Failed to map TPM Final Event Log table @ 0x%lx\n",
efi.tpm_final_log);
efi.tpm_final_log = EFI_INVALID_TABLE_ADDR;
- return -ENOMEM;
+ ret = -ENOMEM;
+ goto out;
}
tbl_size = tpm2_calc_event_log_size(final_tbl->events,
final_tbl->nr_events,
- (void *)efi.tpm_log);
+ log_tbl->log);
memblock_reserve((unsigned long)final_tbl,
tbl_size + sizeof(*final_tbl));
early_memunmap(final_tbl, sizeof(*final_tbl));
efi_tpm_final_log_size = tbl_size;
- return 0;
+out:
+ early_memunmap(log_tbl, sizeof(*log_tbl));
+ return ret;
}
diff --git a/include/linux/tpm_eventlog.h b/include/linux/tpm_eventlog.h
index dccc97e6135c..190a33968a91 100644
--- a/include/linux/tpm_eventlog.h
+++ b/include/linux/tpm_eventlog.h
@@ -158,7 +158,6 @@ static inline int __calc_tpm2_event_size(struct tcg_pcr_event2_head *event,
{
struct tcg_efi_specid_event_head *efispecid;
struct tcg_event_field *event_field;
- void *mapping = NULL;
int mapping_size;
void *marker;
void *marker_start;
@@ -176,9 +175,9 @@ static inline int __calc_tpm2_event_size(struct tcg_pcr_event2_head *event,
/* Map the event header */
if (do_mapping) {
mapping_size = marker - marker_start;
- mapping = early_memremap((unsigned long)marker_start,
+ event = early_memremap((unsigned long)marker_start,
mapping_size);
- if (!mapping) {
+ if (!event) {
size = 0;
goto out;
}
@@ -199,9 +198,9 @@ static inline int __calc_tpm2_event_size(struct tcg_pcr_event2_head *event,
if (do_mapping) {
early_memunmap(mapping, mapping_size);
mapping_size = marker - marker_start + halg_size;
- mapping = early_memremap((unsigned long)marker_start,
+ event = early_memremap((unsigned long)marker_start,
mapping_size);
- if (!mapping) {
+ if (!event) {
size = 0;
goto out;
}
@@ -219,9 +218,9 @@ static inline int __calc_tpm2_event_size(struct tcg_pcr_event2_head *event,
if (do_mapping) {
early_memunmap(mapping, mapping_size);
mapping_size = marker - marker_start;
- mapping = early_memremap((unsigned long)marker_start,
+ event = early_memremap((unsigned long)marker_start,
mapping_size);
- if (!mapping) {
+ if (!event) {
size = 0;
goto out;
}
@@ -243,11 +242,11 @@ static inline int __calc_tpm2_event_size(struct tcg_pcr_event2_head *event,
* we don't need to map it
*/
if (do_mapping) {
- early_memunmap(marker_start, mapping_size);
+ early_memunmap(event, mapping_size);
mapping_size += sizeof(event_field->event_size);
- mapping = early_memremap((unsigned long)marker_start,
- mapping_size);
- if (!mapping) {
+ event = early_memremap((unsigned long)marker_start,
+ mapping_size);
+ if (!event) {
size = 0;
goto out;
}
@@ -257,8 +256,6 @@ 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))
- size = 0;
out:
if (do_mapping)
early_memunmap(mapping, mapping_size);
Re: [PATCH V5 2/4] tpm: Reserve the TPM final events table
On Thu, May 2, 2019 at 12:15 AM Ard Biesheuvel
wrote:
>
> (+ Ingo)
>
> On Tue, 30 Apr 2019 at 21:52, Matthew Garrett wrote:
> >
> > On Tue, Apr 30, 2019 at 6:07 AM Bartosz Szczepanek
> > wrote:
> > >
> > > I may be a little late with this comment, but I've just tested these
> > > patches on aarch64 platform (from the top of jjs/master) and got
> > > kernel panic ("Unable to handle kernel read", full log at the end of
> > > mail). I think there's problem with below call to
> > > tpm2_calc_event_log_size(), where physical address of efi.tpm_log is
> > > passed as (void *) and never remapped:
> >
> > Yes, it looks like this is just broken. Can you try with the attached patch?
>
> I'm a bit uncomfortable with EFI code that is obviously broken and
> untested being queued for the next merge window in another tree.
The patchset was Cc:ed to linux-efi@. Is there anything else I should
have done to ensure you picked it up rather than Jarkko?
Re: [PATCH V5 2/4] tpm: Reserve the TPM final events table
On Thu, May 2, 2019 at 1:32 AM Jarkko Sakkinen
wrote:
>
> On Tue, Apr 30, 2019 at 03:07:09PM +0200, Bartosz Szczepanek wrote:
> > I may be a little late with this comment, but I've just tested these
> > patches on aarch64 platform (from the top of jjs/master) and got
> > kernel panic ("Unable to handle kernel read", full log at the end of
> > mail). I think there's problem with below call to
> > tpm2_calc_event_log_size(), where physical address of efi.tpm_log is
> > passed as (void *) and never remapped:
>
> Not late. This is not part of any PR yet. Thank you for the
> feedback!
>
> Matthew, can you send an updated version of the whole patch set
> with fixes to this issue and also reordering of the includes?
Yes, I'll resend and let's do this again for 5.3.
Re: [PATCH V5 2/4] tpm: Reserve the TPM final events table
On Tue, Apr 30, 2019 at 03:07:09PM +0200, Bartosz Szczepanek wrote:
> I may be a little late with this comment, but I've just tested these
> patches on aarch64 platform (from the top of jjs/master) and got
> kernel panic ("Unable to handle kernel read", full log at the end of
> mail). I think there's problem with below call to
> tpm2_calc_event_log_size(), where physical address of efi.tpm_log is
> passed as (void *) and never remapped:
Not late. This is not part of any PR yet. Thank you for the
feedback!
Matthew, can you send an updated version of the whole patch set
with fixes to this issue and also reordering of the includes?
/Jarkko
Re: [PATCH V5 2/4] tpm: Reserve the TPM final events table
(+ Ingo)
On Tue, 30 Apr 2019 at 21:52, Matthew Garrett wrote:
>
> On Tue, Apr 30, 2019 at 6:07 AM Bartosz Szczepanek wrote:
> >
> > I may be a little late with this comment, but I've just tested these
> > patches on aarch64 platform (from the top of jjs/master) and got
> > kernel panic ("Unable to handle kernel read", full log at the end of
> > mail). I think there's problem with below call to
> > tpm2_calc_event_log_size(), where physical address of efi.tpm_log is
> > passed as (void *) and never remapped:
>
> Yes, it looks like this is just broken. Can you try with the attached patch?
I'm a bit uncomfortable with EFI code that is obviously broken and
untested being queued for the next merge window in another tree.
What is currently queued there? Can we revert this change for the time
being, and resubmit it via the EFI tree for v5.3?
Re: [PATCH V5 2/4] tpm: Reserve the TPM final events table
Second patch tries to unmap "mapping" which is not declared. I'm on top of jjs/master and your TPM_MEMREMAP patches are already there, so the first patch applied cleanly. Using it, kernel still panicked on boot: EFI stub: Booting Linux Kernel... EFI stub: EFI_RNG_PROTOCOL unavailable, no randomness supplied EFI stub: Using DTB from configuration table EFI stub: Exiting boot services and installing virtual address map... [0.00] Booting Linux on physical CPU 0x00 [0x420f5162] [0.00] Linux version 5.1.0-rc2+ (root@localhost.localdomain) (gcc version 7.3.1 20180712 (Red Hat 7.3.1-6) (GCC)) #78 SMP Wed May 1 01:05:38 EDT 2019 [0.00] earlycon: pl11 at MMIO 0x00040202 (options '115200n8') [0.00] printk: bootconsole [pl11] enabled [0.00] efi: Getting EFI parameters from FDT: [0.00] efi: EFI v2.60 by Cavium Inc. TX2-FW-Release-7.2-build_08-0-g14f8c5bf8a Apr 15 2019 18:51:41 [0.00] efi: TPMFinalLog=0xed5f SMBIOS=0xfad9 SMBIOS 3.0=0xed53 ACPI 2.0=0xeda9 ESRT=0xfafdb218 MEMATTR=0xf8489018 TPMEventLog=0xedaa9018 MEMRESERVE=0xedaa8018 [0.00] Unhandled fault at 0x7dfffe77a018 [0.00] Mem abort info: [0.00] ESR = 0x9603 [0.00] Exception class = DABT (current EL), IL = 32 bits [0.00] SET = 0, FnV = 0 [0.00] EA = 0, S1PTW = 0 [0.00] Data abort info: [0.00] ISV = 0, ISS = 0x0003 [0.00] CM = 0, WnR = 0 [0.00] swapper pgtable: 4k pages, 48-bit VAs, pgdp = (ptrval) [0.00] [7dfffe77a018] pgd=81b12003 [0.00] [ cut here ] [0.00] kernel BUG at arch/arm64/mm/fault.c:189! [0.00] Internal error: Oops - BUG: 0 [#1] SMP [0.00] Modules linked in: [0.00] CPU: 0 PID: 0 Comm: swapper Not tainted 5.1.0-rc2+ #78 [0.00] pstate: 60400089 (nZCv daIf +PAN -UAO) [0.00] pc : show_pte+0x1d0/0x1f0 [0.00] lr : show_pte+0x88/0x1f0 [0.00] sp : 11533b30 [0.00] x29: 11533b30 x28: 115473c0 [0.00] x27: 11542500 x26: 7dfffe73a010 [0.00] x25: 0018 x24: 0025 [0.00] x23: 00fb x22: 117fc000 [0.00] x21: 10f32000 x20: [0.00] x19: 7dfffe77a018 x18: [0.00] x17: x16: [0.00] x15: 1153d708 x14: 1172f420 [0.00] x13: 1172f069 x12: 11568000 [0.00] x11: 11533800 x10: 11533800 [0.00] x9 : 1153ef58 x8 : 303030303030303d [0.00] x7 : 646770205d383130 x6 : 1172e7ff [0.00] x5 : x4 : [0.00] x3 : x2 : [0.00] x1 : 81b12000 x0 : 0ff8 [0.00] Process swapper (pid: 0, stack limit = 0x(ptrval)) [0.00] Call trace: [0.00] show_pte+0x1d0/0x1f0 [0.00] do_mem_abort+0xa8/0xb0 [0.00] el1_da+0x20/0xc4 [0.00] efi_tpm_eventlog_init+0xe8/0x268 [0.00] efi_config_parse_tables+0x180/0x29c [0.00] uefi_init+0x1d0/0x22c [0.00] efi_init+0x90/0x180 [0.00] setup_arch+0x1f4/0x5fc [0.00] start_kernel+0x90/0x51c [0.00] Code: 910d6000 94030b20 17e6 d503201f (d421) [0.00] random: get_random_bytes called from print_oops_end_marker+0x54/0x70 with crng_init=0 [0.00] ---[ end trace ]--- [0.00] Kernel panic - not syncing: Attempted to kill the idle task! [0.00] ---[ end Kernel panic - not syncing: Attempted to kill the idle task! ]---
Re: [PATCH V5 2/4] tpm: Reserve the TPM final events table
On Tue, Apr 30, 2019 at 12:51 PM Matthew Garrett wrote:
> Yes, it looks like this is just broken. Can you try with the attached patch?
Actually, please try this one.
diff --git a/drivers/firmware/efi/tpm.c b/drivers/firmware/efi/tpm.c
index 2ccaa6661aaf..db0fdaa9c666 100644
--- a/drivers/firmware/efi/tpm.c
+++ b/drivers/firmware/efi/tpm.c
@@ -28,6 +28,7 @@ static int tpm2_calc_event_log_size(void *data, int count, void *size_info)
if (event_size == 0)
return -1;
size += event_size;
+ count--;
}
return size;
@@ -41,6 +42,7 @@ int __init efi_tpm_eventlog_init(void)
struct linux_efi_tpm_eventlog *log_tbl;
struct efi_tcg2_final_events_table *final_tbl;
unsigned int tbl_size;
+ int ret = 0;
if (efi.tpm_log == EFI_INVALID_TABLE_ADDR) {
/*
@@ -60,10 +62,9 @@ int __init efi_tpm_eventlog_init(void)
tbl_size = sizeof(*log_tbl) + log_tbl->size;
memblock_reserve(efi.tpm_log, tbl_size);
- early_memunmap(log_tbl, sizeof(*log_tbl));
if (efi.tpm_final_log == EFI_INVALID_TABLE_ADDR)
- return 0;
+ goto out;
final_tbl = early_memremap(efi.tpm_final_log, sizeof(*final_tbl));
@@ -71,17 +72,20 @@ int __init efi_tpm_eventlog_init(void)
pr_err("Failed to map TPM Final Event Log table @ 0x%lx\n",
efi.tpm_final_log);
efi.tpm_final_log = EFI_INVALID_TABLE_ADDR;
- return -ENOMEM;
+ ret = -ENOMEM;
+ goto out;
}
tbl_size = tpm2_calc_event_log_size(final_tbl->events,
final_tbl->nr_events,
- (void *)efi.tpm_log);
+ log_tbl->log);
memblock_reserve((unsigned long)final_tbl,
tbl_size + sizeof(*final_tbl));
early_memunmap(final_tbl, sizeof(*final_tbl));
efi_tpm_final_log_size = tbl_size;
- return 0;
+out:
+ early_memunmap(log_tbl, sizeof(*log_tbl));
+ return ret;
}
diff --git a/include/linux/tpm_eventlog.h b/include/linux/tpm_eventlog.h
index dccc97e6135c..662410710ac0 100644
--- a/include/linux/tpm_eventlog.h
+++ b/include/linux/tpm_eventlog.h
@@ -158,7 +158,6 @@ static inline int __calc_tpm2_event_size(struct tcg_pcr_event2_head *event,
{
struct tcg_efi_specid_event_head *efispecid;
struct tcg_event_field *event_field;
- void *mapping = NULL;
int mapping_size;
void *marker;
void *marker_start;
@@ -176,9 +175,9 @@ static inline int __calc_tpm2_event_size(struct tcg_pcr_event2_head *event,
/* Map the event header */
if (do_mapping) {
mapping_size = marker - marker_start;
- mapping = early_memremap((unsigned long)marker_start,
+ event = early_memremap((unsigned long)marker_start,
mapping_size);
- if (!mapping) {
+ if (!event) {
size = 0;
goto out;
}
@@ -199,9 +198,9 @@ static inline int __calc_tpm2_event_size(struct tcg_pcr_event2_head *event,
if (do_mapping) {
early_memunmap(mapping, mapping_size);
mapping_size = marker - marker_start + halg_size;
- mapping = early_memremap((unsigned long)marker_start,
+ event = early_memremap((unsigned long)marker_start,
mapping_size);
- if (!mapping) {
+ if (!event) {
size = 0;
goto out;
}
@@ -219,9 +218,9 @@ static inline int __calc_tpm2_event_size(struct tcg_pcr_event2_head *event,
if (do_mapping) {
early_memunmap(mapping, mapping_size);
mapping_size = marker - marker_start;
- mapping = early_memremap((unsigned long)marker_start,
+ event = early_memremap((unsigned long)marker_start,
mapping_size);
- if (!mapping) {
+ if (!event) {
size = 0;
goto out;
}
@@ -243,11 +242,11 @@ static inline int __calc_tpm2_event_size(struct tcg_pcr_event2_head *event,
* we don't need to map it
*/
if (do_mapping) {
- early_memunmap(marker_start, mapping_size);
+ early_memunmap(mapping, mapping_size);
mapping_size += sizeof(event_field->event_size);
- mapping = early_memremap((unsigned long)marker_start,
- mapping_size);
- if (!mapping) {
+ event = early_memremap((unsigned long)marker_start,
+ mapping_size);
+ if (!event) {
size = 0;
goto out;
}
@@ -257,8 +256,6 @@ 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))
- size = 0;
out:
if (do_mapping)
early_memunmap(mapping, mapping_size);
Re: [PATCH V5 2/4] tpm: Reserve the TPM final events table
On Tue, Apr 30, 2019 at 6:07 AM Bartosz Szczepanek wrote:
>
> I may be a little late with this comment, but I've just tested these
> patches on aarch64 platform (from the top of jjs/master) and got
> kernel panic ("Unable to handle kernel read", full log at the end of
> mail). I think there's problem with below call to
> tpm2_calc_event_log_size(), where physical address of efi.tpm_log is
> passed as (void *) and never remapped:
Yes, it looks like this is just broken. Can you try with the attached patch?
diff --git a/drivers/firmware/efi/tpm.c b/drivers/firmware/efi/tpm.c
index fe48150f06d1..9711bd34f8ae 100644
--- a/drivers/firmware/efi/tpm.c
+++ b/drivers/firmware/efi/tpm.c
@@ -28,6 +28,7 @@ static int tpm2_calc_event_log_size(void *data, int count, void *size_info)
if (event_size == 0)
return -1;
size += event_size;
+ count--;
}
return size;
@@ -41,6 +42,7 @@ int __init efi_tpm_eventlog_init(void)
struct linux_efi_tpm_eventlog *log_tbl;
struct efi_tcg2_final_events_table *final_tbl;
unsigned int tbl_size;
+ int ret = 0;
if (efi.tpm_log == EFI_INVALID_TABLE_ADDR) {
/*
@@ -60,10 +62,9 @@ int __init efi_tpm_eventlog_init(void)
tbl_size = sizeof(*log_tbl) + log_tbl->size;
memblock_reserve(efi.tpm_log, tbl_size);
- early_memunmap(log_tbl, sizeof(*log_tbl));
if (efi.tpm_final_log == EFI_INVALID_TABLE_ADDR)
- return 0;
+ goto out;
final_tbl = early_memremap(efi.tpm_final_log, sizeof(*final_tbl));
@@ -71,17 +72,20 @@ int __init efi_tpm_eventlog_init(void)
pr_err("Failed to map TPM Final Event Log table @ 0x%lx\n",
efi.tpm_final_log);
efi.tpm_final_log = EFI_INVALID_TABLE_ADDR;
- return -ENOMEM;
+ ret = -ENOMEM;
+ goto out;
}
tbl_size = tpm2_calc_event_log_size(final_tbl->events,
final_tbl->nr_events,
- (void *)efi.tpm_log);
+ log_tbl->log);
memblock_reserve((unsigned long)final_tbl,
tbl_size + sizeof(*final_tbl));
early_memunmap(final_tbl, sizeof(*final_tbl));
efi_tpm_final_log_size = tbl_size;
- return 0;
+out:
+ early_memunmap(log_tbl, sizeof(*log_tbl));
+ return ret;
}
diff --git a/include/linux/tpm_eventlog.h b/include/linux/tpm_eventlog.h
index 0ca27bc053af..9cfbb14f54e6 100644
--- a/include/linux/tpm_eventlog.h
+++ b/include/linux/tpm_eventlog.h
@@ -161,7 +161,6 @@ static inline int __calc_tpm2_event_size(struct tcg_pcr_event2_head *event,
{
struct tcg_efi_specid_event_head *efispecid;
struct tcg_event_field *event_field;
- void *mapping = NULL;
int mapping_size;
void *marker;
void *marker_start;
@@ -179,9 +178,9 @@ static inline int __calc_tpm2_event_size(struct tcg_pcr_event2_head *event,
/* Map the event header */
if (do_mapping) {
mapping_size = marker - marker_start;
- mapping = TPM_MEMREMAP((unsigned long)marker_start,
- mapping_size);
- if (!mapping) {
+ event = TPM_MEMREMAP((unsigned long)marker_start,
+ mapping_size);
+ if (!event) {
size = 0;
goto out;
}
@@ -200,11 +199,11 @@ static inline int __calc_tpm2_event_size(struct tcg_pcr_event2_head *event,
/* Map the digest's algorithm identifier */
if (do_mapping) {
- TPM_MEMUNMAP(mapping, mapping_size);
+ TPM_MEMUNMAP(event, mapping_size);
mapping_size = marker - marker_start + halg_size;
- mapping = TPM_MEMREMAP((unsigned long)marker_start,
- mapping_size);
- if (!mapping) {
+ event = TPM_MEMREMAP((unsigned long)marker_start,
+ mapping_size);
+ if (!event) {
size = 0;
goto out;
}
@@ -220,11 +219,11 @@ static inline int __calc_tpm2_event_size(struct tcg_pcr_event2_head *event,
/* Map the digest content itself */
if (do_mapping) {
- TPM_MEMUNMAP(mapping, mapping_size);
+ TPM_MEMUNMAP(event, mapping_size);
mapping_size = marker - marker_start;
- mapping = TPM_MEMREMAP((unsigned long)marker_start,
- mapping_size);
- if (!mapping) {
+ event = TPM_MEMREMAP((unsigned long)marker_start,
+ mapping_size);
+ if (!event) {
size = 0;
goto out;
}
@@ -246,11 +245,11 @@ static inline int __calc_tpm2_event_size(struct tcg_pcr_event2_head *event,
* we don't need to map it
*/
if (do_mapping) {
- TPM_MEMUNMAP(marker_start, mapping_size);
+ TPM_MEMUNMAP(event, mapping_size);
mapping_size += sizeof(event_field->event_size);
- mapping = TPM_MEMREMAP((unsigned long)marker_start,
+ event = TPM_MEMREMAP((unsigned long)marker_start,
mapping_size);
- if (!mapping) {
+ if (!event) {
size = 0;
goto out;
}
@@ -260,11 +259,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))
- size = 0;
out:
if (do_mapping)
- TPM_MEMUNMAP(mapping, mapping_size);
+ TPM_MEMUNMAP(event, mapping_size);
return size;
}
Re: [PATCH V5 2/4] tpm: Reserve the TPM final events table
I may be a little late with this comment, but I've just tested these
patches on aarch64 platform (from the top of jjs/master) and got
kernel panic ("Unable to handle kernel read", full log at the end of
mail). I think there's problem with below call to
tpm2_calc_event_log_size(), where physical address of efi.tpm_log is
passed as (void *) and never remapped:
> + tbl_size = tpm2_calc_event_log_size(final_tbl->events,
> + final_tbl->nr_events,
> + (void *)efi.tpm_log);
This is later used to get efispecid:
> efispecid = (struct tcg_efi_specid_event_head *)event_header->event;
It seems event_header is not mapped during dereference. This is
somewhat expected, because it comes from different, already unmapped
memory region (region of initial TPM log) than "event" itself (which
comes from TPM final log).
Also, value passed as size_info shouldn't be pointing to
linux_efi_tpm_eventlog with its size and version fields, but to the
first event (header event) within. I tried with log_tbl->log and it
worked fine (I omitted unmapping part). On the other hand, with bare
log_tbl it still fails. Not sure how does it even work on other
platforms.
One more thing that's not clear for me – shouldn't the value returned
from early_memremap be used for further accesses? Throughout
__calc_tpm2_event_size() "mapping" is only checked for being zero.
When it is, you're still unmapping it – is it correct?
> + while (count > 0) {
> + header = data + size;
> + event_size = __calc_tpm2_event_size(header, size_info, true);
> + if (event_size == 0)
> + return -1;
> + size += event_size;
> + }
Loop condition here is always true, by the way.
One information about my setup – I'm working with below local diff to
enable operation on ARM:
> --- a/drivers/firmware/efi/libstub/arm-stub.c
> +++ b/drivers/firmware/efi/libstub/arm-stub.c
> @@ -194,6 +194,7 @@ unsigned long efi_entry(void *handle, efi_system_table_t
> *sys_table,
>
> /* Ask the firmware to clear memory on unclean shutdown */
>efi_enable_reset_attack_mitigation(sys_table);
> + efi_retrieve_tpm2_eventlog(sys_table);
Full log of kernel panic follows.
EFI stub: Booting Linux Kernel...
EFI stub: EFI_RNG_PROTOCOL unavailable, no randomness supplied
EFI stub: Using DTB from configuration table
EFI stub: Exiting boot services and installing virtual address map...
[0.00] Booting Linux on physical CPU 0x00 [0x420f5162]
[0.00] Linux version 5.1.0-rc2+ (root@localhost.localdomain)
(gcc version 7.3.1 20180712 (Red Hat 7.3.1-6) (GCC)) #69 SMP Fri Apr
26 03:20:57 EDT 2019
[0.00] earlycon: pl11 at MMIO 0x00040202 (options '115200n8')
[0.00] printk: bootconsole [pl11] enabled
[0.00] efi: Getting EFI parameters from FDT:
[0.00] efi: EFI v2.60 by Cavium Inc.
TX2-FW-Release-7.2-build_08-0-g14f8c5bf8a Apr 15 2019 18:51:41
[0.00] efi: TPMFinalLog=0xed5f SMBIOS=0xfad9 SMBIOS
3.0=0xed53 ACPI 2.0=0xeda9 ESRT=0xfafdb218
MEMATTR=0xf8489018 TPMEventLog=0xedaa9018 MEMRESERVE=0xedaa8018
[0.00] Unable to handle kernel read from unreadable memory at
virtual address edaa9050
[0.00] Mem abort info:
[0.00] ESR = 0x9604
[0.00] Exception class = DABT (current EL), IL = 32 bits
[0.00] SET = 0, FnV = 0
[0.00] EA = 0, S1PTW = 0
[0.00] Data abort info:
[0.00] ISV = 0, ISS = 0x0004
[0.00] CM = 0, WnR = 0
[0.00] [edaa9050] user address but active_mm is swapper
[0.00] Internal error: Oops: 9604 [#1] SMP
[0.00] Modules linked in:
[0.00] CPU: 0 PID: 0 Comm: swapper Not tainted 5.1.0-rc2+ #69
[0.00] pstate: 60400089 (nZCv daIf +PAN -UAO)
[0.00] pc : efi_tpm_eventlog_init+0xfc/0x26c
[0.00] lr : efi_tpm_eventlog_init+0xf4/0x26c
[0.00] sp : 11533ce0
[0.00] x29: 11533ce0 x28: edaa8018
[0.00] x27: 7dfffe6fa010 x26: 0023
[0.00] x25: 7dfffe6fa000 x24: edaa9038
[0.00] x23: x22: 7dfffe6fa010
[0.00] x21: 1153d000 x20: 7dfffe6fa018
[0.00] x19: 11542500 x18:
[0.00] x17: 0435 x16:
[0.00] x15: 1153d708 x14: 6576454d50542020
[0.00] x13: 3831303938343866 x12: 78303d525454414d
[0.00] x11: 454d202038313262 x10: 6466616678303d54
[0.00] x9 : 1153ef58 x8 : 0200
[0.00] x7 : 0a30 x6 : 110d2a18
[0.00] x5 : 013a x4 : 04c5
[0.00] x3 : 11714000 x2 : 0002
[0.00] x1 : 7dfffe6fa000 x0 : 7dfffe73a010
[0.00] Process
[PATCH V5 2/4] tpm: Reserve the TPM final events table
From: Matthew Garrett
UEFI systems provide a boot services protocol for obtaining the TPM
event log, but this is unusable after ExitBootServices() is called.
Unfortunately ExitBootServices() itself triggers additional TPM events
that then can't be obtained using this protocol. The platform provides a
mechanism for the OS to obtain these events by recording them to a
separate UEFI configuration table which the OS can then map.
Unfortunately this table isn't self describing in terms of providing its
length, so we need to parse the events inside it to figure out how long
it is. Since the table isn't mapped at this point, we need to extend the
length calculation function to be able to map the event as it goes
along.
Signed-off-by: Matthew Garrett
---
drivers/char/tpm/eventlog/tpm2.c | 2 +-
drivers/firmware/efi/efi.c | 2 +
drivers/firmware/efi/tpm.c | 51 -
include/linux/efi.h | 9 +++
include/linux/tpm_eventlog.h | 94 +---
5 files changed, 148 insertions(+), 10 deletions(-)
diff --git a/drivers/char/tpm/eventlog/tpm2.c b/drivers/char/tpm/eventlog/tpm2.c
index 1a977bdd3bd2..de1d9f7e5a92 100644
--- a/drivers/char/tpm/eventlog/tpm2.c
+++ b/drivers/char/tpm/eventlog/tpm2.c
@@ -40,7 +40,7 @@
static size_t calc_tpm2_event_size(struct tcg_pcr_event2_head *event,
struct tcg_pcr_event *event_header)
{
- return __calc_tpm2_event_size(event, event_header);
+ return __calc_tpm2_event_size(event, event_header, false);
}
static void *tpm2_bios_measurements_start(struct seq_file *m, loff_t *pos)
diff --git a/drivers/firmware/efi/efi.c b/drivers/firmware/efi/efi.c
index 4c46ff6f2242..bf4e9a254e23 100644
--- a/drivers/firmware/efi/efi.c
+++ b/drivers/firmware/efi/efi.c
@@ -53,6 +53,7 @@ struct efi __read_mostly efi = {
.mem_attr_table = EFI_INVALID_TABLE_ADDR,
.rng_seed = EFI_INVALID_TABLE_ADDR,
.tpm_log= EFI_INVALID_TABLE_ADDR,
+ .tpm_final_log = EFI_INVALID_TABLE_ADDR,
.mem_reserve= EFI_INVALID_TABLE_ADDR,
};
EXPORT_SYMBOL(efi);
@@ -485,6 +486,7 @@ static __initdata efi_config_table_type_t common_tables[] =
{
{EFI_MEMORY_ATTRIBUTES_TABLE_GUID, "MEMATTR", _attr_table},
{LINUX_EFI_RANDOM_SEED_TABLE_GUID, "RNG", _seed},
{LINUX_EFI_TPM_EVENT_LOG_GUID, "TPMEventLog", _log},
+ {LINUX_EFI_TPM_FINAL_LOG_GUID, "TPMFinalLog", _final_log},
{LINUX_EFI_MEMRESERVE_TABLE_GUID, "MEMRESERVE", _reserve},
{NULL_GUID, NULL, NULL},
};
diff --git a/drivers/firmware/efi/tpm.c b/drivers/firmware/efi/tpm.c
index 0cbeb3d46b18..2ccaa6661aaf 100644
--- a/drivers/firmware/efi/tpm.c
+++ b/drivers/firmware/efi/tpm.c
@@ -10,24 +10,50 @@
#include
#include
#include
+#include
#include
+int efi_tpm_final_log_size;
+EXPORT_SYMBOL(efi_tpm_final_log_size);
+
+static int tpm2_calc_event_log_size(void *data, int count, void *size_info)
+{
+ struct tcg_pcr_event2_head *header;
+ int event_size, size = 0;
+
+ while (count > 0) {
+ header = data + size;
+ event_size = __calc_tpm2_event_size(header, size_info, true);
+ if (event_size == 0)
+ return -1;
+ size += event_size;
+ }
+
+ return size;
+}
+
/*
* Reserve the memory associated with the TPM Event Log configuration table.
*/
int __init efi_tpm_eventlog_init(void)
{
struct linux_efi_tpm_eventlog *log_tbl;
+ struct efi_tcg2_final_events_table *final_tbl;
unsigned int tbl_size;
- if (efi.tpm_log == EFI_INVALID_TABLE_ADDR)
+ if (efi.tpm_log == EFI_INVALID_TABLE_ADDR) {
+ /*
+* We can't calculate the size of the final events without the
+* first entry in the TPM log, so bail here.
+*/
return 0;
+ }
log_tbl = early_memremap(efi.tpm_log, sizeof(*log_tbl));
if (!log_tbl) {
pr_err("Failed to map TPM Event Log table @ 0x%lx\n",
- efi.tpm_log);
+ efi.tpm_log);
efi.tpm_log = EFI_INVALID_TABLE_ADDR;
return -ENOMEM;
}
@@ -35,6 +61,27 @@ int __init efi_tpm_eventlog_init(void)
tbl_size = sizeof(*log_tbl) + log_tbl->size;
memblock_reserve(efi.tpm_log, tbl_size);
early_memunmap(log_tbl, sizeof(*log_tbl));
+
+ if (efi.tpm_final_log == EFI_INVALID_TABLE_ADDR)
+ return 0;
+
+ final_tbl = early_memremap(efi.tpm_final_log, sizeof(*final_tbl));
+
+ if (!final_tbl) {
+ pr_err("Failed to map TPM Final Event Log table @ 0x%lx\n",
+ efi.tpm_final_log);
+ efi.tpm_final_log = EFI_INVALID_TABLE_ADDR;
+ return -ENOMEM;
+ }
+
+ tbl_size =
