diff mbox

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

Message ID 1479306245-14456-1-git-send-email-stefanb@linux.vnet.ibm.com
State New
Headers show

Commit Message

Stefan Berger Nov. 16, 2016, 2:24 p.m. UTC
The virtual TPM driver must not access the hosts's event log,
otherwise we get crashes from that.

Signed-off-by: Stefan Berger <stefanb@linux.vnet.ibm.com>
---
 drivers/char/tpm/tpm_eventlog.c | 3 +++
 1 file changed, 3 insertions(+)

Comments

Jarkko Sakkinen Nov. 16, 2016, 3:37 p.m. UTC | #1
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 <stefanb@linux.vnet.ibm.com>

Can you give me a "Fixes" line (no need to send a new patch)?

> ---
>  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(&chip->dev,
>  			"%s: ERROR - event log already initialized\n",
> -- 
> 2.4.3
> 

/Jarkko

------------------------------------------------------------------------------
Stefan Berger Nov. 16, 2016, 3:41 p.m. UTC | #2
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 <stefanb@linux.vnet.ibm.com>
> Can you give me a "Fixes" line (no need to send a new patch)?

I haven't bisected, yet.... but will do that today.

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(&chip->dev,
>>   			"%s: ERROR - event log already initialized\n",
>> -- 
>> 2.4.3
>>
> /Jarkko
>


------------------------------------------------------------------------------
Stefan Berger Nov. 16, 2016, 5:07 p.m. UTC | #3
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 <stefanb@linux.vnet.ibm.com>
>> 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 ffffc9000d8c0000
[  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]  ffffc900027b7c78 ffffffff8140ca11 ffff8802ad548300 
ffffc9000d8c0000
[  173.600137]  ffffc900027b7c98 ffffffff8106b99f ffff8802ad548300 
ffffc9000d8c0000
[  173.605189]  ffffc900027b7ca8 ffffffff8106b9dc ffffc900027b7cc8 
ffffffff81496c75
[  173.608722] Call Trace:
[  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]
[  173.608722]  [<ffffffffa00e1921>] tpm_bios_log_setup+0x31/0x170 [tpm]
[  173.608722]  [<ffffffffa00df0dc>] tpm_chip_register+0x4c/0x200 [tpm]
[  173.608722]  [<ffffffffa029e309>] vtpm_proxy_work+0x19/0x30 
[tpm_vtpm_proxy]
[  173.608722]  [<ffffffff810c4593>] process_one_work+0x1f3/0x560
[  173.608722]  [<ffffffff810c4511>] ? process_one_work+0x171/0x560
[  173.608722]  [<ffffffff810c494e>] worker_thread+0x4e/0x480
[  173.608722]  [<ffffffff810c4900>] ? process_one_work+0x560/0x560
[  173.608722]  [<ffffffff810c4900>] ? process_one_work+0x560/0x560
[  173.608722]  [<ffffffff810ca994>] kthread+0xf4/0x110
[  173.608722]  [<ffffffff810ca8a0>] ? kthread_park+0x60/0x60
[  173.608722]  [<ffffffff817a1c15>] 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(&chip->dev,
>>>               "%s: ERROR - event log already initialized\n",
>>> -- 
>>> 2.4.3
>>>
>> /Jarkko
>>
>


------------------------------------------------------------------------------
Jason Gunthorpe Nov. 16, 2016, 8:07 p.m. UTC | #4
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]  [<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

------------------------------------------------------------------------------
Jarkko Sakkinen Nov. 17, 2016, 8:34 p.m. UTC | #5
On Wed, Nov 16, 2016 at 01:07:59PM -0700, 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..
> 
> 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?

Here we read the data from completely separate ACPI table i.e. TCPA. PPI
on the other hand is located in the device node of DSDT or SSDT.

You are right. The acpi_dev_handle should be checked in the sense that
it is a gate for using the ACPI functionality but trying to open TCPA
should not cause anything devastating.

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

acpi_get_table() should be safe and the code also calls
acpi_os_unmap_iomem() correctly.

Maybe there's something wrong in the kernel outside the TPM driver...

> Jason

/Jarkko

------------------------------------------------------------------------------
diff mbox

Patch

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(&chip->dev,
 			"%s: ERROR - event log already initialized\n",