tpmdd-devel.lists.sourceforge.net archive mirror
 help / color / mirror / Atom feed
* [PATCH 1/3] tpm-chip: Move idr_replace calls to appropriate places
@ 2017-07-04 13:56 Alexander Steffen
       [not found] ` <20170704135648.7360-1-Alexander.Steffen-d0qZbvYSIPpWk0Htik3J/w@public.gmane.org>
  0 siblings, 1 reply; 5+ messages in thread
From: Alexander Steffen @ 2017-07-04 13:56 UTC (permalink / raw)
  To: tpmdd-devel-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f

According to the comments, adding/removing the chip from the list should be
the first/last action in (un)register. But currently it is done in a
subfunction in the middle of the process. Moving the code from the
subfunctions to the appropriate places within (un)register ensures that the
code matches the comments.

Signed-off-by: Alexander Steffen <Alexander.Steffen-d0qZbvYSIPpWk0Htik3J/w@public.gmane.org>
---
 drivers/char/tpm/tpm-chip.c | 20 ++++++++++----------
 1 file changed, 10 insertions(+), 10 deletions(-)

diff --git a/drivers/char/tpm/tpm-chip.c b/drivers/char/tpm/tpm-chip.c
index 67ec9d3..a353b7a 100644
--- a/drivers/char/tpm/tpm-chip.c
+++ b/drivers/char/tpm/tpm-chip.c
@@ -327,11 +327,6 @@ static int tpm_add_char_device(struct tpm_chip *chip)
 		}
 	}
 
-	/* Make the chip available. */
-	mutex_lock(&idr_lock);
-	idr_replace(&dev_nums_idr, chip, chip->dev_num);
-	mutex_unlock(&idr_lock);
-
 	return rc;
 }
 
@@ -339,11 +334,6 @@ static void tpm_del_char_device(struct tpm_chip *chip)
 {
 	cdev_device_del(&chip->cdev, &chip->dev);
 
-	/* Make the chip unavailable. */
-	mutex_lock(&idr_lock);
-	idr_replace(&dev_nums_idr, NULL, chip->dev_num);
-	mutex_unlock(&idr_lock);
-
 	/* Make the driver uncallable. */
 	down_write(&chip->ops_sem);
 	if (chip->flags & TPM_CHIP_FLAG_TPM2)
@@ -438,6 +428,11 @@ int tpm_chip_register(struct tpm_chip *chip)
 		return rc;
 	}
 
+	/* Make the chip available. */
+	mutex_lock(&idr_lock);
+	idr_replace(&dev_nums_idr, chip, chip->dev_num);
+	mutex_unlock(&idr_lock);
+
 	return 0;
 }
 EXPORT_SYMBOL_GPL(tpm_chip_register);
@@ -457,6 +452,11 @@ EXPORT_SYMBOL_GPL(tpm_chip_register);
  */
 void tpm_chip_unregister(struct tpm_chip *chip)
 {
+	/* Make the chip unavailable. */
+	mutex_lock(&idr_lock);
+	idr_replace(&dev_nums_idr, NULL, chip->dev_num);
+	mutex_unlock(&idr_lock);
+
 	tpm_del_legacy_sysfs(chip);
 	tpm_bios_log_teardown(chip);
 	if (chip->flags & TPM_CHIP_FLAG_TPM2)
-- 
2.7.4


------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot

^ permalink raw reply	[flat|nested] 5+ messages in thread

* [PATCH 2/3] tpm-chip: Return TPM error codes from auto_startup functions
       [not found] ` <20170704135648.7360-1-Alexander.Steffen-d0qZbvYSIPpWk0Htik3J/w@public.gmane.org>
