[PATCH RESEND 0/3] Export broken TPMs to user space

[PATCH RESEND 0/3] Export broken TPMs to user space

[Alexander Steffen]

TPMs may break sometimes, in which case the kernel currently refuses to
talk to them at all. As far as the kernel is concerned, this is okay,
since it cannot fix them anyway. But there might be user space
applications that can be used for further diagnosis or even to fix the
underlying problem. In order to allow this, a broken device should be
exported to user space, as long as we can at least talk to it, that is,
as long as it provides well-formed answers to commands, even though it
might return error codes that we do not expect.

Alexander Steffen (3):
  tpm-chip: Move idr_replace calls to appropriate places
  tpm-chip: Return TPM error codes from auto_startup functions
  tpm-chip: Export TPM device to user space even when startup failed

 drivers/char/tpm/tpm-chip.c      | 29 +++++++++++++++++------------
 drivers/char/tpm/tpm-interface.c |  6 ++----
 drivers/char/tpm/tpm2-cmd.c      |  5 ++---
 3 files changed, 21 insertions(+), 19 deletions(-)

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

[PATCH RESEND 1/3] tpm-chip: Move idr_replace calls to appropriate places

[Alexander Steffen]

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

Re: [PATCH RESEND 1/3] tpm-chip: Move idr_replace calls to appropriate places

[Jarkko Sakkinen]

On Thu, Aug 24, 2017 at 10:37:12AM +0200, Alexander Steffen wrote:
> 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@infineon.com>
> ---
>  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)
> -- 

This is unnecessary and questionable code shuffling in a very critical
places of the driver code where race conditions are easily introduced.

If you don't have a better reason to do this, I'm not going to take
this. I also fail to see the connection to the patch set as whole.

/Jarkko

Re: [PATCH RESEND 1/3] tpm-chip: Move idr_replace calls to appropriate places

[Alexander.Steffen-d0qZbvYSIPpWk0Htik3J/w]

> On Thu, Aug 24, 2017 at 10:37:12AM +0200, Alexander Steffen wrote:
> > 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)
> > --
> 
> This is unnecessary and questionable code shuffling in a very critical places of
> the driver code where race conditions are easily introduced.

Can you explain what race conditions you fear here?

My understanding of the code so far is this: There are two separate paths to the TPM (from kernel and user space), that share the common driver code (tpm_transmit and everything below), but that can (in theory) exist without the other, i.e. the kernel can use the TPM without ever exporting it to user space and user space applications can send commands to the TPM without the kernel using the TPM for anything.

If the kernel wants to use the TPM, it needs to go through tpm_chip_find_get at some point. Every request from user space passes through tpm_common_write (except for everything from tpm-sysfs.c, that also somehow lacks the serialization imposed by tpm_try_get_ops, but that is a different problem). By not placing the chip into dev_nums_idr, I prevent the kernel from using the TPM while leaving the user space path intact.

So, based on those assumptions, that the kernel and user space paths are independent, until they meet at tpm_transmit, which is serialized by tpm_try_get_ops, it should not matter in what order I make the device available for kernel or user space usage, or whether I do not make it available for one of them at all. What race conditions could there be?

> If you don't have a better reason to do this, I'm not going to take this.

The comments currently state that the idr_replace calls should be the first/last step in the process, so either the code or the comments are wrong and need to be changed. I opted for changing the code, since, as explained above, I cannot see how the kernel and user space paths in this place interact (i.e. as far as I understand the code, you can place the idr_replace call anywhere after the call to tpm*_auto_startup, without being able to detect a difference). Also, "somewhere in the middle, add the chip to the list" does not make for a very useful comment ;-)

> I also fail to see the connection to the patch set as whole.

PATCH 3/3 needs a way to skip the idr_replace call under some circumstances, and this seemed like a cleaner solution than passing around additional flags, that also fixed the comment/code mismatch.

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

[PATCH RESEND 2/3] tpm-chip: Return TPM error codes from auto_startup functions

[Alexander Steffen]

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 78fb41d..fe11f2a 100644
--- a/drivers/char/tpm/tpm-interface.c
+++ b/drivers/char/tpm/tpm-interface.c
@@ -1000,7 +1000,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)
 {
@@ -1015,10 +1016,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 e1a41b7..54a3123 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

Re: [PATCH RESEND 2/3] tpm-chip: Return TPM error codes from auto_startup functions

[Jarkko Sakkinen]

On Thu, Aug 24, 2017 at 10:37:13AM +0200, Alexander Steffen wrote:
> 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;

So what good this change makes in the end? I'm not sure I'm following.

>  	}
>  
>  	tpm_sysfs_add_device(chip);
> diff --git a/drivers/char/tpm/tpm-interface.c b/drivers/char/tpm/tpm-interface.c
> index 78fb41d..fe11f2a 100644
> --- a/drivers/char/tpm/tpm-interface.c
> +++ b/drivers/char/tpm/tpm-interface.c
> @@ -1000,7 +1000,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.
>   */

A nitpick.

According to https://www.kernel.org/doc/Documentation/kernel-doc-nano-HOWTO.txt
you ought to use "Return:" keyword ;-)

I would favor here:

  "Return: same as with tpm_transmit_cmd()"

