Re: Regression from efi: call get_event_log before ExitBootServices

[Jarkko Sakkinen]

On Mon, Mar 12, 2018 at 11:41:25AM +0100, Paul Menzel wrote:
> Dear Jarkko,
> 
> 
> On 03/12/18 11:17, Jarkko Sakkinen wrote:
> > On Sat, 2018-03-10 at 10:45 +, Thiebaud Weksteen wrote:
> > > On Fri, Mar 9, 2018 at 5:54 PM Jeremy Cline  wrote:
> > > > 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?
> > 
> > Check if the BIOS is up to date?
> 
> How would that help? The no regression policy demands, that Linux continues
> working on systems, where it worked before. So upgrading the firmware is no
> solution, is it? Until a solution is found, the commits should be reverted.

Nope. You are right.

/Jarkko
--
To unsubscribe from this list: send the line "unsubscribe linux-efi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Regression from efi: call get_event_log before ExitBootServices

[Ard Biesheuvel]

On 13 March 2018 at 13:41, Jeremy Cline  wrote:
> On 03/13/2018 03:59 AM, Ard Biesheuvel wrote:
>> On 13 March 2018 at 07:47, Hans de Goede  wrote:
>>> Hi,
>>>
>>>
>>> On 12-03-18 20:55, Thiebaud Weksteen wrote:

>> ...

 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
 Boot)?
>>>
>>>
>>> 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.
>>>
>>
>> That is a very good point, and I missed that this is a 64-bit kernel
>> running on 32-bit UEFI.
>>
>> The TPM code does use efi_call_proto() directly, and now I am thinking
>> it is perhaps the allocate_pages() call that simply only initializes
>> the low 32-bits of log_tbl.
>>
>> Jeremy, could you please try initializing tcg2_protocol and log_tbl to
>> NULL at the start of the function?
>>
>
> That was it, it boots when those are initialized to NULL.
>

Thanks for confirming. I'll send out a patch.
--
To unsubscribe from this list: send the line "unsubscribe linux-efi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Regression from efi: call get_event_log before ExitBootServices

[Jeremy Cline]

On 03/13/2018 03:59 AM, Ard Biesheuvel wrote:
> On 13 March 2018 at 07:47, Hans de Goede  wrote:
>> Hi,
>>
>>
>> On 12-03-18 20:55, Thiebaud Weksteen wrote:
>>>
> ...
>>>
>>> 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
>>> Boot)?
>>
>>
>> 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.
>>
> 
> That is a very good point, and I missed that this is a 64-bit kernel
> running on 32-bit UEFI.
> 
> The TPM code does use efi_call_proto() directly, and now I am thinking
> it is perhaps the allocate_pages() call that simply only initializes
> the low 32-bits of log_tbl.
> 
> Jeremy, could you please try initializing tcg2_protocol and log_tbl to
> NULL at the start of the function?
> 

That was it, it boots when those are initialized to NULL.

Regards,
Jeremy
--
To unsubscribe from this list: send the line "unsubscribe linux-efi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Regression from efi: call get_event_log before ExitBootServices

[Andy Shevchenko]

On Tue, Mar 13, 2018 at 9:47 AM, Hans de Goede  wrote:
> On 12-03-18 20:55, Thiebaud Weksteen wrote:

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

Almost.

Lenovo Thinkpad 10 tablet has 64-bit firmware.

-- 
With Best Regards,
Andy Shevchenko
--
To unsubscribe from this list: send the line "unsubscribe linux-efi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Regression from efi: call get_event_log before ExitBootServices

[Ard Biesheuvel]

On 13 March 2018 at 10:23, Thiebaud Weksteen  wrote:
> On Tue, Mar 13, 2018 at 8:59 AM Ard Biesheuvel 
> wrote:
>
>> On 13 March 2018 at 07:47, Hans de Goede  wrote:
...
>> > 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.
>> >
>
> Yes, that was my guess as well.
>
>> That is a very good point, and I missed that this is a 64-bit kernel
>> running on 32-bit UEFI.
>
>> The TPM code does use efi_call_proto() directly, and now I am thinking
>> it is perhaps the allocate_pages() call that simply only initializes
>> the low 32-bits of log_tbl.
>
> That make sense. Would you know what happens to the arguments of the
> function in this case? (I'm thinking _location ?)

log_location and log_last_entry are always 64-bit quantities, and
efi_bool_t is always u8, so that shouldn't matter.

> Would it be safer to skip the code completely on EFI_MIXED systems?
>

Obviously, but I would like to avoid that if possible. Let's see first
if we've pinpointed it now.
--
To unsubscribe from this list: send the line "unsubscribe linux-efi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Regression from efi: call get_event_log before ExitBootServices

[Thiebaud Weksteen]

On Tue, Mar 13, 2018 at 8:59 AM Ard Biesheuvel 
wrote:

> On 13 March 2018 at 07:47, Hans de Goede  wrote:
> > Hi,
> >
> >
> > On 12-03-18 20:55, Thiebaud Weksteen wrote:
> >>
> ...
> >>
> >> 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
> >> Boot)?
> >
> >
> > 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.
> >

Yes, that was my guess as well.

> That is a very good point, and I missed that this is a 64-bit kernel
> running on 32-bit UEFI.

> The TPM code does use efi_call_proto() directly, and now I am thinking
> it is perhaps the allocate_pages() call that simply only initializes
> the low 32-bits of log_tbl.

That make sense. Would you know what happens to the arguments of the
function in this case? (I'm thinking _location ?)
Would it be safer to skip the code completely on EFI_MIXED systems?

> Jeremy, could you please try initializing tcg2_protocol and log_tbl to
> NULL at the start of the function?
--
To unsubscribe from this list: send the line "unsubscribe linux-efi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Regression from efi: call get_event_log before ExitBootServices

[Hans de Goede]

Hi,

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

On 12 March 2018 at 19:55, Thiebaud Weksteen  wrote:

On Mon, Mar 12, 2018 at 7:33 PM Jeremy Cline  wrote:


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

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

ard.biesheu...@linaro.org>

wrote:


On 12 March 2018 at 17:01, Jeremy Cline  wrote:

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

On 12 March 2018 at 14:30, Jeremy Cline  wrote:

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

On 10 March 2018 at 10:45, Thiebaud Weksteen 

wrote:

On Fri, Mar 9, 2018 at 5:54 PM Jeremy Cline 

wrote:



On Fri, Mar 09, 2018 at 10:43:50AM +, 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, _guid, NULL,
 _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 **) _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,
