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