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

[Jarkko Sakkinen]

On Tue, Nov 22, 2016 at 08:07:42AM +0200, Jarkko Sakkinen wrote:
> On Sat, Nov 19, 2016 at 11:32:55AM -0700, Jason Gunthorpe wrote:
> > On Thu, Nov 17, 2016 at 06:15:20PM -0500, Stefan Berger wrote:
> > 
> > > >>Further, I had the impression that the error unwinding following 
> > > >>-ENODEV has
> > > >>an issue related to sysfs.
> > > >I don't follow this comment..
> > > 
> > > I have encountered this error here, which gets masked when applying the
> > > previously shown patch.
> > 
> > If tpm_chip_register fails vtpm must not call tpm_chip_unregister:
> > 
> > > [   58.271017]  [] dpm_sysfs_remove+0x22/0x60
> > > [   58.271017]  [] device_del+0x58/0x280
> > > [   58.271017]  [] tpm_chip_unregister+0x40/0xb0 [tpm]
> > > [   58.271017]  [] vtpm_proxy_fops_release+0x40/0x60 
> > > [tpm_vtpm_proxy]
> > 
> > So, this is a vtpm thing I missed for 'tpm: Get rid of 
> > TPM_CHIP_FLAG_REGISTERED'
> > 
> > Does this do the trick?
> 
> Tested-by: Jarkko Sakkinen 

Reviewed-by: Jarkko Sakkinen 

/Jarkko

--
___
tpmdd-devel mailing list
tpmdd-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/tpmdd-devel

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

[Jarkko Sakkinen]

On Sat, Nov 19, 2016 at 11:32:55AM -0700, Jason Gunthorpe wrote:
> On Thu, Nov 17, 2016 at 06:15:20PM -0500, Stefan Berger wrote:
> 
> > >>Further, I had the impression that the error unwinding following -ENODEV 
> > >>has
> > >>an issue related to sysfs.
> > >I don't follow this comment..
> > 
> > I have encountered this error here, which gets masked when applying the
> > previously shown patch.
> 
> If tpm_chip_register fails vtpm must not call tpm_chip_unregister:
> 
> > [   58.271017]  [] dpm_sysfs_remove+0x22/0x60
> > [   58.271017]  [] device_del+0x58/0x280
> > [   58.271017]  [] tpm_chip_unregister+0x40/0xb0 [tpm]
> > [   58.271017]  [] vtpm_proxy_fops_release+0x40/0x60 
> > [tpm_vtpm_proxy]
> 
> So, this is a vtpm thing I missed for 'tpm: Get rid of 
> TPM_CHIP_FLAG_REGISTERED'
> 
> Does this do the trick?

Tested-by: Jarkko Sakkinen 

/Jarkko

> diff --git a/drivers/char/tpm/tpm_vtpm_proxy.c 
> b/drivers/char/tpm/tpm_vtpm_proxy.c
> index 3d6f6ca81def75..d3a41f9d65c85c 100644
> --- a/drivers/char/tpm/tpm_vtpm_proxy.c
> +++ b/drivers/char/tpm/tpm_vtpm_proxy.c
> @@ -42,6 +42,7 @@ struct proxy_dev {
>   long state;  /* internal state */
>  #define STATE_OPENED_FLAGBIT(0)
>  #define STATE_WAIT_RESPONSE_FLAG BIT(1)  /* waiting for emulator response */
> +#define STATE_REGISTERED_FLAG BIT(2)
>  
>   size_t req_len;  /* length of queued TPM request */
>   size_t resp_len; /* length of queued TPM response */
> @@ -372,6 +373,7 @@ static void vtpm_proxy_work(struct work_struct *work)
>   if (rc)
>   goto err;
>  
> + proxy_dev->state |= STATE_REGISTERED_FLAG;
>   return;
>  
>  err:
> @@ -516,7 +518,8 @@ static void vtpm_proxy_delete_device(struct proxy_dev 
> *proxy_dev)
>*/
>   vtpm_proxy_fops_undo_open(proxy_dev);
>  
> - tpm_chip_unregister(proxy_dev->chip);
> + if (proxy_dev->state & STATE_REGISTERED_FLAG)
> + tpm_chip_unregister(proxy_dev->chip);
>  
>   vtpm_proxy_delete_proxy_dev(proxy_dev);
>  }
> 

--
___
tpmdd-devel mailing list
tpmdd-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/tpmdd-devel

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

[Stefan Berger]