>  int tpm1_auto_startup(struct tpm_chip *chip)
>  {
> @@ -1015,10 +1016,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 e1a41b7..54a3123 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
> 

At this point I can only see aero improvement how the driver works and a
very minor clean up.

/Jarkko

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

Re: [PATCH RESEND 2/3] tpm-chip: Return TPM error codes from auto_startup functions

[Alexander.Steffen-d0qZbvYSIPpWk0Htik3J/w]

> > 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;
> 
> So what good this change makes in the end? I'm not sure I'm following.

This is just refactoring the code without changing the functionality. Before this change, tpm*_auto_startup could only return negative error codes (or 0 for success). With this change, tpm*_auto_startup can also return positive errors codes for TPM errors (but still 0 for success). Therefore, tpm_chip_register, the only caller of tpm*_auto_startup, now needs to convert positive error codes to -ENODEV, so that the external behavior is unchanged.

> >  	}
> >
> >  	tpm_sysfs_add_device(chip);
> > diff --git a/drivers/char/tpm/tpm-interface.c
> > b/drivers/char/tpm/tpm-interface.c
> > index 78fb41d..fe11f2a 100644
> > --- a/drivers/char/tpm/tpm-interface.c
> > +++ b/drivers/char/tpm/tpm-interface.c
> > @@ -1000,7 +1000,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.
> >   */
> 
> A nitpick.
> 
> According to https://www.kernel.org/doc/Documentation/kernel-doc-nano-
> HOWTO.txt
> you ought to use "Return:" keyword ;-)
> 
> I would favor here:
> 
>   "Return: same as with tpm_transmit_cmd()"

Agreed. I will fix that together with the rest, once the discussions for the other patches in the series have come to a conclusion.

> >  int tpm1_auto_startup(struct tpm_chip *chip)  { @@ -1015,10 +1016,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 e1a41b7..54a3123 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
> >
> 
> At this point I can only see aero improvement how the driver works and a
> very minor clean up.
> 
> /Jarkko

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

[PATCH RESEND 3/3] tpm-chip: Export TPM device to user space even when startup failed

[Alexander Steffen]

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

Re: [PATCH RESEND 3/3] tpm-chip: Export TPM device to user space even when startup failed

[Jarkko Sakkinen]

On Thu, Aug 24, 2017 at 10:37:14AM +0200, Alexander Steffen wrote:
> 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@infineon.com>

I can understand the benefits but you could make the same argument for
any class devices that kernel handles. I don't think it is that common
to let user space access malfunctioning devices.

Adding linux-security-module.

PS. You should have in *all* patches that you don't tag as RFC have
linux-kernel@vger.kernel.org. Now you have it in *none* of your patches.
When you don't have RFC you are saying out loud that this is production
ready code that should be included to the mainline kernel.

PPS. This patch set should be obviously RFC. It's  avery questionable and
intrusive change. That's why I didn't include linux-kernel.

/Jarkko

Re: [PATCH RESEND 3/3] tpm-chip: Export TPM device to user space even when startup failed

[Alexander.Steffen-d0qZbvYSIPpWk0Htik3J/w]

> On Thu, Aug 24, 2017 at 10:37:14AM +0200, Alexander Steffen wrote:
> > 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>
> 
> I can understand the benefits but you could make the same argument for
> any class devices that kernel handles.

In a similar situation I probably would make that argument for other devices as well :-)

> I don't think it is that common to let user space access malfunctioning
> devices.

But is that just because nobody bothered to implement the necessary logic or for some other reason?

In my opinion, it strongly depends on the effects of the malfunction. If the device does not respond at all or returns only garbage, it is of course not useful to make it available. But the case that I want to handle here has a TPM that communicates correctly, i.e. the TIS layer works fine, it just does not return the application-level responses that the kernel expects. I'd like to think of it more as a transparent communication channel that the kernel provides to user space applications, without interfering with the data that is transmitted, similar to how a TCP socket is a transparent channel, that does not care about HTTP 500 codes being transmitted. So it is okay for the kernel to refuse to use such a device internally (the kernel knows there is nothing it can do with a broken device),
  but why assume that no user space application can use it either?

I'd rather argue that user space applications already need to be prepared to handle TPM errors/malfunctions at any time, so removing this one check does not put them into a worse position. The TPM driver just checks *at startup* that the TPM seems to be okay (i.e. executes all self tests correctly). But if you keep your machine running for two years the TPM may fail at any point and the driver never checks it again, so an application may never assume that simply because /dev/tpm0 exists it can send arbitrary commands there that will always work (in fact, such a look-before-you-leap approach can never be guaranteed to succeed).

Also, what would be the alternative? Your TPM is broken in a way that the kernel does not export it, how do you debug/fix the problem? You cannot even execute a TPM2_GetTestResult to get more detailed error information. So at the moment, we have user space applications bypassing the kernel driver completely, by accessing /dev/mem and reimplementing all the TIS logic. But this is just duplicated code (with its own set of problems, for example, if the kernel driver is active at the same time, there is nothing that prevents concurrent accesses), so nobody wants to implement a similar solution for SPI, and I2C, and all the other interfaces that might be necessary in the future.

> PS. You should have in *all* patches that you don't tag as RFC have linux-
> kernel-u79uwXL29TaiAVqoAR/hOA@public.gmane.org Now you have it in *none* of your patches.
> When you don't have RFC you are saying out loud that this is production
> ready code that should be included to the mainline kernel.
> 
> PPS. This patch set should be obviously RFC. It's  avery questionable and
> intrusive change. That's why I didn't include linux-kernel.

