[PATCH 1/2] tpm: Check for parent device being NULL

[PATCH 1/2] tpm: Check for parent device being NULL

[Stefan Berger]

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 <stefanb-23VcF4HTsmIX0ybBhKVfKdBPR1lH4CV8@public.gmane.org>
---
 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


------------------------------------------------------------------------------

[PATCH 2/2] tpm: Fix error code handling after tpm_bios_log_setup

[Stefan Berger]

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 <stefanb-23VcF4HTsmIX0ybBhKVfKdBPR1lH4CV8@public.gmane.org>
---
 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


------------------------------------------------------------------------------

Re: [PATCH 2/2] tpm: Fix error code handling after tpm_bios_log_setup

[Nayna]

On 11/18/2016 06:00 AM, 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 <stefanb-23VcF4HTsmIX0ybBhKVfKdBPR1lH4CV8@public.gmane.org>
> ---
>   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;

This will return even for (rc == 0).

Thanks & Regards,
   - Nayna

>
>   	tpm_add_ppi(chip);
>


------------------------------------------------------------------------------

Re: [PATCH 2/2] tpm: Fix error code handling after tpm_bios_log_setup

[Jarkko Sakkinen]

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 <stefanb@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
> --- 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

CC to linux-security-module

LGTM

Reviewed-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>

/Jarkko

Re: [PATCH 2/2] tpm: Fix error code handling after tpm_bios_log_setup

[Jason Gunthorpe]

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 <stefanb@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.sakkinen@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 <jgunthorpe@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 <jgunthorpe@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;
 
 	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);
-	if ((rc == 0) || (rc == -ENOMEM))
+	if (rc != -ENODEV)
 		return rc;
 
-	rc = tpm_read_log_of(chip);
-
-	return rc;
+	return tpm_read_log_of(chip);
 }
 
