| Message ID | 20171214160614.11808-4-Alexander.Steffen@infineon.com (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
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
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
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
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
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; }
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(-)