+   _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 = 

Re: Regression from efi: call get_event_log before ExitBootServices

[Ard Biesheuvel]

On 13 March 2018 at 07:59, Ard Biesheuvel  wrote:
> On 13 March 2018 at 07:47, Hans de Goede  wrote:
>> Hi,
>>
>>
>> On 12-03-18 20:55, Thiebaud Weksteen wrote:
>>>
> ...
>>>
>>> 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
>>> Boot)?
>>
>>
>> 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.
>>
>
> That is a very good point, and I missed that this is a 64-bit kernel
> running on 32-bit UEFI.
>
> The TPM code does use efi_call_proto() directly,

*correctly*

> and now I am thinking
> it is perhaps the allocate_pages() call that simply only initializes
> the low 32-bits of log_tbl.
>
> Jeremy, could you please try initializing tcg2_protocol and log_tbl to
> NULL at the start of the function?
--
To unsubscribe from this list: send the line "unsubscribe linux-efi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Regression from efi: call get_event_log before ExitBootServices

[Ard Biesheuvel]

On 13 March 2018 at 07:47, Hans de Goede  wrote:
> Hi,
>
>
> On 12-03-18 20:55, Thiebaud Weksteen wrote:
>>
...
>>
>> 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
>> Boot)?
>
>
> 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.
>

That is a very good point, and I missed that this is a 64-bit kernel
running on 32-bit UEFI.

The TPM code does use efi_call_proto() directly, and now I am thinking
it is perhaps the allocate_pages() call that simply only initializes
the low 32-bits of log_tbl.

Jeremy, could you please try initializing tcg2_protocol and log_tbl to
NULL at the start of the function?
--
To unsubscribe from this list: send the line "unsubscribe linux-efi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Regression from efi: call get_event_log before ExitBootServices

[Hans de Goede]

Hi,

On 12-03-18 20:55, Thiebaud Weksteen wrote:

On Mon, Mar 12, 2018 at 7:33 PM Jeremy Cline  wrote:


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

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

ard.biesheu...@linaro.org>

wrote:


On 12 March 2018 at 17:01, Jeremy Cline  wrote:

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

On 12 March 2018 at 14:30, Jeremy Cline  wrote:

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

On 10 March 2018 at 10:45, Thiebaud Weksteen 

wrote:

On Fri, Mar 9, 2018 at 5:54 PM Jeremy Cline 

wrote:



On Fri, Mar 09, 2018 at 10:43:50AM +, 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, _guid, NULL,
 _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 **) _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,
+   _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;

 

Re: Regression from efi: call get_event_log before ExitBootServices

[Thiebaud Weksteen]

On Mon, Mar 12, 2018 at 10:03 PM Ard Biesheuvel 
wrote:

> On 12 March 2018 at 19:55, Thiebaud Weksteen  wrote:
> > On Mon, Mar 12, 2018 at 7:33 PM Jeremy Cline  wrote:
> >
> >> On 03/12/2018 02:29 PM, Thiebaud Weksteen wrote:
> >> > On Mon, Mar 12, 2018 at 6:30 PM Ard Biesheuvel <
> > ard.biesheu...@linaro.org>
> >> > wrote:
> >> >
> >> >> On 12 March 2018 at 17:01, Jeremy Cline  wrote:
> >> >>> On 03/12/2018 10:56 AM, Ard Biesheuvel wrote:
> >>  On 12 March 2018 at 14:30, Jeremy Cline  wrote:
> >> > On 03/12/2018 07:08 AM, Ard Biesheuvel wrote:
> >> >> On 10 March 2018 at 10:45, Thiebaud Weksteen 
> >> > wrote:
> >> >>> On Fri, Mar 9, 2018 at 5:54 PM Jeremy Cline 
> >> > wrote:
> >> >>>
> >>  On Fri, Mar 09, 2018 at 10:43:50AM +, 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
> >> 

Re: Regression from efi: call get_event_log before ExitBootServices

[Jeremy Cline]

On 03/12/2018 03:55 PM, Thiebaud Weksteen wrote:
> On Mon, Mar 12, 2018 at 7:33 PM Jeremy Cline  wrote:
> 
>> On 03/12/2018 02:29 PM, Thiebaud Weksteen wrote:
>>> On Mon, Mar 12, 2018 at 6:30 PM Ard Biesheuvel <
> ard.biesheu...@linaro.org>
>>> wrote:
>>>
 On 12 March 2018 at 17:01, Jeremy Cline  wrote:
> On 03/12/2018 10:56 AM, Ard Biesheuvel wrote:
>> On 12 March 2018 at 14:30, Jeremy Cline  wrote:
>>> On 03/12/2018 07:08 AM, Ard Biesheuvel wrote:
 On 10 March 2018 at 10:45, Thiebaud Weksteen 
>>> wrote:
> On Fri, Mar 9, 2018 at 5:54 PM Jeremy Cline 
>>> wrote:
>
>> On Fri, Mar 09, 2018 at 10:43:50AM +, 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, _guid, NULL,
>> _protocol);
>> @@ -104,9 +106,12 @@ void

Re: Regression from efi: call get_event_log before ExitBootServices

[Ard Biesheuvel]

On 12 March 2018 at 19:55, Thiebaud Weksteen  wrote:
> On Mon, Mar 12, 2018 at 7:33 PM Jeremy Cline  wrote:
>
>> On 03/12/2018 02:29 PM, Thiebaud Weksteen wrote:
>> > On Mon, Mar 12, 2018 at 6:30 PM Ard Biesheuvel <
> ard.biesheu...@linaro.org>
>> > wrote:
>> >
>> >> On 12 March 2018 at 17:01, Jeremy Cline  wrote:
>> >>> On 03/12/2018 10:56 AM, Ard Biesheuvel wrote:
>>  On 12 March 2018 at 14:30, Jeremy Cline  wrote:
>> > On 03/12/2018 07:08 AM, Ard Biesheuvel wrote:
>> >> On 10 March 2018 at 10:45, Thiebaud Weksteen 
>> > wrote:
>> >>> On Fri, Mar 9, 2018 at 5:54 PM Jeremy Cline 
>> > wrote:
>> >>>
>>  On Fri, Mar 09, 2018 at 10:43:50AM +, 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;
>> 
>>  