On 11/19/2016 01:32 PM, Jason Gunthorpe wrote:
> On Thu, Nov 17, 2016 at 06:15:20PM -0500, Stefan Berger wrote:
>
 Further, I had the impression that the error unwinding following -ENODEV 
 has
 an issue related to sysfs.
>>> I don't follow this comment..
>> I have encountered this error here, which gets masked when applying the
>> previously shown patch.
> If tpm_chip_register fails vtpm must not call tpm_chip_unregister:
>
>> [   58.271017]  [] dpm_sysfs_remove+0x22/0x60
>> [   58.271017]  [] device_del+0x58/0x280
>> [   58.271017]  [] tpm_chip_unregister+0x40/0xb0 [tpm]
>> [   58.271017]  [] vtpm_proxy_fops_release+0x40/0x60 
>> [tpm_vtpm_proxy]
> So, this is a vtpm thing I missed for 'tpm: Get rid of 
> TPM_CHIP_FLAG_REGISTERED'
>
> Does this do the trick?

Yes, it does the trick.

I tested this now with your other fix-patch applied to Jarkko's tree. I 
injected a fault by returning -EIO from tpm_read_log_acpi(), which 
otherwise would cause the crash.

Tested-by: Stefan Berger 

>
> diff --git a/drivers/char/tpm/tpm_vtpm_proxy.c 
> b/drivers/char/tpm/tpm_vtpm_proxy.c
> index 3d6f6ca81def75..d3a41f9d65c85c 100644
> --- a/drivers/char/tpm/tpm_vtpm_proxy.c
> +++ b/drivers/char/tpm/tpm_vtpm_proxy.c
> @@ -42,6 +42,7 @@ struct proxy_dev {
>   long state;  /* internal state */
>   #define STATE_OPENED_FLAGBIT(0)
>   #define STATE_WAIT_RESPONSE_FLAG BIT(1)  /* waiting for emulator response */
> +#define STATE_REGISTERED_FLAG BIT(2)
>
>   size_t req_len;  /* length of queued TPM request */
>   size_t resp_len; /* length of queued TPM response */
> @@ -372,6 +373,7 @@ static void vtpm_proxy_work(struct work_struct *work)
>   if (rc)
>   goto err;
>
> + proxy_dev->state |= STATE_REGISTERED_FLAG;
>   return;
>
>   err:
> @@ -516,7 +518,8 @@ static void vtpm_proxy_delete_device(struct proxy_dev 
> *proxy_dev)
>*/
>   vtpm_proxy_fops_undo_open(proxy_dev);
>
> - tpm_chip_unregister(proxy_dev->chip);
> + if (proxy_dev->state & STATE_REGISTERED_FLAG)
> + tpm_chip_unregister(proxy_dev->chip);
>
>   vtpm_proxy_delete_proxy_dev(proxy_dev);
>   }
>


--
___
tpmdd-devel mailing list
tpmdd-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/tpmdd-devel

