Re: [PATCH v4 3/3,RESEND 2] KEYS: trusted: Reserve TPM for seal and unseal operations

From: Sumit Garg
Date: Wed Nov 04 2020 - 02:30:29 EST

Hi Jarkko,

On Wed, 4 Nov 2020 at 06:49, Jarkko Sakkinen
<jarkko.sakkinen@xxxxxxxxxxxxxxx> 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. The
> ownership is only taken inside tpm_send(), but this is not sufficient,
> as in the key load TPM2_CC_LOAD, TPM2_CC_UNSEAL and TPM2_FLUSH_CONTEXT
> need to be done as a one single atom.
>
> Fix this issue by introducting trusted_tpm_load() and trusted_tpm_new(),
> which wrap these operations, and take the TPM chip ownership before
> sending anything.

I am not sure if we really need these new APIs in order to fix this
issue, see below.

> Use tpm_transmit_cmd() to send TPM commands instead
> of tpm_send(), reverting back to the old behaviour.
>
> Fixes: 2e19e10131a0 ("KEYS: trusted: Move TPM2 trusted keys code")
> Reported-by: "James E.J. Bottomley" <James.Bottomley@xxxxxxxxxxxxxxxxxxxxx>
> Cc: stable@xxxxxxxxxxxxxxx
> Cc: David Howells <dhowells@xxxxxxxxxx>
> Cc: Mimi Zohar <zohar@xxxxxxxxxxxxx>
> Cc: Sumit Garg <sumit.garg@xxxxxxxxxx>
> Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen@xxxxxxxxxxxxxxx>
> Reported-by: kernel test robot <lkp@xxxxxxxxx>
> ---
> drivers/char/tpm/tpm.h | 4 --
> include/linux/tpm.h | 5 +-
> security/keys/trusted-keys/trusted_tpm1.c | 78 +++++++++++++++--------
> security/keys/trusted-keys/trusted_tpm2.c | 6 +-
> 4 files changed, 60 insertions(+), 33 deletions(-)
>
> diff --git a/drivers/char/tpm/tpm.h b/drivers/char/tpm/tpm.h
> index 947d1db0a5cc..283f78211c3a 100644
> --- a/drivers/char/tpm/tpm.h
> +++ b/drivers/char/tpm/tpm.h
> @@ -164,8 +164,6 @@ extern const struct file_operations tpmrm_fops;
> extern struct idr dev_nums_idr;
>
> ssize_t tpm_transmit(struct tpm_chip *chip, u8 *buf, size_t bufsiz);
> -ssize_t tpm_transmit_cmd(struct tpm_chip *chip, struct tpm_buf *buf,
> - size_t min_rsp_body_length, const char *desc);
> int tpm_get_timeouts(struct tpm_chip *);
> int tpm_auto_startup(struct tpm_chip *chip);
>
> @@ -194,8 +192,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..804a3f69bbd9 100644
> --- a/include/linux/tpm.h
> +++ b/include/linux/tpm.h
> @@ -397,6 +397,10 @@ 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 ssize_t tpm_transmit_cmd(struct tpm_chip *chip, struct tpm_buf *buf,
> + size_t min_rsp_body_length, const char *desc);
> 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 +414,6 @@ static inline int tpm_is_tpm2(struct tpm_chip *chip)
> {
> return -ENODEV;
> }
> -
> 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 7a937c3c5283..20ca18e17437 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);

Can't we move this TPM 2.0 specific operation within
tpm2_unseal_trusted() instead?

> + if (!ret) {
> + ret = tpm2_unseal_trusted(chip, payload, options);
> + tpm_put_ops(chip);

Ditto.

> + }
> + } 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);

Same here, to move this within tpm2_seal_trusted() instead?

> + if (!ret) {
> + ret = tpm2_seal_trusted(chip, payload, options);
> + tpm_put_ops(chip);

Ditto.

-Sumit

> + }
> + } 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;
> diff --git a/security/keys/trusted-keys/trusted_tpm2.c b/security/keys/trusted-keys/trusted_tpm2.c
> index 08ec7f48f01d..effdb67fac6d 100644
> --- a/security/keys/trusted-keys/trusted_tpm2.c
> +++ b/security/keys/trusted-keys/trusted_tpm2.c
> @@ -130,7 +130,7 @@ int tpm2_seal_trusted(struct tpm_chip *chip,
> goto out;
> }
>
> - rc = tpm_send(chip, buf.data, tpm_buf_length(&buf));
> + rc = tpm_transmit_cmd(chip, &buf, 4, "sealing data");
> if (rc)
> goto out;
>
> @@ -211,7 +211,7 @@ static int tpm2_load_cmd(struct tpm_chip *chip,
> goto out;
> }
>
> - rc = tpm_send(chip, buf.data, tpm_buf_length(&buf));
> + rc = tpm_transmit_cmd(chip, &buf, 4, "loading blob");
> if (!rc)
> *blob_handle = be32_to_cpup(
> (__be32 *) &buf.data[TPM_HEADER_SIZE]);
> @@ -260,7 +260,7 @@ static int tpm2_unseal_cmd(struct tpm_chip *chip,
> options->blobauth /* hmac */,
> TPM_DIGEST_SIZE);
>
> - rc = tpm_send(chip, buf.data, tpm_buf_length(&buf));
> + rc = tpm_transmit_cmd(chip, &buf, 6, "unsealing");
> if (rc > 0)
> rc = -EPERM;
>
> --
> 2.25.1
>