diff mbox

Regression from efi: call get_event_log before ExitBootServices

Message ID 010001620bafa06b-41525407-603e-40a9-ba11-6033b2f5dcc7-000000@email.amazonses.com (mailing list archive)
State New, archived
Headers show

Commit Message

Jeremy Cline March 9, 2018, 4:54 p.m. UTC
On Fri, Mar 09, 2018 at 10:43:50AM +0000, Thiebaud Weksteen wrote:
> Thanks a lot for trying out the patch!
> 
> Please don't modify your install at this stage, I think we are hitting a
> firmware bug and that would be awesome if we can fix how we are handling it.
> So, if we reach that stage in the function it could either be that:
> * The allocation did not succeed, somehow, but the firmware still returned
> EFI_SUCCEED.
> * The size requested is incorrect (I'm thinking something like a 1G of
> log). This would be due to either a miscalculation of log_size (possible)
> or; the returned values of GetEventLog are not correct.
> I'm sending a patch to add checks for these. Could you please apply and
> retest?
> Again, thanks for helping debugging this.

No problem, thanks for the help :)

With the new patch:

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

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


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

Regards,
Jeremy

Comments

ThiƩbaud Weksteen March 10, 2018, 10:45 a.m. UTC | #1
On Fri, Mar 9, 2018 at 5:54 PM Jeremy Cline <jeremy@jcline.org> wrote:

> On Fri, Mar 09, 2018 at 10:43:50AM +0000, Thiebaud Weksteen wrote:
> > Thanks a lot for trying out the patch!
> >
> > Please don't modify your install at this stage, I think we are hitting a
> > firmware bug and that would be awesome if we can fix how we are
handling it.
> > So, if we reach that stage in the function it could either be that:
> > * The allocation did not succeed, somehow, but the firmware still
returned
> > EFI_SUCCEED.
> > * The size requested is incorrect (I'm thinking something like a 1G of
> > log). This would be due to either a miscalculation of log_size
(possible)
> > or; the returned values of GetEventLog are not correct.
> > I'm sending a patch to add checks for these. Could you please apply and
> > retest?
> > Again, thanks for helping debugging this.

> No problem, thanks for the help :)

> With the new patch:

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

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

> diff --git a/drivers/firmware/efi/libstub/tpm.c
b/drivers/firmware/efi/libstub/tpm.c
> index ee3fac109078..1ab5638bc50e 100644
> --- a/drivers/firmware/efi/libstub/tpm.c
> +++ b/drivers/firmware/efi/libstub/tpm.c
> @@ -148,8 +148,11 @@ void
efi_retrieve_tpm2_eventlog_1_2(efi_system_table_t *sys_table_arg)
>          efi_printk(sys_table_arg, "Copying log to new location\n");

>          memset(log_tbl, 0, sizeof(*log_tbl) + log_size);
> +       efi_printk(sys_table_arg, "Successfully memset log_tbl to 0\n");
>          log_tbl->size = log_size;
> +       efi_printk(sys_table_arg, "Set log_tbl->size\n");
>          log_tbl->version = EFI_TCG2_EVENT_LOG_FORMAT_TCG_1_2;
> +       efi_printk(sys_table_arg, "Set log_tbl-version\n");
>          memcpy(log_tbl->log, (void *) first_entry_addr, log_size);

