| Message ID | 1562458725-15999-1-git-send-email-nayna@linux.ibm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2] tpm: tpm_ibm_vtpm: Fix unallocated banks | expand |
On 7/6/19 8:18 PM, Nayna Jain wrote: > The nr_allocated_banks and allocated banks are initialized as part of > tpm_chip_register. Currently, this is done as part of auto startup > function. However, some drivers, like the ibm vtpm driver, do not run > auto startup during initialization. This results in uninitialized memory > issue and causes a kernel panic during boot. > > This patch moves the pcr allocation outside the auto startup function > into tpm_chip_register. This ensures that allocated banks are initialized > in any case. > > Fixes: 879b589210a9 ("tpm: retrieve digest size of unknown algorithms with > PCR read") > Reported-by: Michal Suchanek <msuchanek@suse.de> > Signed-off-by: Nayna Jain <nayna@linux.ibm.com> > Reviewed-by: Mimi Zohar <zohar@linux.ibm.com> > Tested-by: Sachin Sant <sachinp@linux.vnet.ibm.com> > Tested-by: Michal Suchánek <msuchanek@suse.de> > --- > Changelog: > > v2: > * Includes Jarkko's feedbacks > * fixes the function name to tpm_get_pcr_allocation() > * adds new function tpm1_get_pcr_allocation() > * updates patch summary line > * fixes alignment > * adds Reported-by: Michal Suchanek <msuchanek@suse.de> > * Includes Stefan's feedbacks > * Fixes overwriting of return code > * Fixes misplacing of tpm_chip_stop() > * Adds Reviewed-by, Tested-by > > drivers/char/tpm/tpm-chip.c | 22 ++++++++++++++++++++++ > drivers/char/tpm/tpm.h | 2 ++ > drivers/char/tpm/tpm1-cmd.c | 36 ++++++++++++++++++++++++------------ > drivers/char/tpm/tpm2-cmd.c | 6 +----- > 4 files changed, 49 insertions(+), 17 deletions(-) > > diff --git a/drivers/char/tpm/tpm-chip.c b/drivers/char/tpm/tpm-chip.c > index 8804c9e916fd..6589291df355 100644 > --- a/drivers/char/tpm/tpm-chip.c > +++ b/drivers/char/tpm/tpm-chip.c > @@ -550,6 +550,22 @@ static int tpm_add_hwrng(struct tpm_chip *chip) > return hwrng_register(&chip->hwrng); > } > > +/* > + * tpm_get_pcr_allocation() - initialize the chip allocated banks for PCRs > + * @chip: TPM chip to use. > + */ > +static int tpm_get_pcr_allocation(struct tpm_chip *chip) > +{ > + int rc; > + > + if (chip->flags & TPM_CHIP_FLAG_TPM2) > + rc = tpm2_get_pcr_allocation(chip); For tpm2 case you need: if (rc > 0) rc = -ENODEV; Because tpm2_get_pcr_allocation(chip) returns ssize_t with return values from tpm_transmit_cmd > 0 indicating a TPM 2 error code... Stefan
On Sat, 2019-07-06 at 20:18 -0400, Nayna Jain wrote: > +/* > + * tpm_get_pcr_allocation() - initialize the chip allocated banks for PCRs > + * @chip: TPM chip to use. > + */ > +static int tpm_get_pcr_allocation(struct tpm_chip *chip) > +{ > + int rc; > + > + if (chip->flags & TPM_CHIP_FLAG_TPM2) > + rc = tpm2_get_pcr_allocation(chip); > + else > + rc = tpm1_get_pcr_allocation(chip); > + > + return rc; > +} It is just a trivial static function, which means that kdoc comment is not required and neither it is useful. Please remove that. I would rewrite the function like: static int tpm_get_pcr_allocation(struct tpm_chip *chip) { int rc; rc = (chip->flags & TPM_CHIP_FLAG_TPM2) ? tpm2_get_pcr_allocation(chip) : tpm1_get_pcr_allocation(chip); return rc > 0 ? -ENODEV : rc; } This addresses the issue that Stefan also pointed out. You have to deal with the TPM error codes. /Jarkko
Hi Jarkko, On Mon, 2019-07-08 at 18:11 +0300, Jarkko Sakkinen wrote: > On Sat, 2019-07-06 at 20:18 -0400, Nayna Jain wrote: > > +/* > > + * tpm_get_pcr_allocation() - initialize the chip allocated banks for PCRs > > + * @chip: TPM chip to use. > > + */ > > +static int tpm_get_pcr_allocation(struct tpm_chip *chip) > > +{ > > + int rc; > > + > > + if (chip->flags & TPM_CHIP_FLAG_TPM2) > > + rc = tpm2_get_pcr_allocation(chip); > > + else > > + rc = tpm1_get_pcr_allocation(chip); > > + > > + return rc; > > +} > > It is just a trivial static function, which means that kdoc comment is > not required and neither it is useful. Please remove that. I would > rewrite the function like: > > static int tpm_get_pcr_allocation(struct tpm_chip *chip) > { > int rc; > > rc = (chip->flags & TPM_CHIP_FLAG_TPM2) ? > tpm2_get_pcr_allocation(chip) : > tpm1_get_pcr_allocation(chip); > > return rc > 0 ? -ENODEV : rc; > } > > This addresses the issue that Stefan also pointed out. You have to > deal with the TPM error codes. Hm, in the past I was told by Christoph not to use the ternary operator. Have things changed? Other than removing the comment, the only other difference is the return. Mimi
On Mon, Jul 08, 2019 at 06:24:04PM -0400, Mimi Zohar wrote: > > static int tpm_get_pcr_allocation(struct tpm_chip *chip) > > { > > int rc; > > > > rc = (chip->flags & TPM_CHIP_FLAG_TPM2) ? > > tpm2_get_pcr_allocation(chip) : > > tpm1_get_pcr_allocation(chip); > > > > > return rc > 0 ? -ENODEV : rc; > > } > > > > This addresses the issue that Stefan also pointed out. You have to > > deal with the TPM error codes. > > Hm, in the past I was told by Christoph not to use the ternary > operator. Have things changed? Other than removing the comment, the > only other difference is the return. In the end it is a matter of personal preference, but I find the quote version above using the ternary horribly obsfucated.
On Mon, Jul 08, 2019 at 06:24:04PM -0400, Mimi Zohar wrote: > Hi Jarkko, > > On Mon, 2019-07-08 at 18:11 +0300, Jarkko Sakkinen wrote: > > On Sat, 2019-07-06 at 20:18 -0400, Nayna Jain wrote: > > > +/* > > > + * tpm_get_pcr_allocation() - initialize the chip allocated banks for PCRs > > > + * @chip: TPM chip to use. > > > + */ > > > +static int tpm_get_pcr_allocation(struct tpm_chip *chip) > > > +{ > > > + int rc; > > > + > > > + if (chip->flags & TPM_CHIP_FLAG_TPM2) > > > + rc = tpm2_get_pcr_allocation(chip); > > > + else > > > + rc = tpm1_get_pcr_allocation(chip); > > > + > > > + return rc; > > > +} > > > > It is just a trivial static function, which means that kdoc comment is > > not required and neither it is useful. Please remove that. I would > > rewrite the function like: > > > > static int tpm_get_pcr_allocation(struct tpm_chip *chip) > > { > > int rc; > > > > rc = (chip->flags & TPM_CHIP_FLAG_TPM2) ? > > tpm2_get_pcr_allocation(chip) : > > tpm1_get_pcr_allocation(chip); > > > > > return rc > 0 ? -ENODEV : rc; > > } > > > > This addresses the issue that Stefan also pointed out. You have to > > deal with the TPM error codes. > > Hm, in the past I was told by Christoph not to use the ternary > operator. Have things changed? Other than removing the comment, the > only other difference is the return. I also dislike most use of ternary expressions.. Also, it is not so nice that TPM error codes and errno are multiplexed on the same 'int' type - very hard to get this right. I'm not sure anything actually needs the tpm error, and if it does maybe we should be mapping those special cases to errno instead. Jason
On Mon, Jul 08, 2019 at 06:24:04PM -0400, Mimi Zohar wrote: > > static int tpm_get_pcr_allocation(struct tpm_chip *chip) > > { > > int rc; > > > > rc = (chip->flags & TPM_CHIP_FLAG_TPM2) ? > > tpm2_get_pcr_allocation(chip) : > > tpm1_get_pcr_allocation(chip); > > > > > return rc > 0 ? -ENODEV : rc; > > } > > > > This addresses the issue that Stefan also pointed out. You have to > > deal with the TPM error codes. > > Hm, in the past I was told by Christoph not to use the ternary > operator. Have things changed? Other than removing the comment, the > only other difference is the return. Lets purge the snippet: rc = (chip->flags & TPM_CHIP_FLAG_TPM2) ? tpm2_get_pcr_allocation(chip) : tpm1_get_pcr_allocation(chip); In this statement ternary operator does make sense because it is the most readable way to express what is going on. We assign something selecting one of the two options as the value of rc based on a condition. It is like a natural fit for a ternary-operation and less messy than two assigment statements. On the other hand: return rc > 0 ? -ENODEV : rc; Here a better form would definitely be: if (rc > 0) return - ENODEV; return rc; It is just two hard to grasp the logic when ternary operation is used. Total ban of any language construct would be just utterly stupid. I would instead use common sense here. /Jarkko
On Mon, Jul 08, 2019 at 03:43:04PM -0700, Christoph Hellwig wrote: > On Mon, Jul 08, 2019 at 06:24:04PM -0400, Mimi Zohar wrote: > > > static int tpm_get_pcr_allocation(struct tpm_chip *chip) > > > { > > > int rc; > > > > > > rc = (chip->flags & TPM_CHIP_FLAG_TPM2) ? > > > tpm2_get_pcr_allocation(chip) : > > > tpm1_get_pcr_allocation(chip); > > > > > > > > return rc > 0 ? -ENODEV : rc; > > > } > > > > > > This addresses the issue that Stefan also pointed out. You have to > > > deal with the TPM error codes. > > > > Hm, in the past I was told by Christoph not to use the ternary > > operator. Have things changed? Other than removing the comment, the > > only other difference is the return. > > In the end it is a matter of personal preference, but I find the > quote version above using the ternary horribly obsfucated. I fully agree that the return statement is an obsfucated mess and not a good place at all for using ternary operator. /Jarkko
Hi Jarkko, On 07/09/2019 12:38 PM, Jarkko Sakkinen wrote: > On Mon, Jul 08, 2019 at 03:43:04PM -0700, Christoph Hellwig wrote: >> On Mon, Jul 08, 2019 at 06:24:04PM -0400, Mimi Zohar wrote: >>>> static int tpm_get_pcr_allocation(struct tpm_chip *chip) >>>> { >>>> int rc; >>>> >>>> rc = (chip->flags & TPM_CHIP_FLAG_TPM2) ? >>>> tpm2_get_pcr_allocation(chip) : >>>> tpm1_get_pcr_allocation(chip); >>>> return rc > 0 ? -ENODEV : rc; >>>> } >>>> >>>> This addresses the issue that Stefan also pointed out. You have to >>>> deal with the TPM error codes. >>> Hm, in the past I was told by Christoph not to use the ternary >>> operator. Have things changed? Other than removing the comment, the >>> only other difference is the return. >> In the end it is a matter of personal preference, but I find the >> quote version above using the ternary horribly obsfucated. > I fully agree that the return statement is an obsfucated mess and > not a good place at all for using ternary operator. I have posted the v3 version that includes the suggested corrections by you and Stefan. Sorry for some delay. Michal and Sachin, I would appreciate if you can test the v3 version, please ? Thanks & Regards, - Nayna
diff --git a/drivers/char/tpm/tpm-chip.c b/drivers/char/tpm/tpm-chip.c index 8804c9e916fd..6589291df355 100644 --- a/drivers/char/tpm/tpm-chip.c +++ b/drivers/char/tpm/tpm-chip.c @@ -550,6 +550,22 @@ static int tpm_add_hwrng(struct tpm_chip *chip) return hwrng_register(&chip->hwrng); } +/* + * tpm_get_pcr_allocation() - initialize the chip allocated banks for PCRs + * @chip: TPM chip to use. + */ +static int tpm_get_pcr_allocation(struct tpm_chip *chip) +{ + int rc; + + if (chip->flags & TPM_CHIP_FLAG_TPM2) + rc = tpm2_get_pcr_allocation(chip); + else + rc = tpm1_get_pcr_allocation(chip); + + return rc; +} + /* * tpm_chip_register() - create a character device for the TPM chip * @chip: TPM chip to use. @@ -569,6 +585,12 @@ int tpm_chip_register(struct tpm_chip *chip) if (rc) return rc; rc = tpm_auto_startup(chip); + if (rc) { + tpm_chip_stop(chip); + return rc; + } + + rc = tpm_get_pcr_allocation(chip); tpm_chip_stop(chip); if (rc) return rc; diff --git a/drivers/char/tpm/tpm.h b/drivers/char/tpm/tpm.h index 2cce072f25b5..d571df3694c3 100644 --- a/drivers/char/tpm/tpm.h +++ b/drivers/char/tpm/tpm.h @@ -399,6 +399,7 @@ int tpm1_pcr_read(struct tpm_chip *chip, u32 pcr_idx, u8 *res_buf); ssize_t tpm1_getcap(struct tpm_chip *chip, u32 subcap_id, cap_t *cap, const char *desc, size_t min_cap_length); int tpm1_get_random(struct tpm_chip *chip, u8 *out, size_t max); +int tpm1_get_pcr_allocation(struct tpm_chip *chip); unsigned long tpm_calc_ordinal_duration(struct tpm_chip *chip, u32 ordinal); int tpm_pm_suspend(struct device *dev); int tpm_pm_resume(struct device *dev); @@ -454,6 +455,7 @@ int tpm2_unseal_trusted(struct tpm_chip *chip, ssize_t tpm2_get_tpm_pt(struct tpm_chip *chip, u32 property_id, u32 *value, const char *desc); +ssize_t tpm2_get_pcr_allocation(struct tpm_chip *chip); int tpm2_auto_startup(struct tpm_chip *chip); void tpm2_shutdown(struct tpm_chip *chip, u16 shutdown_type); unsigned long tpm2_calc_ordinal_duration(struct tpm_chip *chip, u32 ordinal); diff --git a/drivers/char/tpm/tpm1-cmd.c b/drivers/char/tpm/tpm1-cmd.c index 85dcf2654d11..260a3917f0fe 100644 --- a/drivers/char/tpm/tpm1-cmd.c +++ b/drivers/char/tpm/tpm1-cmd.c @@ -696,18 +696,6 @@ int tpm1_auto_startup(struct tpm_chip *chip) goto out; } - chip->allocated_banks = kcalloc(1, sizeof(*chip->allocated_banks), - GFP_KERNEL); - if (!chip->allocated_banks) { - rc = -ENOMEM; - goto out; - } - - chip->allocated_banks[0].alg_id = TPM_ALG_SHA1; - chip->allocated_banks[0].digest_size = hash_digest_size[HASH_ALGO_SHA1]; - chip->allocated_banks[0].crypto_id = HASH_ALGO_SHA1; - chip->nr_allocated_banks = 1; - return rc; out: if (rc > 0) @@ -776,3 +764,27 @@ int tpm1_pm_suspend(struct tpm_chip *chip, u32 tpm_suspend_pcr) return rc; } +/** + * tpm1_get_pcr_allocation() - initialize the allocated bank + * @chip: TPM chip to use. + * + * The function initializes the SHA1 allocated bank to extend PCR + * + * Return: + * * 0 on success, + * * < 0 on error. + */ +int tpm1_get_pcr_allocation(struct tpm_chip *chip) +{ + chip->allocated_banks = kcalloc(1, sizeof(*chip->allocated_banks), + GFP_KERNEL); + if (!chip->allocated_banks) + return -ENOMEM; + + chip->allocated_banks[0].alg_id = TPM_ALG_SHA1; + chip->allocated_banks[0].digest_size = hash_digest_size[HASH_ALGO_SHA1]; + chip->allocated_banks[0].crypto_id = HASH_ALGO_SHA1; + chip->nr_allocated_banks = 1; + + return 0; +} diff --git a/drivers/char/tpm/tpm2-cmd.c b/drivers/char/tpm/tpm2-cmd.c index e74c5b7b64bf..b4384d0e3741 100644 --- a/drivers/char/tpm/tpm2-cmd.c +++ b/drivers/char/tpm/tpm2-cmd.c @@ -841,7 +841,7 @@ struct tpm2_pcr_selection { u8 pcr_select[3]; } __packed; -static ssize_t tpm2_get_pcr_allocation(struct tpm_chip *chip) +ssize_t tpm2_get_pcr_allocation(struct tpm_chip *chip) { struct tpm2_pcr_selection pcr_selection; struct tpm_buf buf; @@ -1041,10 +1041,6 @@ int tpm2_auto_startup(struct tpm_chip *chip) goto out; } - rc = tpm2_get_pcr_allocation(chip); - if (rc) - goto out; - rc = tpm2_get_cc_attrs_tbl(chip); out:
The nr_allocated_banks and allocated banks are initialized as part of tpm_chip_register. Currently, this is done as part of auto startup function. However, some drivers, like the ibm vtpm driver, do not run auto startup during initialization. This results in uninitialized memory issue and causes a kernel panic during boot. This patch moves the pcr allocation outside the auto startup function into tpm_chip_register. This ensures that allocated banks are initialized in any case. Fixes: 879b589210a9 ("tpm: retrieve digest size of unknown algorithms with PCR read") Reported-by: Michal Suchanek <msuchanek@suse.de> Signed-off-by: Nayna Jain <nayna@linux.ibm.com> Reviewed-by: Mimi Zohar <zohar@linux.ibm.com> Tested-by: Sachin Sant <sachinp@linux.vnet.ibm.com> Tested-by: Michal Suchánek <msuchanek@suse.de> --- Changelog: v2: * Includes Jarkko's feedbacks * fixes the function name to tpm_get_pcr_allocation() * adds new function tpm1_get_pcr_allocation() * updates patch summary line * fixes alignment * adds Reported-by: Michal Suchanek <msuchanek@suse.de> * Includes Stefan's feedbacks * Fixes overwriting of return code * Fixes misplacing of tpm_chip_stop() * Adds Reviewed-by, Tested-by drivers/char/tpm/tpm-chip.c | 22 ++++++++++++++++++++++ drivers/char/tpm/tpm.h | 2 ++ drivers/char/tpm/tpm1-cmd.c | 36 ++++++++++++++++++++++++------------ drivers/char/tpm/tpm2-cmd.c | 6 +----- 4 files changed, 49 insertions(+), 17 deletions(-)