Re: [PATCH] tpm: vtpm_proxy: Do not access host's event log

[Stefan Berger <stefanb-23VcF4HTsmIX0ybBhKVfKdBPR1lH4CV8@public.gmane.org>]

On 11/16/2016 03:07 PM, Jason Gunthorpe wrote:
> On Wed, Nov 16, 2016 at 12:07:23PM -0500, Stefan Berger wrote:
>> The culprit seems to be 'tpm: fix the missing .owner in
>> tpm_bios_measurements_ops'
> That is unlikely, it is probably the patch before which calls read_log
> unconditionally now. That suggests the crashing is a little random..

I ran the vtpm driver test suite (with -j32) a few times at that patch 
and it didn't crash. It crashes severely with later patches applied. 
Here's the current experimental patch that fixes these problems:

iff --git a/drivers/char/tpm/tpm_acpi.c b/drivers/char/tpm/tpm_acpi.c
index 0cb43ef..a73295a 100644
--- a/drivers/char/tpm/tpm_acpi.c
+++ b/drivers/char/tpm/tpm_acpi.c
@@ -56,6 +56,9 @@ int tpm_read_log_acpi(struct tpm_chip *chip)

      log = &chip->log;

+    if (!chip->acpi_dev_handle)
+        return 0;
+

// So ACPI is not supported on this device, but ACPI support is compiled 
in. I am returning 0 here, assuming it's not an OF device and the 
corresponding OF function need not be called (see below).

      /* Find TCPA entry in RSDT (ACPI_LOGICAL_ADDRESSING) */
      status = acpi_get_table(ACPI_SIG_TCPA, 1,
                  (struct acpi_table_header **)&buff);
diff --git a/drivers/char/tpm/tpm_eventlog.c 
b/drivers/char/tpm/tpm_eventlog.c
index fb603a7..12b0356 100644
--- a/drivers/char/tpm/tpm_eventlog.c
+++ b/drivers/char/tpm/tpm_eventlog.c
@@ -380,7 +380,8 @@ static int tpm_read_log(struct tpm_chip *chip)
      if ((rc == 0) || (rc == -ENOMEM))
          return rc;

-    rc = tpm_read_log_of(chip);
+    if (!(chip->flags & TPM_CHIP_FLAG_VIRTUAL))
+        rc = tpm_read_log_of(chip);

// I am not sure how to handle this case, in case we get here, which 
would only be on an OF device (following 'return 0;' above), but we 
don't want to attempt to read the log there, either. I think the most 
straight-forward way would be to gate this whole function with a flag 
that only the vtpm proxy driver has: TPM_CHIP_FLAG_NO_FIRMWARE_LOG.

      return rc;


    Stefan

>
> tpm_read_log_acpi should check if the chip has a acpi_dev_handle
> before running, but it also shouldn't crash - so there are two bugs
> here I guess.. Please do that instead of the checking the virual flag.
>
> Jarkko, you know acpi better, we switched ppi to search starting from
> the acpi_dev_handle for its data - can we do the same for event log?
>
>> The crash looks like this:
>> [  173.608722]  [<ffffffff8140ca11>] dump_stack+0x63/0x82
>> [  173.608722]  [<ffffffff8106b99f>] iounmap.part.1+0x7f/0x90
>> [  173.608722]  [<ffffffff8106b9dc>] iounmap+0x2c/0x30
>> [  173.608722]  [<ffffffff81496c75>] acpi_os_map_cleanup.part.10+0x31/0x3e
>> [  173.608722]  [<ffffffff8179629c>] acpi_os_unmap_iomem+0xcb/0xd2
>> [  173.608722]  [<ffffffffa00e1b28>] read_log+0xc8/0x19e [tpm]
> This seems really strange ACPI should not crash like this - yes it
> will wrongly read the log for the system into the vtpm, but that
> *should* work.
>
> Are you doing anything special with this test like high parallism or
> something?  Any chance you can look at little more? The tpm code looks
> OK to me, the map and unmap are properly paired - but the bad address
> from the iounmap suggests the refcounting in acpi is not working or
> something along those lines?
>
> Jason
>


------------------------------------------------------------------------------