Sorry, thanks for explaining, will do that next time.

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

Re: [PATCH RESEND 3/3] tpm-chip: Export TPM device to user space even when startup failed

[Jarkko Sakkinen]

On Mon, Aug 28, 2017 at 05:15:58PM +0000, Alexander.Steffen@infineon.com wrote:
> But is that just because nobody bothered to implement the necessary
> logic or for some other reason?

We do not want user space to access broken hardware. It's a huge risk
for system stability and potentially could be used for evil purposes.

This is not going to mainline as it is not suitable for general
consumption. You must use a patched kernel if you want this.

/Jarkko

Re: [tpmdd-devel] [PATCH RESEND 3/3] tpm-chip: Export TPM device to user space even when startup failed

[Michal Suchánek]

Hello,

On Tue, 29 Aug 2017 15:55:09 +0300
Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> wrote:

> On Mon, Aug 28, 2017 at 05:15:58PM +0000,
> Alexander.Steffen@infineon.com wrote:
> > But is that just because nobody bothered to implement the necessary
> > logic or for some other reason?  
> 
> We do not want user space to access broken hardware. It's a huge risk
> for system stability and potentially could be used for evil purposes.
> 
> This is not going to mainline as it is not suitable for general
> consumption. You must use a patched kernel if you want this.
> 
> /Jarkko
> 

It has been pointed out that userspace applications that use direct IO
access exist for the purpose. So using a kernel driver is an
improvement over that if the interface is otherwise sane.

What do you expect is the potential for instability or evil use?

With a kernel driver arbitrating the bus access as it would in any
other case I do not see much potential for instability. If there are
cases when the arbitration fails they can surely be more likely
triggered in cases other than userspace sending arbitrary requests to a
device which is in a state the kernel does not support but otherwise
responsive.

If you really think that accessing a device that is in unsupported
state at boot (as opposed to getting unto unsupported state during
device operation after boot) is a real problem it can be selectable as
compile time option so people who do not want the code do not get it.

Thanks

Michal

Re: [PATCH RESEND 3/3] tpm-chip: Export TPM device to user space even when startup failed

[Peter Huewe]

Thabks Michal!

Am 29. August 2017 15:17:39 MESZ schrieb "Michal Suchánek" <msuchanek@suse.de>:
>Hello,
>
>On Tue, 29 Aug 2017 15:55:09 +0300
>Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> wrote:
>
>> On Mon, Aug 28, 2017 at 05:15:58PM +0000,
>> Alexander.Steffen@infineon.com wrote:
>> > But is that just because nobody bothered to implement the necessary
>> > logic or for some other reason?  
>> 
>> We do not want user space to access broken hardware. It's a huge risk
>> for system stability and potentially could be used for evil purposes.
>> 
>> This is not going to mainline as it is not suitable for general
>> consumption. You must use a patched kernel if you want this.
>> 
>> /Jarkko
>> 
>
>It has been pointed out that userspace applications that use direct IO
>access exist for the purpose. So using a kernel driver is an
>improvement over that if the interface is otherwise sane.
+1

>
>What do you expect is the potential for instability or evil use?
>
>With a kernel driver arbitrating the bus access as it would in any
>other case I do not see much potential for instability. 

Exactly.
 

>If there are
>cases when the arbitration fails they can surely be more likely
>triggered in cases other than userspace sending arbitrary requests to a
>device which is in a state the kernel does not support but otherwise
>responsive.

Its the same as with every other device - as long as it communicates like a tpm, talks like a tpm, behaves like a tpm - it's a tpm. 
Even if the kernel does not recognize the tpm's internal state.


>If you really think that accessing a device that is in unsupported
>state at boot (as opposed to getting unto unsupported state during
>device operation after boot) is a real problem it can be selectable as
>compile time option so people who do not want the code do not get it.

I would more favor a module option, 
but honestly I don't see a reason to not pull this code in, as it is.
Maybe mark the kernel as tainted if you think it is an issue.

Adding this as a out of tree patch is far from userfriendly.
If there is a chance to debug and fix a "unknown" state using the kernel as the communication layer, I would like to use this way.


Peter

>
>Thanks
>
>Michal
>
>------------------------------------------------------------------------------
>Check out the vibrant tech community on one of the world's most
>engaging tech sites, Slashdot.org! http://sdm.link/slashdot
>_______________________________________________
>tpmdd-devel mailing list
>tpmdd-devel@lists.sourceforge.net
>https://lists.sourceforge.net/lists/listinfo/tpmdd-devel

-- 
Sent from my mobile

------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
_______________________________________________
tpmdd-devel mailing list
tpmdd-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/tpmdd-devel

Re: [tpmdd-devel] [PATCH RESEND 3/3] tpm-chip: Export TPM device to user space even when startup failed

[Jarkko Sakkinen]

