| Message ID | 1479306245-14456-1-git-send-email-stefanb@linux.vnet.ibm.com |
|---|---|
| State | New |
| Headers | show |
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 ------------------------------------------------------------------------------
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 > ------------------------------------------------------------------------------
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 >> > ------------------------------------------------------------------------------
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 ------------------------------------------------------------------------------
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 --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",
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(+)