| Message ID | 1576173515.15886.7.camel@HansenPartnership.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [URGENT,FIX] security: keys: trusted: fix lost handle flush | expand |
On Thu Dec 12 19, James Bottomley wrote: >The original code, before it was moved into security/keys/trusted-keys >had a flush after the blob unseal. Without that flush, the volatile >handles increase in the TPM until it becomes unusable and the system >either has to be rebooted or the TPM volatile area manually flushed. >Fix by adding back the lost flush, which we now have to export because >of the relocation of the trusted key code may cause the consumer to be >modular. > >Signed-off-by: James Bottomley <James.Bottomley@HansenPartnership.com> >Fixes: 2e19e10131a0 ("KEYS: trusted: Move TPM2 trusted keys code") > Reviewed-by: Jerry Snitselaar <jsnitsel@redhat.com> >--- > drivers/char/tpm/tpm.h | 1 - > drivers/char/tpm/tpm2-cmd.c | 1 + > include/linux/tpm.h | 1 + > security/keys/trusted-keys/trusted_tpm2.c | 1 + > 4 files changed, 3 insertions(+), 1 deletion(-) > >diff --git a/drivers/char/tpm/tpm.h b/drivers/char/tpm/tpm.h >index b9e1547be6b5..5620747da0cf 100644 >--- a/drivers/char/tpm/tpm.h >+++ b/drivers/char/tpm/tpm.h >@@ -218,7 +218,6 @@ int tpm2_pcr_read(struct tpm_chip *chip, u32 pcr_idx, > int tpm2_pcr_extend(struct tpm_chip *chip, u32 pcr_idx, > struct tpm_digest *digests); > int tpm2_get_random(struct tpm_chip *chip, u8 *dest, size_t max); >-void tpm2_flush_context(struct tpm_chip *chip, u32 handle); > ssize_t tpm2_get_tpm_pt(struct tpm_chip *chip, u32 property_id, > u32 *value, const char *desc); > >diff --git a/drivers/char/tpm/tpm2-cmd.c b/drivers/char/tpm/tpm2-cmd.c >index fdb457704aa7..13696deceae8 100644 >--- a/drivers/char/tpm/tpm2-cmd.c >+++ b/drivers/char/tpm/tpm2-cmd.c >@@ -362,6 +362,7 @@ void tpm2_flush_context(struct tpm_chip *chip, u32 handle) > tpm_transmit_cmd(chip, &buf, 0, "flushing context"); > tpm_buf_destroy(&buf); > } >+EXPORT_SYMBOL_GPL(tpm2_flush_context); > > struct tpm2_get_cap_out { > u8 more_data; >diff --git a/include/linux/tpm.h b/include/linux/tpm.h >index 0d6e949ba315..03e9b184411b 100644 >--- a/include/linux/tpm.h >+++ b/include/linux/tpm.h >@@ -403,6 +403,7 @@ extern int tpm_pcr_extend(struct tpm_chip *chip, u32 pcr_idx, > extern int tpm_send(struct tpm_chip *chip, void *cmd, size_t buflen); > extern int tpm_get_random(struct tpm_chip *chip, u8 *data, size_t max); > extern struct tpm_chip *tpm_default_chip(void); >+void tpm2_flush_context(struct tpm_chip *chip, u32 handle); > #else > static inline int tpm_is_tpm2(struct tpm_chip *chip) > { >diff --git a/security/keys/trusted-keys/trusted_tpm2.c b/security/keys/trusted-keys/trusted_tpm2.c >index a9810ac2776f..08ec7f48f01d 100644 >--- a/security/keys/trusted-keys/trusted_tpm2.c >+++ b/security/keys/trusted-keys/trusted_tpm2.c >@@ -309,6 +309,7 @@ int tpm2_unseal_trusted(struct tpm_chip *chip, > return rc; > > rc = tpm2_unseal_cmd(chip, payload, options, blob_handle); >+ tpm2_flush_context(chip, blob_handle); > > return rc; > } >-- >2.16.4 >
On Thu, 12 Dec 2019 at 23:28, James Bottomley <James.Bottomley@hansenpartnership.com> wrote: > > The original code, before it was moved into security/keys/trusted-keys > had a flush after the blob unseal. Without that flush, the volatile > handles increase in the TPM until it becomes unusable and the system > either has to be rebooted or the TPM volatile area manually flushed. > Fix by adding back the lost flush, which we now have to export because > of the relocation of the trusted key code may cause the consumer to be > modular. > > Signed-off-by: James Bottomley <James.Bottomley@HansenPartnership.com> > Fixes: 2e19e10131a0 ("KEYS: trusted: Move TPM2 trusted keys code") > Overall looks good to me with following minor comment. > --- > drivers/char/tpm/tpm.h | 1 - > drivers/char/tpm/tpm2-cmd.c | 1 + > include/linux/tpm.h | 1 + > security/keys/trusted-keys/trusted_tpm2.c | 1 + > 4 files changed, 3 insertions(+), 1 deletion(-) > > diff --git a/drivers/char/tpm/tpm.h b/drivers/char/tpm/tpm.h > index b9e1547be6b5..5620747da0cf 100644 > --- a/drivers/char/tpm/tpm.h > +++ b/drivers/char/tpm/tpm.h > @@ -218,7 +218,6 @@ int tpm2_pcr_read(struct tpm_chip *chip, u32 pcr_idx, > int tpm2_pcr_extend(struct tpm_chip *chip, u32 pcr_idx, > struct tpm_digest *digests); > int tpm2_get_random(struct tpm_chip *chip, u8 *dest, size_t max); > -void tpm2_flush_context(struct tpm_chip *chip, u32 handle); > ssize_t tpm2_get_tpm_pt(struct tpm_chip *chip, u32 property_id, > u32 *value, const char *desc); > > diff --git a/drivers/char/tpm/tpm2-cmd.c b/drivers/char/tpm/tpm2-cmd.c > index fdb457704aa7..13696deceae8 100644 > --- a/drivers/char/tpm/tpm2-cmd.c > +++ b/drivers/char/tpm/tpm2-cmd.c > @@ -362,6 +362,7 @@ void tpm2_flush_context(struct tpm_chip *chip, u32 handle) > tpm_transmit_cmd(chip, &buf, 0, "flushing context"); > tpm_buf_destroy(&buf); > } > +EXPORT_SYMBOL_GPL(tpm2_flush_context); > > struct tpm2_get_cap_out { > u8 more_data; > diff --git a/include/linux/tpm.h b/include/linux/tpm.h > index 0d6e949ba315..03e9b184411b 100644 > --- a/include/linux/tpm.h > +++ b/include/linux/tpm.h > @@ -403,6 +403,7 @@ extern int tpm_pcr_extend(struct tpm_chip *chip, u32 pcr_idx, > extern int tpm_send(struct tpm_chip *chip, void *cmd, size_t buflen); > extern int tpm_get_random(struct tpm_chip *chip, u8 *data, size_t max); > extern struct tpm_chip *tpm_default_chip(void); > +void tpm2_flush_context(struct tpm_chip *chip, u32 handle); Shouldn't this be declared as "extern" similar to other APIs? Also, I think we need "#else" part for this API as well. -Sumit > #else > static inline int tpm_is_tpm2(struct tpm_chip *chip) > { > diff --git a/security/keys/trusted-keys/trusted_tpm2.c b/security/keys/trusted-keys/trusted_tpm2.c > index a9810ac2776f..08ec7f48f01d 100644 > --- a/security/keys/trusted-keys/trusted_tpm2.c > +++ b/security/keys/trusted-keys/trusted_tpm2.c > @@ -309,6 +309,7 @@ int tpm2_unseal_trusted(struct tpm_chip *chip, > return rc; > > rc = tpm2_unseal_cmd(chip, payload, options, blob_handle); > + tpm2_flush_context(chip, blob_handle); > > return rc; > } > -- > 2.16.4 >
On Fri, 2019-12-13 at 11:10 +0530, Sumit Garg wrote: > On Thu, 12 Dec 2019 at 23:28, James Bottomley > <James.Bottomley@hansenpartnership.com> wrote: > > > > The original code, before it was moved into security/keys/trusted- > > keys had a flush after the blob unseal. Without that flush, the > > volatile handles increase in the TPM until it becomes unusable and > > the system either has to be rebooted or the TPM volatile area > > manually flushed. Fix by adding back the lost flush, which we now > > have to export because of the relocation of the trusted key code > > may cause the consumer to be modular. > > > > Signed-off-by: James Bottomley <James.Bottomley@HansenPartnership.c > > om> > > Fixes: 2e19e10131a0 ("KEYS: trusted: Move TPM2 trusted keys code") > > > > Overall looks good to me with following minor comment. > > > --- > > drivers/char/tpm/tpm.h | 1 - > > drivers/char/tpm/tpm2-cmd.c | 1 + > > include/linux/tpm.h | 1 + > > security/keys/trusted-keys/trusted_tpm2.c | 1 + > > 4 files changed, 3 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/char/tpm/tpm.h b/drivers/char/tpm/tpm.h > > index b9e1547be6b5..5620747da0cf 100644 > > --- a/drivers/char/tpm/tpm.h > > +++ b/drivers/char/tpm/tpm.h > > @@ -218,7 +218,6 @@ int tpm2_pcr_read(struct tpm_chip *chip, u32 > > pcr_idx, > > int tpm2_pcr_extend(struct tpm_chip *chip, u32 pcr_idx, > > struct tpm_digest *digests); > > int tpm2_get_random(struct tpm_chip *chip, u8 *dest, size_t max); > > -void tpm2_flush_context(struct tpm_chip *chip, u32 handle); > > ssize_t tpm2_get_tpm_pt(struct tpm_chip *chip, u32 property_id, > > u32 *value, const char *desc); > > > > diff --git a/drivers/char/tpm/tpm2-cmd.c b/drivers/char/tpm/tpm2- > > cmd.c > > index fdb457704aa7..13696deceae8 100644 > > --- a/drivers/char/tpm/tpm2-cmd.c > > +++ b/drivers/char/tpm/tpm2-cmd.c > > @@ -362,6 +362,7 @@ void tpm2_flush_context(struct tpm_chip *chip, > > u32 handle) > > tpm_transmit_cmd(chip, &buf, 0, "flushing context"); > > tpm_buf_destroy(&buf); > > } > > +EXPORT_SYMBOL_GPL(tpm2_flush_context); > > > > struct tpm2_get_cap_out { > > u8 more_data; > > diff --git a/include/linux/tpm.h b/include/linux/tpm.h > > index 0d6e949ba315..03e9b184411b 100644 > > --- a/include/linux/tpm.h > > +++ b/include/linux/tpm.h > > @@ -403,6 +403,7 @@ extern int tpm_pcr_extend(struct tpm_chip > > *chip, u32 pcr_idx, > > extern int tpm_send(struct tpm_chip *chip, void *cmd, size_t > > buflen); > > extern int tpm_get_random(struct tpm_chip *chip, u8 *data, size_t > > max); > > extern struct tpm_chip *tpm_default_chip(void); > > +void tpm2_flush_context(struct tpm_chip *chip, u32 handle); > > Shouldn't this be declared as "extern" similar to other APIs? extern has no meaning for function declarations and our coding guide does say do not use it but I think it's advisory not mandatory so I've no objection to changing it if we prefer consistency over the style guide. > Also, I think we need "#else" part for this API as well. No, we shouldn't ... the #else part is only for functions which are called when the TPM isn't compiled in. That should never happen with tpm2_flush_context, so if it ever does we want the compile to break. James
On Fri, 2019-12-13 at 07:35 -0500, James Bottomley wrote: > On Fri, 2019-12-13 at 11:10 +0530, Sumit Garg wrote: [...] > > Also, I think we need "#else" part for this API as well. > > No, we shouldn't ... the #else part is only for functions which are > called when the TPM isn't compiled in. That should never happen with > tpm2_flush_context, so if it ever does we want the compile to break. Just on this bit, it looks like we've given insufficient thought to what it means to move TPM internals using code outside of the tpm directory. I think we really need two include/linux files, one tpm.h for external code that going to do stuff which it would do anyway even if a TPM weren't compiled in, like the PCR read and extend. The other tpm-internal.h for code that wants access to TPM internal functions like flushing and session handling and will take care itself of the case where the TPM isn't compiled in, like the trusted key code does through Kconfig dependencies. The test should be easy: if it was originally in drivers/char/tpm/tpm.h it belongs in include/linux/tpm- internals.h James
On Fri, 13 Dec 2019 at 19:19, James Bottomley <James.Bottomley@hansenpartnership.com> wrote: > > On Fri, 2019-12-13 at 07:35 -0500, James Bottomley wrote: > > On Fri, 2019-12-13 at 11:10 +0530, Sumit Garg wrote: > [...] > > > Also, I think we need "#else" part for this API as well. > > > > No, we shouldn't ... the #else part is only for functions which are > > called when the TPM isn't compiled in. That should never happen with > > tpm2_flush_context, so if it ever does we want the compile to break. > > Just on this bit, it looks like we've given insufficient thought to > what it means to move TPM internals using code outside of the tpm > directory. I think we really need two include/linux files, one tpm.h > for external code that going to do stuff which it would do anyway even > if a TPM weren't compiled in, like the PCR read and extend. The other > tpm-internal.h for code that wants access to TPM internal functions > like flushing and session handling and will take care itself of the > case where the TPM isn't compiled in, like the trusted key code does > through Kconfig dependencies. The test should be easy: if it was > originally in drivers/char/tpm/tpm.h it belongs in include/linux/tpm- > internals.h > Your approach sounds good to me. But how about just moving the APIs that needs to be used independently of TPM compilation to include/linux/tpm-externals.h from drivers/char/tpm/tpm.h. As the initial thoughts while I was moving contents to drivers/char/tpm/tpm.h was to keep a consolidated view of TPM header for a particular user. > > > +void tpm2_flush_context(struct tpm_chip *chip, u32 handle); I agree with you that the above API should remain as it is and should be moved out of the following check: #if defined(CONFIG_TCG_TPM) || defined(CONFIG_TCG_TPM_MODULE) ... #else ... #endif -Sumit > James >
On Mon, 2019-12-16 at 11:47 +0530, Sumit Garg wrote: > On Fri, 13 Dec 2019 at 19:19, James Bottomley > <James.Bottomley@hansenpartnership.com> wrote: > > > > On Fri, 2019-12-13 at 07:35 -0500, James Bottomley wrote: > > > On Fri, 2019-12-13 at 11:10 +0530, Sumit Garg wrote: > > > > [...] > > > > Also, I think we need "#else" part for this API as well. > > > > > > No, we shouldn't ... the #else part is only for functions which > > > are called when the TPM isn't compiled in. That should never > > > happen with tpm2_flush_context, so if it ever does we want the > > > compile to break. > > > > Just on this bit, it looks like we've given insufficient thought to > > what it means to move TPM internals using code outside of the tpm > > directory. I think we really need two include/linux files, one > > tpm.h for external code that going to do stuff which it would do > > anyway even if a TPM weren't compiled in, like the PCR read and > > extend. The other tpm-internal.h for code that wants access to TPM > > internal functions like flushing and session handling and will take > > care itself of the case where the TPM isn't compiled in, like the > > trusted key code does through Kconfig dependencies. The test > > should be easy: if it was originally in drivers/char/tpm/tpm.h it > > belongs in include/linux/tpm-internals.h > > > > Your approach sounds good to me. But how about just moving the APIs > that needs to be used independently of TPM compilation to > include/linux/tpm-externals.h from drivers/char/tpm/tpm.h. As the > initial thoughts while I was moving contents to > drivers/char/tpm/tpm.h was to keep a consolidated view of TPM header > for a particular user. If we do that, we have to change every current user of tpm.h, because that file was originally only for the external users (mostly PCR extends). I'd rather avoid the churn and keep them using tpm.h, hence the tpm-internal.h proposal. > > > > +void tpm2_flush_context(struct tpm_chip *chip, u32 handle); > > I agree with you that the above API should remain as it is and should > be moved out of the following check: > > #if defined(CONFIG_TCG_TPM) || defined(CONFIG_TCG_TPM_MODULE) > ... > #else > ... > #endif That merely changes where the compile breaks. If it's within it breaks on the actual offending file. If it's without, you don't find out until kernel link time. I'm reasonably ambivalent about this but my HA days have given me a preference for failing fast, so in the file itself. James
On Thu, 2019-12-12 at 12:58 -0500, James Bottomley wrote: > The original code, before it was moved into security/keys/trusted-keys > had a flush after the blob unseal. Without that flush, the volatile > handles increase in the TPM until it becomes unusable and the system > either has to be rebooted or the TPM volatile area manually flushed. > Fix by adding back the lost flush, which we now have to export because > of the relocation of the trusted key code may cause the consumer to be > modular. > > Signed-off-by: James Bottomley <James.Bottomley@HansenPartnership.com> > Fixes: 2e19e10131a0 ("KEYS: trusted: Move TPM2 trusted keys code") Reviewed-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> Will collect to my rc3 PR, thank you. /Jarkko
diff --git a/drivers/char/tpm/tpm.h b/drivers/char/tpm/tpm.h index b9e1547be6b5..5620747da0cf 100644 --- a/drivers/char/tpm/tpm.h +++ b/drivers/char/tpm/tpm.h @@ -218,7 +218,6 @@ int tpm2_pcr_read(struct tpm_chip *chip, u32 pcr_idx, int tpm2_pcr_extend(struct tpm_chip *chip, u32 pcr_idx, struct tpm_digest *digests); int tpm2_get_random(struct tpm_chip *chip, u8 *dest, size_t max); -void tpm2_flush_context(struct tpm_chip *chip, u32 handle); ssize_t tpm2_get_tpm_pt(struct tpm_chip *chip, u32 property_id, u32 *value, const char *desc); diff --git a/drivers/char/tpm/tpm2-cmd.c b/drivers/char/tpm/tpm2-cmd.c index fdb457704aa7..13696deceae8 100644 --- a/drivers/char/tpm/tpm2-cmd.c +++ b/drivers/char/tpm/tpm2-cmd.c @@ -362,6 +362,7 @@ void tpm2_flush_context(struct tpm_chip *chip, u32 handle) tpm_transmit_cmd(chip, &buf, 0, "flushing context"); tpm_buf_destroy(&buf); } +EXPORT_SYMBOL_GPL(tpm2_flush_context); struct tpm2_get_cap_out { u8 more_data; diff --git a/include/linux/tpm.h b/include/linux/tpm.h index 0d6e949ba315..03e9b184411b 100644 --- a/include/linux/tpm.h +++ b/include/linux/tpm.h @@ -403,6 +403,7 @@ extern int tpm_pcr_extend(struct tpm_chip *chip, u32 pcr_idx, extern int tpm_send(struct tpm_chip *chip, void *cmd, size_t buflen); extern int tpm_get_random(struct tpm_chip *chip, u8 *data, size_t max); extern struct tpm_chip *tpm_default_chip(void); +void tpm2_flush_context(struct tpm_chip *chip, u32 handle); #else static inline int tpm_is_tpm2(struct tpm_chip *chip) { diff --git a/security/keys/trusted-keys/trusted_tpm2.c b/security/keys/trusted-keys/trusted_tpm2.c index a9810ac2776f..08ec7f48f01d 100644 --- a/security/keys/trusted-keys/trusted_tpm2.c +++ b/security/keys/trusted-keys/trusted_tpm2.c @@ -309,6 +309,7 @@ int tpm2_unseal_trusted(struct tpm_chip *chip, return rc; rc = tpm2_unseal_cmd(chip, payload, options, blob_handle); + tpm2_flush_context(chip, blob_handle); return rc; }
The original code, before it was moved into security/keys/trusted-keys had a flush after the blob unseal. Without that flush, the volatile handles increase in the TPM until it becomes unusable and the system either has to be rebooted or the TPM volatile area manually flushed. Fix by adding back the lost flush, which we now have to export because of the relocation of the trusted key code may cause the consumer to be modular. Signed-off-by: James Bottomley <James.Bottomley@HansenPartnership.com> Fixes: 2e19e10131a0 ("KEYS: trusted: Move TPM2 trusted keys code") --- drivers/char/tpm/tpm.h | 1 - drivers/char/tpm/tpm2-cmd.c | 1 + include/linux/tpm.h | 1 + security/keys/trusted-keys/trusted_tpm2.c | 1 + 4 files changed, 3 insertions(+), 1 deletion(-)