On Tue, Aug 29, 2017 at 03:53:17PM +0200, Peter Huewe wrote:
> Thabks Michal!
> 
> Am 29. August 2017 15:17:39 MESZ schrieb "Michal Suchánek" <msuchanek@suse.de>:
> >Hello,
> >
> >On Tue, 29 Aug 2017 15:55:09 +0300
> >Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> wrote:
> >
> >> On Mon, Aug 28, 2017 at 05:15:58PM +0000,
> >> Alexander.Steffen@infineon.com wrote:
> >> > But is that just because nobody bothered to implement the necessary
> >> > logic or for some other reason?  
> >> 
> >> We do not want user space to access broken hardware. It's a huge risk
> >> for system stability and potentially could be used for evil purposes.
> >> 
> >> This is not going to mainline as it is not suitable for general
> >> consumption. You must use a patched kernel if you want this.
> >> 
> >> /Jarkko
> >> 
> >
> >It has been pointed out that userspace applications that use direct IO
> >access exist for the purpose. So using a kernel driver is an
> >improvement over that if the interface is otherwise sane.
> +1
> 
> >
> >What do you expect is the potential for instability or evil use?
> >
> >With a kernel driver arbitrating the bus access as it would in any
> >other case I do not see much potential for instability. 
> 
> Exactly.
>  
> 
> >If there are
> >cases when the arbitration fails they can surely be more likely
> >triggered in cases other than userspace sending arbitrary requests to a
> >device which is in a state the kernel does not support but otherwise
> >responsive.
> 
> Its the same as with every other device - as long as it communicates like a tpm, talks like a tpm, behaves like a tpm - it's a tpm. 
> Even if the kernel does not recognize the tpm's internal state.
> 
> 
> >If you really think that accessing a device that is in unsupported
> >state at boot (as opposed to getting unto unsupported state during
> >device operation after boot) is a real problem it can be selectable as
> >compile time option so people who do not want the code do not get it.
> 
> I would more favor a module option, 
> but honestly I don't see a reason to not pull this code in, as it is.
> Maybe mark the kernel as tainted if you think it is an issue.
> 
> Adding this as a out of tree patch is far from userfriendly.
> If there is a chance to debug and fix a "unknown" state using the kernel as the communication layer, I would like to use this way.
> 
> 
> Peter

As I said in previous response it's not a good idea to give all-in
access at this point. You cannot turn back from that once it is in
the mainline.

I would rather suggest scoping lowest common denominator subset of
TPM commands that you need and allow only those commands to pass
through. If the subset turns out to be too limited, you can expand
it later on.

/Jarkko

Re: [PATCH RESEND 3/3] tpm-chip: Export TPM device to user space even when startup failed

[Jarkko Sakkinen]

On Tue, Aug 29, 2017 at 03:17:39PM +0200, Michal Suchánek wrote:
> Hello,
> 
> On Tue, 29 Aug 2017 15:55:09 +0300
> Jarkko Sakkinen <jarkko.sakkinen-VuQAYsv1563Yd54FQh9/CA@public.gmane.org> wrote:
> 
> > On Mon, Aug 28, 2017 at 05:15:58PM +0000,
> > Alexander.Steffen-d0qZbvYSIPpWk0Htik3J/w@public.gmane.org wrote:
> > > But is that just because nobody bothered to implement the necessary
> > > logic or for some other reason?  
> > 
> > We do not want user space to access broken hardware. It's a huge risk
> > for system stability and potentially could be used for evil purposes.
> > 
> > This is not going to mainline as it is not suitable for general
> > consumption. You must use a patched kernel if you want this.
> > 
> > /Jarkko
> > 
> 
> It has been pointed out that userspace applications that use direct IO
> access exist for the purpose. So using a kernel driver is an
> improvement over that if the interface is otherwise sane.
> 
> What do you expect is the potential for instability or evil use?

By definition the use of broken hardware can have unpredictable effects.
Use a patched kernel if you want to do it.

/Jarkko

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

Re: [tpmdd-devel] [PATCH RESEND 3/3] tpm-chip: Export TPM device to user space even when startup failed

[Jarkko Sakkinen]

On Wed, Aug 30, 2017 at 01:15:10PM +0300, Jarkko Sakkinen wrote:
> On Tue, Aug 29, 2017 at 03:17:39PM +0200, Michal Suchánek wrote:
> > Hello,
> > 
> > On Tue, 29 Aug 2017 15:55:09 +0300
> > Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> wrote:
> > 
> > > On Mon, Aug 28, 2017 at 05:15:58PM +0000,
> > > Alexander.Steffen@infineon.com wrote:
> > > > But is that just because nobody bothered to implement the necessary
> > > > logic or for some other reason?  
> > > 
> > > We do not want user space to access broken hardware. It's a huge risk
> > > for system stability and potentially could be used for evil purposes.
> > > 
> > > This is not going to mainline as it is not suitable for general
> > > consumption. You must use a patched kernel if you want this.
> > > 
> > > /Jarkko
> > > 
> > 
> > It has been pointed out that userspace applications that use direct IO
> > access exist for the purpose. So using a kernel driver is an
> > improvement over that if the interface is otherwise sane.
> > 
> > What do you expect is the potential for instability or evil use?
> 
> By definition the use of broken hardware can have unpredictable effects.
> Use a patched kernel if you want to do it.
> 
> /Jarkko

I.e. too many unknown unknowns for mainline.

I could consider a solution for the TPM error case on self-test that
allows only restricted subset of commands.

The patch description did not go to *any* detail on how it is used so
practically it's unreviewable at this point. There's a big burder of
proof and now there's only hand waving.

/Jarkko

Re: [tpmdd-devel] [PATCH RESEND 3/3] tpm-chip: Export TPM device to user space even when startup failed

[Michal Suchánek]