+/*
+ * 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;
 
 	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;
-- 
2.7.4

Re: [PATCH 2/2] tpm: Fix error code handling after tpm_bios_log_setup

[Jarkko Sakkinen]

On Sat, Nov 19, 2016 at 11:22:28AM -0700, 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 <stefanb@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.sakkinen@linux.intel.com>
> 
> Erm, what about rc == 0? And all the other problems?

Sorry my bad. I was not thinking clearly.

This whole situation looks like a mess. I gave a lot of thought on this
during my plane trips.

> 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 :/

Yup. The reason I'm not weighting that yet so much is that I do not know
the environment.

> 
> From 8768bcb8cd2a5a17cc4d811a9298b20c3a2c0884 Mon Sep 17 00:00:00 2001
> From: Jason Gunthorpe <jgunthorpe@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 <jgunthorpe@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;
>  
>  	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);
> -	if ((rc == 0) || (rc == -ENOMEM))
> +	if (rc != -ENODEV)
>  		return rc;
>  
> -	rc = tpm_read_log_of(chip);
> -
> -	return rc;
> +	return tpm_read_log_of(chip);
>  }
>  
> +/*
> + * 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;
> -	}

WTF. I really have to be much more focused when I looked this. That
is more than wrong... Too much multitasking last couple of weeks. That's
my excuse...

I can consider putting the patch set to the next release but I really
would want yet another version with change log what fixes were done and
why.

/Jarkko

Re: [PATCH 2/2] tpm: Fix error code handling after tpm_bios_log_setup

[Jarkko Sakkinen]

On Sun, Nov 20, 2016 at 09:46:25AM +0000, Jarkko Sakkinen wrote:
> On Sat, Nov 19, 2016 at 11:22:28AM -0700, 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 <stefanb@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.sakkinen@linux.intel.com>
> > 
> > Erm, what about rc == 0? And all the other problems?
> 
> Sorry my bad. I was not thinking clearly.
> 
> This whole situation looks like a mess. I gave a lot of thought on this
> during my plane trips.
> 
> > 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 :/
> 
> Yup. The reason I'm not weighting that yet so much is that I do not know
> the environment.
> 
> > 
> > From 8768bcb8cd2a5a17cc4d811a9298b20c3a2c0884 Mon Sep 17 00:00:00 2001
> > From: Jason Gunthorpe <jgunthorpe@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 <jgunthorpe@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;
> >  
> >  	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);
> > -	if ((rc == 0) || (rc == -ENOMEM))
> > +	if (rc != -ENODEV)
> >  		return rc;
> >  
> > -	rc = tpm_read_log_of(chip);
> > -
> > -	return rc;
> > +	return tpm_read_log_of(chip);
> >  }
> >  
> > +/*
> > + * 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;
> > -	}
> 
> WTF. I really have to be much more focused when I looked this. That
> is more than wrong... Too much multitasking last couple of weeks. That's
> my excuse...
> 
> I can consider putting the patch set to the next release but I really
> would want yet another version with change log what fixes were done and
> why.

I mean one more version of the patch set. Otherwise, I probably just
postpone the whole patch set to 4.10.

/Jarkko

Re: [PATCH 2/2] tpm: Fix error code handling after tpm_bios_log_setup

[Jarkko Sakkinen]

I was able to apply the patch after manually editing the mbox file.
Not nice. It'd be better to send these things with git-send-email
to simplify the mgmt (like getting them into patchwork for starters)
especially in a situation that is bit chaotic to begin with :-/

It's there. I'm not yet sure what to do with these changes. I'll
probably move my pull request towards late next week. Lets see if
things stabilize. Too early to decide.

In any case I won't squash the fixes to existing series even if I
include them to the pull request. I'll add new tested-by's only to
the fixes.

/Jarkko

On Sat, Nov 19, 2016 at 11:22:28AM -0700, 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 <stefanb@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.sakkinen@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 <jgunthorpe@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 <jgunthorpe@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;
>  
>  	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);
> -	if ((rc == 0) || (rc == -ENOMEM))
> +	if (rc != -ENODEV)
>  		return rc;
>  
> -	rc = tpm_read_log_of(chip);
> -
> -	return rc;
> +	return tpm_read_log_of(chip);
>  }
>  
> +/*
> + * 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;
>  
>  	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;
> -- 
> 2.7.4
> 

Re: [PATCH 2/2] tpm: Fix error code handling after tpm_bios_log_setup

[Jason Gunthorpe]

On Sun, Nov 20, 2016 at 12:14:51PM +0000, Jarkko Sakkinen wrote:
> I was able to apply the patch after manually editing the mbox file.
> Not nice. It'd be better to send these things with git-send-email
> to simplify the mgmt (like getting them into patchwork for starters)
> especially in a situation that is bit chaotic to begin with :-/

It is in patchwork just fine:

https://patchwork.kernel.org/patch/9438323/

git am is unfortunately much more limited than patchwork, but you can
apply right out of patchwork.

You forgot to add your signed-off-by though.

And this one fixes the sysfs problem

https://patchwork.kernel.org/patch/9438331/

Here is a commit message:

tpm: Do not call tpm_chip_unregister if register has not succeeded

This is no longer allowed, vtpm was relying on the check inside the
tpm core which was removed. Fixes an oops during error unwind.

Fixes: a8ed92219a04 ("tpm: Get rid of TPM_CHIP_FLAG_REGISTERED")
Signed-off-by: Jason Gunthorpe <jgunthorpe-ePGOBjL8dl3ta4EC/59zMFaTQe2KTcn/@public.gmane.org>
Tested-by: Stefan Berger <stefanb-23VcF4HTsmIX0ybBhKVfKdBPR1lH4CV8@public.gmane.org>

That just leaves the ACPI thing..

Jason

------------------------------------------------------------------------------

Re: [PATCH 2/2] tpm: Fix error code handling after tpm_bios_log_setup

[Jarkko Sakkinen]

On Mon, Nov 21, 2016 at 10:55:20AM -0700, Jason Gunthorpe wrote:
> On Sun, Nov 20, 2016 at 12:14:51PM +0000, Jarkko Sakkinen wrote:
> > I was able to apply the patch after manually editing the mbox file.
> > Not nice. It'd be better to send these things with git-send-email
> > to simplify the mgmt (like getting them into patchwork for starters)
> > especially in a situation that is bit chaotic to begin with :-/
> 
> It is in patchwork just fine:
> 
> https://patchwork.kernel.org/patch/9438323/
> 
> git am is unfortunately much more limited than patchwork, but you can
> apply right out of patchwork.
> 
> You forgot to add your signed-off-by though.

It's not forgotten. I'm yet to test it properly. I just quickly wanted
to make it available...

/Jarkko

> 
> And this one fixes the sysfs problem
> 
> https://patchwork.kernel.org/patch/9438331/
> 
> Here is a commit message:
> 
> tpm: Do not call tpm_chip_unregister if register has not succeeded
> 
> This is no longer allowed, vtpm was relying on the check inside the
> tpm core which was removed. Fixes an oops during error unwind.
> 
> Fixes: a8ed92219a04 ("tpm: Get rid of TPM_CHIP_FLAG_REGISTERED")
> Signed-off-by: Jason Gunthorpe <jgunthorpe@obsidianresearch.com>
> Tested-by: Stefan Berger <stefanb@linux.vnet.ibm.com>
> 
> That just leaves the ACPI thing..
> 
> Jason

Re: [PATCH 2/2] tpm: Fix error code handling after tpm_bios_log_setup

[Jarkko Sakkinen]

On Mon, Nov 21, 2016 at 10:55:20AM -0700, Jason Gunthorpe wrote:
> On Sun, Nov 20, 2016 at 12:14:51PM +0000, Jarkko Sakkinen wrote:
> > I was able to apply the patch after manually editing the mbox file.
> > Not nice. It'd be better to send these things with git-send-email
> > to simplify the mgmt (like getting them into patchwork for starters)
> > especially in a situation that is bit chaotic to begin with :-/
> 
> It is in patchwork just fine:
> 
> https://patchwork.kernel.org/patch/9438323/
> 
> git am is unfortunately much more limited than patchwork, but you can
> apply right out of patchwork.
> 
> You forgot to add your signed-off-by though.
> 
> And this one fixes the sysfs problem
> 
> https://patchwork.kernel.org/patch/9438331/
> 
> Here is a commit message:
> 
> tpm: Do not call tpm_chip_unregister if register has not succeeded
> 
> This is no longer allowed, vtpm was relying on the check inside the
> tpm core which was removed. Fixes an oops during error unwind.
> 
> Fixes: a8ed92219a04 ("tpm: Get rid of TPM_CHIP_FLAG_REGISTERED")
> Signed-off-by: Jason Gunthorpe <jgunthorpe-ePGOBjL8dl3ta4EC/59zMFaTQe2KTcn/@public.gmane.org>
> Tested-by: Stefan Berger <stefanb-23VcF4HTsmIX0ybBhKVfKdBPR1lH4CV8@public.gmane.org>
> 
> That just leaves the ACPI thing..

I like the original commit message more. It documents the change.

/Jarkko
> 
> Jason

------------------------------------------------------------------------------

Re: [PATCH 2/2] tpm: Fix error code handling after tpm_bios_log_setup

[Jarkko Sakkinen]

On Mon, Nov 21, 2016 at 10:32:44PM +0200, Jarkko Sakkinen wrote:
> On Mon, Nov 21, 2016 at 10:55:20AM -0700, Jason Gunthorpe wrote:
> > On Sun, Nov 20, 2016 at 12:14:51PM +0000, Jarkko Sakkinen wrote:
> > > I was able to apply the patch after manually editing the mbox file.
> > > Not nice. It'd be better to send these things with git-send-email
> > > to simplify the mgmt (like getting them into patchwork for starters)
> > > especially in a situation that is bit chaotic to begin with :-/
> > 
> > It is in patchwork just fine:
> > 
> > https://patchwork.kernel.org/patch/9438323/
> > 
> > git am is unfortunately much more limited than patchwork, but you can
> > apply right out of patchwork.
> > 
> > You forgot to add your signed-off-by though.
> > 
> > And this one fixes the sysfs problem
> > 
> > https://patchwork.kernel.org/patch/9438331/
> > 
> > Here is a commit message:
> > 
> > tpm: Do not call tpm_chip_unregister if register has not succeeded
> > 
> > This is no longer allowed, vtpm was relying on the check inside the
> > tpm core which was removed. Fixes an oops during error unwind.
> > 
> > Fixes: a8ed92219a04 ("tpm: Get rid of TPM_CHIP_FLAG_REGISTERED")
> > Signed-off-by: Jason Gunthorpe <jgunthorpe@obsidianresearch.com>
> > Tested-by: Stefan Berger <stefanb@linux.vnet.ibm.com>
> > 
> > That just leaves the ACPI thing..
> 
> I like the original commit message more. It documents the change.

I added to the commits reviewed-by's and signed-off-by's as I agree on
the correctness of the changes. I'll do more formal testing before
adding testd-by...

/Jarkko

Re: [PATCH 2/2] tpm: Fix error code handling after tpm_bios_log_setup

[Jason Gunthorpe]

On Mon, Nov 21, 2016 at 10:32:44PM +0200, Jarkko Sakkinen wrote:

> > And this one fixes the sysfs problem
> > 
> > https://patchwork.kernel.org/patch/9438331/
> > 
> > Here is a commit message:
> > 
> > tpm: Do not call tpm_chip_unregister if register has not succeeded
> > 
> > This is no longer allowed, vtpm was relying on the check inside the
> > tpm core which was removed. Fixes an oops during error unwind.
> > 
> > Fixes: a8ed92219a04 ("tpm: Get rid of TPM_CHIP_FLAG_REGISTERED")
> > Signed-off-by: Jason Gunthorpe <jgunthorpe@obsidianresearch.com>
> > Tested-by: Stefan Berger <stefanb@linux.vnet.ibm.com>
> > 
> > That just leaves the ACPI thing..
> 
> I like the original commit message more. It documents the change.

??

This is for  https://patchwork.kernel.org/patch/9438331/ which didn't
have a message yet.

Jason

Re: [PATCH 2/2] tpm: Fix error code handling after tpm_bios_log_setup

[Jarkko Sakkinen]

On Mon, Nov 21, 2016 at 01:37:08PM -0700, Jason Gunthorpe wrote:
> On Mon, Nov 21, 2016 at 10:32:44PM +0200, Jarkko Sakkinen wrote:
> 
> > > And this one fixes the sysfs problem
> > > 
> > > https://patchwork.kernel.org/patch/9438331/
> > > 
> > > Here is a commit message:
> > > 
> > > tpm: Do not call tpm_chip_unregister if register has not succeeded
> > > 
> > > This is no longer allowed, vtpm was relying on the check inside the
> > > tpm core which was removed. Fixes an oops during error unwind.
> > > 
> > > Fixes: a8ed92219a04 ("tpm: Get rid of TPM_CHIP_FLAG_REGISTERED")
> > > Signed-off-by: Jason Gunthorpe <jgunthorpe-ePGOBjL8dl3ta4EC/59zMFaTQe2KTcn/@public.gmane.org>
> > > Tested-by: Stefan Berger <stefanb-23VcF4HTsmIX0ybBhKVfKdBPR1lH4CV8@public.gmane.org>
> > > 
> > > That just leaves the ACPI thing..
> > 
> > I like the original commit message more. It documents the change.
> 
> ??
> 
> This is for  https://patchwork.kernel.org/patch/9438331/ which didn't
> have a message yet.

This was the commit message when I did git am:

    tpm: vtpm_proxy: Do not access host's event log
    
    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]  [<ffffffff8155bd32>] dpm_sysfs_remove+0x22/0x60
    > [   58.271017]  [<ffffffff8154e438>] device_del+0x58/0x280
    > [   58.271017]  [<ffffffffa024c020>] tpm_chip_unregister+0x40/0xb0 [tpm]
    > [   58.271017]  [<ffffffffa0292360>] 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?

/Jarkko

------------------------------------------------------------------------------

Re: [PATCH 2/2] tpm: Fix error code handling after tpm_bios_log_setup

[Jason Gunthorpe]

On Mon, Nov 21, 2016 at 10:46:10PM +0200, Jarkko Sakkinen wrote:
> On Mon, Nov 21, 2016 at 01:37:08PM -0700, Jason Gunthorpe wrote:
> > On Mon, Nov 21, 2016 at 10:32:44PM +0200, Jarkko Sakkinen wrote:
> > 
> > > > And this one fixes the sysfs problem
> > > > 
> > > > https://patchwork.kernel.org/patch/9438331/
> > > > 
> > > > Here is a commit message:
> > > > 
> > > > tpm: Do not call tpm_chip_unregister if register has not succeeded
> > > > 
> > > > This is no longer allowed, vtpm was relying on the check inside the
> > > > tpm core which was removed. Fixes an oops during error unwind.
> > > > 
> > > > Fixes: a8ed92219a04 ("tpm: Get rid of TPM_CHIP_FLAG_REGISTERED")
> > > > Signed-off-by: Jason Gunthorpe <jgunthorpe@obsidianresearch.com>
> > > > Tested-by: Stefan Berger <stefanb@linux.vnet.ibm.com>
> > > > 
> > > > That just leaves the ACPI thing..
> > > 
> > > I like the original commit message more. It documents the change.
> > 
> > ??
> > 
> > This is for  https://patchwork.kernel.org/patch/9438331/ which didn't
> > have a message yet.
> 
> This was the commit message when I did git am:

Well, that is just a conversation from email, not a commit message,
you need to edit it. The above I gave you is much better.

Jason

Re: [PATCH 2/2] tpm: Fix error code handling after tpm_bios_log_setup

[Nayna]

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 <stefanb@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.sakkinen@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 <jgunthorpe@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 <jgunthorpe@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;
>

Re: [PATCH 2/2] tpm: Fix error code handling after tpm_bios_log_setup

[Jason Gunthorpe]

On Mon, Nov 21, 2016 at 12:06:20AM +0530, Nayna wrote:
> >  	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:

That is fine, we should never read the log twice.

> >index fb603a74cbd29e..2a15b866ac257a 100644
> >+++ 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.

No, if acpi is -ENOMEM then 'if (rc != -ENODEV)' is true and it
returns -ENOMEM.

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

Yes

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

Yes

Jason

Re: [PATCH 2/2] tpm: Fix error code handling after tpm_bios_log_setup

[Jarkko Sakkinen]

On Mon, Nov 21, 2016 at 10:15:30AM -0700, Jason Gunthorpe wrote:
> On Mon, Nov 21, 2016 at 12:06:20AM +0530, Nayna wrote:
> > >  	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:
> 
> That is fine, we should never read the log twice.

I just went through tpm_bios_log_setup(), tpm_read_log_of() and
tpm_read_log_acpi(). The error handling is sound now but the condition
should be in tpm_bios_log_setup(). Not in tpm_chip_register().

It is applied but if you don't mind I would like make a small commit
that moves the condition to that function.

I think tpm_chip_register() should only care when something went wrong.

/Jarkko

Re: [PATCH 2/2] tpm: Fix error code handling after tpm_bios_log_setup

[Jason Gunthorpe]

On Mon, Nov 21, 2016 at 10:29:15PM +0200, Jarkko Sakkinen wrote:

> I just went through tpm_bios_log_setup(), tpm_read_log_of() and
> tpm_read_log_acpi(). The error handling is sound now but the condition
> should be in tpm_bios_log_setup(). Not in tpm_chip_register().
> 
> It is applied but if you don't mind I would like make a small commit
> that moves the condition to that function.

I'm not particularly concerned where the check lives..

Jason

Re: [PATCH 2/2] tpm: Fix error code handling after tpm_bios_log_setup

[Jarkko Sakkinen]

On Tue, Nov 22, 2016 at 09:37:20AM -0700, Jason Gunthorpe wrote:
> On Mon, Nov 21, 2016 at 10:29:15PM +0200, Jarkko Sakkinen wrote:
> 
> > I just went through tpm_bios_log_setup(), tpm_read_log_of() and
> > tpm_read_log_acpi(). The error handling is sound now but the condition
> > should be in tpm_bios_log_setup(). Not in tpm_chip_register().
> > 
> > It is applied but if you don't mind I would like make a small commit
> > that moves the condition to that function.
> 
> I'm not particularly concerned where the check lives..

It's just a minor glitch but still an obvious inconsistency. Maybe
I'll ignore for th moment as everything is well tested.

There is still two commits in the series that are untested and
unreviewed.

/Jarkko

------------------------------------------------------------------------------

Re: [PATCH 2/2] tpm: Fix error code handling after tpm_bios_log_setup

[Nayna]

On 11/21/2016 10:45 PM, Jason Gunthorpe wrote:
> On Mon, Nov 21, 2016 at 12:06:20AM +0530, Nayna wrote:
>>>   	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:
>
> That is fine, we should never read the log twice.
>
>>> index fb603a74cbd29e..2a15b866ac257a 100644
>>> +++ 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.
>
> No, if acpi is -ENOMEM then 'if (rc != -ENODEV)' is true and it
> returns -ENOMEM.

Yeah, I missed it. Sorry.
>
>>>   	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.
>
> Yes
>
>> 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 ?
>
> Yes

Thanks for explaining. Looks good now.

Thanks & Regards,
   - Nayna

>
> Jason
>

Re: [PATCH 1/2] tpm: Check for parent device being NULL

[Jarkko Sakkinen]

On Thu, Nov 17, 2016 at 07:30:03PM -0500, Stefan Berger wrote:
> 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 <stefanb-23VcF4HTsmIX0ybBhKVfKdBPR1lH4CV8@public.gmane.org>
> ---
>  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

CC to linux-security-module

LGTM

Reviewed-by: Jarkko Sakkinen <jarkko.sakkinen-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>

/Jarkko

------------------------------------------------------------------------------