@ 2017-07-04 13:56   ` Alexander Steffen
  2017-07-04 13:56   ` [PATCH 3/3] tpm-chip: Export TPM device to user space even when startup failed Alexander Steffen
  2017-07-09 21:24   ` [PATCH 1/3] tpm-chip: Move idr_replace calls to appropriate places Jason Gunthorpe
  2 siblings, 0 replies; 5+ messages in thread
From: Alexander Steffen @ 2017-07-04 13:56 UTC (permalink / raw)
  To: tpmdd-devel-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f

The auto_startup functions for TPM1 and TPM2 convert all TPM error codes to
ENODEV on exit. But since there is only one caller for those functions,
this code can be consolidated within the caller. Additionally, this allows
the caller to distinguish between a TPM being present (but returning error
responses for some commands) and no TPM being present (or the TPM
malfunctioning completely).

Signed-off-by: Alexander Steffen <Alexander.Steffen-d0qZbvYSIPpWk0Htik3J/w@public.gmane.org>
---
 drivers/char/tpm/tpm-chip.c      | 4 +++-
 drivers/char/tpm/tpm-interface.c | 6 ++----
 drivers/char/tpm/tpm2-cmd.c      | 5 ++---
 3 files changed, 7 insertions(+), 8 deletions(-)

diff --git a/drivers/char/tpm/tpm-chip.c b/drivers/char/tpm/tpm-chip.c
index a353b7a..f20fcb7 100644
--- a/drivers/char/tpm/tpm-chip.c
+++ b/drivers/char/tpm/tpm-chip.c
@@ -404,8 +404,10 @@ int tpm_chip_register(struct tpm_chip *chip)
 			rc = tpm2_auto_startup(chip);
 		else
 			rc = tpm1_auto_startup(chip);
-		if (rc)
+		if (rc < 0)
 			return rc;
+		else if (rc > 0)
+			return -ENODEV;
 	}
 
 	tpm_sysfs_add_device(chip);
diff --git a/drivers/char/tpm/tpm-interface.c b/drivers/char/tpm/tpm-interface.c
index 3123a6e..4784ccc 100644
--- a/drivers/char/tpm/tpm-interface.c
+++ b/drivers/char/tpm/tpm-interface.c
@@ -997,7 +997,8 @@ EXPORT_SYMBOL_GPL(tpm_do_selftest);
  *                     sequence
  * @chip: TPM chip to use
  *
- * Returns 0 on success, < 0 in case of fatal error.
+ * Returns 0 on success, < 0 in case of fatal error or a value > 0 representing
+ * a TPM error code.
  */
 int tpm1_auto_startup(struct tpm_chip *chip)
 {
@@ -1012,10 +1013,7 @@ int tpm1_auto_startup(struct tpm_chip *chip)
 		goto out;
 	}
 
-	return rc;
 out:
-	if (rc > 0)
-		rc = -ENODEV;
 	return rc;
 }
 
diff --git a/drivers/char/tpm/tpm2-cmd.c b/drivers/char/tpm/tpm2-cmd.c
index f7f34b2a..0a64714 100644
--- a/drivers/char/tpm/tpm2-cmd.c
+++ b/drivers/char/tpm/tpm2-cmd.c
@@ -1074,7 +1074,8 @@ static int tpm2_get_cc_attrs_tbl(struct tpm_chip *chip)
  *                     sequence
  * @chip: TPM chip to use
  *
- * Returns 0 on success, < 0 in case of fatal error.
+ * Returns 0 on success, < 0 in case of fatal error or a value > 0 representing
+ * a TPM error code.
  */
 int tpm2_auto_startup(struct tpm_chip *chip)
 {
@@ -1109,8 +1110,6 @@ int tpm2_auto_startup(struct tpm_chip *chip)
 	rc = tpm2_get_cc_attrs_tbl(chip);
 
 out:
-	if (rc > 0)
-		rc = -ENODEV;
 	return rc;
 }
 
-- 
2.7.4


------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot

^ permalink raw reply	[flat|nested] 5+ messages in thread

* [PATCH 3/3] tpm-chip: Export TPM device to user space even when startup failed
       [not found] ` <20170704135648.7360-1-Alexander.Steffen-d0qZbvYSIPpWk0Htik3J/w@public.gmane.org>
  2017-07-04 13:56   ` [PATCH 2/3] tpm-chip: Return TPM error codes from auto_startup functions Alexander Steffen
@ 2017-07-04 13:56   ` Alexander Steffen
  2017-07-09 21:24   ` [PATCH 1/3] tpm-chip: Move idr_replace calls to appropriate places Jason Gunthorpe
  2 siblings, 0 replies; 5+ messages in thread
From: Alexander Steffen @ 2017-07-04 13:56 UTC (permalink / raw)
  To: tpmdd-devel-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f

When one of the commands during the auto_startup sequences does not return
TPM_RC_SUCCESS, tpm_chip_register misleadingly returns ENODEV, even though
a TPM device is definitely present.

An error response during those sequences is indeed unexpected, so to
prevent subsequent errors, the kernel should not make use of the TPM
device. But user space applications still might be able to communicate with
the TPM, so they can be used to further diagnose and/or fix the problem. To
allow this, with this patch the device is still exported to user space,
even if a TPM error code has been received, but the kernel itself will not
be allowed to use the device for anything.

This is not a hypothetical scenario, but there are devices in the wild that
show this behavior. With this fix, those devices can be recovered from
their failed state.

Signed-off-by: Alexander Steffen <Alexander.Steffen-d0qZbvYSIPpWk0Htik3J/w@public.gmane.org>
---
 drivers/char/tpm/tpm-chip.c | 15 +++++++++------
 1 file changed, 9 insertions(+), 6 deletions(-)

diff --git a/drivers/char/tpm/tpm-chip.c b/drivers/char/tpm/tpm-chip.c
index f20fcb7..a4baa56 100644
--- a/drivers/char/tpm/tpm-chip.c
+++ b/drivers/char/tpm/tpm-chip.c
@@ -390,7 +390,7 @@ static int tpm_add_legacy_sysfs(struct tpm_chip *chip)
  *
  * Creates a character device for the TPM chip and adds sysfs attributes for
  * the device. As the last step this function adds the chip to the list of TPM
- * chips available for in-kernel use.
+ * chips available for in-kernel use, if the TPM startup was successful.
  *
  * This function should be only called after the chip initialization is
  * complete.
@@ -398,6 +398,7 @@ static int tpm_add_legacy_sysfs(struct tpm_chip *chip)
 int tpm_chip_register(struct tpm_chip *chip)
 {
 	int rc;
+	bool startup_successful = true;
 
 	if (chip->ops->flags & TPM_OPS_AUTO_STARTUP) {
 		if (chip->flags & TPM_CHIP_FLAG_TPM2)
@@ -407,7 +408,7 @@ int tpm_chip_register(struct tpm_chip *chip)
 		if (rc < 0)
 			return rc;
 		else if (rc > 0)
-			return -ENODEV;
+			startup_successful = false;
 	}
 
 	tpm_sysfs_add_device(chip);
@@ -430,10 +431,12 @@ int tpm_chip_register(struct tpm_chip *chip)
 		return rc;
 	}
 
-	/* Make the chip available. */
-	mutex_lock(&idr_lock);
-	idr_replace(&dev_nums_idr, chip, chip->dev_num);
-	mutex_unlock(&idr_lock);
+	if (startup_successful) {
+		/* Make the chip available. */
+		mutex_lock(&idr_lock);
+		idr_replace(&dev_nums_idr, chip, chip->dev_num);
+		mutex_unlock(&idr_lock);
+	}
 
 	return 0;
 }
-- 
2.7.4


------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH 1/3] tpm-chip: Move idr_replace calls to appropriate places
       [not found] ` <20170704135648.7360-1-Alexander.Steffen-d0qZbvYSIPpWk0Htik3J/w@public.gmane.org>
  2017-07-04 13:56   ` [PATCH 2/3] tpm-chip: Return TPM error codes from auto_startup functions Alexander Steffen
  2017-07-04 13:56   ` [PATCH 3/3] tpm-chip: Export TPM device to user space even when startup failed Alexander Steffen