On Wed, 30 Aug 2017 13:20:02 +0300
Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> wrote:

> On Wed, Aug 30, 2017 at 01:15:10PM +0300, Jarkko Sakkinen wrote:
> > On Tue, Aug 29, 2017 at 03:17:39PM +0200, Michal Suchánek wrote:  
> > > Hello,
> > > 
> > > On Tue, 29 Aug 2017 15:55:09 +0300
> > > Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> wrote:
> > >   
> > > > On Mon, Aug 28, 2017 at 05:15:58PM +0000,
> > > > Alexander.Steffen@infineon.com wrote:  
> > > > > But is that just because nobody bothered to implement the
> > > > > necessary logic or for some other reason?    
> > > > 
> > > > We do not want user space to access broken hardware. It's a
> > > > huge risk for system stability and potentially could be used
> > > > for evil purposes.
> > > > 
> > > > This is not going to mainline as it is not suitable for general
> > > > consumption. You must use a patched kernel if you want this.
> > > > 
> > > > /Jarkko
> > > >   
> > > 
> > > It has been pointed out that userspace applications that use
> > > direct IO access exist for the purpose. So using a kernel driver
> > > is an improvement over that if the interface is otherwise sane.
> > > 
> > > What do you expect is the potential for instability or evil use?  
> > 
> > By definition the use of broken hardware can have unpredictable
> > effects. Use a patched kernel if you want to do it.
> > 
> > /Jarkko  
> 
> I.e. too many unknown unknowns for mainline.
> 
> I could consider a solution for the TPM error case on self-test that
> allows only restricted subset of commands.
> 
> The patch description did not go to *any* detail on how it is used so
> practically it's unreviewable at this point. There's a big burder of
> proof and now there's only hand waving.
> 
Hello,

there was a bug patched recently in which Linux was not sending the
shutdown command on system shutdown. Presumably with this bug some TPMs
consider being under attack and stop performing most functions.
However, you should be able to read the log if this is implemented
sanely. For that the TPM needs to be accessible.

There are probably other cases when the TPM might be useless for system
use but it might be useful to access it. For example, does Linux handle
uninitialized TPMs?

Thanks

Michal

Re: [tpmdd-devel] [PATCH RESEND 3/3] tpm-chip: Export TPM device to user space even when startup failed

[Jarkko Sakkinen]

On Wed, Aug 30, 2017 at 12:34:16PM +0200, Michal Suchánek wrote:
> On Wed, 30 Aug 2017 13:20:02 +0300
> Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> wrote:
> 
> > On Wed, Aug 30, 2017 at 01:15:10PM +0300, Jarkko Sakkinen wrote:
> > > On Tue, Aug 29, 2017 at 03:17:39PM +0200, Michal Suchánek wrote:  
> > > > Hello,
> > > > 
> > > > On Tue, 29 Aug 2017 15:55:09 +0300
> > > > Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> wrote:
> > > >   
> > > > > On Mon, Aug 28, 2017 at 05:15:58PM +0000,
> > > > > Alexander.Steffen@infineon.com wrote:  
> > > > > > But is that just because nobody bothered to implement the
> > > > > > necessary logic or for some other reason?    
> > > > > 
> > > > > We do not want user space to access broken hardware. It's a
> > > > > huge risk for system stability and potentially could be used
> > > > > for evil purposes.
> > > > > 
> > > > > This is not going to mainline as it is not suitable for general
> > > > > consumption. You must use a patched kernel if you want this.
> > > > > 
> > > > > /Jarkko
> > > > >   
> > > > 
> > > > It has been pointed out that userspace applications that use
> > > > direct IO access exist for the purpose. So using a kernel driver
> > > > is an improvement over that if the interface is otherwise sane.
> > > > 
> > > > What do you expect is the potential for instability or evil use?  
> > > 
> > > By definition the use of broken hardware can have unpredictable
> > > effects. Use a patched kernel if you want to do it.
> > > 
> > > /Jarkko  
> > 
> > I.e. too many unknown unknowns for mainline.
> > 
> > I could consider a solution for the TPM error case on self-test that
> > allows only restricted subset of commands.
> > 
> > The patch description did not go to *any* detail on how it is used so
> > practically it's unreviewable at this point. There's a big burder of
> > proof and now there's only hand waving.
> > 
> Hello,
> 
> there was a bug patched recently in which Linux was not sending the
> shutdown command on system shutdown. Presumably with this bug some TPMs
> consider being under attack and stop performing most functions.
> However, you should be able to read the log if this is implemented
> sanely. For that the TPM needs to be accessible.
> 
> There are probably other cases when the TPM might be useless for system
> use but it might be useful to access it. For example, does Linux handle
> uninitialized TPMs?
> 
> Thanks
> 
> Michal

Agreed. I still think it would make sense to start with a limited subset
of TPM commands, not with all-command-allowed.

I guess Alexander should be able to propose such subset.

/Jarkko

RE: [tpmdd-devel] [PATCH RESEND 3/3] tpm-chip: Export TPM device to user space even when startup failed

[Alexander.Steffen]

