From mboxrd@z Thu Jan 1 00:00:00 1970 From: Hans de Goede Subject: Re: Regression from efi: call get_event_log before ExitBootServices Date: Tue, 13 Mar 2018 09:08:38 +0100 Message-ID: References: <01000161fc0b4755-df0621f4-ab5d-479a-b425-adf98427a308-000000@email.amazonses.com> <010001620bafa06b-41525407-603e-40a9-ba11-6033b2f5dcc7-000000@email.amazonses.com> <010001621a9e5069-0b1a6328-97e4-4396-9438-b90f5b8c82a4-000000@email.amazonses.com> <010001621b287e42-58955302-cc14-4212-b7b0-e6e358633dab-000000@email.amazonses.com> <010001621b7ce5a3-b80c55b8-be68-4b44-ab52-4949e8ddb8d0-000000@email.amazonses.com> Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: Content-Language: en-US Sender: linux-kernel-owner@vger.kernel.org To: Ard Biesheuvel , Thiebaud Weksteen Cc: Jeremy Cline , Javier Martinez Canillas , Jarkko Sakkinen , linux-efi@vger.kernel.org, linux-integrity@vger.kernel.org, tpmdd-devel@lists.sourceforge.net, Linux Kernel Mailing List List-Id: tpmdd-devel@lists.sourceforge.net Hi, On 12-03-18 22:02, Ard Biesheuvel wrote: > On 12 March 2018 at 19:55, Thiebaud Weksteen wrote: >> On Mon, Mar 12, 2018 at 7:33 PM Jeremy Cline wrote: >> >>> On 03/12/2018 02:29 PM, Thiebaud Weksteen wrote: >>>> On Mon, Mar 12, 2018 at 6:30 PM Ard Biesheuvel < >> ard.biesheuvel@linaro.org> >>>> wrote: >>>> >>>>> On 12 March 2018 at 17:01, Jeremy Cline wrote: >>>>>> On 03/12/2018 10:56 AM, Ard Biesheuvel wrote: >>>>>>> On 12 March 2018 at 14:30, Jeremy Cline wrote: >>>>>>>> On 03/12/2018 07:08 AM, Ard Biesheuvel wrote: >>>>>>>>> On 10 March 2018 at 10:45, Thiebaud Weksteen >>>> wrote: >>>>>>>>>> On Fri, Mar 9, 2018 at 5:54 PM Jeremy Cline >>>> wrote: >>>>>>>>>> >>>>>>>>>>> On Fri, Mar 09, 2018 at 10:43:50AM +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