* [RFC][PATCH 0/3] Export broken TPMs to user space
@ 2017-12-14 16:06 Alexander Steffen
2017-12-14 16:06 ` [RFC][PATCH 1/3] tpm-chip: Move idr_replace calls to appropriate places Alexander Steffen
` (3 more replies)
0 siblings, 4 replies; 13+ messages in thread
From: Alexander Steffen @ 2017-12-14 16:06 UTC (permalink / raw
To: jarkko.sakkinen, linux-integrity; +Cc: Alexander Steffen
I had already published those patches a while ago on the old mailing list, and there were some discussions, but we never came to a definite conclusion. I'd still like to get those patches applied, so here they are again, together with a summary of the discussions. The original description of the series is at the very end.
Back then there were some comments to not change the location of the idr_replace calls (patch 1/3), because that might break something, but no specifics were mentioned. I still believe my current change to be correct (and/or not worse than before). If you disagree, please describe a concrete scenario where this change is detrimental, so that I get a better understanding of the code. If nobody understands the code well enough to point out any flaws, but you still think it is wrong to change it, then maybe we should throw it away and rewrite it from scratch to make it easier to understand ;-)
There were also some concerns that allowing user space to access misbehaving/broken devices (patch 3/3) could have ill effects on the stability or even security of the system. To this it was pointed out:
* User space already has ways to access the device even without the kernel driver (e.g. direct access via /dev/mem), but having the kernel driver as a clean interface is preferable.
* The kernel driver has to be prepared to handle misbehaving devices in any case, not only when the device indicates failed selftests during boot (what happens for example when the device breaks, after the driver has already been loaded?). In the end, the kernel just provides a transparent interface to the device (at least as long as /dev/tpm0 is concerned), that is only as good or as bad as the underlying device.
* It is not uncommon for the kernel to talk to broken devices. If there are SMART failures, there is no code that prevents you from getting access to your hard disk.
* As for security, 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.
---
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 | 4 +---
3 files changed, 20 insertions(+), 19 deletions(-)
--
2.7.4
^ permalink raw reply [flat|nested] 13+ messages in thread
* [RFC][PATCH 1/3] tpm-chip: Move idr_replace calls to appropriate places
2017-12-14 16:06 [RFC][PATCH 0/3] Export broken TPMs to user space Alexander Steffen
@ 2017-12-14 16:06 ` Alexander Steffen
2017-12-14 19:27 ` Jason Gunthorpe
` (2 more replies)
2017-12-14 16:06 ` [RFC][PATCH 2/3] tpm-chip: Return TPM error codes from auto_startup functions Alexander Steffen
` (2 subsequent siblings)
3 siblings, 3 replies; 13+ messages in thread
From: Alexander Steffen @ 2017-12-14 16:06 UTC (permalink / raw
To: jarkko.sakkinen, linux-integrity; +Cc: 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@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 a114e8f..25ebe49 100644
--- a/drivers/char/tpm/tpm-chip.c
+++ b/drivers/char/tpm/tpm-chip.c
@@ -321,11 +321,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;
}
@@ -333,11 +328,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)
@@ -432,6 +422,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);
@@ -451,6 +446,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
^ permalink raw reply related [flat|nested] 13+ messages in thread
* [RFC][PATCH 2/3] tpm-chip: Return TPM error codes from auto_startup functions
2017-12-14 16:06 [RFC][PATCH 0/3] Export broken TPMs to user space Alexander Steffen
2017-12-14 16:06 ` [RFC][PATCH 1/3] tpm-chip: Move idr_replace calls to appropriate places Alexander Steffen
@ 2017-12-14 16:06 ` Alexander Steffen
2017-12-14 16:06 ` [RFC][PATCH 3/3] tpm-chip: Export TPM device to user space even when startup failed Alexander Steffen
2017-12-15 14:47 ` [RFC][PATCH 0/3] Export broken TPMs to user space Jarkko Sakkinen
3 siblings, 0 replies; 13+ messages in thread
From: Alexander Steffen @ 2017-12-14 16:06 UTC (permalink / raw
To: jarkko.sakkinen, linux-integrity; +Cc: 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@infineon.com>
---
drivers/char/tpm/tpm-chip.c | 4 +++-
drivers/char/tpm/tpm-interface.c | 6 ++----
drivers/char/tpm/tpm2-cmd.c | 4 +---
3 files changed, 6 insertions(+), 8 deletions(-)
diff --git a/drivers/char/tpm/tpm-chip.c b/drivers/char/tpm/tpm-chip.c
index 25ebe49..9cbe1ef 100644
--- a/drivers/char/tpm/tpm-chip.c
+++ b/drivers/char/tpm/tpm-chip.c
@@ -398,8 +398,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 ebe0a1d..38e85ac 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 f40d206..6cda355 100644
--- a/drivers/char/tpm/tpm2-cmd.c
+++ b/drivers/char/tpm/tpm2-cmd.c
@@ -1045,7 +1045,7 @@ 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.
+ * Return: Same as with tpm_transmit_cmd.
*/
int tpm2_auto_startup(struct tpm_chip *chip)
{
@@ -1080,8 +1080,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
^ permalink raw reply related [flat|nested] 13+ messages in thread
* [RFC][PATCH 3/3] tpm-chip: Export TPM device to user space even when startup failed
2017-12-14 16:06 [RFC][PATCH 0/3] Export broken TPMs to user space Alexander Steffen
2017-12-14 16:06 ` [RFC][PATCH 1/3] tpm-chip: Move idr_replace calls to appropriate places Alexander Steffen
2017-12-14 16:06 ` [RFC][PATCH 2/3] tpm-chip: Return TPM error codes from auto_startup functions Alexander Steffen
@ 2017-12-14 16:06 ` Alexander Steffen
2017-12-14 19:28 ` Jason Gunthorpe
2018-01-18 18:45 ` Jarkko Sakkinen
2017-12-15 14:47 ` [RFC][PATCH 0/3] Export broken TPMs to user space Jarkko Sakkinen
3 siblings, 2 replies; 13+ messages in thread
From: Alexander Steffen @ 2017-12-14 16:06 UTC (permalink / raw
To: jarkko.sakkinen, linux-integrity; +Cc: 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@infineon.com>
---
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 9cbe1ef..c4636e1 100644
--- a/drivers/char/tpm/tpm-chip.c
+++ b/drivers/char/tpm/tpm-chip.c
@@ -384,7 +384,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.
@@ -392,6 +392,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)
@@ -401,7 +402,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);
@@ -424,10 +425,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
^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [RFC][PATCH 1/3] tpm-chip: Move idr_replace calls to appropriate places
2017-12-14 16:06 ` [RFC][PATCH 1/3] tpm-chip: Move idr_replace calls to appropriate places Alexander Steffen
@ 2017-12-14 19:27 ` Jason Gunthorpe
2017-12-15 9:26 ` Alexander.Steffen
2018-01-18 18:38 ` Jarkko Sakkinen
2018-01-18 18:41 ` Jarkko Sakkinen
2 siblings, 1 reply; 13+ messages in thread
From: Jason Gunthorpe @ 2017-12-14 19:27 UTC (permalink / raw
To: Alexander Steffen; +Cc: jarkko.sakkinen, linux-integrity
On Thu, Dec 14, 2017 at 05:06:12PM +0100, 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.
The code is in the proper place.
The visibility of the IDR and the cdev are linked together, they
should be in the same place, side by side.
It doesn't make sense to dis-associate them in the code... If you have
a problem with comments they should be revised instead.
Jason
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [RFC][PATCH 3/3] tpm-chip: Export TPM device to user space even when startup failed
2017-12-14 16:06 ` [RFC][PATCH 3/3] tpm-chip: Export TPM device to user space even when startup failed Alexander Steffen
@ 2017-12-14 19:28 ` Jason Gunthorpe
2018-01-18 18:45 ` Jarkko Sakkinen
1 sibling, 0 replies; 13+ messages in thread
From: Jason Gunthorpe @ 2017-12-14 19:28 UTC (permalink / raw
To: Alexander Steffen; +Cc: jarkko.sakkinen, linux-integrity
On Thu, Dec 14, 2017 at 05:06:14PM +0100, 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>
> 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 9cbe1ef..c4636e1 100644
> +++ b/drivers/char/tpm/tpm-chip.c
> @@ -384,7 +384,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.
> @@ -392,6 +392,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)
> @@ -401,7 +402,7 @@ int tpm_chip_register(struct tpm_chip *chip)
> if (rc < 0)
> return rc;
> else if (rc > 0)
> - return -ENODEV;
> + startup_successful = false;
> }
The sysfs files probably shouldn't be created either in this case, and
the RM cdev should be disabled too.
Jsaon
^ permalink raw reply [flat|nested] 13+ messages in thread
* RE: [RFC][PATCH 1/3] tpm-chip: Move idr_replace calls to appropriate places
2017-12-14 19:27 ` Jason Gunthorpe
@ 2017-12-15 9:26 ` Alexander.Steffen
0 siblings, 0 replies; 13+ messages in thread
From: Alexander.Steffen @ 2017-12-15 9:26 UTC (permalink / raw
To: jgg; +Cc: jarkko.sakkinen, linux-integrity
> On Thu, Dec 14, 2017 at 05:06:12PM +0100, 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.
>
> The code is in the proper place.
>
> The visibility of the IDR and the cdev are linked together, they
> should be in the same place, side by side.
Could you explain why that is? My understanding is that the cdev is for the user space to talk to the TPM, the IDR is for the kernel to talk to the TPM. They share the underlying device, but should otherwise be completely independent. You could (in theory) have the user space interface without the kernel interface or vice versa. What am I missing?
> It doesn't make sense to dis-associate them in the code... If you have
> a problem with comments they should be revised instead.
>
> Jason
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [RFC][PATCH 0/3] Export broken TPMs to user space
2017-12-14 16:06 [RFC][PATCH 0/3] Export broken TPMs to user space Alexander Steffen
` (2 preceding siblings ...)
2017-12-14 16:06 ` [RFC][PATCH 3/3] tpm-chip: Export TPM device to user space even when startup failed Alexander Steffen
@ 2017-12-15 14:47 ` Jarkko Sakkinen
3 siblings, 0 replies; 13+ messages in thread
From: Jarkko Sakkinen @ 2017-12-15 14:47 UTC (permalink / raw
To: Alexander Steffen; +Cc: linux-integrity
On Thu, Dec 14, 2017 at 05:06:11PM +0100, Alexander Steffen wrote:
> I had already published those patches a while ago on the old mailing list, and there were some discussions, but we never came to a definite conclusion. I'd still like to get those patches applied, so here they are again, together with a summary of the discussions. The original description of the series is at the very end.
>
> Back then there were some comments to not change the location of the idr_replace calls (patch 1/3), because that might break something, but no specifics were mentioned. I still believe my current change to be correct (and/or not worse than before). If you disagree, please describe a concrete scenario where this change is detrimental, so that I get a better understanding of the code. If nobody understands the code well enough to point out any flaws, but you still think it is wrong to change it, then maybe we should throw it away and rewrite it from scratch to make it easier to understand ;-)
>
> There were also some concerns that allowing user space to access misbehaving/broken devices (patch 3/3) could have ill effects on the stability or even security of the system. To this it was pointed out:
> * User space already has ways to access the device even without the kernel driver (e.g. direct access via /dev/mem), but having the kernel driver as a clean interface is preferable.
> * The kernel driver has to be prepared to handle misbehaving devices in any case, not only when the device indicates failed selftests during boot (what happens for example when the device breaks, after the driver has already been loaded?). In the end, the kernel just provides a transparent interface to the device (at least as long as /dev/tpm0 is concerned), that is only as good or as bad as the underlying device.
> * It is not uncommon for the kernel to talk to broken devices. If there are SMART failures, there is no code that prevents you from getting access to your hard disk.
> * As for security, 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.
>
> ---
>
> 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 | 4 +---
> 3 files changed, 20 insertions(+), 19 deletions(-)
>
> --
> 2.7.4
>
I'll do a proper review past the holidays.
/Jarkko
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [RFC][PATCH 1/3] tpm-chip: Move idr_replace calls to appropriate places
2017-12-14 16:06 ` [RFC][PATCH 1/3] tpm-chip: Move idr_replace calls to appropriate places Alexander Steffen
2017-12-14 19:27 ` Jason Gunthorpe
@ 2018-01-18 18:38 ` Jarkko Sakkinen
2018-01-18 18:41 ` Jarkko Sakkinen
2 siblings, 0 replies; 13+ messages in thread
From: Jarkko Sakkinen @ 2018-01-18 18:38 UTC (permalink / raw
To: Alexander Steffen; +Cc: linux-integrity
On Thu, Dec 14, 2017 at 05:06:12PM +0100, 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>
What is the problem you are having that this commit tries to
address. Rather would not make changes for cosmetic reasons
to init code.
/Jarkko
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [RFC][PATCH 1/3] tpm-chip: Move idr_replace calls to appropriate places
2017-12-14 16:06 ` [RFC][PATCH 1/3] tpm-chip: Move idr_replace calls to appropriate places Alexander Steffen
2017-12-14 19:27 ` Jason Gunthorpe
2018-01-18 18:38 ` Jarkko Sakkinen
@ 2018-01-18 18:41 ` Jarkko Sakkinen
2 siblings, 0 replies; 13+ messages in thread
From: Jarkko Sakkinen @ 2018-01-18 18:41 UTC (permalink / raw
To: Alexander Steffen; +Cc: linux-integrity
On Thu, Dec 14, 2017 at 05:06:12PM +0100, 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>
NAK
Not compatible with init_module().
http://man7.org/linux/man-pages/man2/init_module.2.html
/Jarkko
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [RFC][PATCH 3/3] tpm-chip: Export TPM device to user space even when startup failed
2017-12-14 16:06 ` [RFC][PATCH 3/3] tpm-chip: Export TPM device to user space even when startup failed Alexander Steffen
2017-12-14 19:28 ` Jason Gunthorpe
@ 2018-01-18 18:45 ` Jarkko Sakkinen
2018-01-18 19:24 ` Jason Gunthorpe
1 sibling, 1 reply; 13+ messages in thread
From: Jarkko Sakkinen @ 2018-01-18 18:45 UTC (permalink / raw
To: Alexander Steffen; +Cc: linux-integrity
On Thu, Dec 14, 2017 at 05:06:14PM +0100, 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>
Agree with Jason's comments.
For SGX code that I've been upstreaming to arch/x86 tree the feedback
was that local variable declarations should be in line length order,
which makes sense to me i.e. startup_succesful should be the first
declaration.
why you need that variable anyway, cannot you deduce it from rc?
/Jarkko
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [RFC][PATCH 3/3] tpm-chip: Export TPM device to user space even when startup failed
2018-01-18 18:45 ` Jarkko Sakkinen
@ 2018-01-18 19:24 ` Jason Gunthorpe
2018-01-23 12:39 ` Jarkko Sakkinen
0 siblings, 1 reply; 13+ messages in thread
From: Jason Gunthorpe @ 2018-01-18 19:24 UTC (permalink / raw
To: Jarkko Sakkinen; +Cc: Alexander Steffen, linux-integrity
On Thu, Jan 18, 2018 at 08:45:10PM +0200, Jarkko Sakkinen wrote:
> For SGX code that I've been upstreaming to arch/x86 tree the feedback
> was that local variable declarations should be in line length order,
> which makes sense to me i.e. startup_succesful should be the first
> declaration.
AFAIK that 'reverse christmas tree' style particularly local to the
net tree, not a general kernel style guideline. We've never used it in
TPM for instance.
Jason
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [RFC][PATCH 3/3] tpm-chip: Export TPM device to user space even when startup failed
2018-01-18 19:24 ` Jason Gunthorpe
@ 2018-01-23 12:39 ` Jarkko Sakkinen
0 siblings, 0 replies; 13+ messages in thread
From: Jarkko Sakkinen @ 2018-01-23 12:39 UTC (permalink / raw
To: Jason Gunthorpe; +Cc: Alexander Steffen, linux-integrity
On Thu, Jan 18, 2018 at 12:24:41PM -0700, Jason Gunthorpe wrote:
> On Thu, Jan 18, 2018 at 08:45:10PM +0200, Jarkko Sakkinen wrote:
>
> > For SGX code that I've been upstreaming to arch/x86 tree the feedback
> > was that local variable declarations should be in line length order,
> > which makes sense to me i.e. startup_succesful should be the first
> > declaration.
>
> AFAIK that 'reverse christmas tree' style particularly local to the
> net tree, not a general kernel style guideline. We've never used it in
> TPM for instance.
>
> Jason
And apparently also for x86 but if it is not general principle I'm fine
not using it.
/Jarkko
^ permalink raw reply [flat|nested] 13+ messages in thread
end of thread, other threads:[~2018-01-23 12:39 UTC | newest]
Thread overview: 13+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2017-12-14 16:06 [RFC][PATCH 0/3] Export broken TPMs to user space Alexander Steffen
2017-12-14 16:06 ` [RFC][PATCH 1/3] tpm-chip: Move idr_replace calls to appropriate places Alexander Steffen
2017-12-14 19:27 ` Jason Gunthorpe
2017-12-15 9:26 ` Alexander.Steffen
2018-01-18 18:38 ` Jarkko Sakkinen
2018-01-18 18:41 ` Jarkko Sakkinen
2017-12-14 16:06 ` [RFC][PATCH 2/3] tpm-chip: Return TPM error codes from auto_startup functions Alexander Steffen
2017-12-14 16:06 ` [RFC][PATCH 3/3] tpm-chip: Export TPM device to user space even when startup failed Alexander Steffen
2017-12-14 19:28 ` Jason Gunthorpe
2018-01-18 18:45 ` Jarkko Sakkinen
2018-01-18 19:24 ` Jason Gunthorpe
2018-01-23 12:39 ` Jarkko Sakkinen
2017-12-15 14:47 ` [RFC][PATCH 0/3] Export broken TPMs to user space Jarkko Sakkinen
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.