@ 2017-07-09 21:24   ` Jason Gunthorpe
       [not found]     ` <20170709212430.GB19327-ePGOBjL8dl3ta4EC/59zMFaTQe2KTcn/@public.gmane.org>
  2 siblings, 1 reply; 5+ messages in thread
From: Jason Gunthorpe @ 2017-07-09 21:24 UTC (permalink / raw)
  To: Alexander Steffen; +Cc: tpmdd-devel-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f

On Tue, Jul 04, 2017 at 03:56:46PM +0200, Alexander Steffen wrote:
> According to the comments, adding/removing the chip from the list should be
> the first/last action in (un)register.

The comments are misleading..

> diff --git a/drivers/char/tpm/tpm-chip.c b/drivers/char/tpm/tpm-chip.c
> index 67ec9d3..a353b7a 100644
> +++ b/drivers/char/tpm/tpm-chip.c
> @@ -327,11 +327,6 @@ static int tpm_add_char_device(struct tpm_chip *chip)
>  		}
>  	}
>  
> -	/* Make the chip available. */
> -	mutex_lock(&idr_lock);
> -	idr_replace(&dev_nums_idr, chip, chip->dev_num);
> -	mutex_unlock(&idr_lock);

This is actually in the wrong place already, it needs to be done
before cdev_device_add - this is because cdev_device_add generates the
uevent to userspace which could trigger userspace to use the kernel
device. So a patch to move it to the start of this function woud
be good. The function would be better described as 'make visible'