> On Wed, Aug 30, 2017 at 12:34:16PM +0200, Michal Suchánek wrote:
> > On Wed, 30 Aug 2017 13:20:02 +0300
> > Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> wrote:
> >
> > > On Wed, Aug 30, 2017 at 01:15:10PM +0300, Jarkko Sakkinen wrote:
> > > > On Tue, Aug 29, 2017 at 03:17:39PM +0200, Michal Suchánek wrote:
> > > > > Hello,
> > > > >
> > > > > On Tue, 29 Aug 2017 15:55:09 +0300 Jarkko Sakkinen
> > > > > <jarkko.sakkinen@linux.intel.com> wrote:
> > > > >
> > > > > > On Mon, Aug 28, 2017 at 05:15:58PM +0000,
> > > > > > Alexander.Steffen@infineon.com wrote:
> > > > > > > But is that just because nobody bothered to implement the
> > > > > > > necessary logic or for some other reason?
> > > > > >
> > > > > > We do not want user space to access broken hardware. It's a
> > > > > > huge risk for system stability and potentially could be used
> > > > > > for evil purposes.
> > > > > >
> > > > > > This is not going to mainline as it is not suitable for
> > > > > > general consumption. You must use a patched kernel if you want
> this.
> > > > > >
> > > > > > /Jarkko
> > > > > >
> > > > >
> > > > > It has been pointed out that userspace applications that use
> > > > > direct IO access exist for the purpose. So using a kernel driver
> > > > > is an improvement over that if the interface is otherwise sane.
> > > > >
> > > > > What do you expect is the potential for instability or evil use?
> > > >
> > > > By definition the use of broken hardware can have unpredictable
> > > > effects. Use a patched kernel if you want to do it.
> > > >
> > > > /Jarkko
> > >
> > > I.e. too many unknown unknowns for mainline.
> > >
> > > I could consider a solution for the TPM error case on self-test that
> > > allows only restricted subset of commands.
> > >
> > > The patch description did not go to *any* detail on how it is used
> > > so practically it's unreviewable at this point. There's a big burder
> > > of proof and now there's only hand waving.
> > >
> > Hello,
> >
> > there was a bug patched recently in which Linux was not sending the
> > shutdown command on system shutdown. Presumably with this bug some
> > TPMs consider being under attack and stop performing most functions.
> > However, you should be able to read the log if this is implemented
> > sanely. For that the TPM needs to be accessible.
> >
> > There are probably other cases when the TPM might be useless for
> > system use but it might be useful to access it. For example, does
> > Linux handle uninitialized TPMs?
> >
> > Thanks
> >
> > Michal

The two scenarios that I had in mind:

1. A TPM in failure mode. This signals a broken device, yes, but it is part of the specification, so we should support it. If it was unreasonable to talk to such a device, why specify failure mode in the first place?

2. A TPM in some vendor-specific mode. This is what happens during field upgrade with some Infineon TPMs. During the field upgrade process, the communication looks TPM-like (i.e. the usual 10-byte header is present), but it is not part of any (public) specification, and standard TPM commands are obviously unsupported. This works fine with the current driver, as long as you do not interrupt the upgrade process, as the driver never checks the device again during usage. But if the upgrade process is interrupted (e.g. by power loss) and the TPM has to start again in this vendor-specific mode, then the device simply disappears from the system and you have no chance to fix the problem.

> Agreed. I still think it would make sense to start with a limited subset of TPM
> commands, not with all-command-allowed.
> 
> I guess Alexander should be able to propose such subset.

For scenario #1 you could probably come up with a list of commands that are generally useful. But once you are restricted to those five commands, you block iterative debugging of the "I see where the problem might be, could you try to execute ..." fashion by requiring the other person to patch and rebuild their kernel.

For scenario #2 I see no chance to do that in a generic way. I could maybe tell you what the commands in this mode currently look like for Infineon TPMs, so that they can be whitelisted, but they might look different in the future and they are certainly different for other vendor's implementations.

So with both scenarios you end up with a lot of infrastructure for this whitelist approach in general and add a growing list of allowed commands, that are initially never present in the kernel version where you need them most, and where it then takes several months or years until they finally reach your users through official channels. And with all that in place, you gain what exactly? Why is it even useful to whitelist functionality based on command codes? If the device is broken, it might be that just some TIS registers do not work correctly. Or maybe just some parameter combinations for specific commands do not work anymore (e.g. your RSA hardware is broken, so RSA operations fail, but ECC is still good).

My point is: You already need to guard against misbehaving devices, but you cannot do so by trying to classify devices as either "good" or "bad". A device that initially looks good can turn bad at any time. So for example, you cannot run an endless loop until the good device returns the expected answer, but you always have to use proper timeouts. If such safeguards are missing somewhere, then it is already a bug that needs to be fixed. But if such safeguards are present everywhere where needed, there should be nothing that a good or a bad device can do, that harms the kernel or any (well-written) application.

Alexander

Re: [tpmdd-devel] [PATCH RESEND 3/3] tpm-chip: Export TPM device to user space even when startup failed

[Jarkko Sakkinen]

On Thu, Aug 31, 2017 at 04:18:42PM +0000, Alexander.Steffen@infineon.com wrote:
> > I guess Alexander should be able to propose such subset.
> 
> For scenario #1 you could probably come up with a list of commands
> that are generally useful. But once you are restricted to those five
> commands, you block iterative debugging of the "I see where the
> problem might be, could you try to execute ..." fashion by requiring
> the other person to patch and rebuild their kernel.

If the subset turns out to be wrong, it can be revisited.

