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

From: Jason Gunthorpe <jgunthorpe-ePGOBjL8dl3ta4EC/59zMFaTQe2KTcn/@public.gmane.org>
To: Stefan Berger
	<stefanb-23VcF4HTsmIX0ybBhKVfKdBPR1lH4CV8@public.gmane.org>
Cc: tpmdd-devel-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f@public.gmane.org
Subject: Re: [PATCH] tpm: vtpm_proxy: Do not access host's event log
Date: Thu, 17 Nov 2016 11:10:06 -0700	[thread overview]
Message-ID: <20161117181006.GA26039@obsidianresearch.com> (raw)
In-Reply-To: <ef1f954d-fc52-0522-01f7-b0e31ea14c59-23VcF4HTsmIX0ybBhKVfKdBPR1lH4CV8@public.gmane.org>

1;2802;0cOn Thu, Nov 17, 2016 at 07:35:05AM -0500, Stefan Berger wrote:
> 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:

I can't see how setting owner has any bearing on this.. I also don't
see why it should ever fail at all... It would be great to get a root
cause here - could it be memory corruption????

Getting a really bad feeling from this  :(

> iff --git a/drivers/char/tpm/tpm_acpi.c b/drivers/char/tpm/tpm_acpi.c
> index 0cb43ef..a73295a 100644
> +++ 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).

Return -ENODEV

> +    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.

OF is already fine, it checks chip->dev.parent->of_node so it will
exit properly for vtpm, no need for this.

Jason

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