Re: [tpmdd-devel] [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 = >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
>>
>


--
___
tpmdd-devel mailing list
tpmdd-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/tpmdd-devel

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

[Jason Gunthorpe]

On Thu, Nov 17, 2016 at 01:25:54PM -0500, Stefan Berger wrote:
> 
> In the case of x86, tpm_read_log_of() is a stub return -ENODEV, which in
> turn fails the whole device:

Somehow this got screwed up during the lengthy review. ENODEV is the
right return from the leaf routines but the tests in tpm_eventlog di
not get fixed:

> http://git.infradead.org/users/jjs/linux-tpmdd.git/blob/4d388433e85f8257f5a9344a7acf6f499ba2b29e:/drivers/char/tpm/tpm_eventlog.h#l87

Is wrong, should be:

if (rc != -ENODEV)
   return rc;

And the one in tpm_bios_log_setup should be

if (rc != 0 && rc != -ENODEV)
return rc;

> I think the OF log reading code will also need to check for chip->dev.parent
> being NULL.

Currect! Lets get that fixed too. :(

> Further, I had the impression that the error unwinding following -ENODEV has
> an issue related to sysfs.

I don't follow this comment..

Jason

--
___
tpmdd-devel mailing list
tpmdd-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/tpmdd-devel

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

[Jason Gunthorpe]

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 = >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

--
___
tpmdd-devel mailing list
tpmdd-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/tpmdd-devel

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

[Jason Gunthorpe]

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

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]  [] dump_stack+0x63/0x82
> [  173.608722]  [] iounmap.part.1+0x7f/0x90
> [  173.608722]  [] iounmap+0x2c/0x30
> [  173.608722]  [] acpi_os_map_cleanup.part.10+0x31/0x3e
> [  173.608722]  [] acpi_os_unmap_iomem+0xcb/0xd2
> [  173.608722]  [] 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

--
___
tpmdd-devel mailing list
tpmdd-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/tpmdd-devel

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

[Stefan Berger]

On 11/16/2016 10:41 AM, Stefan Berger wrote:
> On 11/16/2016 10:37 AM, Jarkko Sakkinen wrote:
>> On Wed, Nov 16, 2016 at 09:24:05AM -0500, Stefan Berger wrote:
>>> The virtual TPM driver must not access the hosts's event log,
>>> otherwise we get crashes from that.
>>>
>>> Signed-off-by: Stefan Berger 
>> Can you give me a "Fixes" line (no need to send a new patch)?
>
> I haven't bisected, yet but will do that today.

The culprit seems to be 'tpm: fix the missing .owner in 
tpm_bios_measurements_ops'

'Something' now can only have a single owner?

The crash looks like this:

[  173.597916] iounmap: bad address c9000d8c
[  173.599149] CPU: 10 PID: 686 Comm: kworker/10:2 Not tainted 
4.9.0-rc5+ #578
[  173.600051] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), 
BIOS rel-1.9.0-156-g3560877 04/01/2014
[  173.600137] Workqueue: tpm-vtpm vtpm_proxy_work [tpm_vtpm_proxy]
[  173.600137]  c900027b7c78 8140ca11 8802ad548300 
c9000d8c
[  173.600137]  c900027b7c98 8106b99f 8802ad548300 
c9000d8c
[  173.605189]  c900027b7ca8 8106b9dc c900027b7cc8 
81496c75
[  173.608722] Call Trace:
[  173.608722]  [] dump_stack+0x63/0x82
[  173.608722]  [] iounmap.part.1+0x7f/0x90
[  173.608722]  [] iounmap+0x2c/0x30
[  173.608722]  [] acpi_os_map_cleanup.part.10+0x31/0x3e
[  173.608722]  [] acpi_os_unmap_iomem+0xcb/0xd2
[  173.608722]  [] read_log+0xc8/0x19e [tpm]
[  173.608722]  [] tpm_bios_log_setup+0x31/0x170 [tpm]
[  173.608722]  [] tpm_chip_register+0x4c/0x200 [tpm]
[  173.608722]  [] vtpm_proxy_work+0x19/0x30 
[tpm_vtpm_proxy]
[  173.608722]  [] process_one_work+0x1f3/0x560
[  173.608722]  [] ? process_one_work+0x171/0x560
[  173.608722]  [] worker_thread+0x4e/0x480
[  173.608722]  [] ? process_one_work+0x560/0x560
[  173.608722]  [] ? process_one_work+0x560/0x560
[  173.608722]  [] kthread+0xf4/0x110
[  173.608722]  [] ? kthread_park+0x60/0x60
[  173.608722]  [] ret_from_fork+0x25/0x30


 Stefan

>
> Also I am wondering whether we should introduce a flag 
> TPM_CHIP_NO_FIRMWARE_LOG that is checked below. The 
> TPM_CHIP_FLAG_VIRTUAL may not be ideal, also because it is set due to 
> the device not having a parent device, which may not be related. 
> Thoughts? That new flag would only be set by the vtpm proxy driver.
>
>Stefan
>
>>
>>> ---
>>>   drivers/char/tpm/tpm_eventlog.c | 3 +++
>>>   1 file changed, 3 insertions(+)
>>>
>>> diff --git a/drivers/char/tpm/tpm_eventlog.c 
>>> b/drivers/char/tpm/tpm_eventlog.c
>>> index fb603a7..e0abf40 100644
>>> --- a/drivers/char/tpm/tpm_eventlog.c
>>> +++ b/drivers/char/tpm/tpm_eventlog.c
>>> @@ -369,6 +369,9 @@ static int tpm_read_log(struct tpm_chip *chip)
>>>   {
>>>   int rc;
>>>   +if (chip->flags & TPM_CHIP_FLAG_VIRTUAL)
>>> +return -EFAULT;
>>> +
>>>   if (chip->log.bios_event_log != NULL) {
>>>   dev_dbg(>dev,
>>>   "%s: ERROR - event log already initialized\n",
>>> -- 
>>> 2.4.3
>>>
>> /Jarkko
>>
>


--
___
tpmdd-devel mailing list
tpmdd-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/tpmdd-devel