Re: Regression from efi: call get_event_log before ExitBootServices

[Thiebaud Weksteen]

On Mon, Mar 12, 2018 at 7:33 PM Jeremy Cline  wrote:

> On 03/12/2018 02:29 PM, Thiebaud Weksteen wrote:
> > On Mon, Mar 12, 2018 at 6:30 PM Ard Biesheuvel <
ard.biesheu...@linaro.org>
> > wrote:
> >
> >> On 12 March 2018 at 17:01, Jeremy Cline  wrote:
> >>> On 03/12/2018 10:56 AM, Ard Biesheuvel wrote:
>  On 12 March 2018 at 14:30, Jeremy Cline  wrote:
> > On 03/12/2018 07:08 AM, Ard Biesheuvel wrote:
> >> On 10 March 2018 at 10:45, Thiebaud Weksteen 
> > wrote:
> >>> On Fri, Mar 9, 2018 at 5:54 PM Jeremy Cline 
> > wrote:
> >>>
>  On Fri, Mar 09, 2018 at 10:43:50AM +, 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, _guid, NULL,
>  _protocol);
>  @@ -104,9 +106,12 @@ void
>  efi_retrieve_tpm2_eventlog_1_2(efi_system_table_t 

Re: Regression from efi: call get_event_log before ExitBootServices

[Jeremy Cline]

On 03/12/2018 02:29 PM, Thiebaud Weksteen wrote:
> On Mon, Mar 12, 2018 at 6:30 PM Ard Biesheuvel 
> wrote:
> 
>> On 12 March 2018 at 17:01, Jeremy Cline  wrote:
>>> On 03/12/2018 10:56 AM, Ard Biesheuvel wrote:
 On 12 March 2018 at 14:30, Jeremy Cline  wrote:
> On 03/12/2018 07:08 AM, Ard Biesheuvel wrote:
>> On 10 March 2018 at 10:45, Thiebaud Weksteen 
> wrote:
>>> On Fri, Mar 9, 2018 at 5:54 PM Jeremy Cline 
> wrote:
>>>
 On Fri, Mar 09, 2018 at 10:43:50AM +, 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, _guid, NULL,
 _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 **) _tbl);
 +   num_pages = 

Re: Regression from efi: call get_event_log before ExitBootServices

[Jeremy Cline]

On 03/12/2018 01:30 PM, Ard Biesheuvel wrote:
> On 12 March 2018 at 17:01, Jeremy Cline  wrote:
>> On 03/12/2018 10:56 AM, Ard Biesheuvel wrote:
>>> On 12 March 2018 at 14:30, Jeremy Cline  wrote:
 On 03/12/2018 07:08 AM, Ard Biesheuvel wrote:
> On 10 March 2018 at 10:45, Thiebaud Weksteen  wrote:
>> On Fri, Mar 9, 2018 at 5:54 PM Jeremy Cline  wrote:
>>
>>> On Fri, Mar 09, 2018 at 10:43:50AM +, 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, _guid, NULL,
>>> _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 **) _tbl);
>>> +   num_pages = DIV_ROUND_UP(sizeof(*log_tbl) + log_size, 
>>> EFI_PAGE_SIZE);
>>> +   status = efi_call_early(allocate_pages,
>>> +   EFI_ALLOCATE_ANY_PAGES,
>>> + 

Re: Regression from efi: call get_event_log before ExitBootServices

[Thiebaud Weksteen]

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

> On 12 March 2018 at 17:01, Jeremy Cline  wrote:
> > On 03/12/2018 10:56 AM, Ard Biesheuvel wrote:
> >> On 12 March 2018 at 14:30, Jeremy Cline  wrote:
> >>> On 03/12/2018 07:08 AM, Ard Biesheuvel wrote:
>  On 10 March 2018 at 10:45, Thiebaud Weksteen 
wrote:
> > On Fri, Mar 9, 2018 at 5:54 PM Jeremy Cline 
wrote:
> >
> >> On Fri, Mar 09, 2018 at 10:43:50AM +, 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, _guid, NULL,
> >> _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 **) _tbl);
> >> +   num_pages = DIV_ROUND_UP(sizeof(*log_tbl) + log_size,
EFI_PAGE_SIZE);
> >> +   status = 

Re: Regression from efi: call get_event_log before ExitBootServices

[Ard Biesheuvel]

On 12 March 2018 at 17:01, Jeremy Cline  wrote:
> On 03/12/2018 10:56 AM, Ard Biesheuvel wrote:
>> On 12 March 2018 at 14:30, Jeremy Cline  wrote:
>>> On 03/12/2018 07:08 AM, Ard Biesheuvel wrote:
 On 10 March 2018 at 10:45, Thiebaud Weksteen  wrote:
> On Fri, Mar 9, 2018 at 5:54 PM Jeremy Cline  wrote:
>
>> On Fri, Mar 09, 2018 at 10:43:50AM +, 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, _guid, NULL,
>> _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 **) _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,
>> +   _tbl_alloc);
>>
>> if (status != EFI_SUCCESS) {
>> 

Re: Regression from efi: call get_event_log before ExitBootServices

[Jeremy Cline]

On 03/12/2018 10:56 AM, Ard Biesheuvel wrote:
> On 12 March 2018 at 14:30, Jeremy Cline  wrote:
>> On 03/12/2018 07:08 AM, Ard Biesheuvel wrote:
>>> On 10 March 2018 at 10:45, Thiebaud Weksteen  wrote:
 On Fri, Mar 9, 2018 at 5:54 PM Jeremy Cline  wrote:

> On Fri, Mar 09, 2018 at 10:43:50AM +, 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, _guid, NULL,
> _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 **) _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,
> +   _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 = 

Re: Regression from efi: call get_event_log before ExitBootServices

[Ard Biesheuvel]

On 12 March 2018 at 14:30, Jeremy Cline  wrote:
> On 03/12/2018 07:08 AM, Ard Biesheuvel wrote:
>> On 10 March 2018 at 10:45, Thiebaud Weksteen  wrote:
>>> On Fri, Mar 9, 2018 at 5:54 PM Jeremy Cline  wrote:
>>>
 On Fri, Mar 09, 2018 at 10:43:50AM +, 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, _guid, NULL,