> For scenario #2 I see no chance to do that in a generic way. I could
> maybe tell you what the commands in this mode currently look like for
> Infineon TPMs, so that they can be whitelisted, but they might look
> different in the future and they are certainly different for other
> vendor's implementations.

It's easy to check whether a command is vendor specific and allow to
pass those through.

/Jarkko

Re: [PATCH RESEND 3/3] tpm-chip: Export TPM device to user space even when startup failed

[Peter Huewe]

Am 30. August 2017 12:15:10 MESZ schrieb Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>:
>On Tue, Aug 29, 2017 at 03:17:39PM +0200, Michal Suchánek wrote:
>> Hello,
>> 
>> On Tue, 29 Aug 2017 15:55:09 +0300
>> Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> wrote:
>> 
>> > On Mon, Aug 28, 2017 at 05:15:58PM +0000,
>> > Alexander.Steffen@infineon.com wrote:
>> > > But is that just because nobody bothered to implement the
>necessary
>> > > logic or for some other reason?  
>> > 
>> > We do not want user space to access broken hardware. It's a huge
>risk
>> > for system stability and potentially could be used for evil
>purposes.
>> > 
>> > This is not going to mainline as it is not suitable for general
>> > consumption. You must use a patched kernel if you want this.
>> > 
>> > /Jarkko
>> > 
>> 
>> It has been pointed out that userspace applications that use direct
>IO
>> access exist for the purpose. So using a kernel driver is an
>> improvement over that if the interface is otherwise sane.
>> 
>> What do you expect is the potential for instability or evil use?
>
>By definition the use of broken hardware can have unpredictable
>effects.
>Use a patched kernel if you want to do it.

If the s.m.a.r.t selftest of your hard disk fails, you can still access it, even though the hw selftest says it is broken.
Same situation.



>
>/Jarkko
>
>------------------------------------------------------------------------------
>Check out the vibrant tech community on one of the world's most
>engaging tech sites, Slashdot.org! http://sdm.link/slashdot
>_______________________________________________
>tpmdd-devel mailing list
>tpmdd-devel@lists.sourceforge.net
>https://lists.sourceforge.net/lists/listinfo/tpmdd-devel

-- 
Sent from my mobile

------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
_______________________________________________
tpmdd-devel mailing list
tpmdd-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/tpmdd-devel

Re: [tpmdd-devel] [PATCH RESEND 3/3] tpm-chip: Export TPM device to user space even when startup failed

[Jarkko Sakkinen]

On Wed, Aug 30, 2017 at 12:41:51PM +0200, Peter Huewe wrote:
> 
> 
> Am 30. August 2017 12:15:10 MESZ schrieb Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>:
> >On Tue, Aug 29, 2017 at 03:17:39PM +0200, Michal Suchánek wrote:
> >> Hello,
> >> 
> >> On Tue, 29 Aug 2017 15:55:09 +0300
> >> Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> wrote:
> >> 
> >> > On Mon, Aug 28, 2017 at 05:15:58PM +0000,
> >> > Alexander.Steffen@infineon.com wrote:
> >> > > But is that just because nobody bothered to implement the
> >necessary
> >> > > logic or for some other reason?  
> >> > 
> >> > We do not want user space to access broken hardware. It's a huge
> >risk
> >> > for system stability and potentially could be used for evil
> >purposes.
> >> > 
> >> > This is not going to mainline as it is not suitable for general
> >> > consumption. You must use a patched kernel if you want this.
> >> > 
> >> > /Jarkko
> >> > 
> >> 
> >> It has been pointed out that userspace applications that use direct
> >IO
> >> access exist for the purpose. So using a kernel driver is an
> >> improvement over that if the interface is otherwise sane.
> >> 
> >> What do you expect is the potential for instability or evil use?
> >
> >By definition the use of broken hardware can have unpredictable
> >effects.
> >Use a patched kernel if you want to do it.
> 
> If the s.m.a.r.t selftest of your hard disk fails, you can still
> access it, even though the hw selftest says it is broken.
> Same situation.

Not sure if you can compare these directly although I get your point.

Waiting for more comments on this. At the moment I'm still dilated to
restricted access because it gives more variables for the future.

/Jarkko

RE: [tpmdd-devel] [PATCH RESEND 3/3] tpm-chip: Export TPM device to user space even when startup failed

[Alexander.Steffen]

> On Wed, Aug 30, 2017 at 12:41:51PM +0200, Peter Huewe wrote:
> >
> >
> > Am 30. August 2017 12:15:10 MESZ schrieb Jarkko Sakkinen
> <jarkko.sakkinen@linux.intel.com>:
> > >On Tue, Aug 29, 2017 at 03:17:39PM +0200, Michal Suchánek wrote:
> > >> Hello,
> > >>
> > >> On Tue, 29 Aug 2017 15:55:09 +0300
> > >> Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> wrote:
> > >>
> > >> > On Mon, Aug 28, 2017 at 05:15:58PM +0000,
> > >> > Alexander.Steffen@infineon.com wrote:
> > >> > > But is that just because nobody bothered to implement the
> > >necessary
> > >> > > logic or for some other reason?
> > >> >
> > >> > We do not want user space to access broken hardware. It's a huge
> > >risk
> > >> > for system stability and potentially could be used for evil
> > >purposes.
> > >> >
> > >> > This is not going to mainline as it is not suitable for general
> > >> > consumption. You must use a patched kernel if you want this.
> > >> >
> > >> > /Jarkko
> > >> >
> > >>
> > >> It has been pointed out that userspace applications that use direct
> > >IO
> > >> access exist for the purpose. So using a kernel driver is an
> > >> improvement over that if the interface is otherwise sane.
> > >>
> > >> What do you expect is the potential for instability or evil use?
> > >
> > >By definition the use of broken hardware can have unpredictable
> > >effects.
> > >Use a patched kernel if you want to do it.
> >
> > If the s.m.a.r.t selftest of your hard disk fails, you can still
> > access it, even though the hw selftest says it is broken.
> > Same situation.
> 
> Not sure if you can compare these directly although I get your point.
> 
> Waiting for more comments on this. At the moment I'm still dilated to
> restricted access because it gives more variables for the future.
> 
> /Jarkko

