Re: TPM/Verifiers testing bug?

[Max Tottenham <mtottenh@akamai.com>]

On 01/14, Daniel Kiper wrote:
> On Wed, Jan 09, 2019 at 02:16:16PM +0000, Max Tottenham wrote:
> > Hi Folks
> >
> > I was curious about the upstream work on the verifiers framework (and
> > the TPM patches). I have both a TPM 2.0 based system and a QEMU + swtpm
> > setup with which to test. I compiled the head of the master branch, if I
> > boot into the grub shell and run the following commands:
> >
> > grub> insmod verifiers
> > grub> insmod tpm
> > grub> normal
> >
> > I get a machine crash:
> >
> > qemu-system-x86_64: Trying to execute code outside RAM or ROM at
> > 0x00000000000b0000
> > This usually means one of the following happened:
> >
> > (1) You told QEMU to execute a kernel for the wrong machine type, and it
> > crashed on startup (eg trying to run a raspberry pi kernel on a
> > versatilepb QEMU machine)
> > (2) You didn't give QEMU a kernel or BIOS filename at all, and QEMU
> > executed a ROM full of no-op instructions until it fell off the end
> > (3) Your guest kernel has a bug and crashed by jumping off into nowhere
> >
> > This is almost always one of the first two, so check your command line
> > and that you are using the right type of kernel for this machine.
> > If you think option (3) is likely then you can try debugging your guest
> > with the -d debug options; in particular -d guest_errors will cause the
> > log to include a dump of the guest register state at this point.
> >
> > Execution cannot continue; stopping here.
> >
> >
> > Software versions:
> > Qemu version:  v2.11.0           (0a0dc59)
> > OVMF git tag:  edk2-stable201811 (8558838)
> > swtpm git tag: tpm2-preview.v2   (f98592c)
> >
> >
> > Running the same on real hardware also produces a crash, any thoughts?
> 
> Adding Matthew who is the author of the patch series.
> 
> Daniel

I went ahead and did some debugging. Below is a patch that seems to fix
my problem. Although those calls to grub_efi_open_protocol() in the tpm
module should probably check their return value and do something sane if
0x0 is returned.

-- 
Max Tottenham       | mtottenh@akamai.com
Senior Software Engineer, Server Platform Engineering
/(* Akamai Technologies

From 023be8fe8a4f77dbcbf94015bef81385a7681679 Mon Sep 17 00:00:00 2001
From: Max Tottenham <mtottenh@akamai.com>
Date: Mon, 14 Jan 2019 14:03:29 +0000
Subject: [PATCH] Fix bug in GRUB2 TPM module

  The value of tpm_handle changes between successive calls to
  tpm_handle_find(), as instead of simply copying the stored pointer we end up
  taking the address of said pointer when using the cached value of grub_tpm_handle.

  This causes grub_efi_open_protocol() to return a nullptr in
  grub_tpm2_execute() and grub_tpm2_log_event(). Said nullptr goes unchecked
  and efi_call_5(tpm->hash_log_extend_event,...) ends up jumping to 0x0, Qemu
  crashes once video ROM is reached at 0xb0000.

  This patch seems to do the trick of fixing that bug, but we should
  also ensure that all calls to grub_efi_open_protocol() are checked so
  that we don't start executing low memory.
---
 grub-core/commands/efi/tpm.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/grub-core/commands/efi/tpm.c b/grub-core/commands/efi/tpm.c
index 563ceba7a..7475fd87b 100644
--- a/grub-core/commands/efi/tpm.c
+++ b/grub-core/commands/efi/tpm.c
@@ -89,7 +89,7 @@ grub_tpm_handle_find (grub_efi_handle_t *tpm_handle,
 
   if (grub_tpm_handle != NULL)
     {
-      *tpm_handle = &grub_tpm_handle;
+      *tpm_handle = grub_tpm_handle;
       *protocol_version = grub_tpm_version;
       return 1;
     }
-- 
2.17.0