Maybe resend this patch with only that change..

>  {
>  	cdev_device_del(&chip->cdev, &chip->dev);
>  
> -	/* Make the chip unavailable. */
> -	mutex_lock(&idr_lock);
> -	idr_replace(&dev_nums_idr, NULL, chip->dev_num);
> -	mutex_unlock(&idr_lock);
> -

The placement of this does not matter so much, but keeping it after
the cdev_device_del is easier to understand as it matches the
(corrected) setup order..

Jason

------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH 1/3] tpm-chip: Move idr_replace calls to appropriate places
       [not found]     ` <20170709212430.GB19327-ePGOBjL8dl3ta4EC/59zMFaTQe2KTcn/@public.gmane.org>
@ 2017-07-27 14:07       ` Alexander.Steffen-d0qZbvYSIPpWk0Htik3J/w
  0 siblings, 0 replies; 5+ messages in thread
From: Alexander.Steffen-d0qZbvYSIPpWk0Htik3J/w @ 2017-07-27 14:07 UTC (permalink / raw)
  To: jgunthorpe-ePGOBjL8dl3ta4EC/59zMFaTQe2KTcn/
  Cc: tpmdd-devel-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f

> > diff --git a/drivers/char/tpm/tpm-chip.c b/drivers/char/tpm/tpm-chip.c
> > index 67ec9d3..a353b7a 100644
> > +++ b/drivers/char/tpm/tpm-chip.c
> > @@ -327,11 +327,6 @@ static int tpm_add_char_device(struct tpm_chip
> *chip)
> >  		}
> >  	}
> >
> > -	/* Make the chip available. */
> > -	mutex_lock(&idr_lock);
> > -	idr_replace(&dev_nums_idr, chip, chip->dev_num);
> > -	mutex_unlock(&idr_lock);
> 
> This is actually in the wrong place already, it needs to be done before
> cdev_device_add - this is because cdev_device_add generates the uevent to
> userspace which could trigger userspace to use the kernel device. So a patch
> to move it to the start of this function woud be good. The function would be
> better described as 'make visible'

I have looked again at the code and I am not sure this is an issue. The call to idr_replace is only necessary to enable in-kernel usage (i.e. RNG, IMA, ...) of the TPM, but it should not affect userspace in any way. So the location of the idr_replace call does not matter much, as long as the TPM is already initialized. In fact, the main purpose of this patch series (please see PATCH 3/3) is to export the device to userspace without calling idr_replace at all under some circumstances.

Or is there something I missed? The only function that ever tries to access the value stored by idr_replace is tpm_chip_find_get. It is usually called with TPM_ANY_NUM, selecting any TPM that might be present. If no TPM is present (or if idr_replace has not been called) the caller needs to deal with the situation already.

Alexander
------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot

^ permalink raw reply	[flat|nested] 5+ messages in thread

end of thread, other threads:[~2017-07-27 14:07 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-07-04 13:56 [PATCH 1/3] tpm-chip: Move idr_replace calls to appropriate places Alexander Steffen
     [not found] ` <20170704135648.7360-1-Alexander.Steffen-d0qZbvYSIPpWk0Htik3J/w@public.gmane.org>
2017-07-04 13:56   ` [PATCH 2/3] tpm-chip: Return TPM error codes from auto_startup functions Alexander Steffen
2017-07-04 13:56   ` [PATCH 3/3] tpm-chip: Export TPM device to user space even when startup failed Alexander Steffen
2017-07-09 21:24   ` [PATCH 1/3] tpm-chip: Move idr_replace calls to appropriate places Jason Gunthorpe
     [not found]     ` <20170709212430.GB19327-ePGOBjL8dl3ta4EC/59zMFaTQe2KTcn/@public.gmane.org>
2017-07-27 14:07       ` Alexander.Steffen-d0qZbvYSIPpWk0Htik3J/w

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).