I think, this is a perfect example. In both cases we have intelligent devices, that can detect that they are broken and report it correctly (instead of just malfunction randomly). This also shows that those devices are not so broken that they completely misbehave (i.e. significantly violate their spec), they just do not provide the full functionality anymore.

Could you give me one example how you'd imagine such a device to impact stability? My patch already prevents the kernel from using broken TPMs for anything, it only provides the communication facilities for user space applications.

With regard to security/attacks, it is precisely the TPM's job to protect itself (and your secrets). You get all your security guarantees from the TPM, not the driver code, so it does not matter what the driver does, even with a broken TPM.

I understand your point of not wanting to enable functionality that you cannot disable anymore, but I fail to see why you'd ever want to disable (part of) this functionality again. Knowing more about the potential problems would allow me to prevent them from the beginning or to come up with a better safeguard than the command-based whitelisting.

Alexander

Re: [tpmdd-devel] [PATCH RESEND 3/3] tpm-chip: Export TPM device to user space even when startup failed

[Jarkko Sakkinen]

On Thu, Aug 31, 2017 at 04:26:10PM +0000, Alexander.Steffen@infineon.com wrote:
> > On Wed, Aug 30, 2017 at 12:41:51PM +0200, Peter Huewe wrote:
> > >
> > >
> > > Am 30. August 2017 12:15:10 MESZ schrieb Jarkko Sakkinen
> > <jarkko.sakkinen@linux.intel.com>:
> > > >On Tue, Aug 29, 2017 at 03:17:39PM +0200, Michal Suchánek wrote:
> > > >> Hello,
> > > >>
> > > >> On Tue, 29 Aug 2017 15:55:09 +0300
> > > >> Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> wrote:
> > > >>
> > > >> > On Mon, Aug 28, 2017 at 05:15:58PM +0000,
> > > >> > Alexander.Steffen@infineon.com wrote:
> > > >> > > But is that just because nobody bothered to implement the
> > > >necessary
> > > >> > > logic or for some other reason?
> > > >> >
> > > >> > We do not want user space to access broken hardware. It's a huge
> > > >risk
> > > >> > for system stability and potentially could be used for evil
> > > >purposes.
> > > >> >
> > > >> > This is not going to mainline as it is not suitable for general
> > > >> > consumption. You must use a patched kernel if you want this.
> > > >> >
> > > >> > /Jarkko
> > > >> >
> > > >>
> > > >> It has been pointed out that userspace applications that use direct
> > > >IO
> > > >> access exist for the purpose. So using a kernel driver is an
> > > >> improvement over that if the interface is otherwise sane.
> > > >>
> > > >> What do you expect is the potential for instability or evil use?
> > > >
> > > >By definition the use of broken hardware can have unpredictable
> > > >effects.
> > > >Use a patched kernel if you want to do it.
> > >
> > > If the s.m.a.r.t selftest of your hard disk fails, you can still
> > > access it, even though the hw selftest says it is broken.
> > > Same situation.
> > 
> > Not sure if you can compare these directly although I get your point.
> > 
> > Waiting for more comments on this. At the moment I'm still dilated to
> > restricted access because it gives more variables for the future.
> > 
> > /Jarkko
> 
> I think, this is a perfect example. In both cases we have intelligent devices, that can detect that they are broken and report it correctly (instead of just malfunction randomly). This also shows that those devices are not so broken that they completely misbehave (i.e. significantly violate their spec), they just do not provide the full functionality anymore.
> 
> Could you give me one example how you'd imagine such a device to impact stability? My patch already prevents the kernel from using broken TPMs for anything, it only provides the communication facilities for user space applications.
> 
> With regard to security/attacks, it is precisely the TPM's job to protect itself (and your secrets). You get all your security guarantees from the TPM, not the driver code, so it does not matter what the driver does, even with a broken TPM.
> 
> I understand your point of not wanting to enable functionality that you cannot disable anymore, but I fail to see why you'd ever want to disable (part of) this functionality again. Knowing more about the potential problems would allow me to prevent them from the beginning or to come up with a better safeguard than the command-based whitelisting.
> 
> Alexander

I think we should hold with this patch set up until we get a mailing
list for tpmdd, which we don't have right now. I've submitted a request
to postmaster@vger.kernel.org but haven't received any feedback. I'll
inform in the linux-security-module list when the new list is created
so you know.

Some of the comments make sense like the one Peter mentioned earlier
and your comments about TPM security but you failed to address these
in the cover letter. For next iteration put this to the cover letter.

Thanks.

/Jarkko