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

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

[Jarkko Sakkinen]

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

If there is a problem in the TPM driver, this does not fix the
problem. It will mask the problem. Maybe there's an ACPI regression
in the rc tree?

This is a funky situation because those lines need to be there but
I do not want them before it is root caused that it is not a TPM
bug.

/Jarkko

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

[Stefan Berger]

On 11/17/2016 03:37 PM, Jarkko Sakkinen wrote:
> On Thu, Nov 17, 2016 at 07:35:05AM -0500, Stefan Berger wrote:
>> 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;
>> +
> If there is a problem in the TPM driver, this does not fix the
> problem. It will mask the problem. Maybe there's an ACPI regression
> in the rc tree?

Following the path from here :

http://lxr.free-electrons.com/source/drivers/acpi/acpica/tbxface.c#L282

acpi_get_table_with_size -> acpi_tb_validate_table -> acpi_tb_acquire_table

I see acpi_os_map_memory being called in acpi_tb_acquire_table but not 
the corresponding acpi_os_unmap_memory...

     Stefan


>
> This is a funky situation because those lines need to be there but
> I do not want them before it is root caused that it is not a TPM
> bug.
>
> /Jarkko
>

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

[Stefan Berger]

On 11/18/2016 09:11 AM, Stefan Berger wrote:
> On 11/17/2016 03:37 PM, Jarkko Sakkinen wrote:
>> On Thu, Nov 17, 2016 at 07:35:05AM -0500, Stefan Berger wrote:
>>> 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;
>>> +
>> If there is a problem in the TPM driver, this does not fix the
>> problem. It will mask the problem. Maybe there's an ACPI regression
>> in the rc tree?
>
> Following the path from here :
>
> http://lxr.free-electrons.com/source/drivers/acpi/acpica/tbxface.c#L282
>
> acpi_get_table_with_size -> acpi_tb_validate_table -> 
> acpi_tb_acquire_table
>
> I see acpi_os_map_memory being called in acpi_tb_acquire_table but not 
> the corresponding acpi_os_unmap_memory...
>

And to add to this: the call to acpi_get_table() in tpm_acpi.c alone is 
causing the crash. I am fairly sure that it has something to do with the 
memory mapping call above not being matched by an unmapping call.


>     Stefan
>
>
>>
>> This is a funky situation because those lines need to be there but
>> I do not want them before it is root caused that it is not a TPM
>> bug.
>>
>> /Jarkko
>>
>


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

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

[Stefan Berger]

On 11/18/2016 09:15 AM, Stefan Berger wrote:
> On 11/18/2016 09:11 AM, Stefan Berger wrote:
>> On 11/17/2016 03:37 PM, Jarkko Sakkinen wrote:
>>> On Thu, Nov 17, 2016 at 07:35:05AM -0500, Stefan Berger wrote:
>>>> 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;
>>>> +
>>> If there is a problem in the TPM driver, this does not fix the
>>> problem. It will mask the problem. Maybe there's an ACPI regression
>>> in the rc tree?
>>
>> Following the path from here :
>>
>> http://lxr.free-electrons.com/source/drivers/acpi/acpica/tbxface.c#L282
>>
>> acpi_get_table_with_size -> acpi_tb_validate_table -> 
>> acpi_tb_acquire_table
>>
>> I see acpi_os_map_memory being called in acpi_tb_acquire_table but 
>> not the corresponding acpi_os_unmap_memory...
>>
>
> And to add to this: the call to acpi_get_table() in tpm_acpi.c alone 
> is causing the crash. I am fairly sure that it has something to do 
> with the memory mapping call above not being matched by an unmapping 
> call.

Have to take that all back. This is not the reason. What seems to be the 
reason is just the acpi_os_map_iomem() call. Without calling the 
acpi_os_unmap_iomem() later on, which I assume should be allowed, the 
driver will crash when the device terminates.

     Stefan

>
>
>>     Stefan
>>
>>
>>>
>>> This is a funky situation because those lines need to be there but
>>> I do not want them before it is root caused that it is not a TPM
>>> bug.
>>>
>>> /Jarkko
>>>
>>
>


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

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

[Jason Gunthorpe]

On Fri, Nov 18, 2016 at 11:58:08AM -0500, Stefan Berger wrote:

> reason is just the acpi_os_map_iomem() call. Without calling the
> acpi_os_unmap_iomem() later on, which I assume should be allowed, the driver
> will crash when the device terminates.

The oops looks like unbalacned map/unmap, can you add some tracing
around the failing address and see if that is possibly true, and who
does it?

Jason