_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 **) _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,
+   _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 = 

Re: Regression from efi: call get_event_log before ExitBootServices

[Jeremy Cline]

On 03/12/2018 07:08 AM, Ard Biesheuvel wrote:
> On 10 March 2018 at 10:45, Thiebaud Weksteen  wrote:
>> On Fri, Mar 9, 2018 at 5:54 PM Jeremy Cline  wrote:
>>
>>> On Fri, Mar 09, 2018 at 10:43:50AM +, 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.

Regards,
Jeremy
--
To unsubscribe from this list: send the line "unsubscribe linux-efi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Regression from efi: call get_event_log before ExitBootServices

[Ard Biesheuvel]

On 10 March 2018 at 10:45, Thiebaud Weksteen  wrote:
> On Fri, Mar 9, 2018 at 5:54 PM Jeremy Cline  wrote:
>
>> On Fri, Mar 09, 2018 at 10:43:50AM +, 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?
--
To unsubscribe from this list: send the line "unsubscribe linux-efi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Regression from efi: call get_event_log before ExitBootServices

[Paul Menzel]

Dear Jarkko,


On 03/12/18 11:17, Jarkko Sakkinen wrote:

On Sat, 2018-03-10 at 10:45 +, Thiebaud Weksteen wrote:

On Fri, Mar 9, 2018 at 5:54 PM Jeremy Cline  wrote:

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?


Check if the BIOS is up to date?


How would that help? The no regression policy demands, that Linux 
continues working on systems, where it worked before. So upgrading the 
firmware is no solution, is it? Until a solution is found, the commits 
should be reverted.



Kind regards,

Paul



smime.p7s
Description: S/MIME Cryptographic Signature

Re: Regression from efi: call get_event_log before ExitBootServices

[Jarkko Sakkinen]

On Sat, 2018-03-10 at 10:45 +, Thiebaud Weksteen wrote:
> On Fri, Mar 9, 2018 at 5:54 PM Jeremy Cline  wrote:
> > 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?

Check if the BIOS is up to date?

/Jarkko
--
To unsubscribe from this list: send the line "unsubscribe linux-efi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Regression from efi: call get_event_log before ExitBootServices

[Thiebaud Weksteen]

On Fri, Mar 9, 2018 at 5:54 PM Jeremy Cline  wrote:

> On Fri, Mar 09, 2018 at 10:43:50AM +, 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?

Thanks


> Regards,
> Jeremy
--
To unsubscribe from this list: send the line "unsubscribe linux-efi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Regression from efi: call get_event_log before ExitBootServices

[James Bottomley]

On Fri, 2018-03-09 at 10:29 +0100, Hans de Goede wrote:
> Hi,
> 
> On 08-03-18 18:26, Jeremy Cline wrote:
> > 
> > On 03/08/2018 11:50 AM, Hans de Goede wrote:
[...]
> > > > The UEFI firmware does some measurements and so does shim. So
> > > > you should have some event logs. What version of shim are you
> > > > using? And also would be good to know if it's the same shim
> > > > version that Jeremy is using.
> > > 
> > > That is a very good question, I'm using: shim-ia32-13-0.7.x86_64, 
> > > which is the last version for F27 AFAICT.
> > 
> > All my tablet has installed is shim-0.8-10.x86_64, no shim-ia32.
> 
> Yes my bad, although if the kernel changes break booting on systems
> without the shim that is still good to know and something which
> we probably ought to fix.

My laptop is set up with secure boot but without shim using a shim
protocol thin layer to check the kernel signature against db variables:

https://git.kernel.org/pub/scm/linux/kernel/git/jejb/efitools.git/tree/ShimReplace.c

and I haven't seen any breakage, so not having a shim that does
measurements works for me all the way up to -rc4.

James

--
To unsubscribe from this list: send the line "unsubscribe linux-efi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Regression from efi: call get_event_log before ExitBootServices

[Jeremy Cline]

On Fri, Mar 09, 2018 at 10:43:50AM +, 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);"

Regards,
Jeremy
--
To unsubscribe from this list: send the line "unsubscribe linux-efi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Regression from efi: call get_event_log before ExitBootServices

[Thiebaud Weksteen]

---
 drivers/firmware/efi/libstub/tpm.c | 21 +
 1 file changed, 21 insertions(+)

diff --git a/drivers/firmware/efi/libstub/tpm.c 
b/drivers/firmware/efi/libstub/tpm.c
index 773afcd6a37c..ee3fac109078 100644
--- a/drivers/firmware/efi/libstub/tpm.c
+++ b/drivers/firmware/efi/libstub/tpm.c
@@ -112,6 +112,22 @@ void efi_retrieve_tpm2_eventlog_1_2(efi_system_table_t 
*sys_table_arg)
log_size = log_last_entry - log_location + last_entry_size;
}
 
+   if (log_size == 0) {
+   efi_printk(sys_table_arg, "log_size == 0\n");
+   }
+   else if (log_size < 1 * 1024 * 1024) {
+   efi_printk(sys_table_arg, "log_size < 1M\n");
+   }
+   else if (log_size < 500 * 1024 * 1024) {
+   efi_printk(sys_table_arg, "log_size < 500M\n");
+   }
+   else if (log_size < 1000 * 1024 * 1024) {
+   efi_printk(sys_table_arg, "log_size < 1G\n");
+   }
+   else {
+   efi_printk(sys_table_arg, "log_size > 1G\n");
+   }
+
efi_printk(sys_table_arg, "Allocating memory for storing the logs\n");
/* Allocate space for the logs and copy them. */
status = efi_call_early(allocate_pool, EFI_LOADER_DATA,
@@ -124,6 +140,11 @@ void efi_retrieve_tpm2_eventlog_1_2(efi_system_table_t 
*sys_table_arg)
   "Unable to allocate memory for event log\n");
