On 11/19/2016 11:52 PM, Jason Gunthorpe wrote:
> On Fri, Nov 18, 2016 at 07:52:49AM -0800, Jarkko Sakkinen wrote:
>> On Thu, Nov 17, 2016 at 07:30:04PM -0500, Stefan Berger wrote:
>>> 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 <stef...@linux.vnet.ibm.com>
>>> 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
>>> +++ 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);
>>
>> CC to linux-security-module
>>
>> LGTM
>>
>> Reviewed-by: Jarkko Sakkinen <jarkko.sakki...@linux.intel.com>
>
> Erm, what about rc == 0? And all the other problems?
>
> Here, use this (untested) should take care of everything on this
> topic..
>
> The two things I haven't seen explained are the sysfs unregister crash
> and the acpi iounmap crash :/
>
> From 8768bcb8cd2a5a17cc4d811a9298b20c3a2c0884 Mon Sep 17 00:00:00 2001
> From: Jason Gunthorpe <jguntho...@obsidianresearch.com>
> Date: Sat, 19 Nov 2016 11:18:28 -0700
> Subject: [PATCH] tpm: Fix handling of missing event log
>
> The event log is an optional firmware feature, if the firmware
> does not support it then the securityfs files should not be created
> and no other notification given.
>
> - Uniformly return -ENODEV from the tpm_bios_log_setup cone if
> no event log is detected.
> - Check in ACPI if this node was discovered via ACPI.
> - Improve the check in OF to make sure there is a parent and to
> fail detection if the two log properties are not declared
> - Pass through all other error codes instead of filtering just some
>
> Signed-off-by: Jason Gunthorpe <jguntho...@obsidianresearch.com>
> ---
> drivers/char/tpm/tpm-chip.c | 2 +-
> drivers/char/tpm/tpm_acpi.c | 8 +++++++-
> drivers/char/tpm/tpm_eventlog.c | 26 +++++++++++++-------------
> drivers/char/tpm/tpm_of.c | 11 +++++------
> 4 files changed, 26 insertions(+), 21 deletions(-)
>
> diff --git a/drivers/char/tpm/tpm-chip.c b/drivers/char/tpm/tpm-chip.c
> index 3f27753d96aab5..7a4869151d3b90 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 != 0 && rc != -ENODEV)
> return rc;
This will return in case of -EFAULT as well, where the check is that log
is already initialized. Do we want to fail the probe here as well ?
-EFAULT is returned from tpm_read_log() as below:
tpm_read_log() has
if (chip->log.bios_event_log != NULL) {
dev_dbg(&chip->dev,
"%s: ERROR - event log already initialized\n",
__func__);
return -EFAULT;
}
>
> tpm_add_ppi(chip);
> diff --git a/drivers/char/tpm/tpm_acpi.c b/drivers/char/tpm/tpm_acpi.c
> index 0cb43ef5f79a6e..99366bf64f3359 100644
> --- a/drivers/char/tpm/tpm_acpi.c
> +++ b/drivers/char/tpm/tpm_acpi.c
> @@ -56,12 +56,18 @@ int tpm_read_log_acpi(struct tpm_chip *chip)
>
> log = &chip->log;
>
> + /* Unfortuntely ACPI does not associate the event log with a specific
> + * TPM, like PPI. Thus all ACPI TPMs will read the same log.
> + */
> + if (!chip->acpi_dev_handle)
> + return -ENODEV;
> +
> /* Find TCPA entry in RSDT (ACPI_LOGICAL_ADDRESSING) */
> status = acpi_get_table(ACPI_SIG_TCPA, 1,
> (struct acpi_table_header **)&buff);
>
> if (ACPI_FAILURE(status))
> - return -EIO;
> + return -ENODEV;
>
> switch(buff->platform_class) {
> case BIOS_SERVER:
> diff --git a/drivers/char/tpm/tpm_eventlog.c b/drivers/char/tpm/tpm_eventlog.c
> index fb603a74cbd29e..2a15b866ac257a 100644
> --- a/drivers/char/tpm/tpm_eventlog.c
> +++ b/drivers/char/tpm/tpm_eventlog.c
> @@ -377,14 +377,21 @@ static int tpm_read_log(struct tpm_chip *chip)
> }
>
> rc = tpm_read_log_acpi(chip);
This is to understand..
It can return -ENOMEM error here, contd below...
> - if ((rc == 0) || (rc == -ENOMEM))
> + if (rc != -ENODEV)
> return rc;
>
> - rc = tpm_read_log_of(chip);
> -
> - return rc;
> + return tpm_read_log_of(chip);
So, in ACPI if -ENOMEM error is returned, it will continue to
tpm_read_log_of(chip), which will return -ENODEV. So, -ENOMEM error is
now masked with -ENODEV error.
Next, in tpm_chip_register(), this will be considered as -ENODEV
> + if (rc != 0 && rc != -ENODEV)
> return rc;
and so will not fail the probe.
It doesn't create securityfs files, but also does not fail the probe for
memory error. Is it the expected behavior ?
> }
>
> +/*
> + * tpm_bios_log_setup() - Read the event log from the firmware
> + * @chip: TPM chip to use.
> + *
> + * If an event log is found then the securityfs files are setup to
> + * export it to userspace, otherwise nothing is done.
> + *
> + * Returns -ENODEV if the firmware has no event log.
> + */
> int tpm_bios_log_setup(struct tpm_chip *chip)
> {
> const char *name = dev_name(&chip->dev);
> @@ -395,15 +402,8 @@ int tpm_bios_log_setup(struct tpm_chip *chip)
> return 0;
>
> rc = tpm_read_log(chip);
> - /*
> - * read_log failure means event log is not supported except for ENOMEM.
> - */
> - if (rc < 0) {
> - if (rc == -ENOMEM)
> - return -ENODEV;
> - else
> - return rc;
> - }
> + if (rc)
> + return rc;
>
> cnt = 0;
> chip->bios_dir[cnt] = securityfs_create_dir(name, NULL);
> diff --git a/drivers/char/tpm/tpm_of.c b/drivers/char/tpm/tpm_of.c
> index 36df9df4c472b9..7dee42d7b5e05c 100644
> --- a/drivers/char/tpm/tpm_of.c
> +++ b/drivers/char/tpm/tpm_of.c
> @@ -29,13 +29,16 @@ 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;
>
> sizep = of_get_property(np, "linux,sml-size", NULL);
> - if (sizep == NULL)
> + basep = of_get_property(np, "linux,sml-base", NULL);
> + if (sizep == NULL && basep == NULL)
> + return -ENODEV;
> + if (sizep == NULL || basep == NULL)
> return -EIO;
To confirm my understanding,
For -ENODEV, it means that both properties are not supported, so event
log is not supported.
For -EIO , it means that event log is supported but there is some
failure in getting one of them, so should fail the probe.
Is my understanding right ?
Thanks & Regards,
- Nayna
>
> if (*sizep == 0) {
> @@ -43,10 +46,6 @@ int tpm_read_log_of(struct tpm_chip *chip)
> return -EIO;
> }
>
> - basep = of_get_property(np, "linux,sml-base", NULL);
> - if (basep == NULL)
> - return -EIO;
> -
> log->bios_event_log = kmalloc(*sizep, GFP_KERNEL);
> if (!log->bios_event_log)
> return -ENOMEM;
>
------------------------------------------------------------------------------
_______________________________________________
tpmdd-devel mailing list
tpmdd-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/tpmdd-devel
