Hi,

On 12-03-18 22:02, Ard Biesheuvel wrote:

On 12 March 2018 at 19:55, Thiebaud Weksteen [off-list ref] wrote:

quoted

On Mon, Mar 12, 2018 at 7:33 PM Jeremy Cline [off-list ref] wrote:

quoted

On 03/12/2018 02:29 PM, Thiebaud Weksteen wrote:

quoted

On Mon, Mar 12, 2018 at 6:30 PM Ard Biesheuvel <

ard.biesheuvel@linaro.org>

quoted

quoted

wrote:

quoted

On 12 March 2018 at 17:01, Jeremy Cline [off-list ref] wrote:

quoted

On 03/12/2018 10:56 AM, Ard Biesheuvel wrote:

quoted

On 12 March 2018 at 14:30, Jeremy Cline [off-list ref] wrote:

quoted

On 03/12/2018 07:08 AM, Ard Biesheuvel wrote:

quoted

On 10 March 2018 at 10:45, Thiebaud Weksteen [off-list ref]

wrote:

quoted

quoted

quoted

quoted

quoted

quoted

On Fri, Mar 9, 2018 at 5:54 PM Jeremy Cline [off-list ref]

wrote:

quoted

quoted

quoted

quoted

quoted

quoted

quoted

On Fri, Mar 09, 2018 at 10:43:50AM +0000, Thiebaud Weksteen

wrote:

quoted

quoted

quoted

quoted

quoted

quoted

quoted

quoted

quoted

quoted

Thanks a lot for trying out the patch!

Please don't modify your install at this stage, I think we are

hitting a

quoted

quoted

quoted

quoted

quoted

quoted

quoted

quoted

firmware bug and that would be awesome if we can fix how we are

handling it.

quoted

quoted

So, if we reach that stage in the function it could either be

that:

quoted

quoted

quoted

quoted

quoted

quoted

quoted

quoted

* The allocation did not succeed, somehow, but the firmware

still

quoted

quoted

quoted

quoted

quoted

quoted

quoted

quoted

returned

quoted

quoted

EFI_SUCCEED.
* The size requested is incorrect (I'm thinking something like a

1G of

quoted

quoted

quoted

quoted

quoted

quoted

quoted

quoted

log). This would be due to either a miscalculation of log_size

(possible)

quoted

quoted

or; the returned values of GetEventLog are not correct.
I'm sending a patch to add checks for these. Could you please

apply and

quoted

quoted

quoted

quoted

quoted

quoted

quoted

quoted

retest?
Again, thanks for helping debugging this.

quoted

No problem, thanks for the help :)

quoted

With the new patch:

quoted

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

quoted

And then it hangs. I added a couple more print statements:

quoted

diff --git a/drivers/firmware/efi/libstub/tpm.c

b/drivers/firmware/efi/libstub/tpm.c

quoted

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)

quoted

          efi_printk(sys_table_arg, "Copying log to new

location\n");

quoted

quoted

quoted

quoted

quoted

quoted

quoted

          memset(log_tbl, 0, sizeof(*log_tbl) + log_size);
+       efi_printk(sys_table_arg, "Successfully memset log_tbl to

0\n");

quoted

quoted

quoted

quoted

quoted

quoted

quoted

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

quoted

quoted

quoted

quoted

quoted

quoted

quoted

quoted

quoted

          efi_printk(sys_table_arg, "Installing the log into the

configuration table\n");

quoted

and it's hanging at "memset(log_tbl, 0, sizeof(*log_tbl) +

log_size);"

quoted

quoted

quoted

quoted

quoted

quoted

Thanks. Well, it looks like the memory that is supposedly

allocated

quoted

quoted

is not

quoted

quoted

quoted

quoted

quoted

quoted

usable. I'm thinking this is a firmware bug.
Ard, would you agree on this assumption? Thoughts on how to

proceed?

quoted

quoted

quoted

quoted

quoted

quoted

quoted

quoted

I am rather puzzled why the allocate_pool() should succeed and the
subsequent memset() should fail. This does not look like an issue

that

quoted

quoted

quoted

quoted

quoted

is intimately related to TPM2 support, rather an issue in the

firmware

quoted

quoted

quoted

quoted

quoted

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

quoted

quoted

quoted

quoted

memset() call.

Could you try the following please? (attached as well in case gmail

mangles it)

quoted

quoted

quoted

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

quoted

quoted

quoted

+       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;

quoted

quoted

quoted

         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,

quoted

quoted

quoted

quoted

                         EFI_TCG2_EVENT_LOG_FORMAT_TCG_1_2,
                         &log_location, &log_last_entry, &truncated);

rather than at the memset() call.

quoted

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?

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

quoted

quoted

quoted

I understand how annoying this is for you, and I think we should try
to fix this, but reverting the patches outright isn't the solution
either.

quoted

Which UEFI vendor and version does your system report?

You should be able to find this info using the "ver" command in the UEFI
shell.
The UEFI vendor is Insyde (see first message).

quoted

Ah, thanks!

quoted

EFI Specification Revision      : 2.40
EFI Vendor                      : INSYDE Corp.
EFI Revision                    : 21573.83

Thiebaud,

If we don't manage to resolve this, do you see any way to blacklist
systems based on this information? Would it be reasonable, say, to
require UEFI v2.5 or later for TPM2 support? Or doesn't that make any
sense (I am aware that the TCG EFI spec and the UEFI spec are somewhat
orthogonal, but it also depends on the hardware you are targetting, I
guess). Otherwise, we could use a more specific match, perhaps?

This is of course depending on whether we reach consensus on whether
we should make any changes at all for what appears to be a single
sample of a certain piece of hardware, where other samples running the
same firmware (right?) are working fine.

Right, I don't think a blacklist is a good idea until we understand the
problem better. Both the hard and firmware of Jeremy's tablet are pretty
generic, so I don't think there is anything special there.

One of the reason why this may work on my tablet of the same model is
because I do use shim (Jeremy does not) + a different grub version,
which perhaps leads to a different memory layout or different parts
of memory being initialized...

Regards,

Hans