Re: [tpmdd-devel] [PATCH] tpm: drop chip->is_open and chip->duration_adjusted
On 11/15/2016 05:14 AM, Jarkko Sakkinen wrote:
> Use atomic bitops for chip->flags so that we do not need chip->is_open
> and chip->duration_adjusted anymore.
>
> Signed-off-by: Jarkko Sakkinen
> ---
> drivers/char/tpm/st33zp24/st33zp24.c | 6 +++---
> drivers/char/tpm/tpm-chip.c | 14 --
> drivers/char/tpm/tpm-dev.c | 9 +++--
> drivers/char/tpm/tpm-interface.c | 30 +++---
> drivers/char/tpm/tpm-sysfs.c | 2 +-
> drivers/char/tpm/tpm.h | 14 +++---
> drivers/char/tpm/tpm2-cmd.c | 2 +-
> drivers/char/tpm/tpm_crb.c | 2 +-
> drivers/char/tpm/tpm_i2c_nuvoton.c | 8
> drivers/char/tpm/tpm_tis_core.c | 26 +-
> drivers/char/tpm/tpm_vtpm_proxy.c| 2 +-
> 11 files changed, 57 insertions(+), 58 deletions(-)
>
> diff --git a/drivers/char/tpm/st33zp24/st33zp24.c
> b/drivers/char/tpm/st33zp24/st33zp24.c
> index 6f060c7..14734a0 100644
> --- a/drivers/char/tpm/st33zp24/st33zp24.c
> +++ b/drivers/char/tpm/st33zp24/st33zp24.c
> @@ -267,7 +267,7 @@ static int wait_for_stat(struct tpm_chip *chip, u8 mask,
> unsigned long timeout,
>
> stop = jiffies + timeout;
>
> - if (chip->flags & TPM_CHIP_FLAG_IRQ) {
> + if (test_bit(TPM_CHIP_FLAG_IRQ, &chip->flags)) {
> cur_intrs = tpm_dev->intrs;
> clear_interruption(tpm_dev);
> enable_irq(tpm_dev->irq);
> @@ -429,7 +429,7 @@ static int st33zp24_send(struct tpm_chip *chip, unsigned
> char *buf,
> if (ret < 0)
> goto out_err;
>
> - if (chip->flags & TPM_CHIP_FLAG_IRQ) {
> + if (test_bit(TPM_CHIP_FLAG_IRQ, &chip->flags)) {
> ordinal = be32_to_cpu(*((__be32 *) (buf + 6)));
>
> ret = wait_for_stat(chip, TPM_STS_DATA_AVAIL | TPM_STS_VALID,
> @@ -586,7 +586,7 @@ int st33zp24_probe(void *phy_id, const struct
> st33zp24_phy_ops *ops,
> goto _tpm_clean_answer;
>
> tpm_dev->irq = irq;
> - chip->flags |= TPM_CHIP_FLAG_IRQ;
> + set_bit(TPM_CHIP_FLAG_IRQ, &chip->flags);
>
> disable_irq_nosync(tpm_dev->irq);
> }
> diff --git a/drivers/char/tpm/tpm-chip.c b/drivers/char/tpm/tpm-chip.c
> index 3f27753..04819e1 100644
> --- a/drivers/char/tpm/tpm-chip.c
> +++ b/drivers/char/tpm/tpm-chip.c
> @@ -183,7 +183,7 @@ struct tpm_chip *tpm_chip_alloc(struct device *dev,
> goto out;
>
> if (!dev)
> - chip->flags |= TPM_CHIP_FLAG_VIRTUAL;
> + set_bit(TPM_CHIP_FLAG_VIRTUAL, &chip->flags);
>
> cdev_init(&chip->cdev, &tpm_fops);
> chip->cdev.owner = THIS_MODULE;
> @@ -271,7 +271,7 @@ static void tpm_del_char_device(struct tpm_chip *chip)
>
> /* Make the driver uncallable. */
> down_write(&chip->ops_sem);
> - if (chip->flags & TPM_CHIP_FLAG_TPM2)
> + if (test_bit(TPM_CHIP_FLAG_TPM2, &chip->flags))
> tpm2_shutdown(chip, TPM2_SU_CLEAR);
> chip->ops = NULL;
> up_write(&chip->ops_sem);
> @@ -281,7 +281,8 @@ static void tpm_del_legacy_sysfs(struct tpm_chip *chip)
> {
> struct attribute **i;
>
> - if (chip->flags & (TPM_CHIP_FLAG_TPM2 | TPM_CHIP_FLAG_VIRTUAL))
> + if (test_bit(TPM_CHIP_FLAG_TPM2, &chip->flags) ||
> + test_bit(TPM_CHIP_FLAG_VIRTUAL, &chip->flags))
> return;
>
> sysfs_remove_link(&chip->dev.parent->kobj, "ppi");
> @@ -299,8 +300,9 @@ static int tpm_add_legacy_sysfs(struct tpm_chip *chip)
> struct attribute **i;
> int rc;
>
> - if (chip->flags & (TPM_CHIP_FLAG_TPM2 | TPM_CHIP_FLAG_VIRTUAL))
> - return 0;
> + if (test_bit(TPM_CHIP_FLAG_TPM2, &chip->flags) ||
> + test_bit(TPM_CHIP_FLAG_VIRTUAL, &chip->flags))
> + return;
guess should be "return 0;"
Thanks & Regards,
- Nayna
>
> rc = __compat_only_sysfs_link_entry_to_kobj(
> &chip->dev.parent->kobj, &chip->dev.kobj, "ppi");
> @@ -335,7 +337,7 @@ int tpm_chip_register(struct tpm_chip *chip)
> int rc;
>
> if (chip->ops->flags & TPM_OPS_AUTO_STARTUP) {
> - if (chip->flags & TPM_CHIP_FLAG_TPM2)
> + if (test_bit(TPM_CHIP_FLAG_TPM2, &chip->flags))
> rc = tpm2_auto_startup(chip);
> else
> rc = tpm1_auto_startup(chip);
> diff --git a/drivers/char/tpm/tpm-dev.c b/drivers/char/tpm/tpm-dev.c
> index 912ad30..3738c38 100644
> --- a/drivers/char/tpm/tpm-dev.c
> +++ b/drivers/char/tpm/tpm-dev.c
> @@ -57,17 +57,14 @@ static int tpm_open(struct inode *inode, struct file
> *file)
> container_of(inode->i_cdev, struct tpm_chip, cdev);
> struct file_priv *priv;
>
> - /* It's assured that the chip will be opened just once,
> - * by the check of is_open variable, which is protected
> - * by driver_lock. */
> - if (test_and_set_bit(0, &chip->is_open)) {
> +
Re: [tpmdd-devel] [PATCH RFC 2/2] tpm: refactor tpm2_get_tpm_pt to tpm2_getcap_cmd
On 11/12/2016 05:32 AM, Jarkko Sakkinen wrote:
> On Fri, Nov 11, 2016 at 09:51:45AM +0530, Nayna wrote:
>>
>>
>> On 10/09/2016 03:44 PM, Jarkko Sakkinen wrote:
>>> Refactored tpm2_get_tpm_pt to tpm2_getcap_cmd, which means that it also
>>> takes capability ID as input. This is required to access
>>> TPM_CAP_HANDLES, which contains metadata needed for swapping transient
>>> data.
>>>
>>> Signed-off-by: Jarkko Sakkinen
>>> ---
>>>drivers/char/tpm/tpm.h | 6 +++-
>>>drivers/char/tpm/tpm2-cmd.c | 64
>>> -
>>>drivers/char/tpm/tpm_tis_core.c | 3 +-
>>>3 files changed, 38 insertions(+), 35 deletions(-)
>>>
>>> diff --git a/drivers/char/tpm/tpm.h b/drivers/char/tpm/tpm.h
>>> index 0fab6d5..8176f42 100644
>>> --- a/drivers/char/tpm/tpm.h
>>> +++ b/drivers/char/tpm/tpm.h
>>> @@ -85,6 +85,10 @@ enum tpm2_capabilities {
>>> TPM2_CAP_TPM_PROPERTIES = 6,
>>>};
>>>
>>> +enum tpm2_properties {
>>> + TPM2_PT_FAMILY_INDICATOR= 0x100,
>>> +};
>>> +
>>>enum tpm2_startup_types {
>>> TPM2_SU_CLEAR = 0x,
>>> TPM2_SU_STATE = 0x0001,
>>> @@ -485,7 +489,7 @@ int tpm2_seal_trusted(struct tpm_chip *chip,
>>>int tpm2_unseal_trusted(struct tpm_chip *chip,
>>> struct trusted_key_payload *payload,
>>> struct trusted_key_options *options);
>>> -ssize_t tpm2_get_tpm_pt(struct tpm_chip *chip, u32 property_id,
>>> +ssize_t tpm2_getcap_cmd(struct tpm_chip *chip, u32 cap_id, u32 property_id,
>>> u32 *value, const char *desc);
>>>
>>>int tpm2_auto_startup(struct tpm_chip *chip);
>>> diff --git a/drivers/char/tpm/tpm2-cmd.c b/drivers/char/tpm/tpm2-cmd.c
>>> index 2900e18..fcf3d86 100644
>>> --- a/drivers/char/tpm/tpm2-cmd.c
>>> +++ b/drivers/char/tpm/tpm2-cmd.c
>>> @@ -111,13 +111,13 @@ struct tpm2_pcr_extend_in {
>>> u8 digest[TPM_DIGEST_SIZE];
>>>} __packed;
>>>
>>> -struct tpm2_get_tpm_pt_in {
>>> +struct tpm2_getcap_in {
>>> __be32 cap_id;
>>> __be32 property_id;
>>> __be32 property_cnt;
>>>} __packed;
>>>
>>> -struct tpm2_get_tpm_pt_out {
>>> +struct tpm2_getcap_out {
>>> u8 more_data;
>>> __be32 subcap_id;
>>> __be32 property_cnt;
>>> @@ -140,8 +140,8 @@ union tpm2_cmd_params {
>>> struct tpm2_pcr_read_inpcrread_in;
>>> struct tpm2_pcr_read_out pcrread_out;
>>> struct tpm2_pcr_extend_in pcrextend_in;
>>> - struct tpm2_get_tpm_pt_in get_tpm_pt_in;
>>> - struct tpm2_get_tpm_pt_out get_tpm_pt_out;
>>> + struct tpm2_getcap_in getcap_in;
>>> + struct tpm2_getcap_out getcap_out;
>>> struct tpm2_get_random_in getrandom_in;
>>> struct tpm2_get_random_out getrandom_out;
>>>};
>>> @@ -435,16 +435,6 @@ int tpm2_get_random(struct tpm_chip *chip, u8 *out,
>>> size_t max)
>>> return total ? total : -EIO;
>>>}
>>>
>>> -#define TPM2_GET_TPM_PT_IN_SIZE \
>>> - (sizeof(struct tpm_input_header) + \
>>> -sizeof(struct tpm2_get_tpm_pt_in))
>>> -
>>> -static const struct tpm_input_header tpm2_get_tpm_pt_header = {
>>> - .tag = cpu_to_be16(TPM2_ST_NO_SESSIONS),
>>> - .length = cpu_to_be32(TPM2_GET_TPM_PT_IN_SIZE),
>>> - .ordinal = cpu_to_be32(TPM2_CC_GET_CAPABILITY)
>>> -};
>>> -
>>>/**
>>> * Append TPMS_AUTH_COMMAND to the buffer. The buffer must be allocated
>>> with
>>> * tpm_buf_alloc().
>>> @@ -750,35 +740,43 @@ out:
>>> return rc;
>>>}
>>>
>>> +#define TPM2_GETCAP_IN_SIZE \
>>> + (sizeof(struct tpm_input_header) + sizeof(struct tpm2_getcap_in))
>>> +
>>> +static const struct tpm_input_header tpm2_getcap_header = {
>>> + .tag = cpu_to_be16(TPM2_ST_NO_SESSIONS),
>>> + .length = cpu_to_be32(TPM2_GETCAP_IN_SIZE),
>>> + .ordinal = cpu_to_be32(TPM2_CC_GET_CAPABILITY)
>>> +};
>>> +
>>>/**
>>> - * tpm2_get_tpm_pt() - get value of a TPM_CAP_TPM_PROPERTIES type property
>>> - * @chip: TPM chip to use.
>>> - * @property_id: property ID.
>>> - * @value: output variable.
>>> + * tpm2_getcap_cmd() - execute a TPM2_GetCapability command
>>> + * @chip: TPM chip to use
>>> + * @cap_id:capability ID
>>> + * @property_id: property ID
>>> + * @value: value of the property
>>> * @desc:passed to tpm_transmit_cmd()
>>> *
>>> - * 0 is returned when the operation is successful. If a negative number is
>>> - * returned it remarks a POSIX error code. If a positive number is returned
>>> - * it remarks a TPM error.
>>> + * Return: same as with tpm_transmit_cmd
>>> */
>>> -ssize_t tpm2_get_tpm_pt(struct tpm_chip *chip, u32 property_id, u32
>>> *value,
>>> - const char *desc)
>>> +ssize_t tpm2_getcap_cmd(struct tpm_chip *chip, u32 cap_id, u32 property_id,
>>> + u32 *value, const char *desc)
>>
>> This function currently returns single value "u32 *value" as output data.
>>
Re: [tpmdd-devel] [PATCH] tpm: vtpm_proxy: Do not access host's event log
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; + // 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). /* Find TCPA entry in RSDT (ACPI_LOGICAL_ADDRESSING) */ status = acpi_get_table(ACPI_SIG_TCPA, 1, (struct acpi_table_header **)&buff); diff --git a/drivers/char/tpm/tpm_eventlog.c b/drivers/char/tpm/tpm_eventlog.c index fb603a7..12b0356 100644 --- a/drivers/char/tpm/tpm_eventlog.c +++ b/drivers/char/tpm/tpm_eventlog.c @@ -380,7 +380,8 @@ static int tpm_read_log(struct tpm_chip *chip) if ((rc == 0) || (rc == -ENOMEM)) return rc; -rc = tpm_read_log_of(chip); +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. return rc; Stefan > > 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: drop chip->is_open and chip->duration_adjusted
On Thu, Nov 17, 2016 at 04:10:29PM +0530, Nayna wrote: > On 11/15/2016 05:14 AM, Jarkko Sakkinen wrote: > > - if (chip->flags & (TPM_CHIP_FLAG_TPM2 | TPM_CHIP_FLAG_VIRTUAL)) > > - return 0; > > + if (test_bit(TPM_CHIP_FLAG_TPM2, &chip->flags) || > > + test_bit(TPM_CHIP_FLAG_VIRTUAL, &chip->flags)) > > + return; > > guess should be "return 0;" Ouch. Good catch. Thank you. /Jarkko -- ___ tpmdd-devel mailing list tpmdd-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/tpmdd-devel
Re: [tpmdd-devel] [PATCH RFC 2/2] tpm: refactor tpm2_get_tpm_pt to tpm2_getcap_cmd
On Thu, Nov 17, 2016 at 05:20:36PM +0530, Nayna wrote: > I tested this for capability TPM2_CAP_PCRS. It seems TPM2_CAP_PCRS > capability always returns full PCR allocation, and more_data as 0, So, I > think the idea of looping over based on more_data may not work for this > capability. You can always request one value at a time until there's no more. If you request N values, depending on the hardware, the hardware returns to you anything from 1 to N values. If you implement a function that requests N values in the command, you *must* handle the case where moreData is 1 even if the hardware you are testing that never happens. That's the reason why I would start with a function that you request one property of one capability and optimize it in future if it doesn't scale for some workload. Do you have a workload where it doesn't scale? /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
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 -- ___ 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
On 11/17/2016 01:10 PM, Jason Gunthorpe wrote: > 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. In the case of x86, tpm_read_log_of() is a stub return -ENODEV, which in turn fails the whole device: http://git.infradead.org/users/jjs/linux-tpmdd.git/blob/4d388433e85f8257f5a9344a7acf6f499ba2b29e:/drivers/char/tpm/tpm_eventlog.h#l87 http://git.infradead.org/users/jjs/linux-tpmdd.git/blob/4d388433e85f8257f5a9344a7acf6f499ba2b29e:/drivers/char/tpm/tpm-chip.c#l348 So we must not run into that or handle -ENODEV differently or return a different error code in the stub function. I think the OF log reading code will also need to check for chip->dev.parent being NULL. Further, I had the impression that the error unwinding following -ENODEV has an issue related to sysfs. Stefan > > 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
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
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] [] 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? 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 -- ___ 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
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 -- ___ 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
On 11/17/2016 01:33 PM, Jason Gunthorpe wrote: > 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; Can you show a patch that shows where to place these two? > >> 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.. I have encountered this error here, which gets masked when applying the previously shown patch. [ 58.270643] BUG: unable to handle kernel NULL pointer dereference at 0088 [ 58.271017] IP: [] kernfs_find_ns+0x1f/0x140 [ 58.271017] PGD 0 [ 58.271017] Oops: [#1] SMP [ 58.271017] Modules linked in: tpm_vtpm_proxy nf_conntrack_netbios_ns nf_conntrack_broadcast ip6t_rpfilter ip6t_REJECT nf_reject_ipv6 xt_conntrack ebtable_nat ebtable_broute bridge stp llc ebtable_filter ebtables ip6table_nat nf_conntrack_ipv6 nf_defrag_ipv6 nf_nat_ipv6 ip6table_mangle ip6table_security ip6table_raw ip6table_filter ip6_tables iptable_nat nf_conntrack_ipv4 nf_defrag_ipv4 nf_nat_ipv4 nf_nat nf_conntrack iptable_mangle iptable_security iptable_raw crc32c_intel tpm_tis joydev virtio_balloon i2c_piix4 pcspkr i2c_core tpm_tis_core tpm nfsd auth_rpcgss nfs_acl lockd grace sunrpc 8139too virtio_pci 8139cp virtio_ring ata_generic mii serio_raw pata_acpi virtio floppy [ 58.271017] CPU: 10 PID: 1420 Comm: vtpmctrl Not tainted 4.9.0-rc5+ #607 [ 58.271017] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS rel-1.9.0-156-g3560877 04/01/2014 [ 58.271017] task: 8802abb58000 task.stack: c90002604000 [ 58.271017] RIP: 0010:[] [] kernfs_find_ns+0x1f/0x140 [ 58.271017] RSP: 0018:c90002607af8 EFLAGS: 00010292 [ 58.271017] RAX: RBX: RCX: 8802abb58820 [ 58.271017] RDX: RSI: 81885960 RDI: [ 58.271017] RBP: c90002607b28 R08: R09: b39d2cf2 [ 58.271017] R10: R11: R12: 81885960 [ 58.271017] R13: R14: 81885960 R15: 8802acaf4360 [ 58.271017] FS: () GS:8802b248() knlGS: [ 58.271017] CS: 0010 DS: ES: CR0: 80050033 [ 58.271017] CR2: 0088 CR3: 01c0a000 CR4: 06e0 [ 58.271017] Stack: [ 58.271017] 1efbca09 81885960 [ 58.271017] 8802b0168fe0 8802acaf4360 c90002607b50 812e9cd3 [ 58.271017] 81cf3dc0 8802a8654000 c90002607b70 [ 58.271017] Call Trace: [ 58.271017] [] kernfs_find_and_get_ns+0x33/0x60 [ 58.271017] [] sysfs_unmerge_group+0x1d/0x60 [ 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] [ 58.271017] [] __fput+0xdf/0x1f0 [ 58.271017] [] fput+0xe/0x10 [ 58.271017] [] task_work_run+0x7e/0xa0 [ 58.271017] [] do_exit+0x2f8/0xb20 [ 58.271017] [] ? get_signal+0xc2/0x6d0 [ 58.271017] [] do_group_exit+0x50/0xd0 [ 58.271017] [] get_signal+0x28f/0x6d0 [ 58.271017] [] do_signal+0x37/0x6a0 [ 58.271017] [] ? vtpm_proxy_fops_read+0x13b/0x1b0 [tpm_vtpm_proxy] [ 58.271017] [] ? wake_atomic_t_function+0x70/0x70 [ 58.271017] [] ? security_file_permission+0x9b/0xc0 [ 58.271017] [] exit_to_usermode_loop+0x76/0xb0 [ 58.271017] [] syscall_return_slowpath+0xaf/0xc0 [ 58.271017] [] entry_SYSCALL_64_fastpath+0xac/0xae [ 58.271017] Code: 66 66 66 2e 0f 1f 84 00 00 00 00 00 66 66 66 66 90 55 49 89 f8 48 89 e5 41 57 41 56 41 55 41 54 49 89 f6 53 49 89 d5 48 83 ec 08 <44> 0f b7 a7 88 00 00 00 8b 05 13 af 9e 00 48 8b 5f 68 66 41 83 [ 58.271017] RIP [] kernfs_find_ns+0x1f/0x140 [ 58.271017] RSP [ 58.271017] CR2: 0088 [ 58.271017] ---[ end trace 88bc09bcfa89f874 ]--- [ 58.271017] Fixing recursive fault but reboot is needed! -- ___ tpmdd-devel mailing list tpmdd-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/tpmdd
Re: [tpmdd-devel] [PATCH] tpm: vtpm_proxy: Do not access host's event log
On Thu, Nov 17, 2016 at 06:15:20PM -0500, Stefan Berger wrote: > On 11/17/2016 01:33 PM, Jason Gunthorpe wrote: > > 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; > > > Can you show a patch that shows where to place these two? The disasterous error handling for cases that you mentioned: http://git.infradead.org/users/jjs/linux-tpmdd.git/commitdiff/d660a91a1b9dd80f5c2c973e3369400d3c9f9848 I'm sorry I let these slip in the code review. /Jarkko -- ___ tpmdd-devel mailing list tpmdd-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/tpmdd-devel
[tpmdd-devel] [PATCH 1/2] tpm: Check for parent device being NULL
Check for the parent device being NULL before accessing it. This prevents crashed with the vtpm_proxy driver, since this driver does not have a parent device. Signed-off-by: Stefan Berger --- drivers/char/tpm/tpm_of.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/char/tpm/tpm_of.c b/drivers/char/tpm/tpm_of.c index 36df9df..72c282e 100644 --- a/drivers/char/tpm/tpm_of.c +++ b/drivers/char/tpm/tpm_of.c @@ -29,7 +29,7 @@ int tpm_read_log_of(struct tpm_chip *chip) struct tpm_bios_log *log; log = &chip->log; - if (chip->dev.parent->of_node) + if (chip->dev.parent && chip->dev.parent->of_node) np = chip->dev.parent->of_node; else return -ENODEV; -- 2.4.3 -- ___ tpmdd-devel mailing list tpmdd-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/tpmdd-devel
[tpmdd-devel] [PATCH 2/2] tpm: Fix error code handling after tpm_bios_log_setup
tpm_bios_log_setup() may return -ENODEV in case no log was found. In this case we do not need to fail the device. Signed-off-by: Stefan Berger --- drivers/char/tpm/tpm-chip.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/char/tpm/tpm-chip.c b/drivers/char/tpm/tpm-chip.c index 3f27753..2d6530b 100644 --- a/drivers/char/tpm/tpm-chip.c +++ b/drivers/char/tpm/tpm-chip.c @@ -346,7 +346,7 @@ int tpm_chip_register(struct tpm_chip *chip) tpm_sysfs_add_device(chip); rc = tpm_bios_log_setup(chip); - if (rc == -ENODEV) + if (rc != -ENODEV) return rc; tpm_add_ppi(chip); -- 2.4.3 -- ___ tpmdd-devel mailing list tpmdd-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/tpmdd-devel
