| Message ID | 20200928153133.114953-2-jarkko.sakkinen@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2,1/2] KEYS: trusted: Fix incorrect handling of tpm_get_random() | expand |
On Mon, 2020-09-28 at 18:31 +0300, Jarkko Sakkinen wrote: > When TPM 2.0 trusted keys code was moved to the trusted keys > subsystem, > the operations were unwrapped from tpm_try_get_ops() and > tpm_put_ops(), > which are used to take temporarily the ownership of the TPM chip. > > Fix this issue by introducting trusted_tpm_load() and > trusted_tpm_new(), > which wrap these operations. > > Fixes: 2e19e10131a0 ("KEYS: trusted: Move TPM2 trusted keys code") > Reported-by: "James E.J. Bottomley" < > James.Bottomley@HansenPartnership.com> > Cc: Sumit Garg <sumit.garg@linaro.org> > Cc: David Howells <dhowells@redhat.com> > Cc: stable@vger.kernel.org > Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> > --- > drivers/char/tpm/tpm.h | 2 - > include/linux/tpm.h | 10 ++- > security/keys/trusted-keys/trusted_tpm1.c | 78 +++++++++++++++---- > ---- > 3 files changed, 62 insertions(+), 28 deletions(-) > > diff --git a/drivers/char/tpm/tpm.h b/drivers/char/tpm/tpm.h > index 947d1db0a5cc..4338573a8d48 100644 > --- a/drivers/char/tpm/tpm.h > +++ b/drivers/char/tpm/tpm.h > @@ -194,8 +194,6 @@ static inline void tpm_msleep(unsigned int > delay_msec) > int tpm_chip_start(struct tpm_chip *chip); > void tpm_chip_stop(struct tpm_chip *chip); > struct tpm_chip *tpm_find_get_ops(struct tpm_chip *chip); > -__must_check int tpm_try_get_ops(struct tpm_chip *chip); > -void tpm_put_ops(struct tpm_chip *chip); > > struct tpm_chip *tpm_chip_alloc(struct device *dev, > const struct tpm_class_ops *ops); > diff --git a/include/linux/tpm.h b/include/linux/tpm.h > index 8f4ff39f51e7..0fe1cb5517ea 100644 > --- a/include/linux/tpm.h > +++ b/include/linux/tpm.h > @@ -397,6 +397,8 @@ static inline u32 tpm2_rc_value(u32 rc) > #if defined(CONFIG_TCG_TPM) || defined(CONFIG_TCG_TPM_MODULE) > > extern int tpm_is_tpm2(struct tpm_chip *chip); > +extern __must_check int tpm_try_get_ops(struct tpm_chip *chip); > +extern void tpm_put_ops(struct tpm_chip *chip); > extern int tpm_pcr_read(struct tpm_chip *chip, u32 pcr_idx, > struct tpm_digest *digest); > extern int tpm_pcr_extend(struct tpm_chip *chip, u32 pcr_idx, > @@ -410,7 +412,13 @@ static inline int tpm_is_tpm2(struct tpm_chip > *chip) > { > return -ENODEV; > } > - > +static inline int tpm_try_get_ops(struct tpm_chip *chip) > +{ > + return -ENODEV; > +} > +static inline void tpm_put_ops(struct tpm_chip *chip) > +{ > +} > static inline int tpm_pcr_read(struct tpm_chip *chip, int pcr_idx, > struct tpm_digest *digest) > { > diff --git a/security/keys/trusted-keys/trusted_tpm1.c > b/security/keys/trusted-keys/trusted_tpm1.c > index c7b1701cdac5..c1dfc32c780b 100644 > --- a/security/keys/trusted-keys/trusted_tpm1.c > +++ b/security/keys/trusted-keys/trusted_tpm1.c > @@ -950,6 +950,51 @@ static struct trusted_key_payload > *trusted_payload_alloc(struct key *key) > return p; > } > > +static int trusted_tpm_load(struct tpm_chip *chip, > + struct trusted_key_payload *payload, > + struct trusted_key_options *options) > +{ > + int ret; > + > + if (tpm_is_tpm2(chip)) { > + ret = tpm_try_get_ops(chip); > + if (!ret) { > + ret = tpm2_unseal_trusted(chip, payload, > options); > + tpm_put_ops(chip); > + } > + } else { > + ret = key_unseal(payload, options); > + } > + > + return ret; > +} > + > +static int trusted_tpm_new(struct tpm_chip *chip, > + struct trusted_key_payload *payload, > + struct trusted_key_options *options) > +{ > + int ret; > + > + ret = tpm_get_random(chip, payload->key, payload->key_len); > + if (ret < 0) > + return ret; > + > + if (ret != payload->key_len) > + return -EIO; > + > + if (tpm_is_tpm2(chip)) { > + ret = tpm_try_get_ops(chip); > + if (!ret) { > + ret = tpm2_seal_trusted(chip, payload, > options); > + tpm_put_ops(chip); > + } > + } else { > + ret = key_seal(payload, options); > + } > + > + return ret; > +} > + > /* > * trusted_instantiate - create a new trusted key > * > @@ -968,12 +1013,6 @@ static int trusted_instantiate(struct key *key, > char *datablob; > int ret = 0; > int key_cmd; > - size_t key_len; > - int tpm2; > - > - tpm2 = tpm_is_tpm2(chip); > - if (tpm2 < 0) > - return tpm2; > > if (datalen <= 0 || datalen > 32767 || !prep->data) > return -EINVAL; > @@ -1011,32 +1050,21 @@ static int trusted_instantiate(struct key > *key, > > switch (key_cmd) { > case Opt_load: > - if (tpm2) > - ret = tpm2_unseal_trusted(chip, payload, > options); > - else > - ret = key_unseal(payload, options); > + ret = trusted_tpm_load(chip, payload, options); > + > dump_payload(payload); > dump_options(options); > + > if (ret < 0) > - pr_info("trusted_key: key_unseal failed > (%d)\n", ret); > + pr_info("%s: load failed (%d)\n", __func__, > ret); > + > break; > case Opt_new: > - key_len = payload->key_len; > - ret = tpm_get_random(chip, payload->key, key_len); > - if (ret < 0) > - goto out; > + ret = trusted_tpm_new(chip, payload, options); > > - if (ret != key_len) { > - pr_info("trusted_key: key_create failed > (%d)\n", ret); > - ret = -EIO; > - goto out; > - } > - if (tpm2) > - ret = tpm2_seal_trusted(chip, payload, > options); > - else > - ret = key_seal(payload, options); > if (ret < 0) > - pr_info("trusted_key: key_seal failed (%d)\n", > ret); > + pr_info("%s: new failed (%d)\n", __func__, > ret); > + > break; > default: > ret = -EINVAL; You didn't actually test this, did you? It trips over the double tpm_try_get_ops, once above then again in tpm_send. This is the hang: [<0>] tpm_try_get_ops+0x3b/0x80 [<0>] tpm_find_get_ops+0x14/0x50 [<0>] tpm_send+0x23/0x80 [<0>] tpm2_seal_trusted+0x4b0/0x6c0 [trusted] [<0>] trusted_instantiate+0x353/0x3a0 [trusted] [<0>] __key_instantiate_and_link+0x50/0x160 [<0>] key_create_or_update+0x438/0x520 [<0>] __x64_sys_add_key+0x102/0x1f0 [<0>] do_syscall_64+0x33/0x80 [<0>] entry_SYSCALL_64_after_hwframe+0x44/0xa9 You need to replace all the tpm_send's in the code with tpm_transmit_cmd. James
diff --git a/drivers/char/tpm/tpm.h b/drivers/char/tpm/tpm.h index 947d1db0a5cc..4338573a8d48 100644 --- a/drivers/char/tpm/tpm.h +++ b/drivers/char/tpm/tpm.h @@ -194,8 +194,6 @@ static inline void tpm_msleep(unsigned int delay_msec) int tpm_chip_start(struct tpm_chip *chip); void tpm_chip_stop(struct tpm_chip *chip); struct tpm_chip *tpm_find_get_ops(struct tpm_chip *chip); -__must_check int tpm_try_get_ops(struct tpm_chip *chip); -void tpm_put_ops(struct tpm_chip *chip); struct tpm_chip *tpm_chip_alloc(struct device *dev, const struct tpm_class_ops *ops); diff --git a/include/linux/tpm.h b/include/linux/tpm.h index 8f4ff39f51e7..0fe1cb5517ea 100644 --- a/include/linux/tpm.h +++ b/include/linux/tpm.h @@ -397,6 +397,8 @@ static inline u32 tpm2_rc_value(u32 rc) #if defined(CONFIG_TCG_TPM) || defined(CONFIG_TCG_TPM_MODULE) extern int tpm_is_tpm2(struct tpm_chip *chip); +extern __must_check int tpm_try_get_ops(struct tpm_chip *chip); +extern void tpm_put_ops(struct tpm_chip *chip); extern int tpm_pcr_read(struct tpm_chip *chip, u32 pcr_idx, struct tpm_digest *digest); extern int tpm_pcr_extend(struct tpm_chip *chip, u32 pcr_idx, @@ -410,7 +412,13 @@ static inline int tpm_is_tpm2(struct tpm_chip *chip) { return -ENODEV; } - +static inline int tpm_try_get_ops(struct tpm_chip *chip) +{ + return -ENODEV; +} +static inline void tpm_put_ops(struct tpm_chip *chip) +{ +} static inline int tpm_pcr_read(struct tpm_chip *chip, int pcr_idx, struct tpm_digest *digest) { diff --git a/security/keys/trusted-keys/trusted_tpm1.c b/security/keys/trusted-keys/trusted_tpm1.c index c7b1701cdac5..c1dfc32c780b 100644 --- a/security/keys/trusted-keys/trusted_tpm1.c +++ b/security/keys/trusted-keys/trusted_tpm1.c @@ -950,6 +950,51 @@ static struct trusted_key_payload *trusted_payload_alloc(struct key *key) return p; } +static int trusted_tpm_load(struct tpm_chip *chip, + struct trusted_key_payload *payload, + struct trusted_key_options *options) +{ + int ret; + + if (tpm_is_tpm2(chip)) { + ret = tpm_try_get_ops(chip); + if (!ret) { + ret = tpm2_unseal_trusted(chip, payload, options); + tpm_put_ops(chip); + } + } else { + ret = key_unseal(payload, options); + } + + return ret; +} + +static int trusted_tpm_new(struct tpm_chip *chip, + struct trusted_key_payload *payload, + struct trusted_key_options *options) +{ + int ret; + + ret = tpm_get_random(chip, payload->key, payload->key_len); + if (ret < 0) + return ret; + + if (ret != payload->key_len) + return -EIO; + + if (tpm_is_tpm2(chip)) { + ret = tpm_try_get_ops(chip); + if (!ret) { + ret = tpm2_seal_trusted(chip, payload, options); + tpm_put_ops(chip); + } + } else { + ret = key_seal(payload, options); + } + + return ret; +} + /* * trusted_instantiate - create a new trusted key * @@ -968,12 +1013,6 @@ static int trusted_instantiate(struct key *key, char *datablob; int ret = 0; int key_cmd; - size_t key_len; - int tpm2; - - tpm2 = tpm_is_tpm2(chip); - if (tpm2 < 0) - return tpm2; if (datalen <= 0 || datalen > 32767 || !prep->data) return -EINVAL; @@ -1011,32 +1050,21 @@ static int trusted_instantiate(struct key *key, switch (key_cmd) { case Opt_load: - if (tpm2) - ret = tpm2_unseal_trusted(chip, payload, options); - else - ret = key_unseal(payload, options); + ret = trusted_tpm_load(chip, payload, options); + dump_payload(payload); dump_options(options); + if (ret < 0) - pr_info("trusted_key: key_unseal failed (%d)\n", ret); + pr_info("%s: load failed (%d)\n", __func__, ret); + break; case Opt_new: - key_len = payload->key_len; - ret = tpm_get_random(chip, payload->key, key_len); - if (ret < 0) - goto out; + ret = trusted_tpm_new(chip, payload, options); - if (ret != key_len) { - pr_info("trusted_key: key_create failed (%d)\n", ret); - ret = -EIO; - goto out; - } - if (tpm2) - ret = tpm2_seal_trusted(chip, payload, options); - else - ret = key_seal(payload, options); if (ret < 0) - pr_info("trusted_key: key_seal failed (%d)\n", ret); + pr_info("%s: new failed (%d)\n", __func__, ret); + break; default: ret = -EINVAL;
When TPM 2.0 trusted keys code was moved to the trusted keys subsystem, the operations were unwrapped from tpm_try_get_ops() and tpm_put_ops(), which are used to take temporarily the ownership of the TPM chip. Fix this issue by introducting trusted_tpm_load() and trusted_tpm_new(), which wrap these operations. Fixes: 2e19e10131a0 ("KEYS: trusted: Move TPM2 trusted keys code") Reported-by: "James E.J. Bottomley" <James.Bottomley@HansenPartnership.com> Cc: Sumit Garg <sumit.garg@linaro.org> Cc: David Howells <dhowells@redhat.com> Cc: stable@vger.kernel.org Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> --- drivers/char/tpm/tpm.h | 2 - include/linux/tpm.h | 10 ++- security/keys/trusted-keys/trusted_tpm1.c | 78 +++++++++++++++-------- 3 files changed, 62 insertions(+), 28 deletions(-)