tpmdd-devel.lists.sourceforge.net archive mirror
 help / color / mirror / Atom feed
From: Ard Biesheuvel <ard.biesheuvel@linaro.org>
To: Thiebaud Weksteen <tweek@google.com>
Cc: Jeremy Cline <jeremy@jcline.org>,
	Hans de Goede <hdegoede@redhat.com>,
	Javier Martinez Canillas <javierm@redhat.com>,
	Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>,
	linux-efi@vger.kernel.org, linux-integrity@vger.kernel.org,
	tpmdd-devel@lists.sourceforge.net,
	Linux Kernel Mailing List <linux-kernel@vger.kernel.org>
Subject: Re: Regression from efi: call get_event_log before ExitBootServices
Date: Mon, 12 Mar 2018 21:02:58 +0000	[thread overview]
Message-ID: <CAKv+Gu-se-O0aNjjA==8_aB9qBaGks7+sUjGvgT8qFekUVg4PA@mail.gmail.com> (raw)
In-Reply-To: <CA+zpnLf=JDUxyorH7yP=08FD_u=+UoQ6ubsKyVLD+4w9raYAKQ@mail.gmail.com>

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.

  reply	other threads:[~2018-03-12 21:02 UTC|newest]

Thread overview: 39+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-03-06 16:00 Regression from efi: call get_event_log before ExitBootServices Jeremy Cline
     [not found] ` <01000161fc0b4755-df0621f4-ab5d-479a-b425-adf98427a308-000000-p/GC64/jrecnJqMo6gzdpkEOCMrvLtNR@public.gmane.org>
2018-03-07  8:41   ` Thiebaud Weksteen via tpmdd-devel
2018-03-07 11:16     ` Hans de Goede
2018-03-07 12:00       ` Javier Martinez Canillas
2018-03-07 17:33     ` Jeremy Cline
2018-03-08  8:45       ` Thiebaud Weksteen
2018-03-08 18:20         ` Jeremy Cline
     [not found] ` <e7c2be5c-cf21-fc2d-efda-d9222d93ffad@redhat.com>
     [not found]   ` <b32f335c-0d77-1749-f7fe-65f512280255@redhat.com>
     [not found]     ` <ade378f6-c997-1d48-a30d-cceee6435fc8@redhat.com>
     [not found]       ` <a3b5f822-f8f4-e2f5-46da-e23e13174f28@redhat.com>
2018-03-08 16:50         ` Hans de Goede
2018-03-08 17:26           ` Jeremy Cline
2018-03-09  9:29             ` Hans de Goede
2018-03-09 10:43               ` Thiebaud Weksteen
2018-03-09 16:54                 ` Jeremy Cline
2018-03-10 10:45                   ` Thiebaud Weksteen
2018-03-12 10:17                     ` Jarkko Sakkinen
2018-03-12 10:41                       ` Paul Menzel
2018-03-16 13:01                         ` Jarkko Sakkinen
2018-03-12 11:08                     ` Ard Biesheuvel
2018-03-12 14:30                       ` Jeremy Cline
2018-03-12 14:56                         ` Ard Biesheuvel
2018-03-12 17:01                           ` Jeremy Cline
2018-03-12 17:30                             ` Ard Biesheuvel
2018-03-12 18:29                               ` Thiebaud Weksteen
2018-03-12 18:33                                 ` Jeremy Cline
2018-03-12 19:55                                   ` Thiebaud Weksteen
2018-03-12 21:02                                     ` Ard Biesheuvel [this message]
2018-03-13  7:24                                       ` Thiebaud Weksteen
2018-03-13  8:08                                       ` Hans de Goede
2018-03-13  1:50                                     ` Jeremy Cline
2018-03-13  7:47                                     ` Hans de Goede
2018-03-13  7:59                                       ` Ard Biesheuvel
2018-03-13  8:02                                         ` Ard Biesheuvel
2018-03-13 10:23                                         ` Thiebaud Weksteen
2018-03-13 10:30                                           ` Ard Biesheuvel
2018-03-13 13:41                                         ` Jeremy Cline
2018-03-13 13:43                                           ` Ard Biesheuvel
2018-03-13 15:00                                             ` Thiebaud Weksteen
2018-03-13 12:51                                       ` Andy Shevchenko
2018-03-12 18:30                               ` Jeremy Cline
2018-03-09 17:03               ` James Bottomley

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to='CAKv+Gu-se-O0aNjjA==8_aB9qBaGks7+sUjGvgT8qFekUVg4PA@mail.gmail.com' \
    --to=ard.biesheuvel@linaro.org \
    --cc=hdegoede@redhat.com \
    --cc=jarkko.sakkinen@linux.intel.com \
    --cc=javierm@redhat.com \
    --cc=jeremy@jcline.org \
    --cc=linux-efi@vger.kernel.org \
    --cc=linux-integrity@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=tpmdd-devel@lists.sourceforge.net \
    --cc=tweek@google.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).