return;
}
+   if (!log_tbl) {
+   efi_printk(sys_table_arg, "Pointer returned from allocation is 
NULL!\n");
+   return;
+   }
+
efi_printk(sys_table_arg, "Copying log to new location\n");
 
memset(log_tbl, 0, sizeof(*log_tbl) + log_size);
-- 
2.16.2.395.g2e18187dfd-goog

--
To unsubscribe from this list: send the line "unsubscribe linux-efi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Regression from efi: call get_event_log before ExitBootServices

[Thiebaud Weksteen]

On Fri, Mar 9, 2018 at 10:29 AM Hans de Goede  wrote:

> Hi,

> On 08-03-18 18:26, Jeremy Cline wrote:
> > On 03/08/2018 11:50 AM, Hans de Goede wrote:
> >>  >> added these now>
> >>
> >> Hi,
> >>
> >> On 07-03-18 12:34, Javier Martinez Canillas wrote:
> >
> > 
> >
> >>> Are you also able to read the TPM event logs?
> >>>
> >>> $ hexdump /sys/kernel/security/tpm0/binary_bios_measurements
> >>
> >> Yes for me that outputs a lot of hex :)
> >
> > For me, /sys/kernel/security/tmp0 doesn't exist on 4.15.6 or 4.16 with
> > the patch reverted.

> Hmm, have you re-enabled the TPM in the BIOS?

> >>> The UEFI firmware does some measurements and so does shim. So you
should
> >>> have some event logs. What version of shim are you using? And also
would
> >>> be good to know if it's the same shim version that Jeremy is using.
> >>
> >> That is a very good question, I'm using: shim-ia32-13-0.7.x86_64,
which is
> >> the last version for F27 AFAICT.
> >
> > All my tablet has installed is shim-0.8-10.x86_64, no shim-ia32.

> Yes my bad, although if the kernel changes break booting on systems
> without the shim that is still good to know and something which
> we probably ought to fix.

> >> But Jeremy's tablet might very well be not using the shim at all, as
> >> I manually installed Fedora 25 on the tablet he now has, before Fedora
> >> supported
> >> machines with 32 bit EFI. I then later did a "dnf distro-sync" to
> >> Fedora-27.
> >>
> >> Jeremy might also very well still be booting using a grub binary I
build
> >> manually back then, without any shim being involved.
> >>
> >> Jeremy what does efibootmgr -v output on your device ?
> >
> > # efibootmgr -v
> > BootCurrent: 0003
> > Timeout: 4 seconds
> > BootOrder: 0003,,0001,2001,2002,2003
> > Boot* Android X64 OS
> >
HD(1,GPT,215e6cf3-e97d-4735-9c4e-7338c8f5a645,0x800,0x32000)/File(\EFI\BOOT\bootx64.efi)RC
> > Boot0001* Internal EFI Shell
> >
FvVol(a881d567-6cb0-4eee-8435-2e72d33e45b5)/FvFile(c57ad6b7-0515-40a8-9d21-551652854e37)RCM&".
> > Boot0003* Fedora
> >
HD(1,GPT,215e6cf3-e97d-4735-9c4e-7338c8f5a645,0x800,0x32000)/File(\EFI\fedora\grubx64.efi)
> > Boot2001* EFI USB Device  RC
> > Boot2002* EFI DVD/CDROM   RC
> > Boot2003* EFI Network RC
> > Boot8087* Udm
> >
FvVol(a881d567-6cb0-4eee-8435-2e72d33e45b5)/FvFile(9a9ab4c1-ee1b-488b-b300-24544a7bd418)
> >
> > I think you're right about it using the old grub binary. I'm
> > embarrassingly unfamiliar with both UEFI and grub, but I'm guessing you
> > set the location of grub.cfg at compile time? When I boot
> > \EFI\fedora\grubx64.efi, it's pulling the grub.cfg from
> > \EFI\redhat\grub.cfg.

> Ah yes, so I did not build my own grub I took one from RHEL as that had
> 32 bit UEFI support before Fedora got it and as I was lazy I copied the
> 32 bit binary over the 64 bit one, so don't let the filename fool you.

> What you could do is install grub2-efi-ia32 from the Fedora 27 repos
> and then use efibootmgr to add an entry pointing to
\EFI\fedora\grubia32.efi
> note that one will look at \EFI\fedora\grub.cfg .

> Then see if the problem persists. A second step would be to also install
> shim-ia32 and point to that...

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.


> Regards,

> Hans
--
To unsubscribe from this list: send the line "unsubscribe linux-efi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Regression from efi: call get_event_log before ExitBootServices

[Hans de Goede]

Hi,

On 08-03-18 18:26, Jeremy Cline wrote:

On 03/08/2018 11:50 AM, Hans de Goede wrote:



Hi,

On 07-03-18 12:34, Javier Martinez Canillas wrote:





Are you also able to read the TPM event logs?

$ hexdump /sys/kernel/security/tpm0/binary_bios_measurements


Yes for me that outputs a lot of hex :)


For me, /sys/kernel/security/tmp0 doesn't exist on 4.15.6 or 4.16 with
the patch reverted.


Hmm, have you re-enabled the TPM in the BIOS?


The UEFI firmware does some measurements and so does shim. So you should
have some event logs. What version of shim are you using? And also would
be good to know if it's the same shim version that Jeremy is using.


That is a very good question, I'm using: shim-ia32-13-0.7.x86_64, which is
the last version for F27 AFAICT.


All my tablet has installed is shim-0.8-10.x86_64, no shim-ia32.


Yes my bad, although if the kernel changes break booting on systems
without the shim that is still good to know and something which
we probably ought to fix.


But Jeremy's tablet might very well be not using the shim at all, as
I manually installed Fedora 25 on the tablet he now has, before Fedora
supported
machines with 32 bit EFI. I then later did a "dnf distro-sync" to
Fedora-27.

Jeremy might also very well still be booting using a grub binary I build
manually back then, without any shim being involved.

Jeremy what does efibootmgr -v output on your device ?


# efibootmgr -v
BootCurrent: 0003
Timeout: 4 seconds
BootOrder: 0003,,0001,2001,2002,2003
Boot* Android X64 OS
HD(1,GPT,215e6cf3-e97d-4735-9c4e-7338c8f5a645,0x800,0x32000)/File(\EFI\BOOT\bootx64.efi)RC
Boot0001* Internal EFI Shell
FvVol(a881d567-6cb0-4eee-8435-2e72d33e45b5)/FvFile(c57ad6b7-0515-40a8-9d21-551652854e37)RCM&".
Boot0003* Fedora
HD(1,GPT,215e6cf3-e97d-4735-9c4e-7338c8f5a645,0x800,0x32000)/File(\EFI\fedora\grubx64.efi)
Boot2001* EFI USB DeviceRC
Boot2002* EFI DVD/CDROM RC
Boot2003* EFI Network   RC
Boot8087* Udm
FvVol(a881d567-6cb0-4eee-8435-2e72d33e45b5)/FvFile(9a9ab4c1-ee1b-488b-b300-24544a7bd418)

