| Message ID | 1562211121-2188-1-git-send-email-nayna@linux.ibm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | tpm: fixes uninitialized allocated banks for IBM vtpm driver | expand |
On Wed, 2019-07-03 at 23:32 -0400, 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") > Signed-off-by: Nayna Jain <nayna@linux.ibm.com> Reviewed-by: Mimi Zohar <zohar@linux.ibm.com>
> On 04-Jul-2019, at 5:29 PM, Mimi Zohar <zohar@linux.ibm.com> wrote: > > On Wed, 2019-07-03 at 23:32 -0400, 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") >> Signed-off-by: Nayna Jain <nayna@linux.ibm.com> > Reviewed-by: Mimi Zohar <zohar@linux.ibm.com> Thanks for the fix. Kernel boots fine with this fix. Tested-by: Sachin Sant <sachinp@linux.vnet.ibm.com> Thanks -Sachin
On Thu, 4 Jul 2019 19:26:36 +0530 Sachin Sant <sachinp@linux.vnet.ibm.com> wrote: > > On 04-Jul-2019, at 5:29 PM, Mimi Zohar <zohar@linux.ibm.com> wrote: > > > > On Wed, 2019-07-03 at 23:32 -0400, 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") > >> Signed-off-by: Nayna Jain <nayna@linux.ibm.com> > > Reviewed-by: Mimi Zohar <zohar@linux.ibm.com> > > Thanks for the fix. Kernel boots fine with this fix. > > Tested-by: Sachin Sant <sachinp@linux.vnet.ibm.com> > Tested-by: Michal Suchánek <msuchanek@suse.de> Thanks Michal
On Wed, 2019-07-03 at 23:32 -0400, 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") > Signed-off-by: Nayna Jain <nayna@linux.ibm.com> Please add Reported-by: Michal Suchanek <msuchanek@suse.de> It is missing. Michal is there a chance you could try this out once Nayna send a new version? > --- > drivers/char/tpm/tpm-chip.c | 37 +++++++++++++++++++++++++++++++++++++ > drivers/char/tpm/tpm.h | 1 + > drivers/char/tpm/tpm1-cmd.c | 12 ------------ > drivers/char/tpm/tpm2-cmd.c | 6 +----- > 4 files changed, 39 insertions(+), 17 deletions(-) > > diff --git a/drivers/char/tpm/tpm-chip.c b/drivers/char/tpm/tpm-chip.c > index 8804c9e916fd..958508bb8379 100644 > --- a/drivers/char/tpm/tpm-chip.c > +++ b/drivers/char/tpm/tpm-chip.c > @@ -550,6 +550,39 @@ static int tpm_add_hwrng(struct tpm_chip *chip) > return hwrng_register(&chip->hwrng); > } > > +/* > + * tpm_pcr_allocation() - initializes the chip allocated banks for PCRs > + */ > +static int tpm_pcr_allocation(struct tpm_chip *chip) Why that name and not tpm_get_pcr_allocation()? Do not get why "get_" has been dropped. Please add it back. Would be senseful to create tpm1_get_pcr_allocation() to tpm1-cmd.c now that a new function needs to be introduced anyway. Please do it for that for TPM 1.x part. /Jarkko
On Fri, 05 Jul 2019 13:42:18 +0300 Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> wrote: > On Wed, 2019-07-03 at 23:32 -0400, 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") > > Signed-off-by: Nayna Jain <nayna@linux.ibm.com> > > Please add > > Reported-by: Michal Suchanek <msuchanek@suse.de> > > It is missing. Michal is there a chance you could try this out once > Nayna send a new version? Should be easy to test. Without the patch the machine crashes on boot so it is quite obvious case. I have a few VMs with the vTPM available. Thanks Michal
On Fri, 2019-07-05 at 13:42 +0300, Jarkko Sakkinen wrote: > On Wed, 2019-07-03 at 23:32 -0400, 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") > > Signed-off-by: Nayna Jain <nayna@linux.ibm.com> > > Please add > > Reported-by: Michal Suchanek <msuchanek@suse.de> > > It is missing. Michal is there a chance you could try this out once > Nayna send a new version? Hey, I saw Michal's tested-by. I can do that minor reorg cosmetic bits myeslf and add Micha's tag. Some issue with the network but I'll push a commit soonish. /Jarkko
On 7/3/19 11:32 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") > Signed-off-by: Nayna Jain <nayna@linux.ibm.com> > --- > drivers/char/tpm/tpm-chip.c | 37 +++++++++++++++++++++++++++++++++++++ > drivers/char/tpm/tpm.h | 1 + > drivers/char/tpm/tpm1-cmd.c | 12 ------------ > drivers/char/tpm/tpm2-cmd.c | 6 +----- > 4 files changed, 39 insertions(+), 17 deletions(-) > > diff --git a/drivers/char/tpm/tpm-chip.c b/drivers/char/tpm/tpm-chip.c > index 8804c9e916fd..958508bb8379 100644 > --- a/drivers/char/tpm/tpm-chip.c > +++ b/drivers/char/tpm/tpm-chip.c > @@ -550,6 +550,39 @@ static int tpm_add_hwrng(struct tpm_chip *chip) > return hwrng_register(&chip->hwrng); > } > > +/* > + * tpm_pcr_allocation() - initializes the chip allocated banks for PCRs > + */ > +static int tpm_pcr_allocation(struct tpm_chip *chip) > +{ > + int rc = 0; > + > + if (chip->flags & TPM_CHIP_FLAG_TPM2) { > + rc = tpm2_get_pcr_allocation(chip); > + if (rc) > + goto out; > + } > + > + /* Initialize TPM 1.2 */ > + 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 0; > +out: > + if (rc < 0) > + rc = -ENODEV; The old code where you lifted this from said: out: if (rc > 0) rc = -ENODEV; return rc; It would not overwrite -ENOMEM with -ENODEV but yours does. I think the correct fix would be to use: if (rc > 0) rc = -ENODEV; > + return rc; > +} > + > /* > * tpm_chip_register() - create a character device for the TPM chip > * @chip: TPM chip to use. > @@ -573,6 +606,10 @@ int tpm_chip_register(struct tpm_chip *chip) > if (rc) > return rc; Above this is tpm_chip_stop(chip) because (afaik) none of the following function calls in tpm_chip_register() needed the TPM, but now with tpm_pcr_allocation() you will need to send a command to the TPM. So I would say you should move the tpm_chip_stop() into the error branch visible above and also after the tpm_pcr_allocation(). > + rc = tpm_pcr_allocation(chip); tpm_chip_stop(chip); > + if (rc) > + return rc; > + > tpm_sysfs_add_device(chip); > > rc = tpm_bios_log_setup(chip); Stefan
On 07/05/2019 10:13 AM, Stefan Berger wrote: > On 7/3/19 11:32 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") >> Signed-off-by: Nayna Jain <nayna@linux.ibm.com> >> --- >> drivers/char/tpm/tpm-chip.c | 37 +++++++++++++++++++++++++++++++++++++ >> drivers/char/tpm/tpm.h | 1 + >> drivers/char/tpm/tpm1-cmd.c | 12 ------------ >> drivers/char/tpm/tpm2-cmd.c | 6 +----- >> 4 files changed, 39 insertions(+), 17 deletions(-) >> >> diff --git a/drivers/char/tpm/tpm-chip.c b/drivers/char/tpm/tpm-chip.c >> index 8804c9e916fd..958508bb8379 100644 >> --- a/drivers/char/tpm/tpm-chip.c >> +++ b/drivers/char/tpm/tpm-chip.c >> @@ -550,6 +550,39 @@ static int tpm_add_hwrng(struct tpm_chip *chip) >> return hwrng_register(&chip->hwrng); >> } >> >> +/* >> + * tpm_pcr_allocation() - initializes the chip allocated banks for PCRs >> + */ >> +static int tpm_pcr_allocation(struct tpm_chip *chip) >> +{ >> + int rc = 0; >> + >> + if (chip->flags & TPM_CHIP_FLAG_TPM2) { >> + rc = tpm2_get_pcr_allocation(chip); >> + if (rc) >> + goto out; >> + } >> + >> + /* Initialize TPM 1.2 */ >> + 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 0; >> +out: >> + if (rc < 0) >> + rc = -ENODEV; > > > The old code where you lifted this from said: > > out: > if (rc > 0) > rc = -ENODEV; > return rc; > > It would not overwrite -ENOMEM with -ENODEV but yours does. > > I think the correct fix would be to use: > > if (rc > 0) > > rc = -ENODEV; > Yes. I think I misread it. Thanks Stefan. Will fix this.. > > > > >> + return rc; >> +} >> + >> /* >> * tpm_chip_register() - create a character device for the TPM chip >> * @chip: TPM chip to use. >> @@ -573,6 +606,10 @@ int tpm_chip_register(struct tpm_chip *chip) >> if (rc) >> return rc; > > Above this is tpm_chip_stop(chip) because (afaik) none of the > following function calls in tpm_chip_register() needed the TPM, but > now with tpm_pcr_allocation() you will need to send a command to the > TPM. So I would say you should move the tpm_chip_stop() into the error > branch visible above and also after the tpm_pcr_allocation(). > > >> + rc = tpm_pcr_allocation(chip); > tpm_chip_stop(chip); I am not sure of the purpose of tpm_stop_chip(), so I have left it as it is. Jarkko, what do you think about the change ? Thanks & Regards, - Nayna
On Fri, 2019-07-05 at 11:32 -0400, Nayna wrote: > I am not sure of the purpose of tpm_stop_chip(), so I have left it as it > is. Jarkko, what do you think about the change ? Stefan right. Your does not work, or will randomly work or not work depending on the chip. You need to turn the TPM on with tpm_chip_start() and turn it off with tpm_chip_stop() once you are done. This is done in tpm_chip_register() before calling tpm_auto_startup(). TPM power management was once in tpm_transmit() but not anymore after my patch set that removed nested tpm_transmit() calls. While you're on it please take into account my earlier feedback. Also, short summary could be "tpm: tpm_ibm_vtpm: Fix unallocated banks" Some oddballs in your patch that I have to ask. if (chip->flags & TPM_CHIP_FLAG_TPM2) { rc = tpm2_get_pcr_allocation(chip); if (rc) goto out; } chip->allocated_banks = kcalloc(1, sizeof(*chip->allocated_banks), GFP_KERNEL); if (!chip->allocated_banks) { rc = -ENOMEM; goto out; } Why you don't return on site and instead jump somewhere? Also the 2nd line for kcalloc() is misaligned. out: if (rc < 0) rc = -ENODEV; This will cause a new regression i.e. you let TPM error codes through. To summarize this patch fixes one regression and introduces two completely new ones... /Jarkko`
On Fri, 2019-07-05 at 20:50 +0300, Jarkko Sakkinen wrote: > To summarize this patch fixes one regression and introduces two > completely new ones... Anyway, take the time and update it. The principle is right anyway. I'll merge it once it is in a better shape... /Jarkko
On 07/05/2019 01:50 PM, Jarkko Sakkinen wrote: > On Fri, 2019-07-05 at 11:32 -0400, Nayna wrote: >> I am not sure of the purpose of tpm_stop_chip(), so I have left it as it >> is. Jarkko, what do you think about the change ? > Stefan right. Your does not work, or will randomly work or not work > depending on the chip. > > You need to turn the TPM on with tpm_chip_start() and turn it off with > tpm_chip_stop() once you are done. This is done in tpm_chip_register() > before calling tpm_auto_startup(). > > TPM power management was once in tpm_transmit() but not anymore after my > patch set that removed nested tpm_transmit() calls. > > While you're on it please take into account my earlier feedback. > > Also, short summary could be "tpm: tpm_ibm_vtpm: Fix unallocated banks" > > Some oddballs in your patch that I have to ask. > > if (chip->flags & TPM_CHIP_FLAG_TPM2) { > rc = tpm2_get_pcr_allocation(chip); > if (rc) > goto out; > } > > chip->allocated_banks = kcalloc(1, sizeof(*chip->allocated_banks), > GFP_KERNEL); > if (!chip->allocated_banks) { > rc = -ENOMEM; > goto out; > } > > Why you don't return on site and instead jump somewhere? Also the > 2nd line for kcalloc() is misaligned. > > out: > if (rc < 0) > rc = -ENODEV; > > This will cause a new regression i.e. you let TPM error codes > through. > > To summarize this patch fixes one regression and introduces two > completely new ones... Thanks Jarkko. I just now posted the v2 version that includes your and Stefan's feedbacks. Thanks & Regards, - Nayna
On Sat, 2019-07-06 at 20:25 -0400, Nayna wrote: > Thanks Jarkko. I just now posted the v2 version that includes your and > Stefan's feedbacks. Looks almost legit :-) /Jarkko
diff --git a/drivers/char/tpm/tpm-chip.c b/drivers/char/tpm/tpm-chip.c index 8804c9e916fd..958508bb8379 100644 --- a/drivers/char/tpm/tpm-chip.c +++ b/drivers/char/tpm/tpm-chip.c @@ -550,6 +550,39 @@ static int tpm_add_hwrng(struct tpm_chip *chip) return hwrng_register(&chip->hwrng); } +/* + * tpm_pcr_allocation() - initializes the chip allocated banks for PCRs + */ +static int tpm_pcr_allocation(struct tpm_chip *chip) +{ + int rc = 0; + + if (chip->flags & TPM_CHIP_FLAG_TPM2) { + rc = tpm2_get_pcr_allocation(chip); + if (rc) + goto out; + } + + /* Initialize TPM 1.2 */ + 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 0; +out: + if (rc < 0) + rc = -ENODEV; + return rc; +} + /* * tpm_chip_register() - create a character device for the TPM chip * @chip: TPM chip to use. @@ -573,6 +606,10 @@ int tpm_chip_register(struct tpm_chip *chip) if (rc) return rc; + rc = tpm_pcr_allocation(chip); + if (rc) + return rc; + tpm_sysfs_add_device(chip); rc = tpm_bios_log_setup(chip); diff --git a/drivers/char/tpm/tpm.h b/drivers/char/tpm/tpm.h index 2cce072f25b5..eabe6b755fa6 100644 --- a/drivers/char/tpm/tpm.h +++ b/drivers/char/tpm/tpm.h @@ -454,6 +454,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..ec5f3693c096 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) 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") Signed-off-by: Nayna Jain <nayna@linux.ibm.com> --- drivers/char/tpm/tpm-chip.c | 37 +++++++++++++++++++++++++++++++++++++ drivers/char/tpm/tpm.h | 1 + drivers/char/tpm/tpm1-cmd.c | 12 ------------ drivers/char/tpm/tpm2-cmd.c | 6 +----- 4 files changed, 39 insertions(+), 17 deletions(-)