>          efi_printk(sys_table_arg, "Installing the log into the
configuration table\n");

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

Thanks. Well, it looks like the memory that is supposedly allocated is not
usable. I'm thinking this is a firmware bug.
Ard, would you agree on this assumption? Thoughts on how to proceed?

Thanks


> Regards,
> Jeremy
Jarkko Sakkinen March 12, 2018, 10:17 a.m. UTC | #2
On Sat, 2018-03-10 at 10:45 +0000, Thiebaud Weksteen wrote:
> On Fri, Mar 9, 2018 at 5:54 PM Jeremy Cline <jeremy@jcline.org> 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
Paul Menzel March 12, 2018, 10:41 a.m. UTC | #3
Dear Jarkko,


On 03/12/18 11:17, Jarkko Sakkinen wrote:
> On Sat, 2018-03-10 at 10:45 +0000, Thiebaud Weksteen wrote:
>> On Fri, Mar 9, 2018 at 5:54 PM Jeremy Cline <jeremy@jcline.org> 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
Ard Biesheuvel March 12, 2018, 11:08 a.m. UTC | #4
On 10 March 2018 at 10:45, Thiebaud Weksteen <tweek@google.com> wrote:
> On Fri, Mar 9, 2018 at 5:54 PM Jeremy Cline <jeremy@jcline.org> wrote:
>
>> On Fri, Mar 09, 2018 at 10:43:50AM +0000, Thiebaud Weksteen wrote:
>> > Thanks a lot for trying out the patch!
>> >
>> > Please don't modify your install at this stage, I think we are hitting a
>> > firmware bug and that would be awesome if we can fix how we are
> handling it.
>> > So, if we reach that stage in the function it could either be that:
>> > * The allocation did not succeed, somehow, but the firmware still
> returned
>> > EFI_SUCCEED.
>> > * The size requested is incorrect (I'm thinking something like a 1G of
>> > log). This would be due to either a miscalculation of log_size
> (possible)
>> > or; the returned values of GetEventLog are not correct.
>> > I'm sending a patch to add checks for these. Could you please apply and
>> > retest?
>> > Again, thanks for helping debugging this.
>
>> No problem, thanks for the help :)
>
>> With the new patch:
>
>> Locating the TCG2Protocol
>> Calling GetEventLog on TCG2Protocol
>> Log returned
>> log_location is not empty
>> log_size != 0
>> log_size < 1M
>> Allocating memory for storing the logs
>> Returned from memory allocation
>> Copying log to new location
>
>> And then it hangs. I added a couple more print statements:
>
>> diff --git a/drivers/firmware/efi/libstub/tpm.c
> b/drivers/firmware/efi/libstub/tpm.c
>> index ee3fac109078..1ab5638bc50e 100644
>> --- a/drivers/firmware/efi/libstub/tpm.c
>> +++ b/drivers/firmware/efi/libstub/tpm.c
>> @@ -148,8 +148,11 @@ void
> efi_retrieve_tpm2_eventlog_1_2(efi_system_table_t *sys_table_arg)
>>          efi_printk(sys_table_arg, "Copying log to new location\n");
>
>>          memset(log_tbl, 0, sizeof(*log_tbl) + log_size);
>> +       efi_printk(sys_table_arg, "Successfully memset log_tbl to 0\n");
>>          log_tbl->size = log_size;
>> +       efi_printk(sys_table_arg, "Set log_tbl->size\n");
>>          log_tbl->version = EFI_TCG2_EVENT_LOG_FORMAT_TCG_1_2;
>> +       efi_printk(sys_table_arg, "Set log_tbl-version\n");
>>          memcpy(log_tbl->log, (void *) first_entry_addr, log_size);
>
>>          efi_printk(sys_table_arg, "Installing the log into the
> configuration table\n");
>
>> and it's hanging at "memset(log_tbl, 0, sizeof(*log_tbl) + log_size);"
>
> Thanks. Well, it looks like the memory that is supposedly allocated is not
> usable. I'm thinking this is a firmware bug.
> Ard, would you agree on this assumption? Thoughts on how to proceed?
>

I am rather puzzled why the allocate_pool() should succeed and the
subsequent memset() should fail. This does not look like an issue that
is intimately related to TPM2 support, rather an issue in the firmware
that happens to get tickled after the change.

Would you mind trying replacing EFI_LOADER_DATA with
EFI_BOOT_SERVICES_DATA in the allocate_pool() call?
Jeremy Cline March 12, 2018, 2:30 p.m. UTC | #5
On 03/12/2018 07:08 AM, Ard Biesheuvel wrote:
> On 10 March 2018 at 10:45, Thiebaud Weksteen <tweek@google.com> wrote:
>> On Fri, Mar 9, 2018 at 5:54 PM Jeremy Cline <jeremy@jcline.org> wrote:
>>
>>> On Fri, Mar 09, 2018 at 10:43:50AM +0000, Thiebaud Weksteen wrote:
>>>> Thanks a lot for trying out the patch!
>>>>
>>>> Please don't modify your install at this stage, I think we are hitting a
>>>> firmware bug and that would be awesome if we can fix how we are
>> handling it.
>>>> So, if we reach that stage in the function it could either be that:
>>>> * The allocation did not succeed, somehow, but the firmware still
>> returned
>>>> EFI_SUCCEED.
>>>> * The size requested is incorrect (I'm thinking something like a 1G of
>>>> log). This would be due to either a miscalculation of log_size
>> (possible)
>>>> or; the returned values of GetEventLog are not correct.
>>>> I'm sending a patch to add checks for these. Could you please apply and
>>>> retest?
>>>> Again, thanks for helping debugging this.
>>
>>> No problem, thanks for the help :)
>>
>>> With the new patch:
>>
>>> Locating the TCG2Protocol
>>> Calling GetEventLog on TCG2Protocol
>>> Log returned
>>> log_location is not empty
>>> log_size != 0
>>> log_size < 1M
>>> Allocating memory for storing the logs
>>> Returned from memory allocation
>>> Copying log to new location
>>
>>> And then it hangs. I added a couple more print statements:
>>
>>> diff --git a/drivers/firmware/efi/libstub/tpm.c
>> b/drivers/firmware/efi/libstub/tpm.c
>>> index ee3fac109078..1ab5638bc50e 100644
>>> --- a/drivers/firmware/efi/libstub/tpm.c
>>> +++ b/drivers/firmware/efi/libstub/tpm.c
>>> @@ -148,8 +148,11 @@ void
>> efi_retrieve_tpm2_eventlog_1_2(efi_system_table_t *sys_table_arg)
>>>          efi_printk(sys_table_arg, "Copying log to new location\n");
>>
>>>          memset(log_tbl, 0, sizeof(*log_tbl) + log_size);
>>> +       efi_printk(sys_table_arg, "Successfully memset log_tbl to 0\n");
>>>          log_tbl->size = log_size;
>>> +       efi_printk(sys_table_arg, "Set log_tbl->size\n");
>>>          log_tbl->version = EFI_TCG2_EVENT_LOG_FORMAT_TCG_1_2;
>>> +       efi_printk(sys_table_arg, "Set log_tbl-version\n");
>>>          memcpy(log_tbl->log, (void *) first_entry_addr, log_size);
>>
>>>          efi_printk(sys_table_arg, "Installing the log into the
>> configuration table\n");
>>
>>> and it's hanging at "memset(log_tbl, 0, sizeof(*log_tbl) + log_size);"
>>
>> Thanks. Well, it looks like the memory that is supposedly allocated is not
>> usable. I'm thinking this is a firmware bug.
>> Ard, would you agree on this assumption? Thoughts on how to proceed?
>>
> 
> I am rather puzzled why the allocate_pool() should succeed and the
> subsequent memset() should fail. This does not look like an issue that
> is intimately related to TPM2 support, rather an issue in the firmware
> that happens to get tickled after the change.
> 
> Would you mind trying replacing EFI_LOADER_DATA with
> EFI_BOOT_SERVICES_DATA in the allocate_pool() call?

Replacing EFI_LOADER_DATA with EFI_BOOT_SERVICES_DATA still hangs at the
memset() call.

Regards,
Jeremy
Jarkko Sakkinen March 16, 2018, 1:01 p.m. UTC | #6
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 +0000, Thiebaud Weksteen wrote:
> > > On Fri, Mar 9, 2018 at 5:54 PM Jeremy Cline <jeremy@jcline.org> 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
diff mbox

Patch

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