I think you're right about it using the old grub binary. I'm
embarrassingly unfamiliar with both UEFI and grub, but I'm guessing you
set the location of grub.cfg at compile time? When I boot
\EFI\fedora\grubx64.efi, it's pulling the grub.cfg from
\EFI\redhat\grub.cfg.


Ah yes, so I did not build my own grub I took one from RHEL as that had
32 bit UEFI support before Fedora got it and as I was lazy I copied the
32 bit binary over the 64 bit one, so don't let the filename fool you.

What you could do is install grub2-efi-ia32 from the Fedora 27 repos
and then use efibootmgr to add an entry pointing to \EFI\fedora\grubia32.efi
note that one will look at \EFI\fedora\grub.cfg .

Then see if the problem persists. A second step would be to also install
shim-ia32 and point to that...

Regards,

Hans
--
To unsubscribe from this list: send the line "unsubscribe linux-efi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Regression from efi: call get_event_log before ExitBootServices

[Jeremy Cline]

On 03/08/2018 03:45 AM, Thiebaud Weksteen wrote:
> Jeremy, Hans, could you both describe precisely how your boot is
> configured? This feature is only triggered when booting the EFI stub of the
> kernel so this may be not executed if you are using something else in
> between.

I put everything I know in the other sub-thread.
> Jeremy, would you be able to modify the efi_retrieve_tpm2_eventlog_1_2
> function to add multiple efi_printk(sys_table_arg,  "message\n"); to test:
> if you get the output on your screen; and isolate which call might be the
> cause of the hang?
> I can forward a debug patch if that helps.

Thanks for the patch, here's the output:

Locating the TCG2Protocol
Calling GetEventLog on TCG2Protocol
Log returned
log_location is not empty
log_size != 0
Allocating memory for storing the logs
Returned from memory allocation
Copying log to new location

At this point it hangs.


Regards,
Jeremy
--
To unsubscribe from this list: send the line "unsubscribe linux-efi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Regression from efi: call get_event_log before ExitBootServices

[Jeremy Cline]

On 03/08/2018 11:50 AM, Hans de Goede wrote:
>  added these now>
> 
> Hi,
> 
> On 07-03-18 12:34, Javier Martinez Canillas wrote:



>> Are you also able to read the TPM event logs?
>>
>> $ hexdump /sys/kernel/security/tpm0/binary_bios_measurements
> 
> Yes for me that outputs a lot of hex :)

For me, /sys/kernel/security/tmp0 doesn't exist on 4.15.6 or 4.16 with
the patch reverted.

>> The UEFI firmware does some measurements and so does shim. So you should
>> have some event logs. What version of shim are you using? And also would
>> be good to know if it's the same shim version that Jeremy is using.
> 
> That is a very good question, I'm using: shim-ia32-13-0.7.x86_64, which is
> the last version for F27 AFAICT.

All my tablet has installed is shim-0.8-10.x86_64, no shim-ia32.

> 
> But Jeremy's tablet might very well be not using the shim at all, as
> I manually installed Fedora 25 on the tablet he now has, before Fedora
> supported
> machines with 32 bit EFI. I then later did a "dnf distro-sync" to
> Fedora-27.
> 
> Jeremy might also very well still be booting using a grub binary I build
> manually back then, without any shim being involved.
> 
> Jeremy what does efibootmgr -v output on your device ?

# efibootmgr -v
BootCurrent: 0003
Timeout: 4 seconds
BootOrder: 0003,,0001,2001,2002,2003
Boot* Android X64 OS
HD(1,GPT,215e6cf3-e97d-4735-9c4e-7338c8f5a645,0x800,0x32000)/File(\EFI\BOOT\bootx64.efi)RC
Boot0001* Internal EFI Shell
FvVol(a881d567-6cb0-4eee-8435-2e72d33e45b5)/FvFile(c57ad6b7-0515-40a8-9d21-551652854e37)RCM&".
Boot0003* Fedora
HD(1,GPT,215e6cf3-e97d-4735-9c4e-7338c8f5a645,0x800,0x32000)/File(\EFI\fedora\grubx64.efi)
Boot2001* EFI USB DeviceRC
Boot2002* EFI DVD/CDROM RC
Boot2003* EFI Network   RC
Boot8087* Udm
FvVol(a881d567-6cb0-4eee-8435-2e72d33e45b5)/FvFile(9a9ab4c1-ee1b-488b-b300-24544a7bd418)

I think you're right about it using the old grub binary. I'm
embarrassingly unfamiliar with both UEFI and grub, but I'm guessing you
set the location of grub.cfg at compile time? When I boot
\EFI\fedora\grubx64.efi, it's pulling the grub.cfg from
\EFI\redhat\grub.cfg.

Regards,
Jeremy
--
To unsubscribe from this list: send the line "unsubscribe linux-efi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Regression from efi: call get_event_log before ExitBootServices

[Hans de Goede]

Hi,

On 07-03-18 12:34, Javier Martinez Canillas wrote:

On 03/07/2018 12:10 PM, Hans de Goede wrote:





Both according to the BIOS and to the /sys/class/tpm/tpm0/device/description
file it is a TPM 2.0.



I see, so you can choose enabling the TPM 1.2 or TPM 2.0 device? At least that's
the case on my X1 Carbon laptop. I've both a hardware TPM 1.2 and a firmware TPM
2.0 that's implemented as an Intel ME application (AFAIU).


This device only has the firmware TPM 2.0 implementation.




I'm actually amazed that this machine has a TPM at all, a quick internet
search shows that it is a software implemented TPM running as part of the
TXE firmware.



A quick search suggests that it comes with Windows 10?


Yes, it comes with Windows 10.


For start, can you please check if you can boot a v4.16-rcX kernel with the
TPM device enabled? That way we will know that at least that it consistently
fails on this machine and is not and isolated issue.


I just tried and v4.16-rc3 boots fine for me, repeatedly.



That's an interesting data point.


I guess Jeremy's model may actually have something in the TPM log


I don't think so. The UEFI firmware already does some measurements and also
does shim. So you *should* have some logs.


