From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752208AbeCMHrQ (ORCPT ); Tue, 13 Mar 2018 03:47:16 -0400 Received: from mail-wr0-f194.google.com ([209.85.128.194]:37990 "EHLO mail-wr0-f194.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751943AbeCMHrO (ORCPT ); Tue, 13 Mar 2018 03:47:14 -0400 X-Google-Smtp-Source: AG47ELvg3uoIwj4Zx6/0u7Z4+1+aLdF3YHBFz6XsHQNBl7JBZGaeKUX1OITETiIP2Fxp3Osxr3ksOQ== Subject: Re: Regression from efi: call get_event_log before ExitBootServices To: Thiebaud Weksteen , Jeremy Cline Cc: Ard Biesheuvel , Javier Martinez Canillas , Jarkko Sakkinen , linux-efi@vger.kernel.org, linux-integrity@vger.kernel.org, tpmdd-devel@lists.sourceforge.net, linux-kernel@vger.kernel.org References: <01000161fc0b4755-df0621f4-ab5d-479a-b425-adf98427a308-000000@email.amazonses.com> <29c1640a-cf19-ca19-7de9-96f202edfb5a@redhat.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> From: Hans de Goede Message-ID: Date: Tue, 13 Mar 2018 08:47:10 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.6.0 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Hi, On 12-03-18 20:55, Thiebaud Weksteen wrote: > On Mon, Mar 12, 2018 at 7:33 PM Jeremy Cline wrote: > >> On 03/12/2018 02:29 PM, Thiebaud Weksteen wrote: >>> On Mon, Mar 12, 2018 at 6:30 PM Ard Biesheuvel < > ard.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? 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