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
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 <[email protected]> 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
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 <[email protected]> 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
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
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.
> 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.
Regards,
Jeremy
On Wed, Mar 7, 2018 at 6:33 PM Jeremy Cline <[email protected]> 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
---
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, &tcg2_guid, NULL,
&tcg2_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,
&log_location, &log_last_entry, &truncated);
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 **) &log_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,
&linux_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
<somehow this part of the thread was missing some email addresses, I've added these now>
Hi,
On 07-03-18 12:34, Javier Martinez Canillas wrote:
> On 03/07/2018 12:10 PM, Hans de Goede wrote:
<snip>
>> 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.
<snip>
>> 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
On 03/08/2018 11:50 AM, Hans de Goede wrote:
> <somehow this part of the thread was missing some email addresses, I've
> added these now>
>
> Hi,
>
> On 07-03-18 12:34, Javier Martinez Canillas wrote:
<snip>
>> 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,0000,0001,2001,2002,2003
Boot0000* 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.
Regards,
Jeremy
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
Hi,
On 08-03-18 18:26, Jeremy Cline wrote:
> On 03/08/2018 11:50 AM, Hans de Goede wrote:
>> <somehow this part of the thread was missing some email addresses, I've
>> added these now>
>>
>> Hi,
>>
>> On 07-03-18 12:34, Javier Martinez Canillas wrote:
>
> <snip>
>
>>> 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,0000,0001,2001,2002,2003
> Boot0000* 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...
Regards,
Hans
On Fri, Mar 9, 2018 at 10:29 AM Hans de Goede <[email protected]> wrote:
> Hi,
> On 08-03-18 18:26, Jeremy Cline wrote:
> > On 03/08/2018 11:50 AM, Hans de Goede wrote:
> >> <somehow this part of the thread was missing some email addresses, I've
> >> added these now>
> >>
> >> Hi,
> >>
> >> On 07-03-18 12:34, Javier Martinez Canillas wrote:
> >
> > <snip>
> >
> >>> 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,0000,0001,2001,2002,2003
> > Boot0000* 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
---
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
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);"
Regards,
Jeremy
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
On Fri, Mar 9, 2018 at 5:54 PM Jeremy Cline <[email protected]> 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
On Sat, 2018-03-10 at 10:45 +0000, Thiebaud Weksteen wrote:
> On Fri, Mar 9, 2018 at 5:54 PM Jeremy Cline <[email protected]> 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
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 <[email protected]> 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
On 10 March 2018 at 10:45, Thiebaud Weksteen <[email protected]> wrote:
> On Fri, Mar 9, 2018 at 5:54 PM Jeremy Cline <[email protected]> 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?
On 03/12/2018 07:08 AM, Ard Biesheuvel wrote:
> On 10 March 2018 at 10:45, Thiebaud Weksteen <[email protected]> wrote:
>> On Fri, Mar 9, 2018 at 5:54 PM Jeremy Cline <[email protected]> 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
On 12 March 2018 at 14:30, Jeremy Cline <[email protected]> wrote:
> On 03/12/2018 07:08 AM, Ard Biesheuvel wrote:
>> On 10 March 2018 at 10:45, Thiebaud Weksteen <[email protected]> wrote:
>>> On Fri, Mar 9, 2018 at 5:54 PM Jeremy Cline <[email protected]> wrote:
>>>
>>>> On Fri, Mar 09, 2018 at 10:43:50AM +0000, Thiebaud Weksteen wrote:
>>>>> Thanks a lot for trying out the patch!
>>>>>
>>>>> Please don't modify your install at this stage, I think we are hitting a
>>>>> firmware bug and that would be awesome if we can fix how we are
>>> handling it.
>>>>> So, if we reach that stage in the function it could either be that:
>>>>> * The allocation did not succeed, somehow, but the firmware still
>>> returned
>>>>> EFI_SUCCEED.
>>>>> * The size requested is incorrect (I'm thinking something like a 1G of
>>>>> log). This would be due to either a miscalculation of log_size
>>> (possible)
>>>>> or; the returned values of GetEventLog are not correct.
>>>>> I'm sending a patch to add checks for these. Could you please apply and
>>>>> retest?
>>>>> Again, thanks for helping debugging this.
>>>
>>>> No problem, thanks for the help :)
>>>
>>>> With the new patch:
>>>
>>>> Locating the TCG2Protocol
>>>> Calling GetEventLog on TCG2Protocol
>>>> Log returned
>>>> log_location is not empty
>>>> log_size != 0
>>>> log_size < 1M
>>>> Allocating memory for storing the logs
>>>> Returned from memory allocation
>>>> Copying log to new location
>>>
>>>> And then it hangs. I added a couple more print statements:
>>>
>>>> diff --git a/drivers/firmware/efi/libstub/tpm.c
>>> b/drivers/firmware/efi/libstub/tpm.c
>>>> index ee3fac109078..1ab5638bc50e 100644
>>>> --- a/drivers/firmware/efi/libstub/tpm.c
>>>> +++ b/drivers/firmware/efi/libstub/tpm.c
>>>> @@ -148,8 +148,11 @@ void
>>> efi_retrieve_tpm2_eventlog_1_2(efi_system_table_t *sys_table_arg)
>>>> efi_printk(sys_table_arg, "Copying log to new location\n");
>>>
>>>> memset(log_tbl, 0, sizeof(*log_tbl) + log_size);
>>>> + efi_printk(sys_table_arg, "Successfully memset log_tbl to 0\n");
>>>> log_tbl->size = log_size;
>>>> + efi_printk(sys_table_arg, "Set log_tbl->size\n");
>>>> log_tbl->version = EFI_TCG2_EVENT_LOG_FORMAT_TCG_1_2;
>>>> + efi_printk(sys_table_arg, "Set log_tbl-version\n");
>>>> memcpy(log_tbl->log, (void *) first_entry_addr, log_size);
>>>
>>>> efi_printk(sys_table_arg, "Installing the log into the
>>> configuration table\n");
>>>
>>>> and it's hanging at "memset(log_tbl, 0, sizeof(*log_tbl) + log_size);"
>>>
>>> Thanks. Well, it looks like the memory that is supposedly allocated is not
>>> usable. I'm thinking this is a firmware bug.
>>> Ard, would you agree on this assumption? Thoughts on how to proceed?
>>>
>>
>> I am rather puzzled why the allocate_pool() should succeed and the
>> subsequent memset() should fail. This does not look like an issue that
>> is intimately related to TPM2 support, rather an issue in the firmware
>> that happens to get tickled after the change.
>>
>> Would you mind trying replacing EFI_LOADER_DATA with
>> EFI_BOOT_SERVICES_DATA in the allocate_pool() call?
>
> Replacing EFI_LOADER_DATA with EFI_BOOT_SERVICES_DATA still hangs at the
> memset() call.
>
Could you try the following please? (attached as well in case gmail mangles it)
diff --git a/drivers/firmware/efi/libstub/tpm.c
b/drivers/firmware/efi/libstub/tpm.c
index 2298560cea72..30d960a344b7 100644
--- a/drivers/firmware/efi/libstub/tpm.c
+++ b/drivers/firmware/efi/libstub/tpm.c
@@ -70,6 +70,8 @@ void
efi_retrieve_tpm2_eventlog_1_2(efi_system_table_t *sys_table_arg)
size_t log_size, last_entry_size;
efi_bool_t truncated;
void *tcg2_protocol;
+ unsigned long num_pages;
+ efi_physical_addr_t log_tbl_alloc;
status = efi_call_early(locate_protocol, &tcg2_guid, NULL,
&tcg2_protocol);
@@ -104,9 +106,12 @@ void
efi_retrieve_tpm2_eventlog_1_2(efi_system_table_t *sys_table_arg)
}
/* Allocate space for the logs and copy them. */
- status = efi_call_early(allocate_pool, EFI_LOADER_DATA,
- sizeof(*log_tbl) + log_size,
- (void **) &log_tbl);
+ num_pages = DIV_ROUND_UP(sizeof(*log_tbl) + log_size, EFI_PAGE_SIZE);
+ status = efi_call_early(allocate_pages,
+ EFI_ALLOCATE_ANY_PAGES,
+ EFI_LOADER_DATA,
+ num_pages,
+ &log_tbl_alloc);
if (status != EFI_SUCCESS) {
efi_printk(sys_table_arg,
@@ -114,6 +119,7 @@ void
efi_retrieve_tpm2_eventlog_1_2(efi_system_table_t *sys_table_arg)
return;
}
+ log_tbl = (struct linux_efi_tpm_eventlog *)(unsigned long)log_tbl_alloc;
memset(log_tbl, 0, sizeof(*log_tbl) + log_size);
log_tbl->size = log_size;
log_tbl->version = EFI_TCG2_EVENT_LOG_FORMAT_TCG_1_2;
@@ -126,7 +132,7 @@ void
efi_retrieve_tpm2_eventlog_1_2(efi_system_table_t *sys_table_arg)
return;
err_free:
- efi_call_early(free_pool, log_tbl);
+ efi_call_early(free_pages, log_tbl_alloc, num_pages);
}
void efi_retrieve_tpm2_eventlog(efi_system_table_t *sys_table_arg)
On 03/12/2018 10:56 AM, Ard Biesheuvel wrote:
> On 12 March 2018 at 14:30, Jeremy Cline <[email protected]> wrote:
>> On 03/12/2018 07:08 AM, Ard Biesheuvel wrote:
>>> On 10 March 2018 at 10:45, Thiebaud Weksteen <[email protected]> wrote:
>>>> On Fri, Mar 9, 2018 at 5:54 PM Jeremy Cline <[email protected]> wrote:
>>>>
>>>>> On Fri, Mar 09, 2018 at 10:43:50AM +0000, Thiebaud Weksteen wrote:
>>>>>> Thanks a lot for trying out the patch!
>>>>>>
>>>>>> Please don't modify your install at this stage, I think we are hitting a
>>>>>> firmware bug and that would be awesome if we can fix how we are
>>>> handling it.
>>>>>> So, if we reach that stage in the function it could either be that:
>>>>>> * The allocation did not succeed, somehow, but the firmware still
>>>> returned
>>>>>> EFI_SUCCEED.
>>>>>> * The size requested is incorrect (I'm thinking something like a 1G of
>>>>>> log). This would be due to either a miscalculation of log_size
>>>> (possible)
>>>>>> or; the returned values of GetEventLog are not correct.
>>>>>> I'm sending a patch to add checks for these. Could you please apply and
>>>>>> retest?
>>>>>> Again, thanks for helping debugging this.
>>>>
>>>>> No problem, thanks for the help :)
>>>>
>>>>> With the new patch:
>>>>
>>>>> Locating the TCG2Protocol
>>>>> Calling GetEventLog on TCG2Protocol
>>>>> Log returned
>>>>> log_location is not empty
>>>>> log_size != 0
>>>>> log_size < 1M
>>>>> Allocating memory for storing the logs
>>>>> Returned from memory allocation
>>>>> Copying log to new location
>>>>
>>>>> And then it hangs. I added a couple more print statements:
>>>>
>>>>> diff --git a/drivers/firmware/efi/libstub/tpm.c
>>>> b/drivers/firmware/efi/libstub/tpm.c
>>>>> index ee3fac109078..1ab5638bc50e 100644
>>>>> --- a/drivers/firmware/efi/libstub/tpm.c
>>>>> +++ b/drivers/firmware/efi/libstub/tpm.c
>>>>> @@ -148,8 +148,11 @@ void
>>>> efi_retrieve_tpm2_eventlog_1_2(efi_system_table_t *sys_table_arg)
>>>>> efi_printk(sys_table_arg, "Copying log to new location\n");
>>>>
>>>>> memset(log_tbl, 0, sizeof(*log_tbl) + log_size);
>>>>> + efi_printk(sys_table_arg, "Successfully memset log_tbl to 0\n");
>>>>> log_tbl->size = log_size;
>>>>> + efi_printk(sys_table_arg, "Set log_tbl->size\n");
>>>>> log_tbl->version = EFI_TCG2_EVENT_LOG_FORMAT_TCG_1_2;
>>>>> + efi_printk(sys_table_arg, "Set log_tbl-version\n");
>>>>> memcpy(log_tbl->log, (void *) first_entry_addr, log_size);
>>>>
>>>>> efi_printk(sys_table_arg, "Installing the log into the
>>>> configuration table\n");
>>>>
>>>>> and it's hanging at "memset(log_tbl, 0, sizeof(*log_tbl) + log_size);"
>>>>
>>>> Thanks. Well, it looks like the memory that is supposedly allocated is not
>>>> usable. I'm thinking this is a firmware bug.
>>>> Ard, would you agree on this assumption? Thoughts on how to proceed?
>>>>
>>>
>>> I am rather puzzled why the allocate_pool() should succeed and the
>>> subsequent memset() should fail. This does not look like an issue that
>>> is intimately related to TPM2 support, rather an issue in the firmware
>>> that happens to get tickled after the change.
>>>
>>> Would you mind trying replacing EFI_LOADER_DATA with
>>> EFI_BOOT_SERVICES_DATA in the allocate_pool() call?
>>
>> Replacing EFI_LOADER_DATA with EFI_BOOT_SERVICES_DATA still hangs at the
>> memset() call.
>>
>
> Could you try the following please? (attached as well in case gmail mangles it)
>
> diff --git a/drivers/firmware/efi/libstub/tpm.c
> b/drivers/firmware/efi/libstub/tpm.c
> index 2298560cea72..30d960a344b7 100644
> --- a/drivers/firmware/efi/libstub/tpm.c
> +++ b/drivers/firmware/efi/libstub/tpm.c
> @@ -70,6 +70,8 @@ void
> efi_retrieve_tpm2_eventlog_1_2(efi_system_table_t *sys_table_arg)
> size_t log_size, last_entry_size;
> efi_bool_t truncated;
> void *tcg2_protocol;
> + unsigned long num_pages;
> + efi_physical_addr_t log_tbl_alloc;
>
> status = efi_call_early(locate_protocol, &tcg2_guid, NULL,
> &tcg2_protocol);
> @@ -104,9 +106,12 @@ void
> efi_retrieve_tpm2_eventlog_1_2(efi_system_table_t *sys_table_arg)
> }
>
> /* Allocate space for the logs and copy them. */
> - status = efi_call_early(allocate_pool, EFI_LOADER_DATA,
> - sizeof(*log_tbl) + log_size,
> - (void **) &log_tbl);
> + num_pages = DIV_ROUND_UP(sizeof(*log_tbl) + log_size, EFI_PAGE_SIZE);
> + status = efi_call_early(allocate_pages,
> + EFI_ALLOCATE_ANY_PAGES,
> + EFI_LOADER_DATA,
> + num_pages,
> + &log_tbl_alloc);
>
> if (status != EFI_SUCCESS) {
> efi_printk(sys_table_arg,
> @@ -114,6 +119,7 @@ void
> efi_retrieve_tpm2_eventlog_1_2(efi_system_table_t *sys_table_arg)
> return;
> }
>
> + log_tbl = (struct linux_efi_tpm_eventlog *)(unsigned long)log_tbl_alloc;
> memset(log_tbl, 0, sizeof(*log_tbl) + log_size);
> log_tbl->size = log_size;
> log_tbl->version = EFI_TCG2_EVENT_LOG_FORMAT_TCG_1_2;
> @@ -126,7 +132,7 @@ void
> efi_retrieve_tpm2_eventlog_1_2(efi_system_table_t *sys_table_arg)
> return;
>
> err_free:
> - efi_call_early(free_pool, log_tbl);
> + efi_call_early(free_pages, log_tbl_alloc, num_pages);
> }
>
> void efi_retrieve_tpm2_eventlog(efi_system_table_t *sys_table_arg)
>
Hi Ard,
When I apply this, it starts hanging at
status = efi_call_proto(efi_tcg2_protocol, get_event_log, tcg2_protocol,
EFI_TCG2_EVENT_LOG_FORMAT_TCG_1_2,
&log_location, &log_last_entry, &truncated);
rather than at the memset() call.
Regards,
Jeremy
On 12 March 2018 at 17:01, Jeremy Cline <[email protected]> wrote:
> On 03/12/2018 10:56 AM, Ard Biesheuvel wrote:
>> On 12 March 2018 at 14:30, Jeremy Cline <[email protected]> wrote:
>>> On 03/12/2018 07:08 AM, Ard Biesheuvel wrote:
>>>> On 10 March 2018 at 10:45, Thiebaud Weksteen <[email protected]> wrote:
>>>>> On Fri, Mar 9, 2018 at 5:54 PM Jeremy Cline <[email protected]> wrote:
>>>>>
>>>>>> On Fri, Mar 09, 2018 at 10:43:50AM +0000, Thiebaud Weksteen wrote:
>>>>>>> Thanks a lot for trying out the patch!
>>>>>>>
>>>>>>> Please don't modify your install at this stage, I think we are hitting a
>>>>>>> firmware bug and that would be awesome if we can fix how we are
>>>>> handling it.
>>>>>>> So, if we reach that stage in the function it could either be that:
>>>>>>> * The allocation did not succeed, somehow, but the firmware still
>>>>> returned
>>>>>>> EFI_SUCCEED.
>>>>>>> * The size requested is incorrect (I'm thinking something like a 1G of
>>>>>>> log). This would be due to either a miscalculation of log_size
>>>>> (possible)
>>>>>>> or; the returned values of GetEventLog are not correct.
>>>>>>> I'm sending a patch to add checks for these. Could you please apply and
>>>>>>> retest?
>>>>>>> Again, thanks for helping debugging this.
>>>>>
>>>>>> No problem, thanks for the help :)
>>>>>
>>>>>> With the new patch:
>>>>>
>>>>>> Locating the TCG2Protocol
>>>>>> Calling GetEventLog on TCG2Protocol
>>>>>> Log returned
>>>>>> log_location is not empty
>>>>>> log_size != 0
>>>>>> log_size < 1M
>>>>>> Allocating memory for storing the logs
>>>>>> Returned from memory allocation
>>>>>> Copying log to new location
>>>>>
>>>>>> And then it hangs. I added a couple more print statements:
>>>>>
>>>>>> diff --git a/drivers/firmware/efi/libstub/tpm.c
>>>>> b/drivers/firmware/efi/libstub/tpm.c
>>>>>> index ee3fac109078..1ab5638bc50e 100644
>>>>>> --- a/drivers/firmware/efi/libstub/tpm.c
>>>>>> +++ b/drivers/firmware/efi/libstub/tpm.c
>>>>>> @@ -148,8 +148,11 @@ void
>>>>> efi_retrieve_tpm2_eventlog_1_2(efi_system_table_t *sys_table_arg)
>>>>>> efi_printk(sys_table_arg, "Copying log to new location\n");
>>>>>
>>>>>> memset(log_tbl, 0, sizeof(*log_tbl) + log_size);
>>>>>> + efi_printk(sys_table_arg, "Successfully memset log_tbl to 0\n");
>>>>>> log_tbl->size = log_size;
>>>>>> + efi_printk(sys_table_arg, "Set log_tbl->size\n");
>>>>>> log_tbl->version = EFI_TCG2_EVENT_LOG_FORMAT_TCG_1_2;
>>>>>> + efi_printk(sys_table_arg, "Set log_tbl-version\n");
>>>>>> memcpy(log_tbl->log, (void *) first_entry_addr, log_size);
>>>>>
>>>>>> efi_printk(sys_table_arg, "Installing the log into the
>>>>> configuration table\n");
>>>>>
>>>>>> and it's hanging at "memset(log_tbl, 0, sizeof(*log_tbl) + log_size);"
>>>>>
>>>>> Thanks. Well, it looks like the memory that is supposedly allocated is not
>>>>> usable. I'm thinking this is a firmware bug.
>>>>> Ard, would you agree on this assumption? Thoughts on how to proceed?
>>>>>
>>>>
>>>> I am rather puzzled why the allocate_pool() should succeed and the
>>>> subsequent memset() should fail. This does not look like an issue that
>>>> is intimately related to TPM2 support, rather an issue in the firmware
>>>> that happens to get tickled after the change.
>>>>
>>>> Would you mind trying replacing EFI_LOADER_DATA with
>>>> EFI_BOOT_SERVICES_DATA in the allocate_pool() call?
>>>
>>> Replacing EFI_LOADER_DATA with EFI_BOOT_SERVICES_DATA still hangs at the
>>> memset() call.
>>>
>>
>> Could you try the following please? (attached as well in case gmail mangles it)
>>
>> diff --git a/drivers/firmware/efi/libstub/tpm.c
>> b/drivers/firmware/efi/libstub/tpm.c
>> index 2298560cea72..30d960a344b7 100644
>> --- a/drivers/firmware/efi/libstub/tpm.c
>> +++ b/drivers/firmware/efi/libstub/tpm.c
>> @@ -70,6 +70,8 @@ void
>> efi_retrieve_tpm2_eventlog_1_2(efi_system_table_t *sys_table_arg)
>> size_t log_size, last_entry_size;
>> efi_bool_t truncated;
>> void *tcg2_protocol;
>> + unsigned long num_pages;
>> + efi_physical_addr_t log_tbl_alloc;
>>
>> status = efi_call_early(locate_protocol, &tcg2_guid, NULL,
>> &tcg2_protocol);
>> @@ -104,9 +106,12 @@ void
>> efi_retrieve_tpm2_eventlog_1_2(efi_system_table_t *sys_table_arg)
>> }
>>
>> /* Allocate space for the logs and copy them. */
>> - status = efi_call_early(allocate_pool, EFI_LOADER_DATA,
>> - sizeof(*log_tbl) + log_size,
>> - (void **) &log_tbl);
>> + num_pages = DIV_ROUND_UP(sizeof(*log_tbl) + log_size, EFI_PAGE_SIZE);
>> + status = efi_call_early(allocate_pages,
>> + EFI_ALLOCATE_ANY_PAGES,
>> + EFI_LOADER_DATA,
>> + num_pages,
>> + &log_tbl_alloc);
>>
>> if (status != EFI_SUCCESS) {
>> efi_printk(sys_table_arg,
>> @@ -114,6 +119,7 @@ void
>> efi_retrieve_tpm2_eventlog_1_2(efi_system_table_t *sys_table_arg)
>> return;
>> }
>>
>> + log_tbl = (struct linux_efi_tpm_eventlog *)(unsigned long)log_tbl_alloc;
>> memset(log_tbl, 0, sizeof(*log_tbl) + log_size);
>> log_tbl->size = log_size;
>> log_tbl->version = EFI_TCG2_EVENT_LOG_FORMAT_TCG_1_2;
>> @@ -126,7 +132,7 @@ void
>> efi_retrieve_tpm2_eventlog_1_2(efi_system_table_t *sys_table_arg)
>> return;
>>
>> err_free:
>> - efi_call_early(free_pool, log_tbl);
>> + efi_call_early(free_pages, log_tbl_alloc, num_pages);
>> }
>>
>> void efi_retrieve_tpm2_eventlog(efi_system_table_t *sys_table_arg)
>>
>
> Hi Ard,
>
> When I apply this, it starts hanging at
>
> status = efi_call_proto(efi_tcg2_protocol, get_event_log, tcg2_protocol,
> EFI_TCG2_EVENT_LOG_FORMAT_TCG_1_2,
> &log_location, &log_last_entry, &truncated);
>
> rather than at the memset() call.
>
That is *very* surprising, given that the change only affects code
that executes after that.
I understand how annoying this is for you, and I think we should try
to fix this, but reverting the patches outright isn't the solution
either.
Which UEFI vendor and version does your system report?
On Mon, Mar 12, 2018 at 6:30 PM Ard Biesheuvel <[email protected]>
wrote:
> On 12 March 2018 at 17:01, Jeremy Cline <[email protected]> wrote:
> > On 03/12/2018 10:56 AM, Ard Biesheuvel wrote:
> >> On 12 March 2018 at 14:30, Jeremy Cline <[email protected]> wrote:
> >>> On 03/12/2018 07:08 AM, Ard Biesheuvel wrote:
> >>>> On 10 March 2018 at 10:45, Thiebaud Weksteen <[email protected]>
wrote:
> >>>>> On Fri, Mar 9, 2018 at 5:54 PM Jeremy Cline <[email protected]>
wrote:
> >>>>>
> >>>>>> On Fri, Mar 09, 2018 at 10:43:50AM +0000, Thiebaud Weksteen wrote:
> >>>>>>> Thanks a lot for trying out the patch!
> >>>>>>>
> >>>>>>> Please don't modify your install at this stage, I think we are
hitting a
> >>>>>>> firmware bug and that would be awesome if we can fix how we are
> >>>>> handling it.
> >>>>>>> So, if we reach that stage in the function it could either be
that:
> >>>>>>> * The allocation did not succeed, somehow, but the firmware still
> >>>>> returned
> >>>>>>> EFI_SUCCEED.
> >>>>>>> * The size requested is incorrect (I'm thinking something like a
1G of
> >>>>>>> log). This would be due to either a miscalculation of log_size
> >>>>> (possible)
> >>>>>>> or; the returned values of GetEventLog are not correct.
> >>>>>>> I'm sending a patch to add checks for these. Could you please
apply and
> >>>>>>> retest?
> >>>>>>> Again, thanks for helping debugging this.
> >>>>>
> >>>>>> No problem, thanks for the help :)
> >>>>>
> >>>>>> With the new patch:
> >>>>>
> >>>>>> Locating the TCG2Protocol
> >>>>>> Calling GetEventLog on TCG2Protocol
> >>>>>> Log returned
> >>>>>> log_location is not empty
> >>>>>> log_size != 0
> >>>>>> log_size < 1M
> >>>>>> Allocating memory for storing the logs
> >>>>>> Returned from memory allocation
> >>>>>> Copying log to new location
> >>>>>
> >>>>>> And then it hangs. I added a couple more print statements:
> >>>>>
> >>>>>> diff --git a/drivers/firmware/efi/libstub/tpm.c
> >>>>> b/drivers/firmware/efi/libstub/tpm.c
> >>>>>> index ee3fac109078..1ab5638bc50e 100644
> >>>>>> --- a/drivers/firmware/efi/libstub/tpm.c
> >>>>>> +++ b/drivers/firmware/efi/libstub/tpm.c
> >>>>>> @@ -148,8 +148,11 @@ void
> >>>>> efi_retrieve_tpm2_eventlog_1_2(efi_system_table_t *sys_table_arg)
> >>>>>> efi_printk(sys_table_arg, "Copying log to new
location\n");
> >>>>>
> >>>>>> memset(log_tbl, 0, sizeof(*log_tbl) + log_size);
> >>>>>> + efi_printk(sys_table_arg, "Successfully memset log_tbl to
0\n");
> >>>>>> log_tbl->size = log_size;
> >>>>>> + efi_printk(sys_table_arg, "Set log_tbl->size\n");
> >>>>>> log_tbl->version = EFI_TCG2_EVENT_LOG_FORMAT_TCG_1_2;
> >>>>>> + efi_printk(sys_table_arg, "Set log_tbl-version\n");
> >>>>>> memcpy(log_tbl->log, (void *) first_entry_addr, log_size);
> >>>>>
> >>>>>> efi_printk(sys_table_arg, "Installing the log into the
> >>>>> configuration table\n");
> >>>>>
> >>>>>> and it's hanging at "memset(log_tbl, 0, sizeof(*log_tbl) +
log_size);"
> >>>>>
> >>>>> Thanks. Well, it looks like the memory that is supposedly allocated
is not
> >>>>> usable. I'm thinking this is a firmware bug.
> >>>>> Ard, would you agree on this assumption? Thoughts on how to proceed?
> >>>>>
> >>>>
> >>>> I am rather puzzled why the allocate_pool() should succeed and the
> >>>> subsequent memset() should fail. This does not look like an issue
that
> >>>> is intimately related to TPM2 support, rather an issue in the
firmware
> >>>> that happens to get tickled after the change.
> >>>>
> >>>> Would you mind trying replacing EFI_LOADER_DATA with
> >>>> EFI_BOOT_SERVICES_DATA in the allocate_pool() call?
> >>>
> >>> Replacing EFI_LOADER_DATA with EFI_BOOT_SERVICES_DATA still hangs at
the
> >>> memset() call.
> >>>
> >>
> >> Could you try the following please? (attached as well in case gmail
mangles it)
> >>
> >> diff --git a/drivers/firmware/efi/libstub/tpm.c
> >> b/drivers/firmware/efi/libstub/tpm.c
> >> index 2298560cea72..30d960a344b7 100644
> >> --- a/drivers/firmware/efi/libstub/tpm.c
> >> +++ b/drivers/firmware/efi/libstub/tpm.c
> >> @@ -70,6 +70,8 @@ void
> >> efi_retrieve_tpm2_eventlog_1_2(efi_system_table_t *sys_table_arg)
> >> size_t log_size, last_entry_size;
> >> efi_bool_t truncated;
> >> void *tcg2_protocol;
> >> + unsigned long num_pages;
> >> + efi_physical_addr_t log_tbl_alloc;
> >>
> >> status = efi_call_early(locate_protocol, &tcg2_guid, NULL,
> >> &tcg2_protocol);
> >> @@ -104,9 +106,12 @@ void
> >> efi_retrieve_tpm2_eventlog_1_2(efi_system_table_t *sys_table_arg)
> >> }
> >>
> >> /* Allocate space for the logs and copy them. */
> >> - status = efi_call_early(allocate_pool, EFI_LOADER_DATA,
> >> - sizeof(*log_tbl) + log_size,
> >> - (void **) &log_tbl);
> >> + num_pages = DIV_ROUND_UP(sizeof(*log_tbl) + log_size,
EFI_PAGE_SIZE);
> >> + status = efi_call_early(allocate_pages,
> >> + EFI_ALLOCATE_ANY_PAGES,
> >> + EFI_LOADER_DATA,
> >> + num_pages,
> >> + &log_tbl_alloc);
> >>
> >> if (status != EFI_SUCCESS) {
> >> efi_printk(sys_table_arg,
> >> @@ -114,6 +119,7 @@ void
> >> efi_retrieve_tpm2_eventlog_1_2(efi_system_table_t *sys_table_arg)
> >> return;
> >> }
> >>
> >> + log_tbl = (struct linux_efi_tpm_eventlog *)(unsigned
long)log_tbl_alloc;
> >> memset(log_tbl, 0, sizeof(*log_tbl) + log_size);
> >> log_tbl->size = log_size;
> >> log_tbl->version = EFI_TCG2_EVENT_LOG_FORMAT_TCG_1_2;
> >> @@ -126,7 +132,7 @@ void
> >> efi_retrieve_tpm2_eventlog_1_2(efi_system_table_t *sys_table_arg)
> >> return;
> >>
> >> err_free:
> >> - efi_call_early(free_pool, log_tbl);
> >> + efi_call_early(free_pages, log_tbl_alloc, num_pages);
> >> }
> >>
> >> void efi_retrieve_tpm2_eventlog(efi_system_table_t *sys_table_arg)
> >>
> >
> > Hi Ard,
> >
> > When I apply this, it starts hanging at
> >
> > status = efi_call_proto(efi_tcg2_protocol, get_event_log, tcg2_protocol,
> > EFI_TCG2_EVENT_LOG_FORMAT_TCG_1_2,
> > &log_location, &log_last_entry, &truncated);
> >
> > rather than at the memset() call.
> >
> That is *very* surprising, given that the change only affects code
> that executes after that.
> I understand how annoying this is for you, and I think we should try
> to fix this, but reverting the patches outright isn't the solution
> either.
> Which UEFI vendor and version does your system report?
You should be able to find this info using the "ver" command in the UEFI
shell.
The UEFI vendor is Insyde (see first message).
On 03/12/2018 01:30 PM, Ard Biesheuvel wrote:
> On 12 March 2018 at 17:01, Jeremy Cline <[email protected]> wrote:
>> On 03/12/2018 10:56 AM, Ard Biesheuvel wrote:
>>> On 12 March 2018 at 14:30, Jeremy Cline <[email protected]> wrote:
>>>> On 03/12/2018 07:08 AM, Ard Biesheuvel wrote:
>>>>> On 10 March 2018 at 10:45, Thiebaud Weksteen <[email protected]> wrote:
>>>>>> On Fri, Mar 9, 2018 at 5:54 PM Jeremy Cline <[email protected]> wrote:
>>>>>>
>>>>>>> On Fri, Mar 09, 2018 at 10:43:50AM +0000, Thiebaud Weksteen wrote:
>>>>>>>> Thanks a lot for trying out the patch!
>>>>>>>>
>>>>>>>> Please don't modify your install at this stage, I think we are hitting a
>>>>>>>> firmware bug and that would be awesome if we can fix how we are
>>>>>> handling it.
>>>>>>>> So, if we reach that stage in the function it could either be that:
>>>>>>>> * The allocation did not succeed, somehow, but the firmware still
>>>>>> returned
>>>>>>>> EFI_SUCCEED.
>>>>>>>> * The size requested is incorrect (I'm thinking something like a 1G of
>>>>>>>> log). This would be due to either a miscalculation of log_size
>>>>>> (possible)
>>>>>>>> or; the returned values of GetEventLog are not correct.
>>>>>>>> I'm sending a patch to add checks for these. Could you please apply and
>>>>>>>> retest?
>>>>>>>> Again, thanks for helping debugging this.
>>>>>>
>>>>>>> No problem, thanks for the help :)
>>>>>>
>>>>>>> With the new patch:
>>>>>>
>>>>>>> Locating the TCG2Protocol
>>>>>>> Calling GetEventLog on TCG2Protocol
>>>>>>> Log returned
>>>>>>> log_location is not empty
>>>>>>> log_size != 0
>>>>>>> log_size < 1M
>>>>>>> Allocating memory for storing the logs
>>>>>>> Returned from memory allocation
>>>>>>> Copying log to new location
>>>>>>
>>>>>>> And then it hangs. I added a couple more print statements:
>>>>>>
>>>>>>> diff --git a/drivers/firmware/efi/libstub/tpm.c
>>>>>> b/drivers/firmware/efi/libstub/tpm.c
>>>>>>> index ee3fac109078..1ab5638bc50e 100644
>>>>>>> --- a/drivers/firmware/efi/libstub/tpm.c
>>>>>>> +++ b/drivers/firmware/efi/libstub/tpm.c
>>>>>>> @@ -148,8 +148,11 @@ void
>>>>>> efi_retrieve_tpm2_eventlog_1_2(efi_system_table_t *sys_table_arg)
>>>>>>> efi_printk(sys_table_arg, "Copying log to new location\n");
>>>>>>
>>>>>>> memset(log_tbl, 0, sizeof(*log_tbl) + log_size);
>>>>>>> + efi_printk(sys_table_arg, "Successfully memset log_tbl to 0\n");
>>>>>>> log_tbl->size = log_size;
>>>>>>> + efi_printk(sys_table_arg, "Set log_tbl->size\n");
>>>>>>> log_tbl->version = EFI_TCG2_EVENT_LOG_FORMAT_TCG_1_2;
>>>>>>> + efi_printk(sys_table_arg, "Set log_tbl-version\n");
>>>>>>> memcpy(log_tbl->log, (void *) first_entry_addr, log_size);
>>>>>>
>>>>>>> efi_printk(sys_table_arg, "Installing the log into the
>>>>>> configuration table\n");
>>>>>>
>>>>>>> and it's hanging at "memset(log_tbl, 0, sizeof(*log_tbl) + log_size);"
>>>>>>
>>>>>> Thanks. Well, it looks like the memory that is supposedly allocated is not
>>>>>> usable. I'm thinking this is a firmware bug.
>>>>>> Ard, would you agree on this assumption? Thoughts on how to proceed?
>>>>>>
>>>>>
>>>>> I am rather puzzled why the allocate_pool() should succeed and the
>>>>> subsequent memset() should fail. This does not look like an issue that
>>>>> is intimately related to TPM2 support, rather an issue in the firmware
>>>>> that happens to get tickled after the change.
>>>>>
>>>>> Would you mind trying replacing EFI_LOADER_DATA with
>>>>> EFI_BOOT_SERVICES_DATA in the allocate_pool() call?
>>>>
>>>> Replacing EFI_LOADER_DATA with EFI_BOOT_SERVICES_DATA still hangs at the
>>>> memset() call.
>>>>
>>>
>>> Could you try the following please? (attached as well in case gmail mangles it)
>>>
>>> diff --git a/drivers/firmware/efi/libstub/tpm.c
>>> b/drivers/firmware/efi/libstub/tpm.c
>>> index 2298560cea72..30d960a344b7 100644
>>> --- a/drivers/firmware/efi/libstub/tpm.c
>>> +++ b/drivers/firmware/efi/libstub/tpm.c
>>> @@ -70,6 +70,8 @@ void
>>> efi_retrieve_tpm2_eventlog_1_2(efi_system_table_t *sys_table_arg)
>>> size_t log_size, last_entry_size;
>>> efi_bool_t truncated;
>>> void *tcg2_protocol;
>>> + unsigned long num_pages;
>>> + efi_physical_addr_t log_tbl_alloc;
>>>
>>> status = efi_call_early(locate_protocol, &tcg2_guid, NULL,
>>> &tcg2_protocol);
>>> @@ -104,9 +106,12 @@ void
>>> efi_retrieve_tpm2_eventlog_1_2(efi_system_table_t *sys_table_arg)
>>> }
>>>
>>> /* Allocate space for the logs and copy them. */
>>> - status = efi_call_early(allocate_pool, EFI_LOADER_DATA,
>>> - sizeof(*log_tbl) + log_size,
>>> - (void **) &log_tbl);
>>> + num_pages = DIV_ROUND_UP(sizeof(*log_tbl) + log_size, EFI_PAGE_SIZE);
>>> + status = efi_call_early(allocate_pages,
>>> + EFI_ALLOCATE_ANY_PAGES,
>>> + EFI_LOADER_DATA,
>>> + num_pages,
>>> + &log_tbl_alloc);
>>>
>>> if (status != EFI_SUCCESS) {
>>> efi_printk(sys_table_arg,
>>> @@ -114,6 +119,7 @@ void
>>> efi_retrieve_tpm2_eventlog_1_2(efi_system_table_t *sys_table_arg)
>>> return;
>>> }
>>>
>>> + log_tbl = (struct linux_efi_tpm_eventlog *)(unsigned long)log_tbl_alloc;
>>> memset(log_tbl, 0, sizeof(*log_tbl) + log_size);
>>> log_tbl->size = log_size;
>>> log_tbl->version = EFI_TCG2_EVENT_LOG_FORMAT_TCG_1_2;
>>> @@ -126,7 +132,7 @@ void
>>> efi_retrieve_tpm2_eventlog_1_2(efi_system_table_t *sys_table_arg)
>>> return;
>>>
>>> err_free:
>>> - efi_call_early(free_pool, log_tbl);
>>> + efi_call_early(free_pages, log_tbl_alloc, num_pages);
>>> }
>>>
>>> void efi_retrieve_tpm2_eventlog(efi_system_table_t *sys_table_arg)
>>>
>>
>> Hi Ard,
>>
>> When I apply this, it starts hanging at
>>
>> status = efi_call_proto(efi_tcg2_protocol, get_event_log, tcg2_protocol,
>> EFI_TCG2_EVENT_LOG_FORMAT_TCG_1_2,
>> &log_location, &log_last_entry, &truncated);
>>
>> rather than at the memset() call.
>>
>
> That is *very* surprising, given that the change only affects code
> that executes after that.
Yeah, I triple-checked because I was so surprised.
> I understand how annoying this is for you, and I think we should try
> to fix this, but reverting the patches outright isn't the solution
> either.
Completely understandable and I'm not the least bit annoyed :)
In case you missed it, Hans has the exact same tablet (I got this one
from him) and he can't reproduce it, but the one he sent me isn't using
shim and has a RHEL build of grub. At Thiebaud's request I didn't change
anything about the setup, but I'm guessing if I restore it to use the
Fedora setup the problem won't appear. I'm happy to make a backup and
verify this hypothesis.
>
> Which UEFI vendor and version does your system report?
>
It's InsydeH20 version BYT70A.YNCHENG.WIN.007
Regards,
Jeremy
On 03/12/2018 02:29 PM, Thiebaud Weksteen wrote:
> On Mon, Mar 12, 2018 at 6:30 PM Ard Biesheuvel <[email protected]>
> wrote:
>
>> On 12 March 2018 at 17:01, Jeremy Cline <[email protected]> wrote:
>>> On 03/12/2018 10:56 AM, Ard Biesheuvel wrote:
>>>> On 12 March 2018 at 14:30, Jeremy Cline <[email protected]> wrote:
>>>>> On 03/12/2018 07:08 AM, Ard Biesheuvel wrote:
>>>>>> On 10 March 2018 at 10:45, Thiebaud Weksteen <[email protected]>
> wrote:
>>>>>>> On Fri, Mar 9, 2018 at 5:54 PM Jeremy Cline <[email protected]>
> wrote:
>>>>>>>
>>>>>>>> On Fri, Mar 09, 2018 at 10:43:50AM +0000, Thiebaud Weksteen wrote:
>>>>>>>>> Thanks a lot for trying out the patch!
>>>>>>>>>
>>>>>>>>> Please don't modify your install at this stage, I think we are
> hitting a
>>>>>>>>> firmware bug and that would be awesome if we can fix how we are
>>>>>>> handling it.
>>>>>>>>> So, if we reach that stage in the function it could either be
> that:
>>>>>>>>> * The allocation did not succeed, somehow, but the firmware still
>>>>>>> returned
>>>>>>>>> EFI_SUCCEED.
>>>>>>>>> * The size requested is incorrect (I'm thinking something like a
> 1G of
>>>>>>>>> log). This would be due to either a miscalculation of log_size
>>>>>>> (possible)
>>>>>>>>> or; the returned values of GetEventLog are not correct.
>>>>>>>>> I'm sending a patch to add checks for these. Could you please
> apply and
>>>>>>>>> retest?
>>>>>>>>> Again, thanks for helping debugging this.
>>>>>>>
>>>>>>>> No problem, thanks for the help :)
>>>>>>>
>>>>>>>> With the new patch:
>>>>>>>
>>>>>>>> Locating the TCG2Protocol
>>>>>>>> Calling GetEventLog on TCG2Protocol
>>>>>>>> Log returned
>>>>>>>> log_location is not empty
>>>>>>>> log_size != 0
>>>>>>>> log_size < 1M
>>>>>>>> Allocating memory for storing the logs
>>>>>>>> Returned from memory allocation
>>>>>>>> Copying log to new location
>>>>>>>
>>>>>>>> And then it hangs. I added a couple more print statements:
>>>>>>>
>>>>>>>> diff --git a/drivers/firmware/efi/libstub/tpm.c
>>>>>>> b/drivers/firmware/efi/libstub/tpm.c
>>>>>>>> index ee3fac109078..1ab5638bc50e 100644
>>>>>>>> --- a/drivers/firmware/efi/libstub/tpm.c
>>>>>>>> +++ b/drivers/firmware/efi/libstub/tpm.c
>>>>>>>> @@ -148,8 +148,11 @@ void
>>>>>>> efi_retrieve_tpm2_eventlog_1_2(efi_system_table_t *sys_table_arg)
>>>>>>>> efi_printk(sys_table_arg, "Copying log to new
> location\n");
>>>>>>>
>>>>>>>> memset(log_tbl, 0, sizeof(*log_tbl) + log_size);
>>>>>>>> + efi_printk(sys_table_arg, "Successfully memset log_tbl to
> 0\n");
>>>>>>>> log_tbl->size = log_size;
>>>>>>>> + efi_printk(sys_table_arg, "Set log_tbl->size\n");
>>>>>>>> log_tbl->version = EFI_TCG2_EVENT_LOG_FORMAT_TCG_1_2;
>>>>>>>> + efi_printk(sys_table_arg, "Set log_tbl-version\n");
>>>>>>>> memcpy(log_tbl->log, (void *) first_entry_addr, log_size);
>>>>>>>
>>>>>>>> efi_printk(sys_table_arg, "Installing the log into the
>>>>>>> configuration table\n");
>>>>>>>
>>>>>>>> and it's hanging at "memset(log_tbl, 0, sizeof(*log_tbl) +
> log_size);"
>>>>>>>
>>>>>>> Thanks. Well, it looks like the memory that is supposedly allocated
> is not
>>>>>>> usable. I'm thinking this is a firmware bug.
>>>>>>> Ard, would you agree on this assumption? Thoughts on how to proceed?
>>>>>>>
>>>>>>
>>>>>> I am rather puzzled why the allocate_pool() should succeed and the
>>>>>> subsequent memset() should fail. This does not look like an issue
> that
>>>>>> is intimately related to TPM2 support, rather an issue in the
> firmware
>>>>>> that happens to get tickled after the change.
>>>>>>
>>>>>> Would you mind trying replacing EFI_LOADER_DATA with
>>>>>> EFI_BOOT_SERVICES_DATA in the allocate_pool() call?
>>>>>
>>>>> Replacing EFI_LOADER_DATA with EFI_BOOT_SERVICES_DATA still hangs at
> the
>>>>> memset() call.
>>>>>
>>>>
>>>> Could you try the following please? (attached as well in case gmail
> mangles it)
>>>>
>>>> diff --git a/drivers/firmware/efi/libstub/tpm.c
>>>> b/drivers/firmware/efi/libstub/tpm.c
>>>> index 2298560cea72..30d960a344b7 100644
>>>> --- a/drivers/firmware/efi/libstub/tpm.c
>>>> +++ b/drivers/firmware/efi/libstub/tpm.c
>>>> @@ -70,6 +70,8 @@ void
>>>> efi_retrieve_tpm2_eventlog_1_2(efi_system_table_t *sys_table_arg)
>>>> size_t log_size, last_entry_size;
>>>> efi_bool_t truncated;
>>>> void *tcg2_protocol;
>>>> + unsigned long num_pages;
>>>> + efi_physical_addr_t log_tbl_alloc;
>>>>
>>>> status = efi_call_early(locate_protocol, &tcg2_guid, NULL,
>>>> &tcg2_protocol);
>>>> @@ -104,9 +106,12 @@ void
>>>> efi_retrieve_tpm2_eventlog_1_2(efi_system_table_t *sys_table_arg)
>>>> }
>>>>
>>>> /* Allocate space for the logs and copy them. */
>>>> - status = efi_call_early(allocate_pool, EFI_LOADER_DATA,
>>>> - sizeof(*log_tbl) + log_size,
>>>> - (void **) &log_tbl);
>>>> + num_pages = DIV_ROUND_UP(sizeof(*log_tbl) + log_size,
> EFI_PAGE_SIZE);
>>>> + status = efi_call_early(allocate_pages,
>>>> + EFI_ALLOCATE_ANY_PAGES,
>>>> + EFI_LOADER_DATA,
>>>> + num_pages,
>>>> + &log_tbl_alloc);
>>>>
>>>> if (status != EFI_SUCCESS) {
>>>> efi_printk(sys_table_arg,
>>>> @@ -114,6 +119,7 @@ void
>>>> efi_retrieve_tpm2_eventlog_1_2(efi_system_table_t *sys_table_arg)
>>>> return;
>>>> }
>>>>
>>>> + log_tbl = (struct linux_efi_tpm_eventlog *)(unsigned
> long)log_tbl_alloc;
>>>> memset(log_tbl, 0, sizeof(*log_tbl) + log_size);
>>>> log_tbl->size = log_size;
>>>> log_tbl->version = EFI_TCG2_EVENT_LOG_FORMAT_TCG_1_2;
>>>> @@ -126,7 +132,7 @@ void
>>>> efi_retrieve_tpm2_eventlog_1_2(efi_system_table_t *sys_table_arg)
>>>> return;
>>>>
>>>> err_free:
>>>> - efi_call_early(free_pool, log_tbl);
>>>> + efi_call_early(free_pages, log_tbl_alloc, num_pages);
>>>> }
>>>>
>>>> void efi_retrieve_tpm2_eventlog(efi_system_table_t *sys_table_arg)
>>>>
>>>
>>> Hi Ard,
>>>
>>> When I apply this, it starts hanging at
>>>
>>> status = efi_call_proto(efi_tcg2_protocol, get_event_log, tcg2_protocol,
>>> EFI_TCG2_EVENT_LOG_FORMAT_TCG_1_2,
>>> &log_location, &log_last_entry, &truncated);
>>>
>>> rather than at the memset() call.
>>>
>
>> That is *very* surprising, given that the change only affects code
>> that executes after that.
>
>> I understand how annoying this is for you, and I think we should try
>> to fix this, but reverting the patches outright isn't the solution
>> either.
>
>> Which UEFI vendor and version does your system report?
>
> You should be able to find this info using the "ver" command in the UEFI
> shell.
> The UEFI vendor is Insyde (see first message).
>
Ah, thanks!
EFI Specification Revision : 2.40
EFI Vendor : INSYDE Corp.
EFI Revision : 21573.83
Regards,
Jeremy
On Mon, Mar 12, 2018 at 7:33 PM Jeremy Cline <[email protected]> wrote:
> On 03/12/2018 02:29 PM, Thiebaud Weksteen wrote:
> > On Mon, Mar 12, 2018 at 6:30 PM Ard Biesheuvel <
[email protected]>
> > wrote:
> >
> >> On 12 March 2018 at 17:01, Jeremy Cline <[email protected]> wrote:
> >>> On 03/12/2018 10:56 AM, Ard Biesheuvel wrote:
> >>>> On 12 March 2018 at 14:30, Jeremy Cline <[email protected]> wrote:
> >>>>> On 03/12/2018 07:08 AM, Ard Biesheuvel wrote:
> >>>>>> On 10 March 2018 at 10:45, Thiebaud Weksteen <[email protected]>
> > wrote:
> >>>>>>> On Fri, Mar 9, 2018 at 5:54 PM Jeremy Cline <[email protected]>
> > wrote:
> >>>>>>>
> >>>>>>>> On Fri, Mar 09, 2018 at 10:43:50AM +0000, Thiebaud Weksteen
wrote:
> >>>>>>>>> Thanks a lot for trying out the patch!
> >>>>>>>>>
> >>>>>>>>> Please don't modify your install at this stage, I think we are
> > hitting a
> >>>>>>>>> firmware bug and that would be awesome if we can fix how we are
> >>>>>>> handling it.
> >>>>>>>>> So, if we reach that stage in the function it could either be
> > that:
> >>>>>>>>> * The allocation did not succeed, somehow, but the firmware
still
> >>>>>>> returned
> >>>>>>>>> EFI_SUCCEED.
> >>>>>>>>> * The size requested is incorrect (I'm thinking something like a
> > 1G of
> >>>>>>>>> log). This would be due to either a miscalculation of log_size
> >>>>>>> (possible)
> >>>>>>>>> or; the returned values of GetEventLog are not correct.
> >>>>>>>>> I'm sending a patch to add checks for these. Could you please
> > apply and
> >>>>>>>>> retest?
> >>>>>>>>> Again, thanks for helping debugging this.
> >>>>>>>
> >>>>>>>> No problem, thanks for the help :)
> >>>>>>>
> >>>>>>>> With the new patch:
> >>>>>>>
> >>>>>>>> Locating the TCG2Protocol
> >>>>>>>> Calling GetEventLog on TCG2Protocol
> >>>>>>>> Log returned
> >>>>>>>> log_location is not empty
> >>>>>>>> log_size != 0
> >>>>>>>> log_size < 1M
> >>>>>>>> Allocating memory for storing the logs
> >>>>>>>> Returned from memory allocation
> >>>>>>>> Copying log to new location
> >>>>>>>
> >>>>>>>> And then it hangs. I added a couple more print statements:
> >>>>>>>
> >>>>>>>> diff --git a/drivers/firmware/efi/libstub/tpm.c
> >>>>>>> b/drivers/firmware/efi/libstub/tpm.c
> >>>>>>>> index ee3fac109078..1ab5638bc50e 100644
> >>>>>>>> --- a/drivers/firmware/efi/libstub/tpm.c
> >>>>>>>> +++ b/drivers/firmware/efi/libstub/tpm.c
> >>>>>>>> @@ -148,8 +148,11 @@ void
> >>>>>>> efi_retrieve_tpm2_eventlog_1_2(efi_system_table_t *sys_table_arg)
> >>>>>>>> efi_printk(sys_table_arg, "Copying log to new
> > location\n");
> >>>>>>>
> >>>>>>>> memset(log_tbl, 0, sizeof(*log_tbl) + log_size);
> >>>>>>>> + efi_printk(sys_table_arg, "Successfully memset log_tbl to
> > 0\n");
> >>>>>>>> log_tbl->size = log_size;
> >>>>>>>> + efi_printk(sys_table_arg, "Set log_tbl->size\n");
> >>>>>>>> log_tbl->version = EFI_TCG2_EVENT_LOG_FORMAT_TCG_1_2;
> >>>>>>>> + efi_printk(sys_table_arg, "Set log_tbl-version\n");
> >>>>>>>> memcpy(log_tbl->log, (void *) first_entry_addr,
log_size);
> >>>>>>>
> >>>>>>>> efi_printk(sys_table_arg, "Installing the log into the
> >>>>>>> configuration table\n");
> >>>>>>>
> >>>>>>>> and it's hanging at "memset(log_tbl, 0, sizeof(*log_tbl) +
> > log_size);"
> >>>>>>>
> >>>>>>> Thanks. Well, it looks like the memory that is supposedly
allocated
> > is not
> >>>>>>> usable. I'm thinking this is a firmware bug.
> >>>>>>> Ard, would you agree on this assumption? Thoughts on how to
proceed?
> >>>>>>>
> >>>>>>
> >>>>>> I am rather puzzled why the allocate_pool() should succeed and the
> >>>>>> subsequent memset() should fail. This does not look like an issue
> > that
> >>>>>> is intimately related to TPM2 support, rather an issue in the
> > firmware
> >>>>>> that happens to get tickled after the change.
> >>>>>>
> >>>>>> Would you mind trying replacing EFI_LOADER_DATA with
> >>>>>> EFI_BOOT_SERVICES_DATA in the allocate_pool() call?
> >>>>>
> >>>>> Replacing EFI_LOADER_DATA with EFI_BOOT_SERVICES_DATA still hangs at
> > the
> >>>>> memset() call.
> >>>>>
> >>>>
> >>>> Could you try the following please? (attached as well in case gmail
> > mangles it)
> >>>>
> >>>> diff --git a/drivers/firmware/efi/libstub/tpm.c
> >>>> b/drivers/firmware/efi/libstub/tpm.c
> >>>> index 2298560cea72..30d960a344b7 100644
> >>>> --- a/drivers/firmware/efi/libstub/tpm.c
> >>>> +++ b/drivers/firmware/efi/libstub/tpm.c
> >>>> @@ -70,6 +70,8 @@ void
> >>>> efi_retrieve_tpm2_eventlog_1_2(efi_system_table_t *sys_table_arg)
> >>>> size_t log_size, last_entry_size;
> >>>> efi_bool_t truncated;
> >>>> void *tcg2_protocol;
> >>>> + unsigned long num_pages;
> >>>> + efi_physical_addr_t log_tbl_alloc;
> >>>>
> >>>> status = efi_call_early(locate_protocol, &tcg2_guid, NULL,
> >>>> &tcg2_protocol);
> >>>> @@ -104,9 +106,12 @@ void
> >>>> efi_retrieve_tpm2_eventlog_1_2(efi_system_table_t *sys_table_arg)
> >>>> }
> >>>>
> >>>> /* Allocate space for the logs and copy them. */
> >>>> - status = efi_call_early(allocate_pool, EFI_LOADER_DATA,
> >>>> - sizeof(*log_tbl) + log_size,
> >>>> - (void **) &log_tbl);
> >>>> + num_pages = DIV_ROUND_UP(sizeof(*log_tbl) + log_size,
> > EFI_PAGE_SIZE);
> >>>> + status = efi_call_early(allocate_pages,
> >>>> + EFI_ALLOCATE_ANY_PAGES,
> >>>> + EFI_LOADER_DATA,
> >>>> + num_pages,
> >>>> + &log_tbl_alloc);
> >>>>
> >>>> if (status != EFI_SUCCESS) {
> >>>> efi_printk(sys_table_arg,
> >>>> @@ -114,6 +119,7 @@ void
> >>>> efi_retrieve_tpm2_eventlog_1_2(efi_system_table_t *sys_table_arg)
> >>>> return;
> >>>> }
> >>>>
> >>>> + log_tbl = (struct linux_efi_tpm_eventlog *)(unsigned
> > long)log_tbl_alloc;
> >>>> memset(log_tbl, 0, sizeof(*log_tbl) + log_size);
> >>>> log_tbl->size = log_size;
> >>>> log_tbl->version = EFI_TCG2_EVENT_LOG_FORMAT_TCG_1_2;
> >>>> @@ -126,7 +132,7 @@ void
> >>>> efi_retrieve_tpm2_eventlog_1_2(efi_system_table_t *sys_table_arg)
> >>>> return;
> >>>>
> >>>> err_free:
> >>>> - efi_call_early(free_pool, log_tbl);
> >>>> + efi_call_early(free_pages, log_tbl_alloc, num_pages);
> >>>> }
> >>>>
> >>>> void efi_retrieve_tpm2_eventlog(efi_system_table_t *sys_table_arg)
> >>>>
> >>>
> >>> Hi Ard,
> >>>
> >>> When I apply this, it starts hanging at
> >>>
> >>> status = efi_call_proto(efi_tcg2_protocol, get_event_log,
tcg2_protocol,
> >>> EFI_TCG2_EVENT_LOG_FORMAT_TCG_1_2,
> >>> &log_location, &log_last_entry, &truncated);
> >>>
> >>> rather than at the memset() call.
> >>>
> >
> >> That is *very* surprising, given that the change only affects code
> >> that executes after that.
> >
Hans, you said you configured the tablet to use the 32-bit version of grub
instead
of 64. Why's that?
Jeremy, could you confirm if you are building the kernel in 64bit mode? Is
your Android install working? (that is, what happens if you boot Boot0000)?
> >> I understand how annoying this is for you, and I think we should try
> >> to fix this, but reverting the patches outright isn't the solution
> >> either.
> >
> >> Which UEFI vendor and version does your system report?
> >
> > You should be able to find this info using the "ver" command in the UEFI
> > shell.
> > The UEFI vendor is Insyde (see first message).
> >
> Ah, thanks!
> EFI Specification Revision : 2.40
> EFI Vendor : INSYDE Corp.
> EFI Revision : 21573.83
> Regards,
> Jeremy
On 12 March 2018 at 19:55, Thiebaud Weksteen <[email protected]> wrote:
> On Mon, Mar 12, 2018 at 7:33 PM Jeremy Cline <[email protected]> wrote:
>
>> On 03/12/2018 02:29 PM, Thiebaud Weksteen wrote:
>> > On Mon, Mar 12, 2018 at 6:30 PM Ard Biesheuvel <
> [email protected]>
>> > wrote:
>> >
>> >> On 12 March 2018 at 17:01, Jeremy Cline <[email protected]> wrote:
>> >>> On 03/12/2018 10:56 AM, Ard Biesheuvel wrote:
>> >>>> On 12 March 2018 at 14:30, Jeremy Cline <[email protected]> wrote:
>> >>>>> On 03/12/2018 07:08 AM, Ard Biesheuvel wrote:
>> >>>>>> On 10 March 2018 at 10:45, Thiebaud Weksteen <[email protected]>
>> > wrote:
>> >>>>>>> On Fri, Mar 9, 2018 at 5:54 PM Jeremy Cline <[email protected]>
>> > wrote:
>> >>>>>>>
>> >>>>>>>> On Fri, Mar 09, 2018 at 10:43:50AM +0000, Thiebaud Weksteen
> wrote:
>> >>>>>>>>> Thanks a lot for trying out the patch!
>> >>>>>>>>>
>> >>>>>>>>> Please don't modify your install at this stage, I think we are
>> > hitting a
>> >>>>>>>>> firmware bug and that would be awesome if we can fix how we are
>> >>>>>>> handling it.
>> >>>>>>>>> So, if we reach that stage in the function it could either be
>> > that:
>> >>>>>>>>> * The allocation did not succeed, somehow, but the firmware
> still
>> >>>>>>> returned
>> >>>>>>>>> EFI_SUCCEED.
>> >>>>>>>>> * The size requested is incorrect (I'm thinking something like a
>> > 1G of
>> >>>>>>>>> log). This would be due to either a miscalculation of log_size
>> >>>>>>> (possible)
>> >>>>>>>>> or; the returned values of GetEventLog are not correct.
>> >>>>>>>>> I'm sending a patch to add checks for these. Could you please
>> > apply and
>> >>>>>>>>> retest?
>> >>>>>>>>> Again, thanks for helping debugging this.
>> >>>>>>>
>> >>>>>>>> No problem, thanks for the help :)
>> >>>>>>>
>> >>>>>>>> With the new patch:
>> >>>>>>>
>> >>>>>>>> Locating the TCG2Protocol
>> >>>>>>>> Calling GetEventLog on TCG2Protocol
>> >>>>>>>> Log returned
>> >>>>>>>> log_location is not empty
>> >>>>>>>> log_size != 0
>> >>>>>>>> log_size < 1M
>> >>>>>>>> Allocating memory for storing the logs
>> >>>>>>>> Returned from memory allocation
>> >>>>>>>> Copying log to new location
>> >>>>>>>
>> >>>>>>>> And then it hangs. I added a couple more print statements:
>> >>>>>>>
>> >>>>>>>> diff --git a/drivers/firmware/efi/libstub/tpm.c
>> >>>>>>> b/drivers/firmware/efi/libstub/tpm.c
>> >>>>>>>> index ee3fac109078..1ab5638bc50e 100644
>> >>>>>>>> --- a/drivers/firmware/efi/libstub/tpm.c
>> >>>>>>>> +++ b/drivers/firmware/efi/libstub/tpm.c
>> >>>>>>>> @@ -148,8 +148,11 @@ void
>> >>>>>>> efi_retrieve_tpm2_eventlog_1_2(efi_system_table_t *sys_table_arg)
>> >>>>>>>> efi_printk(sys_table_arg, "Copying log to new
>> > location\n");
>> >>>>>>>
>> >>>>>>>> memset(log_tbl, 0, sizeof(*log_tbl) + log_size);
>> >>>>>>>> + efi_printk(sys_table_arg, "Successfully memset log_tbl to
>> > 0\n");
>> >>>>>>>> log_tbl->size = log_size;
>> >>>>>>>> + efi_printk(sys_table_arg, "Set log_tbl->size\n");
>> >>>>>>>> log_tbl->version = EFI_TCG2_EVENT_LOG_FORMAT_TCG_1_2;
>> >>>>>>>> + efi_printk(sys_table_arg, "Set log_tbl-version\n");
>> >>>>>>>> memcpy(log_tbl->log, (void *) first_entry_addr,
> log_size);
>> >>>>>>>
>> >>>>>>>> efi_printk(sys_table_arg, "Installing the log into the
>> >>>>>>> configuration table\n");
>> >>>>>>>
>> >>>>>>>> and it's hanging at "memset(log_tbl, 0, sizeof(*log_tbl) +
>> > log_size);"
>> >>>>>>>
>> >>>>>>> Thanks. Well, it looks like the memory that is supposedly
> allocated
>> > is not
>> >>>>>>> usable. I'm thinking this is a firmware bug.
>> >>>>>>> Ard, would you agree on this assumption? Thoughts on how to
> proceed?
>> >>>>>>>
>> >>>>>>
>> >>>>>> I am rather puzzled why the allocate_pool() should succeed and the
>> >>>>>> subsequent memset() should fail. This does not look like an issue
>> > that
>> >>>>>> is intimately related to TPM2 support, rather an issue in the
>> > firmware
>> >>>>>> that happens to get tickled after the change.
>> >>>>>>
>> >>>>>> Would you mind trying replacing EFI_LOADER_DATA with
>> >>>>>> EFI_BOOT_SERVICES_DATA in the allocate_pool() call?
>> >>>>>
>> >>>>> Replacing EFI_LOADER_DATA with EFI_BOOT_SERVICES_DATA still hangs at
>> > the
>> >>>>> memset() call.
>> >>>>>
>> >>>>
>> >>>> Could you try the following please? (attached as well in case gmail
>> > mangles it)
>> >>>>
>> >>>> diff --git a/drivers/firmware/efi/libstub/tpm.c
>> >>>> b/drivers/firmware/efi/libstub/tpm.c
>> >>>> index 2298560cea72..30d960a344b7 100644
>> >>>> --- a/drivers/firmware/efi/libstub/tpm.c
>> >>>> +++ b/drivers/firmware/efi/libstub/tpm.c
>> >>>> @@ -70,6 +70,8 @@ void
>> >>>> efi_retrieve_tpm2_eventlog_1_2(efi_system_table_t *sys_table_arg)
>> >>>> size_t log_size, last_entry_size;
>> >>>> efi_bool_t truncated;
>> >>>> void *tcg2_protocol;
>> >>>> + unsigned long num_pages;
>> >>>> + efi_physical_addr_t log_tbl_alloc;
>> >>>>
>> >>>> status = efi_call_early(locate_protocol, &tcg2_guid, NULL,
>> >>>> &tcg2_protocol);
>> >>>> @@ -104,9 +106,12 @@ void
>> >>>> efi_retrieve_tpm2_eventlog_1_2(efi_system_table_t *sys_table_arg)
>> >>>> }
>> >>>>
>> >>>> /* Allocate space for the logs and copy them. */
>> >>>> - status = efi_call_early(allocate_pool, EFI_LOADER_DATA,
>> >>>> - sizeof(*log_tbl) + log_size,
>> >>>> - (void **) &log_tbl);
>> >>>> + num_pages = DIV_ROUND_UP(sizeof(*log_tbl) + log_size,
>> > EFI_PAGE_SIZE);
>> >>>> + status = efi_call_early(allocate_pages,
>> >>>> + EFI_ALLOCATE_ANY_PAGES,
>> >>>> + EFI_LOADER_DATA,
>> >>>> + num_pages,
>> >>>> + &log_tbl_alloc);
>> >>>>
>> >>>> if (status != EFI_SUCCESS) {
>> >>>> efi_printk(sys_table_arg,
>> >>>> @@ -114,6 +119,7 @@ void
>> >>>> efi_retrieve_tpm2_eventlog_1_2(efi_system_table_t *sys_table_arg)
>> >>>> return;
>> >>>> }
>> >>>>
>> >>>> + log_tbl = (struct linux_efi_tpm_eventlog *)(unsigned
>> > long)log_tbl_alloc;
>> >>>> memset(log_tbl, 0, sizeof(*log_tbl) + log_size);
>> >>>> log_tbl->size = log_size;
>> >>>> log_tbl->version = EFI_TCG2_EVENT_LOG_FORMAT_TCG_1_2;
>> >>>> @@ -126,7 +132,7 @@ void
>> >>>> efi_retrieve_tpm2_eventlog_1_2(efi_system_table_t *sys_table_arg)
>> >>>> return;
>> >>>>
>> >>>> err_free:
>> >>>> - efi_call_early(free_pool, log_tbl);
>> >>>> + efi_call_early(free_pages, log_tbl_alloc, num_pages);
>> >>>> }
>> >>>>
>> >>>> void efi_retrieve_tpm2_eventlog(efi_system_table_t *sys_table_arg)
>> >>>>
>> >>>
>> >>> Hi Ard,
>> >>>
>> >>> When I apply this, it starts hanging at
>> >>>
>> >>> status = efi_call_proto(efi_tcg2_protocol, get_event_log,
> tcg2_protocol,
>> >>> EFI_TCG2_EVENT_LOG_FORMAT_TCG_1_2,
>> >>> &log_location, &log_last_entry, &truncated);
>> >>>
>> >>> rather than at the memset() call.
>> >>>
>> >
>> >> That is *very* surprising, given that the change only affects code
>> >> that executes after that.
>> >
>
> Hans, you said you configured the tablet to use the 32-bit version of grub
> instead
> of 64. Why's that?
>
> Jeremy, could you confirm if you are building the kernel in 64bit mode? Is
> your Android install working? (that is, what happens if you boot Boot0000)?
>
>> >> I understand how annoying this is for you, and I think we should try
>> >> to fix this, but reverting the patches outright isn't the solution
>> >> either.
>> >
>> >> Which UEFI vendor and version does your system report?
>> >
>> > You should be able to find this info using the "ver" command in the UEFI
>> > shell.
>> > The UEFI vendor is Insyde (see first message).
>> >
>
>> Ah, thanks!
>
>> EFI Specification Revision : 2.40
>> EFI Vendor : INSYDE Corp.
>> EFI Revision : 21573.83
>
Thiebaud,
If we don't manage to resolve this, do you see any way to blacklist
systems based on this information? Would it be reasonable, say, to
require UEFI v2.5 or later for TPM2 support? Or doesn't that make any
sense (I am aware that the TCG EFI spec and the UEFI spec are somewhat
orthogonal, but it also depends on the hardware you are targetting, I
guess). Otherwise, we could use a more specific match, perhaps?
This is of course depending on whether we reach consensus on whether
we should make any changes at all for what appears to be a single
sample of a certain piece of hardware, where other samples running the
same firmware (right?) are working fine.
On 03/12/2018 03:55 PM, Thiebaud Weksteen wrote:
> On Mon, Mar 12, 2018 at 7:33 PM Jeremy Cline <[email protected]> wrote:
>
>> On 03/12/2018 02:29 PM, Thiebaud Weksteen wrote:
>>> On Mon, Mar 12, 2018 at 6:30 PM Ard Biesheuvel <
> [email protected]>
>>> wrote:
>>>
>>>> On 12 March 2018 at 17:01, Jeremy Cline <[email protected]> wrote:
>>>>> On 03/12/2018 10:56 AM, Ard Biesheuvel wrote:
>>>>>> On 12 March 2018 at 14:30, Jeremy Cline <[email protected]> wrote:
>>>>>>> On 03/12/2018 07:08 AM, Ard Biesheuvel wrote:
>>>>>>>> On 10 March 2018 at 10:45, Thiebaud Weksteen <[email protected]>
>>> wrote:
>>>>>>>>> On Fri, Mar 9, 2018 at 5:54 PM Jeremy Cline <[email protected]>
>>> wrote:
>>>>>>>>>
>>>>>>>>>> On Fri, Mar 09, 2018 at 10:43:50AM +0000, Thiebaud Weksteen
> wrote:
>>>>>>>>>>> Thanks a lot for trying out the patch!
>>>>>>>>>>>
>>>>>>>>>>> Please don't modify your install at this stage, I think we are
>>> hitting a
>>>>>>>>>>> firmware bug and that would be awesome if we can fix how we are
>>>>>>>>> handling it.
>>>>>>>>>>> So, if we reach that stage in the function it could either be
>>> that:
>>>>>>>>>>> * The allocation did not succeed, somehow, but the firmware
> still
>>>>>>>>> returned
>>>>>>>>>>> EFI_SUCCEED.
>>>>>>>>>>> * The size requested is incorrect (I'm thinking something like a
>>> 1G of
>>>>>>>>>>> log). This would be due to either a miscalculation of log_size
>>>>>>>>> (possible)
>>>>>>>>>>> or; the returned values of GetEventLog are not correct.
>>>>>>>>>>> I'm sending a patch to add checks for these. Could you please
>>> apply and
>>>>>>>>>>> retest?
>>>>>>>>>>> Again, thanks for helping debugging this.
>>>>>>>>>
>>>>>>>>>> No problem, thanks for the help :)
>>>>>>>>>
>>>>>>>>>> With the new patch:
>>>>>>>>>
>>>>>>>>>> Locating the TCG2Protocol
>>>>>>>>>> Calling GetEventLog on TCG2Protocol
>>>>>>>>>> Log returned
>>>>>>>>>> log_location is not empty
>>>>>>>>>> log_size != 0
>>>>>>>>>> log_size < 1M
>>>>>>>>>> Allocating memory for storing the logs
>>>>>>>>>> Returned from memory allocation
>>>>>>>>>> Copying log to new location
>>>>>>>>>
>>>>>>>>>> And then it hangs. I added a couple more print statements:
>>>>>>>>>
>>>>>>>>>> diff --git a/drivers/firmware/efi/libstub/tpm.c
>>>>>>>>> b/drivers/firmware/efi/libstub/tpm.c
>>>>>>>>>> index ee3fac109078..1ab5638bc50e 100644
>>>>>>>>>> --- a/drivers/firmware/efi/libstub/tpm.c
>>>>>>>>>> +++ b/drivers/firmware/efi/libstub/tpm.c
>>>>>>>>>> @@ -148,8 +148,11 @@ void
>>>>>>>>> efi_retrieve_tpm2_eventlog_1_2(efi_system_table_t *sys_table_arg)
>>>>>>>>>> efi_printk(sys_table_arg, "Copying log to new
>>> location\n");
>>>>>>>>>
>>>>>>>>>> memset(log_tbl, 0, sizeof(*log_tbl) + log_size);
>>>>>>>>>> + efi_printk(sys_table_arg, "Successfully memset log_tbl to
>>> 0\n");
>>>>>>>>>> log_tbl->size = log_size;
>>>>>>>>>> + efi_printk(sys_table_arg, "Set log_tbl->size\n");
>>>>>>>>>> log_tbl->version = EFI_TCG2_EVENT_LOG_FORMAT_TCG_1_2;
>>>>>>>>>> + efi_printk(sys_table_arg, "Set log_tbl-version\n");
>>>>>>>>>> memcpy(log_tbl->log, (void *) first_entry_addr,
> log_size);
>>>>>>>>>
>>>>>>>>>> efi_printk(sys_table_arg, "Installing the log into the
>>>>>>>>> configuration table\n");
>>>>>>>>>
>>>>>>>>>> and it's hanging at "memset(log_tbl, 0, sizeof(*log_tbl) +
>>> log_size);"
>>>>>>>>>
>>>>>>>>> Thanks. Well, it looks like the memory that is supposedly
> allocated
>>> is not
>>>>>>>>> usable. I'm thinking this is a firmware bug.
>>>>>>>>> Ard, would you agree on this assumption? Thoughts on how to
> proceed?
>>>>>>>>>
>>>>>>>>
>>>>>>>> I am rather puzzled why the allocate_pool() should succeed and the
>>>>>>>> subsequent memset() should fail. This does not look like an issue
>>> that
>>>>>>>> is intimately related to TPM2 support, rather an issue in the
>>> firmware
>>>>>>>> that happens to get tickled after the change.
>>>>>>>>
>>>>>>>> Would you mind trying replacing EFI_LOADER_DATA with
>>>>>>>> EFI_BOOT_SERVICES_DATA in the allocate_pool() call?
>>>>>>>
>>>>>>> Replacing EFI_LOADER_DATA with EFI_BOOT_SERVICES_DATA still hangs at
>>> the
>>>>>>> memset() call.
>>>>>>>
>>>>>>
>>>>>> Could you try the following please? (attached as well in case gmail
>>> mangles it)
>>>>>>
>>>>>> diff --git a/drivers/firmware/efi/libstub/tpm.c
>>>>>> b/drivers/firmware/efi/libstub/tpm.c
>>>>>> index 2298560cea72..30d960a344b7 100644
>>>>>> --- a/drivers/firmware/efi/libstub/tpm.c
>>>>>> +++ b/drivers/firmware/efi/libstub/tpm.c
>>>>>> @@ -70,6 +70,8 @@ void
>>>>>> efi_retrieve_tpm2_eventlog_1_2(efi_system_table_t *sys_table_arg)
>>>>>> size_t log_size, last_entry_size;
>>>>>> efi_bool_t truncated;
>>>>>> void *tcg2_protocol;
>>>>>> + unsigned long num_pages;
>>>>>> + efi_physical_addr_t log_tbl_alloc;
>>>>>>
>>>>>> status = efi_call_early(locate_protocol, &tcg2_guid, NULL,
>>>>>> &tcg2_protocol);
>>>>>> @@ -104,9 +106,12 @@ void
>>>>>> efi_retrieve_tpm2_eventlog_1_2(efi_system_table_t *sys_table_arg)
>>>>>> }
>>>>>>
>>>>>> /* Allocate space for the logs and copy them. */
>>>>>> - status = efi_call_early(allocate_pool, EFI_LOADER_DATA,
>>>>>> - sizeof(*log_tbl) + log_size,
>>>>>> - (void **) &log_tbl);
>>>>>> + num_pages = DIV_ROUND_UP(sizeof(*log_tbl) + log_size,
>>> EFI_PAGE_SIZE);
>>>>>> + status = efi_call_early(allocate_pages,
>>>>>> + EFI_ALLOCATE_ANY_PAGES,
>>>>>> + EFI_LOADER_DATA,
>>>>>> + num_pages,
>>>>>> + &log_tbl_alloc);
>>>>>>
>>>>>> if (status != EFI_SUCCESS) {
>>>>>> efi_printk(sys_table_arg,
>>>>>> @@ -114,6 +119,7 @@ void
>>>>>> efi_retrieve_tpm2_eventlog_1_2(efi_system_table_t *sys_table_arg)
>>>>>> return;
>>>>>> }
>>>>>>
>>>>>> + log_tbl = (struct linux_efi_tpm_eventlog *)(unsigned
>>> long)log_tbl_alloc;
>>>>>> memset(log_tbl, 0, sizeof(*log_tbl) + log_size);
>>>>>> log_tbl->size = log_size;
>>>>>> log_tbl->version = EFI_TCG2_EVENT_LOG_FORMAT_TCG_1_2;
>>>>>> @@ -126,7 +132,7 @@ void
>>>>>> efi_retrieve_tpm2_eventlog_1_2(efi_system_table_t *sys_table_arg)
>>>>>> return;
>>>>>>
>>>>>> err_free:
>>>>>> - efi_call_early(free_pool, log_tbl);
>>>>>> + efi_call_early(free_pages, log_tbl_alloc, num_pages);
>>>>>> }
>>>>>>
>>>>>> void efi_retrieve_tpm2_eventlog(efi_system_table_t *sys_table_arg)
>>>>>>
>>>>>
>>>>> Hi Ard,
>>>>>
>>>>> When I apply this, it starts hanging at
>>>>>
>>>>> status = efi_call_proto(efi_tcg2_protocol, get_event_log,
> tcg2_protocol,
>>>>> EFI_TCG2_EVENT_LOG_FORMAT_TCG_1_2,
>>>>> &log_location, &log_last_entry, &truncated);
>>>>>
>>>>> rather than at the memset() call.
>>>>>
>>>
>>>> That is *very* surprising, given that the change only affects code
>>>> that executes after that.
>>>
>
> Hans, you said you configured the tablet to use the 32-bit version of grub
> instead
> of 64. Why's that?
>
> Jeremy, could you confirm if you are building the kernel in 64bit mode? Is
> your Android install working? (that is, what happens if you boot Boot0000)?
It's definitely being built in 64bit mode. The Android install entry is
a remnant from Hans, but it appears to be a Red Hat grub so it just
boots to Fedora.
Regards,
Jeremy
On Mon, Mar 12, 2018 at 10:03 PM Ard Biesheuvel <[email protected]>
wrote:
> On 12 March 2018 at 19:55, Thiebaud Weksteen <[email protected]> wrote:
> > On Mon, Mar 12, 2018 at 7:33 PM Jeremy Cline <[email protected]> wrote:
> >
> >> On 03/12/2018 02:29 PM, Thiebaud Weksteen wrote:
> >> > On Mon, Mar 12, 2018 at 6:30 PM Ard Biesheuvel <
> > [email protected]>
> >> > wrote:
> >> >
> >> >> On 12 March 2018 at 17:01, Jeremy Cline <[email protected]> wrote:
> >> >>> On 03/12/2018 10:56 AM, Ard Biesheuvel wrote:
> >> >>>> On 12 March 2018 at 14:30, Jeremy Cline <[email protected]> wrote:
> >> >>>>> On 03/12/2018 07:08 AM, Ard Biesheuvel wrote:
> >> >>>>>> On 10 March 2018 at 10:45, Thiebaud Weksteen <[email protected]>
> >> > wrote:
> >> >>>>>>> On Fri, Mar 9, 2018 at 5:54 PM Jeremy Cline <[email protected]>
> >> > wrote:
> >> >>>>>>>
> >> >>>>>>>> On Fri, Mar 09, 2018 at 10:43:50AM +0000, Thiebaud Weksteen
> > wrote:
> >> >>>>>>>>> Thanks a lot for trying out the patch!
> >> >>>>>>>>>
> >> >>>>>>>>> Please don't modify your install at this stage, I think we
are
> >> > hitting a
> >> >>>>>>>>> firmware bug and that would be awesome if we can fix how we
are
> >> >>>>>>> handling it.
> >> >>>>>>>>> So, if we reach that stage in the function it could either be
> >> > that:
> >> >>>>>>>>> * The allocation did not succeed, somehow, but the firmware
> > still
> >> >>>>>>> returned
> >> >>>>>>>>> EFI_SUCCEED.
> >> >>>>>>>>> * The size requested is incorrect (I'm thinking something
like a
> >> > 1G of
> >> >>>>>>>>> log). This would be due to either a miscalculation of
log_size
> >> >>>>>>> (possible)
> >> >>>>>>>>> or; the returned values of GetEventLog are not correct.
> >> >>>>>>>>> I'm sending a patch to add checks for these. Could you please
> >> > apply and
> >> >>>>>>>>> retest?
> >> >>>>>>>>> Again, thanks for helping debugging this.
> >> >>>>>>>
> >> >>>>>>>> No problem, thanks for the help :)
> >> >>>>>>>
> >> >>>>>>>> With the new patch:
> >> >>>>>>>
> >> >>>>>>>> Locating the TCG2Protocol
> >> >>>>>>>> Calling GetEventLog on TCG2Protocol
> >> >>>>>>>> Log returned
> >> >>>>>>>> log_location is not empty
> >> >>>>>>>> log_size != 0
> >> >>>>>>>> log_size < 1M
> >> >>>>>>>> Allocating memory for storing the logs
> >> >>>>>>>> Returned from memory allocation
> >> >>>>>>>> Copying log to new location
> >> >>>>>>>
> >> >>>>>>>> And then it hangs. I added a couple more print statements:
> >> >>>>>>>
> >> >>>>>>>> diff --git a/drivers/firmware/efi/libstub/tpm.c
> >> >>>>>>> b/drivers/firmware/efi/libstub/tpm.c
> >> >>>>>>>> index ee3fac109078..1ab5638bc50e 100644
> >> >>>>>>>> --- a/drivers/firmware/efi/libstub/tpm.c
> >> >>>>>>>> +++ b/drivers/firmware/efi/libstub/tpm.c
> >> >>>>>>>> @@ -148,8 +148,11 @@ void
> >> >>>>>>> efi_retrieve_tpm2_eventlog_1_2(efi_system_table_t
*sys_table_arg)
> >> >>>>>>>> efi_printk(sys_table_arg, "Copying log to new
> >> > location\n");
> >> >>>>>>>
> >> >>>>>>>> memset(log_tbl, 0, sizeof(*log_tbl) + log_size);
> >> >>>>>>>> + efi_printk(sys_table_arg, "Successfully memset
log_tbl to
> >> > 0\n");
> >> >>>>>>>> log_tbl->size = log_size;
> >> >>>>>>>> + efi_printk(sys_table_arg, "Set log_tbl->size\n");
> >> >>>>>>>> log_tbl->version = EFI_TCG2_EVENT_LOG_FORMAT_TCG_1_2;
> >> >>>>>>>> + efi_printk(sys_table_arg, "Set log_tbl-version\n");
> >> >>>>>>>> memcpy(log_tbl->log, (void *) first_entry_addr,
> > log_size);
> >> >>>>>>>
> >> >>>>>>>> efi_printk(sys_table_arg, "Installing the log into
the
> >> >>>>>>> configuration table\n");
> >> >>>>>>>
> >> >>>>>>>> and it's hanging at "memset(log_tbl, 0, sizeof(*log_tbl) +
> >> > log_size);"
> >> >>>>>>>
> >> >>>>>>> Thanks. Well, it looks like the memory that is supposedly
> > allocated
> >> > is not
> >> >>>>>>> usable. I'm thinking this is a firmware bug.
> >> >>>>>>> Ard, would you agree on this assumption? Thoughts on how to
> > proceed?
> >> >>>>>>>
> >> >>>>>>
> >> >>>>>> I am rather puzzled why the allocate_pool() should succeed and
the
> >> >>>>>> subsequent memset() should fail. This does not look like an
issue
> >> > that
> >> >>>>>> is intimately related to TPM2 support, rather an issue in the
> >> > firmware
> >> >>>>>> that happens to get tickled after the change.
> >> >>>>>>
> >> >>>>>> Would you mind trying replacing EFI_LOADER_DATA with
> >> >>>>>> EFI_BOOT_SERVICES_DATA in the allocate_pool() call?
> >> >>>>>
> >> >>>>> Replacing EFI_LOADER_DATA with EFI_BOOT_SERVICES_DATA still
hangs at
> >> > the
> >> >>>>> memset() call.
> >> >>>>>
> >> >>>>
> >> >>>> Could you try the following please? (attached as well in case
gmail
> >> > mangles it)
> >> >>>>
> >> >>>> diff --git a/drivers/firmware/efi/libstub/tpm.c
> >> >>>> b/drivers/firmware/efi/libstub/tpm.c
> >> >>>> index 2298560cea72..30d960a344b7 100644
> >> >>>> --- a/drivers/firmware/efi/libstub/tpm.c
> >> >>>> +++ b/drivers/firmware/efi/libstub/tpm.c
> >> >>>> @@ -70,6 +70,8 @@ void
> >> >>>> efi_retrieve_tpm2_eventlog_1_2(efi_system_table_t *sys_table_arg)
> >> >>>> size_t log_size, last_entry_size;
> >> >>>> efi_bool_t truncated;
> >> >>>> void *tcg2_protocol;
> >> >>>> + unsigned long num_pages;
> >> >>>> + efi_physical_addr_t log_tbl_alloc;
> >> >>>>
> >> >>>> status = efi_call_early(locate_protocol, &tcg2_guid, NULL,
> >> >>>> &tcg2_protocol);
> >> >>>> @@ -104,9 +106,12 @@ void
> >> >>>> efi_retrieve_tpm2_eventlog_1_2(efi_system_table_t *sys_table_arg)
> >> >>>> }
> >> >>>>
> >> >>>> /* Allocate space for the logs and copy them. */
> >> >>>> - status = efi_call_early(allocate_pool, EFI_LOADER_DATA,
> >> >>>> - sizeof(*log_tbl) + log_size,
> >> >>>> - (void **) &log_tbl);
> >> >>>> + num_pages = DIV_ROUND_UP(sizeof(*log_tbl) + log_size,
> >> > EFI_PAGE_SIZE);
> >> >>>> + status = efi_call_early(allocate_pages,
> >> >>>> + EFI_ALLOCATE_ANY_PAGES,
> >> >>>> + EFI_LOADER_DATA,
> >> >>>> + num_pages,
> >> >>>> + &log_tbl_alloc);
> >> >>>>
> >> >>>> if (status != EFI_SUCCESS) {
> >> >>>> efi_printk(sys_table_arg,
> >> >>>> @@ -114,6 +119,7 @@ void
> >> >>>> efi_retrieve_tpm2_eventlog_1_2(efi_system_table_t *sys_table_arg)
> >> >>>> return;
> >> >>>> }
> >> >>>>
> >> >>>> + log_tbl = (struct linux_efi_tpm_eventlog *)(unsigned
> >> > long)log_tbl_alloc;
> >> >>>> memset(log_tbl, 0, sizeof(*log_tbl) + log_size);
> >> >>>> log_tbl->size = log_size;
> >> >>>> log_tbl->version = EFI_TCG2_EVENT_LOG_FORMAT_TCG_1_2;
> >> >>>> @@ -126,7 +132,7 @@ void
> >> >>>> efi_retrieve_tpm2_eventlog_1_2(efi_system_table_t *sys_table_arg)
> >> >>>> return;
> >> >>>>
> >> >>>> err_free:
> >> >>>> - efi_call_early(free_pool, log_tbl);
> >> >>>> + efi_call_early(free_pages, log_tbl_alloc, num_pages);
> >> >>>> }
> >> >>>>
> >> >>>> void efi_retrieve_tpm2_eventlog(efi_system_table_t
*sys_table_arg)
> >> >>>>
> >> >>>
> >> >>> Hi Ard,
> >> >>>
> >> >>> When I apply this, it starts hanging at
> >> >>>
> >> >>> status = efi_call_proto(efi_tcg2_protocol, get_event_log,
> > tcg2_protocol,
> >> >>> EFI_TCG2_EVENT_LOG_FORMAT_TCG_1_2,
> >> >>> &log_location, &log_last_entry,
&truncated);
> >> >>>
> >> >>> rather than at the memset() call.
> >> >>>
> >> >
> >> >> That is *very* surprising, given that the change only affects code
> >> >> that executes after that.
> >> >
> >
> > Hans, you said you configured the tablet to use the 32-bit version of
grub
> > instead
> > of 64. Why's that?
> >
> > Jeremy, could you confirm if you are building the kernel in 64bit mode?
Is
> > your Android install working? (that is, what happens if you boot
Boot0000)?
> >
> >> >> I understand how annoying this is for you, and I think we should try
> >> >> to fix this, but reverting the patches outright isn't the solution
> >> >> either.
> >> >
> >> >> Which UEFI vendor and version does your system report?
> >> >
> >> > You should be able to find this info using the "ver" command in the
UEFI
> >> > shell.
> >> > The UEFI vendor is Insyde (see first message).
> >> >
> >
> >> Ah, thanks!
> >
> >> EFI Specification Revision : 2.40
> >> EFI Vendor : INSYDE Corp.
> >> EFI Revision : 21573.83
> >
> Thiebaud,
> If we don't manage to resolve this, do you see any way to blacklist
> systems based on this information? Would it be reasonable, say, to
> require UEFI v2.5 or later for TPM2 support? Or doesn't that make any
> sense (I am aware that the TCG EFI spec and the UEFI spec are somewhat
> orthogonal, but it also depends on the hardware you are targetting, I
> guess). Otherwise, we could use a more specific match, perhaps?
I understand we are getting late in the rc and that this should be resolved
quickly. As we have seen, this is a bug related to the firmware more than
the TPM version. So filtering out on the UEFI version make sense. If it is
too late and our only option, I'm ok with filtering out on >= 2.5. A more
specific match would target the exact EFI revision, right? If so, yes that
would be better but I'm not sure how this can be implemented.
> This is of course depending on whether we reach consensus on whether
> we should make any changes at all for what appears to be a single
> sample of a certain piece of hardware, where other samples running the
> same firmware (right?) are working fine.
Right, this is my understanding as well.
Hi,
On 12-03-18 20:55, Thiebaud Weksteen wrote:
> On Mon, Mar 12, 2018 at 7:33 PM Jeremy Cline <[email protected]> wrote:
>
>> On 03/12/2018 02:29 PM, Thiebaud Weksteen wrote:
>>> On Mon, Mar 12, 2018 at 6:30 PM Ard Biesheuvel <
> [email protected]>
>>> wrote:
>>>
>>>> On 12 March 2018 at 17:01, Jeremy Cline <[email protected]> wrote:
>>>>> On 03/12/2018 10:56 AM, Ard Biesheuvel wrote:
>>>>>> On 12 March 2018 at 14:30, Jeremy Cline <[email protected]> wrote:
>>>>>>> On 03/12/2018 07:08 AM, Ard Biesheuvel wrote:
>>>>>>>> On 10 March 2018 at 10:45, Thiebaud Weksteen <[email protected]>
>>> wrote:
>>>>>>>>> On Fri, Mar 9, 2018 at 5:54 PM Jeremy Cline <[email protected]>
>>> wrote:
>>>>>>>>>
>>>>>>>>>> On Fri, Mar 09, 2018 at 10:43:50AM +0000, Thiebaud Weksteen
> wrote:
>>>>>>>>>>> Thanks a lot for trying out the patch!
>>>>>>>>>>>
>>>>>>>>>>> Please don't modify your install at this stage, I think we are
>>> hitting a
>>>>>>>>>>> firmware bug and that would be awesome if we can fix how we are
>>>>>>>>> handling it.
>>>>>>>>>>> So, if we reach that stage in the function it could either be
>>> that:
>>>>>>>>>>> * The allocation did not succeed, somehow, but the firmware
> still
>>>>>>>>> returned
>>>>>>>>>>> EFI_SUCCEED.
>>>>>>>>>>> * The size requested is incorrect (I'm thinking something like a
>>> 1G of
>>>>>>>>>>> log). This would be due to either a miscalculation of log_size
>>>>>>>>> (possible)
>>>>>>>>>>> or; the returned values of GetEventLog are not correct.
>>>>>>>>>>> I'm sending a patch to add checks for these. Could you please
>>> apply and
>>>>>>>>>>> retest?
>>>>>>>>>>> Again, thanks for helping debugging this.
>>>>>>>>>
>>>>>>>>>> No problem, thanks for the help :)
>>>>>>>>>
>>>>>>>>>> With the new patch:
>>>>>>>>>
>>>>>>>>>> Locating the TCG2Protocol
>>>>>>>>>> Calling GetEventLog on TCG2Protocol
>>>>>>>>>> Log returned
>>>>>>>>>> log_location is not empty
>>>>>>>>>> log_size != 0
>>>>>>>>>> log_size < 1M
>>>>>>>>>> Allocating memory for storing the logs
>>>>>>>>>> Returned from memory allocation
>>>>>>>>>> Copying log to new location
>>>>>>>>>
>>>>>>>>>> And then it hangs. I added a couple more print statements:
>>>>>>>>>
>>>>>>>>>> diff --git a/drivers/firmware/efi/libstub/tpm.c
>>>>>>>>> b/drivers/firmware/efi/libstub/tpm.c
>>>>>>>>>> index ee3fac109078..1ab5638bc50e 100644
>>>>>>>>>> --- a/drivers/firmware/efi/libstub/tpm.c
>>>>>>>>>> +++ b/drivers/firmware/efi/libstub/tpm.c
>>>>>>>>>> @@ -148,8 +148,11 @@ void
>>>>>>>>> efi_retrieve_tpm2_eventlog_1_2(efi_system_table_t *sys_table_arg)
>>>>>>>>>> efi_printk(sys_table_arg, "Copying log to new
>>> location\n");
>>>>>>>>>
>>>>>>>>>> memset(log_tbl, 0, sizeof(*log_tbl) + log_size);
>>>>>>>>>> + efi_printk(sys_table_arg, "Successfully memset log_tbl to
>>> 0\n");
>>>>>>>>>> log_tbl->size = log_size;
>>>>>>>>>> + efi_printk(sys_table_arg, "Set log_tbl->size\n");
>>>>>>>>>> log_tbl->version = EFI_TCG2_EVENT_LOG_FORMAT_TCG_1_2;
>>>>>>>>>> + efi_printk(sys_table_arg, "Set log_tbl-version\n");
>>>>>>>>>> memcpy(log_tbl->log, (void *) first_entry_addr,
> log_size);
>>>>>>>>>
>>>>>>>>>> efi_printk(sys_table_arg, "Installing the log into the
>>>>>>>>> configuration table\n");
>>>>>>>>>
>>>>>>>>>> and it's hanging at "memset(log_tbl, 0, sizeof(*log_tbl) +
>>> log_size);"
>>>>>>>>>
>>>>>>>>> Thanks. Well, it looks like the memory that is supposedly
> allocated
>>> is not
>>>>>>>>> usable. I'm thinking this is a firmware bug.
>>>>>>>>> Ard, would you agree on this assumption? Thoughts on how to
> proceed?
>>>>>>>>>
>>>>>>>>
>>>>>>>> I am rather puzzled why the allocate_pool() should succeed and the
>>>>>>>> subsequent memset() should fail. This does not look like an issue
>>> that
>>>>>>>> is intimately related to TPM2 support, rather an issue in the
>>> firmware
>>>>>>>> that happens to get tickled after the change.
>>>>>>>>
>>>>>>>> Would you mind trying replacing EFI_LOADER_DATA with
>>>>>>>> EFI_BOOT_SERVICES_DATA in the allocate_pool() call?
>>>>>>>
>>>>>>> Replacing EFI_LOADER_DATA with EFI_BOOT_SERVICES_DATA still hangs at
>>> the
>>>>>>> memset() call.
>>>>>>>
>>>>>>
>>>>>> Could you try the following please? (attached as well in case gmail
>>> mangles it)
>>>>>>
>>>>>> diff --git a/drivers/firmware/efi/libstub/tpm.c
>>>>>> b/drivers/firmware/efi/libstub/tpm.c
>>>>>> index 2298560cea72..30d960a344b7 100644
>>>>>> --- a/drivers/firmware/efi/libstub/tpm.c
>>>>>> +++ b/drivers/firmware/efi/libstub/tpm.c
>>>>>> @@ -70,6 +70,8 @@ void
>>>>>> efi_retrieve_tpm2_eventlog_1_2(efi_system_table_t *sys_table_arg)
>>>>>> size_t log_size, last_entry_size;
>>>>>> efi_bool_t truncated;
>>>>>> void *tcg2_protocol;
>>>>>> + unsigned long num_pages;
>>>>>> + efi_physical_addr_t log_tbl_alloc;
>>>>>>
>>>>>> status = efi_call_early(locate_protocol, &tcg2_guid, NULL,
>>>>>> &tcg2_protocol);
>>>>>> @@ -104,9 +106,12 @@ void
>>>>>> efi_retrieve_tpm2_eventlog_1_2(efi_system_table_t *sys_table_arg)
>>>>>> }
>>>>>>
>>>>>> /* Allocate space for the logs and copy them. */
>>>>>> - status = efi_call_early(allocate_pool, EFI_LOADER_DATA,
>>>>>> - sizeof(*log_tbl) + log_size,
>>>>>> - (void **) &log_tbl);
>>>>>> + num_pages = DIV_ROUND_UP(sizeof(*log_tbl) + log_size,
>>> EFI_PAGE_SIZE);
>>>>>> + status = efi_call_early(allocate_pages,
>>>>>> + EFI_ALLOCATE_ANY_PAGES,
>>>>>> + EFI_LOADER_DATA,
>>>>>> + num_pages,
>>>>>> + &log_tbl_alloc);
>>>>>>
>>>>>> if (status != EFI_SUCCESS) {
>>>>>> efi_printk(sys_table_arg,
>>>>>> @@ -114,6 +119,7 @@ void
>>>>>> efi_retrieve_tpm2_eventlog_1_2(efi_system_table_t *sys_table_arg)
>>>>>> return;
>>>>>> }
>>>>>>
>>>>>> + log_tbl = (struct linux_efi_tpm_eventlog *)(unsigned
>>> long)log_tbl_alloc;
>>>>>> memset(log_tbl, 0, sizeof(*log_tbl) + log_size);
>>>>>> log_tbl->size = log_size;
>>>>>> log_tbl->version = EFI_TCG2_EVENT_LOG_FORMAT_TCG_1_2;
>>>>>> @@ -126,7 +132,7 @@ void
>>>>>> efi_retrieve_tpm2_eventlog_1_2(efi_system_table_t *sys_table_arg)
>>>>>> return;
>>>>>>
>>>>>> err_free:
>>>>>> - efi_call_early(free_pool, log_tbl);
>>>>>> + efi_call_early(free_pages, log_tbl_alloc, num_pages);
>>>>>> }
>>>>>>
>>>>>> void efi_retrieve_tpm2_eventlog(efi_system_table_t *sys_table_arg)
>>>>>>
>>>>>
>>>>> Hi Ard,
>>>>>
>>>>> When I apply this, it starts hanging at
>>>>>
>>>>> status = efi_call_proto(efi_tcg2_protocol, get_event_log,
> tcg2_protocol,
>>>>> EFI_TCG2_EVENT_LOG_FORMAT_TCG_1_2,
>>>>> &log_location, &log_last_entry, &truncated);
>>>>>
>>>>> rather than at the memset() call.
>>>>>
>>>
>>>> That is *very* surprising, given that the change only affects code
>>>> that executes after that.
>>>
>
> Hans, you said you configured the tablet to use the 32-bit version of grub
> instead
> of 64. Why's that?
Because this tablet, like (almost?) all Bay Trail hardware has a 32 bit UEFI,
even though the processor is 64 bit capable (AFAIK 64 bit Windows drivers were
not ready in time so all Bay Trail devices shipped with a 32 bit Windows).
So this is running a 32 bit grub which boots a 64 bit kernel.
> Jeremy, could you confirm if you are building the kernel in 64bit mode? Is
> your Android install working? (that is, what happens if you boot Boot0000)?
AFAIK the kernel on Jeremy's tablet (which I initially installed) is 64 bit.
Could the problem perhaps be that the new code for the TPM event-log is
missing some handling to deal with running on a 32 bit firmware? I know the
rest of the kernel has special code to deal with this.
Regards,
Hans
On 13 March 2018 at 07:47, Hans de Goede <[email protected]> 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
>> Boot0000)?
>
>
> AFAIK the kernel on Jeremy's tablet (which I initially installed) is 64 bit.
>
> Could the problem perhaps be that the new code for the TPM event-log is
> missing some handling to deal with running on a 32 bit firmware? I know the
> rest of the kernel has special code to deal with this.
>
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?
On 13 March 2018 at 07:59, Ard Biesheuvel <[email protected]> wrote:
> On 13 March 2018 at 07:47, Hans de Goede <[email protected]> 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
>>> Boot0000)?
>>
>>
>> AFAIK the kernel on Jeremy's tablet (which I initially installed) is 64 bit.
>>
>> Could the problem perhaps be that the new code for the TPM event-log is
>> missing some handling to deal with running on a 32 bit firmware? I know the
>> rest of the kernel has special code to deal with this.
>>
>
> 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?
Hi,
On 12-03-18 22:02, Ard Biesheuvel wrote:
> On 12 March 2018 at 19:55, Thiebaud Weksteen <[email protected]> wrote:
>> On Mon, Mar 12, 2018 at 7:33 PM Jeremy Cline <[email protected]> wrote:
>>
>>> On 03/12/2018 02:29 PM, Thiebaud Weksteen wrote:
>>>> On Mon, Mar 12, 2018 at 6:30 PM Ard Biesheuvel <
>> [email protected]>
>>>> wrote:
>>>>
>>>>> On 12 March 2018 at 17:01, Jeremy Cline <[email protected]> wrote:
>>>>>> On 03/12/2018 10:56 AM, Ard Biesheuvel wrote:
>>>>>>> On 12 March 2018 at 14:30, Jeremy Cline <[email protected]> wrote:
>>>>>>>> On 03/12/2018 07:08 AM, Ard Biesheuvel wrote:
>>>>>>>>> On 10 March 2018 at 10:45, Thiebaud Weksteen <[email protected]>
>>>> wrote:
>>>>>>>>>> On Fri, Mar 9, 2018 at 5:54 PM Jeremy Cline <[email protected]>
>>>> wrote:
>>>>>>>>>>
>>>>>>>>>>> On Fri, Mar 09, 2018 at 10:43:50AM +0000, Thiebaud Weksteen
>> wrote:
>>>>>>>>>>>> Thanks a lot for trying out the patch!
>>>>>>>>>>>>
>>>>>>>>>>>> Please don't modify your install at this stage, I think we are
>>>> hitting a
>>>>>>>>>>>> firmware bug and that would be awesome if we can fix how we are
>>>>>>>>>> handling it.
>>>>>>>>>>>> So, if we reach that stage in the function it could either be
>>>> that:
>>>>>>>>>>>> * The allocation did not succeed, somehow, but the firmware
>> still
>>>>>>>>>> returned
>>>>>>>>>>>> EFI_SUCCEED.
>>>>>>>>>>>> * The size requested is incorrect (I'm thinking something like a
>>>> 1G of
>>>>>>>>>>>> log). This would be due to either a miscalculation of log_size
>>>>>>>>>> (possible)
>>>>>>>>>>>> or; the returned values of GetEventLog are not correct.
>>>>>>>>>>>> I'm sending a patch to add checks for these. Could you please
>>>> apply and
>>>>>>>>>>>> retest?
>>>>>>>>>>>> Again, thanks for helping debugging this.
>>>>>>>>>>
>>>>>>>>>>> No problem, thanks for the help :)
>>>>>>>>>>
>>>>>>>>>>> With the new patch:
>>>>>>>>>>
>>>>>>>>>>> Locating the TCG2Protocol
>>>>>>>>>>> Calling GetEventLog on TCG2Protocol
>>>>>>>>>>> Log returned
>>>>>>>>>>> log_location is not empty
>>>>>>>>>>> log_size != 0
>>>>>>>>>>> log_size < 1M
>>>>>>>>>>> Allocating memory for storing the logs
>>>>>>>>>>> Returned from memory allocation
>>>>>>>>>>> Copying log to new location
>>>>>>>>>>
>>>>>>>>>>> And then it hangs. I added a couple more print statements:
>>>>>>>>>>
>>>>>>>>>>> diff --git a/drivers/firmware/efi/libstub/tpm.c
>>>>>>>>>> b/drivers/firmware/efi/libstub/tpm.c
>>>>>>>>>>> index ee3fac109078..1ab5638bc50e 100644
>>>>>>>>>>> --- a/drivers/firmware/efi/libstub/tpm.c
>>>>>>>>>>> +++ b/drivers/firmware/efi/libstub/tpm.c
>>>>>>>>>>> @@ -148,8 +148,11 @@ void
>>>>>>>>>> efi_retrieve_tpm2_eventlog_1_2(efi_system_table_t *sys_table_arg)
>>>>>>>>>>> efi_printk(sys_table_arg, "Copying log to new
>>>> location\n");
>>>>>>>>>>
>>>>>>>>>>> memset(log_tbl, 0, sizeof(*log_tbl) + log_size);
>>>>>>>>>>> + efi_printk(sys_table_arg, "Successfully memset log_tbl to
>>>> 0\n");
>>>>>>>>>>> log_tbl->size = log_size;
>>>>>>>>>>> + efi_printk(sys_table_arg, "Set log_tbl->size\n");
>>>>>>>>>>> log_tbl->version = EFI_TCG2_EVENT_LOG_FORMAT_TCG_1_2;
>>>>>>>>>>> + efi_printk(sys_table_arg, "Set log_tbl-version\n");
>>>>>>>>>>> memcpy(log_tbl->log, (void *) first_entry_addr,
>> log_size);
>>>>>>>>>>
>>>>>>>>>>> efi_printk(sys_table_arg, "Installing the log into the
>>>>>>>>>> configuration table\n");
>>>>>>>>>>
>>>>>>>>>>> and it's hanging at "memset(log_tbl, 0, sizeof(*log_tbl) +
>>>> log_size);"
>>>>>>>>>>
>>>>>>>>>> Thanks. Well, it looks like the memory that is supposedly
>> allocated
>>>> is not
>>>>>>>>>> usable. I'm thinking this is a firmware bug.
>>>>>>>>>> Ard, would you agree on this assumption? Thoughts on how to
>> proceed?
>>>>>>>>>>
>>>>>>>>>
>>>>>>>>> I am rather puzzled why the allocate_pool() should succeed and the
>>>>>>>>> subsequent memset() should fail. This does not look like an issue
>>>> that
>>>>>>>>> is intimately related to TPM2 support, rather an issue in the
>>>> firmware
>>>>>>>>> that happens to get tickled after the change.
>>>>>>>>>
>>>>>>>>> Would you mind trying replacing EFI_LOADER_DATA with
>>>>>>>>> EFI_BOOT_SERVICES_DATA in the allocate_pool() call?
>>>>>>>>
>>>>>>>> Replacing EFI_LOADER_DATA with EFI_BOOT_SERVICES_DATA still hangs at
>>>> the
>>>>>>>> memset() call.
>>>>>>>>
>>>>>>>
>>>>>>> Could you try the following please? (attached as well in case gmail
>>>> mangles it)
>>>>>>>
>>>>>>> diff --git a/drivers/firmware/efi/libstub/tpm.c
>>>>>>> b/drivers/firmware/efi/libstub/tpm.c
>>>>>>> index 2298560cea72..30d960a344b7 100644
>>>>>>> --- a/drivers/firmware/efi/libstub/tpm.c
>>>>>>> +++ b/drivers/firmware/efi/libstub/tpm.c
>>>>>>> @@ -70,6 +70,8 @@ void
>>>>>>> efi_retrieve_tpm2_eventlog_1_2(efi_system_table_t *sys_table_arg)
>>>>>>> size_t log_size, last_entry_size;
>>>>>>> efi_bool_t truncated;
>>>>>>> void *tcg2_protocol;
>>>>>>> + unsigned long num_pages;
>>>>>>> + efi_physical_addr_t log_tbl_alloc;
>>>>>>>
>>>>>>> status = efi_call_early(locate_protocol, &tcg2_guid, NULL,
>>>>>>> &tcg2_protocol);
>>>>>>> @@ -104,9 +106,12 @@ void
>>>>>>> efi_retrieve_tpm2_eventlog_1_2(efi_system_table_t *sys_table_arg)
>>>>>>> }
>>>>>>>
>>>>>>> /* Allocate space for the logs and copy them. */
>>>>>>> - status = efi_call_early(allocate_pool, EFI_LOADER_DATA,
>>>>>>> - sizeof(*log_tbl) + log_size,
>>>>>>> - (void **) &log_tbl);
>>>>>>> + num_pages = DIV_ROUND_UP(sizeof(*log_tbl) + log_size,
>>>> EFI_PAGE_SIZE);
>>>>>>> + status = efi_call_early(allocate_pages,
>>>>>>> + EFI_ALLOCATE_ANY_PAGES,
>>>>>>> + EFI_LOADER_DATA,
>>>>>>> + num_pages,
>>>>>>> + &log_tbl_alloc);
>>>>>>>
>>>>>>> if (status != EFI_SUCCESS) {
>>>>>>> efi_printk(sys_table_arg,
>>>>>>> @@ -114,6 +119,7 @@ void
>>>>>>> efi_retrieve_tpm2_eventlog_1_2(efi_system_table_t *sys_table_arg)
>>>>>>> return;
>>>>>>> }
>>>>>>>
>>>>>>> + log_tbl = (struct linux_efi_tpm_eventlog *)(unsigned
>>>> long)log_tbl_alloc;
>>>>>>> memset(log_tbl, 0, sizeof(*log_tbl) + log_size);
>>>>>>> log_tbl->size = log_size;
>>>>>>> log_tbl->version = EFI_TCG2_EVENT_LOG_FORMAT_TCG_1_2;
>>>>>>> @@ -126,7 +132,7 @@ void
>>>>>>> efi_retrieve_tpm2_eventlog_1_2(efi_system_table_t *sys_table_arg)
>>>>>>> return;
>>>>>>>
>>>>>>> err_free:
>>>>>>> - efi_call_early(free_pool, log_tbl);
>>>>>>> + efi_call_early(free_pages, log_tbl_alloc, num_pages);
>>>>>>> }
>>>>>>>
>>>>>>> void efi_retrieve_tpm2_eventlog(efi_system_table_t *sys_table_arg)
>>>>>>>
>>>>>>
>>>>>> Hi Ard,
>>>>>>
>>>>>> When I apply this, it starts hanging at
>>>>>>
>>>>>> status = efi_call_proto(efi_tcg2_protocol, get_event_log,
>> tcg2_protocol,
>>>>>> EFI_TCG2_EVENT_LOG_FORMAT_TCG_1_2,
>>>>>> &log_location, &log_last_entry, &truncated);
>>>>>>
>>>>>> rather than at the memset() call.
>>>>>>
>>>>
>>>>> That is *very* surprising, given that the change only affects code
>>>>> that executes after that.
>>>>
>>
>> Hans, you said you configured the tablet to use the 32-bit version of grub
>> instead
>> of 64. Why's that?
>>
>> Jeremy, could you confirm if you are building the kernel in 64bit mode? Is
>> your Android install working? (that is, what happens if you boot Boot0000)?
>>
>>>>> I understand how annoying this is for you, and I think we should try
>>>>> to fix this, but reverting the patches outright isn't the solution
>>>>> either.
>>>>
>>>>> Which UEFI vendor and version does your system report?
>>>>
>>>> You should be able to find this info using the "ver" command in the UEFI
>>>> shell.
>>>> The UEFI vendor is Insyde (see first message).
>>>>
>>
>>> Ah, thanks!
>>
>>> EFI Specification Revision : 2.40
>>> EFI Vendor : INSYDE Corp.
>>> EFI Revision : 21573.83
>>
>
> Thiebaud,
>
> If we don't manage to resolve this, do you see any way to blacklist
> systems based on this information? Would it be reasonable, say, to
> require UEFI v2.5 or later for TPM2 support? Or doesn't that make any
> sense (I am aware that the TCG EFI spec and the UEFI spec are somewhat
> orthogonal, but it also depends on the hardware you are targetting, I
> guess). Otherwise, we could use a more specific match, perhaps?
>
> This is of course depending on whether we reach consensus on whether
> we should make any changes at all for what appears to be a single
> sample of a certain piece of hardware, where other samples running the
> same firmware (right?) are working fine.
Right, I don't think a blacklist is a good idea until we understand the
problem better. Both the hard and firmware of Jeremy's tablet are pretty
generic, so I don't think there is anything special there.
One of the reason why this may work on my tablet of the same model is
because I do use shim (Jeremy does not) + a different grub version,
which perhaps leads to a different memory layout or different parts
of memory being initialized...
Regards,
Hans
On Tue, Mar 13, 2018 at 8:59 AM Ard Biesheuvel <[email protected]>
wrote:
> On 13 March 2018 at 07:47, Hans de Goede <[email protected]> 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
> >> Boot0000)?
> >
> >
> > AFAIK the kernel on Jeremy's tablet (which I initially installed) is 64
bit.
> >
> > Could the problem perhaps be that the new code for the TPM event-log is
> > missing some handling to deal with running on a 32 bit firmware? I know
the
> > rest of the kernel has special code to deal with this.
> >
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 &log_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?
On 13 March 2018 at 10:23, Thiebaud Weksteen <[email protected]> wrote:
> On Tue, Mar 13, 2018 at 8:59 AM Ard Biesheuvel <[email protected]>
> wrote:
>
>> On 13 March 2018 at 07:47, Hans de Goede <[email protected]> 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 &log_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.
On Tue, Mar 13, 2018 at 9:47 AM, Hans de Goede <[email protected]> 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
On 03/13/2018 03:59 AM, Ard Biesheuvel wrote:
> On 13 March 2018 at 07:47, Hans de Goede <[email protected]> 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
>>> Boot0000)?
>>
>>
>> AFAIK the kernel on Jeremy's tablet (which I initially installed) is 64 bit.
>>
>> Could the problem perhaps be that the new code for the TPM event-log is
>> missing some handling to deal with running on a 32 bit firmware? I know the
>> rest of the kernel has special code to deal with this.
>>
>
> 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
On 13 March 2018 at 13:41, Jeremy Cline <[email protected]> wrote:
> On 03/13/2018 03:59 AM, Ard Biesheuvel wrote:
>> On 13 March 2018 at 07:47, Hans de Goede <[email protected]> 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
>>>> Boot0000)?
>>>
>>>
>>> AFAIK the kernel on Jeremy's tablet (which I initially installed) is 64 bit.
>>>
>>> Could the problem perhaps be that the new code for the TPM event-log is
>>> missing some handling to deal with running on a 32 bit firmware? I know the
>>> rest of the kernel has special code to deal with this.
>>>
>>
>> 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.
On Tue, Mar 13, 2018 at 2:43 PM Ard Biesheuvel <[email protected]>
wrote:
> On 13 March 2018 at 13:41, Jeremy Cline <[email protected]> wrote:
> > On 03/13/2018 03:59 AM, Ard Biesheuvel wrote:
> >> On 13 March 2018 at 07:47, Hans de Goede <[email protected]> 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
> >>>> Boot0000)?
> >>>
> >>>
> >>> AFAIK the kernel on Jeremy's tablet (which I initially installed) is
64 bit.
> >>>
> >>> Could the problem perhaps be that the new code for the TPM event-log
is
> >>> missing some handling to deal with running on a 32 bit firmware? I
know the
> >>> rest of the kernel has special code to deal with this.
> >>>
> >>
> >> 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.
Sweet!
Jeremy, Hans, thanks for the help debugging!
Ard, thanks for the help fixing the issue!
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 <[email protected]> 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