while my TPM log is empty... Is there anyway to make sure the TPM
log has some info to retreive?



Are you also able to read the TPM event logs?

$ hexdump /sys/kernel/security/tpm0/binary_bios_measurements


Yes for me that outputs a lot of hex :)


The UEFI firmware does some measurements and so does shim. So you should
have some event logs. What version of shim are you using? And also would
be good to know if it's the same shim version that Jeremy is using.


That is a very good question, I'm using: shim-ia32-13-0.7.x86_64, which is
the last version for F27 AFAICT.

But Jeremy's tablet might very well be not using the shim at all, as
I manually installed Fedora 25 on the tablet he now has, before Fedora supported
machines with 32 bit EFI. I then later did a "dnf distro-sync" to Fedora-27.

Jeremy might also very well still be booting using a grub binary I build
manually back then, without any shim being involved.

Jeremy what does efibootmgr -v output on your device ?

Regards,

Hans
--
To unsubscribe from this list: send the line "unsubscribe linux-efi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Regression from efi: call get_event_log before ExitBootServices

[Thiebaud Weksteen]

---
 drivers/firmware/efi/libstub/tpm.c | 11 +++
 1 file changed, 11 insertions(+)

diff --git a/drivers/firmware/efi/libstub/tpm.c 
b/drivers/firmware/efi/libstub/tpm.c
index da661bf8cb96..773afcd6a37c 100644
--- a/drivers/firmware/efi/libstub/tpm.c
+++ b/drivers/firmware/efi/libstub/tpm.c
@@ -74,19 +74,23 @@ void efi_retrieve_tpm2_eventlog_1_2(efi_system_table_t 
*sys_table_arg)
efi_bool_t truncated;
void *tcg2_protocol;
 
+   efi_printk(sys_table_arg, "Locating the TCG2Protocol\n");
status = efi_call_early(locate_protocol, _guid, NULL,
_protocol);
if (status != EFI_SUCCESS)
return;
 
+   efi_printk(sys_table_arg, "Calling GetEventLog on TCG2Protocol\n");
status = efi_call_proto(efi_tcg2_protocol, get_event_log, tcg2_protocol,
EFI_TCG2_EVENT_LOG_FORMAT_TCG_1_2,
_location, _last_entry, );
if (status != EFI_SUCCESS)
return;
+   efi_printk(sys_table_arg, "Log returned\n");
 
if (!log_location)
return;
+   efi_printk(sys_table_arg, "log_location is not empty\n");
first_entry_addr = (unsigned long) log_location;
 
/*
@@ -94,7 +98,9 @@ void efi_retrieve_tpm2_eventlog_1_2(efi_system_table_t 
*sys_table_arg)
 */
if (!log_last_entry) {
log_size = 0;
+   efi_printk(sys_table_arg, "log_size = 0\n");
} else {
+   efi_printk(sys_table_arg, "log_size != 0\n");
last_entry_addr = (unsigned long) log_last_entry;
/*
 * get_event_log only returns the address of the last entry.
@@ -106,26 +112,31 @@ void efi_retrieve_tpm2_eventlog_1_2(efi_system_table_t 
*sys_table_arg)
log_size = log_last_entry - log_location + last_entry_size;
}
 
+   efi_printk(sys_table_arg, "Allocating memory for storing the logs\n");
/* Allocate space for the logs and copy them. */
status = efi_call_early(allocate_pool, EFI_LOADER_DATA,
sizeof(*log_tbl) + log_size,
(void **) _tbl);
 
+   efi_printk(sys_table_arg, "Returned from memory allocation\n");
if (status != EFI_SUCCESS) {
efi_printk(sys_table_arg,
   "Unable to allocate memory for event log\n");
return;
}
+   efi_printk(sys_table_arg, "Copying log to new location\n");
 
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;
memcpy(log_tbl->log, (void *) first_entry_addr, log_size);
 
+   efi_printk(sys_table_arg, "Installing the log into the configuration 
table\n");
status = efi_call_early(install_configuration_table,
_eventlog_guid, log_tbl);
if (status != EFI_SUCCESS)
goto err_free;
+   efi_printk(sys_table_arg, "Done\n");
return;
 
 err_free:
-- 
2.16.2.395.g2e18187dfd-goog

--
To unsubscribe from this list: send the line "unsubscribe linux-efi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Regression from efi: call get_event_log before ExitBootServices

[Thiebaud Weksteen]

On Wed, Mar 7, 2018 at 6:33 PM Jeremy Cline  wrote:

> On 03/07/2018 03:41 AM, Thiebaud Weksteen wrote:
> > Hi,
> >
> > Thanks for testing and sending this report! This patch relies heavily on
> > the functions exposed by the firmware. My first guess would be that
some of
> > these may not be implemented correctly by the manufacturer.
> >
> > Could you share more information on this specific device?
> > Do you have any link to the manufacturer website (I found [1] but it is
> > based on an ARM CPU)?
> > Do you have the option to update your firmware? Is a copy of the
firmware
> > available from the manufacturer?

> I couldn't find a copy of the firmware, unfortunately.

No worries, thanks for looking that up.


> > On your side, I assume no error message got displayed on the screen when
> > booting. Would you be able to try to boot in an UEFI shell [2] and
execute
> > the command "dh -v"?

> Yup, no errors on the screen. I've attached the output of dh -v from the
> UEFI shell.

Great, thanks for that. There is a module that exposes the EfiTcg2Protocol
(TrEEDxe). So I'm going to assume this is properly located and then called.
Unfortunately, this is so early in the boot that we can only rely on the
EFI functions for logging/debugging.

Jeremy, Hans, could you both describe precisely how your boot is
configured? This feature is only triggered when booting the EFI stub of the
kernel so this may be not executed if you are using something else in
between.

Jeremy, would you be able to modify the efi_retrieve_tpm2_eventlog_1_2
function to add multiple efi_printk(sys_table_arg,  "message\n"); to test:
if you get the output on your screen; and isolate which call might be the
cause of the hang?
I can forward a debug patch if that helps.

Thanks



> Regards,
> Jeremy
--
To unsubscribe from this list: send the line "unsubscribe linux-efi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Regression from efi: call get_event_log before ExitBootServices

[Javier Martinez Canillas]

Hi Hans,

On 03/07/2018 12:16 PM, Hans de Goede wrote:
> Hi,
> 
> On 07-03-18 09:41, Thiebaud Weksteen wrote:
>> Hi,
>>
>> Thanks for testing and sending this report! This patch relies heavily on
>> the functions exposed by the firmware. My first guess would be that some of
>> these may not be implemented correctly by the manufacturer.
>>
>> Could you share more information on this specific device?
> 
> I've the same device as Jeremy, but I just tried a 4.16-rc3 kernel
> and I'm not seeing this problem, BIOS settings all default (I loaded
> the BIOS defaults to make sure).
> 
>> Do you have any link to the manufacturer website (I found [1] but it is
>> based on an ARM CPU)?
>> Do you have the option to update your firmware? Is a copy of the firmware
>> available from the manufacturer?
> 
> This is a really cheap Windows tablet which was given away for free in
> the Netherlands with some home-schooling language courses, or something
> similar.
> 
> Both mine and Jeremy tablets come from a website in the Netherlands
> where people can buy/sell used goods.
> 
> Most relevant for this discussion I guess is that this device is
> based on a Bay Trail Z3735G SoC, on which according to the internets:
> https://embedded.communities.intel.com/thread/7868
> 
> The TPM 2.0 it contains is implemented as part of the TXE firmware.
> 
> Since I cannot reproduce I'm thinking that maybe Jeremy actually has
> some log messages in the TPM log, where as mine is empty.  Is there a
> way to make sure some messages are in there?
>

The UEFI firmware does some measurements and so does shim. So you should
have some event logs. What version of shim are you using? And also would
be good to know if it's the same shim version that Jeremy is using.

> Regards,
> 
> Hans
> 

Best regards,
-- 
Javier Martinez Canillas
Software Engineer - Desktop Hardware Enablement
Red Hat
--
To unsubscribe from this list: send the line "unsubscribe linux-efi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Regression from efi: call get_event_log before ExitBootServices

[Hans de Goede]

Hi,

On 07-03-18 09:41, Thiebaud Weksteen wrote:

Hi,

Thanks for testing and sending this report! This patch relies heavily on
the functions exposed by the firmware. My first guess would be that some of
these may not be implemented correctly by the manufacturer.

Could you share more information on this specific device?


I've the same device as Jeremy, but I just tried a 4.16-rc3 kernel
and I'm not seeing this problem, BIOS settings all default (I loaded
the BIOS defaults to make sure).


