Re: Regression from efi: call get_event_log before ExitBootServices
From: Hans de Goede
Date: Tue Mar 13 2018 - 03:47:21 EST
Hi,
On 12-03-18 20:55, Thiebaud Weksteen wrote:
On Mon, Mar 12, 2018 at 7:33 PM Jeremy Cline <jeremy@xxxxxxxxxx> wrote:
On 03/12/2018 02:29 PM, Thiebaud Weksteen wrote:
On Mon, Mar 12, 2018 at 6:30 PM Ard Biesheuvel <
ard.biesheuvel@xxxxxxxxxx>
wrote:
On 12 March 2018 at 17:01, Jeremy Cline <jeremy@xxxxxxxxxx> wrote:
On 03/12/2018 10:56 AM, Ard Biesheuvel wrote:
On 12 March 2018 at 14:30, Jeremy Cline <jeremy@xxxxxxxxxx> wrote:
On 03/12/2018 07:08 AM, Ard Biesheuvel wrote:
On 10 March 2018 at 10:45, Thiebaud Weksteen <tweek@xxxxxxxxxx>
wrote:
On Fri, Mar 9, 2018 at 5:54 PM Jeremy Cline <jeremy@xxxxxxxxxx>
wrote:
On Fri, Mar 09, 2018 at 10:43:50AM +0000, Thiebaud Weksteen
wrote:
Thanks a lot for trying out the patch!
Please don't modify your install at this stage, I think we are
hitting a
firmware bug and that would be awesome if we can fix how we are
handling it.
So, if we reach that stage in the function it could either be
that:
* The allocation did not succeed, somehow, but the firmware
still
returned
EFI_SUCCEED.
* The size requested is incorrect (I'm thinking something like a
1G of
log). This would be due to either a miscalculation of log_size
(possible)
or; the returned values of GetEventLog are not correct.
I'm sending a patch to add checks for these. Could you please
apply and
retest?
Again, thanks for helping debugging this.
No problem, thanks for the help :)
With the new patch:
Locating the TCG2Protocol
Calling GetEventLog on TCG2Protocol
Log returned
log_location is not empty
log_size != 0
log_size < 1M
Allocating memory for storing the logs
Returned from memory allocation
Copying log to new location
And then it hangs. I added a couple more print statements:
diff --git a/drivers/firmware/efi/libstub/tpm.c
b/drivers/firmware/efi/libstub/tpm.c
index ee3fac109078..1ab5638bc50e 100644
--- a/drivers/firmware/efi/libstub/tpm.c
+++ b/drivers/firmware/efi/libstub/tpm.c
@@ -148,8 +148,11 @@ void
efi_retrieve_tpm2_eventlog_1_2(efi_system_table_t *sys_table_arg)
efi_printk(sys_table_arg, "Copying log to new
location\n");
memset(log_tbl, 0, sizeof(*log_tbl) + log_size);
+ efi_printk(sys_table_arg, "Successfully memset log_tbl to
0\n");
log_tbl->size = log_size;
+ efi_printk(sys_table_arg, "Set log_tbl->size\n");
log_tbl->version = EFI_TCG2_EVENT_LOG_FORMAT_TCG_1_2;
+ efi_printk(sys_table_arg, "Set log_tbl-version\n");
memcpy(log_tbl->log, (void *) first_entry_addr,
log_size);
efi_printk(sys_table_arg, "Installing the log into the
configuration table\n");
and it's hanging at "memset(log_tbl, 0, sizeof(*log_tbl) +
log_size);"
Thanks. Well, it looks like the memory that is supposedly
allocated
is not
usable. I'm thinking this is a firmware bug.
Ard, would you agree on this assumption? Thoughts on how to
proceed?
I am rather puzzled why the allocate_pool() should succeed and the
subsequent memset() should fail. This does not look like an issue
that
is intimately related to TPM2 support, rather an issue in the
firmware
that happens to get tickled after the change.
Would you mind trying replacing EFI_LOADER_DATA with
EFI_BOOT_SERVICES_DATA in the allocate_pool() call?
Replacing EFI_LOADER_DATA with EFI_BOOT_SERVICES_DATA still hangs at
the
memset() call.
Could you try the following please? (attached as well in case gmail
mangles it)
diff --git a/drivers/firmware/efi/libstub/tpm.c
b/drivers/firmware/efi/libstub/tpm.c
index 2298560cea72..30d960a344b7 100644
--- a/drivers/firmware/efi/libstub/tpm.c
+++ b/drivers/firmware/efi/libstub/tpm.c
@@ -70,6 +70,8 @@ void
efi_retrieve_tpm2_eventlog_1_2(efi_system_table_t *sys_table_arg)
size_t log_size, last_entry_size;
efi_bool_t truncated;
void *tcg2_protocol;
+ unsigned long num_pages;
+ efi_physical_addr_t log_tbl_alloc;
status = efi_call_early(locate_protocol, &tcg2_guid, NULL,
&tcg2_protocol);
@@ -104,9 +106,12 @@ void
efi_retrieve_tpm2_eventlog_1_2(efi_system_table_t *sys_table_arg)
}
/* Allocate space for the logs and copy them. */
- status = efi_call_early(allocate_pool, EFI_LOADER_DATA,
- sizeof(*log_tbl) + log_size,
- (void **) &log_tbl);
+ num_pages = DIV_ROUND_UP(sizeof(*log_tbl) + log_size,
EFI_PAGE_SIZE);
+ status = efi_call_early(allocate_pages,
+ EFI_ALLOCATE_ANY_PAGES,
+ EFI_LOADER_DATA,
+ num_pages,
+ &log_tbl_alloc);
if (status != EFI_SUCCESS) {
efi_printk(sys_table_arg,
@@ -114,6 +119,7 @@ void
efi_retrieve_tpm2_eventlog_1_2(efi_system_table_t *sys_table_arg)
return;
}
+ log_tbl = (struct linux_efi_tpm_eventlog *)(unsigned
long)log_tbl_alloc;
memset(log_tbl, 0, sizeof(*log_tbl) + log_size);
log_tbl->size = log_size;
log_tbl->version = EFI_TCG2_EVENT_LOG_FORMAT_TCG_1_2;
@@ -126,7 +132,7 @@ void
efi_retrieve_tpm2_eventlog_1_2(efi_system_table_t *sys_table_arg)
return;
err_free:
- efi_call_early(free_pool, log_tbl);
+ efi_call_early(free_pages, log_tbl_alloc, num_pages);
}
void efi_retrieve_tpm2_eventlog(efi_system_table_t *sys_table_arg)
Hi Ard,
When I apply this, it starts hanging at
status = efi_call_proto(efi_tcg2_protocol, get_event_log,
tcg2_protocol,
EFI_TCG2_EVENT_LOG_FORMAT_TCG_1_2,
&log_location, &log_last_entry, &truncated);
rather than at the memset() call.
That is *very* surprising, given that the change only affects code
that executes after that.
Hans, you said you configured the tablet to use the 32-bit version of grub
instead
of 64. Why's that?
Because this tablet, like (almost?) all Bay Trail hardware has a 32 bit UEFI,
even though the processor is 64 bit capable (AFAIK 64 bit Windows drivers were
not ready in time so all Bay Trail devices shipped with a 32 bit Windows).
So this is running a 32 bit grub which boots a 64 bit kernel.
Jeremy, could you confirm if you are building the kernel in 64bit mode? Is
your Android install working? (that is, what happens if you boot Boot0000)?
AFAIK the kernel on Jeremy's tablet (which I initially installed) is 64 bit.
Could the problem perhaps be that the new code for the TPM event-log is
missing some handling to deal with running on a 32 bit firmware? I know the
rest of the kernel has special code to deal with this.
Regards,
Hans