Do you have any link to the manufacturer website (I found [1] but it is
based on an ARM CPU)?
Do you have the option to update your firmware? Is a copy of the firmware
available from the manufacturer?


This is a really cheap Windows tablet which was given away for free in
the Netherlands with some home-schooling language courses, or something
similar.

Both mine and Jeremy tablets come from a website in the Netherlands
where people can buy/sell used goods.

Most relevant for this discussion I guess is that this device is
based on a Bay Trail Z3735G SoC, on which according to the internets:
https://embedded.communities.intel.com/thread/7868

The TPM 2.0 it contains is implemented as part of the TXE firmware.

Since I cannot reproduce I'm thinking that maybe Jeremy actually has
some log messages in the TPM log, where as mine is empty.  Is there a
way to make sure some messages are in there?

Regards,

Hans




On your side, I assume no error message got displayed on the screen when
booting. Would you be able to try to boot in an UEFI shell [2] and execute
the command "dh -v"?

Thanks,
Thiebaud

[1] https://www.gp-electronic.nl/product/7inchtablet
[2]
https://wiki.archlinux.org/index.php/Unified_Extensible_Firmware_Interface#UEFI_Shell

On Tue, Mar 6, 2018 at 5:00 PM Jeremy Cline  wrote:


Hi folks,



Commit 33b6d03469b2 ("efi: call get_event_log before ExitBootServices")
causes my GP-electronic T701 tablet to hang when booting. Reverting the
patch series or hiding the TPM in the BIOS fixes the problem.



I've never fiddled with TPMs before so I'm not sure what what debugging
information to provide. It's got an Atom Z3735G and the UEFI firmware is
InsydeH20 version BYT70A.YNCHENG.WIN.007.




Regards,
Jeremy

--
To unsubscribe from this list: send the line "unsubscribe linux-efi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Regression from efi: call get_event_log before ExitBootServices

[Thiebaud Weksteen]

Hi,

Thanks for testing and sending this report! This patch relies heavily on
the functions exposed by the firmware. My first guess would be that some of
these may not be implemented correctly by the manufacturer.

Could you share more information on this specific device?
Do you have any link to the manufacturer website (I found [1] but it is
based on an ARM CPU)?
Do you have the option to update your firmware? Is a copy of the firmware
available from the manufacturer?

On your side, I assume no error message got displayed on the screen when
booting. Would you be able to try to boot in an UEFI shell [2] and execute
the command "dh -v"?

Thanks,
Thiebaud

[1] https://www.gp-electronic.nl/product/7inchtablet
[2]
https://wiki.archlinux.org/index.php/Unified_Extensible_Firmware_Interface#UEFI_Shell

On Tue, Mar 6, 2018 at 5:00 PM Jeremy Cline  wrote:

> Hi folks,

> Commit 33b6d03469b2 ("efi: call get_event_log before ExitBootServices")
> causes my GP-electronic T701 tablet to hang when booting. Reverting the
> patch series or hiding the TPM in the BIOS fixes the problem.

> I've never fiddled with TPMs before so I'm not sure what what debugging
> information to provide. It's got an Atom Z3735G and the UEFI firmware is
> InsydeH20 version BYT70A.YNCHENG.WIN.007.


> Regards,
> Jeremy
--
To unsubscribe from this list: send the line "unsubscribe linux-efi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Regression from efi: call get_event_log before ExitBootServices

[Jeremy Cline]

Hi folks,

Commit 33b6d03469b2 ("efi: call get_event_log before ExitBootServices")
causes my GP-electronic T701 tablet to hang when booting. Reverting the
patch series or hiding the TPM in the BIOS fixes the problem.

I've never fiddled with TPMs before so I'm not sure what what debugging
information to provide. It's got an Atom Z3735G and the UEFI firmware is
InsydeH20 version BYT70A.YNCHENG.WIN.007.


Regards,
Jeremy
--
To unsubscribe from this list: send the line "unsubscribe linux-efi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html