[PATCH] tpm: fix a race condition tpm2_unseal_trusted()

[PATCH] tpm: fix a race condition tpm2_unseal_trusted()

[Jarkko Sakkinen]

Unseal and load operations should be done as an atomic operation. This
commit introduces unlocked tpm_transmit() so that tpm2_unseal_trusted()
can do the locking by itself.

v2: Introduced an unlocked unseal operation instead of changing locking
    strategy in order to make less intrusive bug fix and thus more
    backportable.

v3: Have also separate __tpm_transmit() that takes 'flags' in order to
    better localize the bug fix and make it easier to backport.

v4: Cleaned up the control flow in tpm2_unseal_trusted. Added the
    missing 'Fixes' line.

Fixes: 0fe5480303a1 ("keys, trusted: seal/unseal with TPM 2.0 chips")
CC: stable@vger.kernel.org
Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
---
 drivers/char/tpm/tpm-interface.c | 16 +++++++++-------
 drivers/char/tpm/tpm.h           | 25 +++++++++++++++++++++----
 drivers/char/tpm/tpm2-cmd.c      | 12 ++++++++----
 3 files changed, 38 insertions(+), 15 deletions(-)

diff --git a/drivers/char/tpm/tpm-interface.c b/drivers/char/tpm/tpm-interface.c
index 43ef0ef..80e702a 100644
--- a/drivers/char/tpm/tpm-interface.c
+++ b/drivers/char/tpm/tpm-interface.c
@@ -330,8 +330,8 @@ EXPORT_SYMBOL_GPL(tpm_calc_ordinal_duration);
 /*
  * Internal kernel interface to transmit TPM commands
  */
-ssize_t tpm_transmit(struct tpm_chip *chip, const char *buf,
-		     size_t bufsiz)
+ssize_t __tpm_transmit(struct tpm_chip *chip, const char *buf,
+		       size_t bufsiz, unsigned int flags)
 {
 	ssize_t rc;
 	u32 count, ordinal;
@@ -350,7 +350,8 @@ ssize_t tpm_transmit(struct tpm_chip *chip, const char *buf,
 		return -E2BIG;
 	}
 
-	mutex_lock(&chip->tpm_mutex);
+	if (flags & TPM_TRANSMIT_LOCK)
+		mutex_lock(&chip->tpm_mutex);
 
 	rc = chip->ops->send(chip, (u8 *) buf, count);
 	if (rc < 0) {
@@ -393,20 +394,21 @@ out_recv:
 		dev_err(&chip->dev,
 			"tpm_transmit: tpm_recv: error %zd\n", rc);
 out:
-	mutex_unlock(&chip->tpm_mutex);
+	if (flags & TPM_TRANSMIT_LOCK)
+		mutex_unlock(&chip->tpm_mutex);
 	return rc;
 }
 
 #define TPM_DIGEST_SIZE 20
 #define TPM_RET_CODE_IDX 6
 
-ssize_t tpm_transmit_cmd(struct tpm_chip *chip, void *cmd,
-			 int len, const char *desc)
+ssize_t __tpm_transmit_cmd(struct tpm_chip *chip, void *cmd,
+			   int len, const char *desc, unsigned int flags)
 {
 	struct tpm_output_header *header;
 	int err;
 
-	len = tpm_transmit(chip, (u8 *) cmd, len);
+	len = __tpm_transmit(chip, cmd, len, flags);
 	if (len <  0)
 		return len;
 	else if (len < TPM_HEADER_SIZE)
diff --git a/drivers/char/tpm/tpm.h b/drivers/char/tpm/tpm.h
index 6e002c4..0a4abf0 100644
--- a/drivers/char/tpm/tpm.h
+++ b/drivers/char/tpm/tpm.h
@@ -476,12 +476,29 @@ extern dev_t tpm_devt;
 extern const struct file_operations tpm_fops;
 extern struct idr dev_nums_idr;
 
+enum tpm_transmit_flags {
+	TPM_TRANSMIT_LOCK,
+};
+
+ssize_t __tpm_transmit(struct tpm_chip *chip, const char *buf,
+		       size_t bufsiz, unsigned int flags);
+ssize_t __tpm_transmit_cmd(struct tpm_chip *chip, void *cmd, int len,
+			   const char *desc, unsigned int flags);
+
+static inline ssize_t tpm_transmit(struct tpm_chip *chip, const char *buf,
+				   size_t bufsiz)
+{
+	return __tpm_transmit(chip, buf, bufsiz, TPM_TRANSMIT_LOCK);
+}
+
+static inline ssize_t tpm_transmit_cmd(struct tpm_chip *chip, void *cmd,
+				       int len, const char *desc)
+{
+	return __tpm_transmit_cmd(chip, cmd, len, desc, TPM_TRANSMIT_LOCK);
+}
+
 ssize_t tpm_getcap(struct tpm_chip *chip, __be32 subcap_id, cap_t *cap,
 		   const char *desc);
-ssize_t tpm_transmit(struct tpm_chip *chip, const char *buf,
-		     size_t bufsiz);
-ssize_t tpm_transmit_cmd(struct tpm_chip *chip, void *cmd, int len,
-			 const char *desc);
 int tpm_get_timeouts(struct tpm_chip *chip);
 int tpm1_auto_startup(struct tpm_chip *chip);
 int tpm_do_selftest(struct tpm_chip *chip);
diff --git a/drivers/char/tpm/tpm2-cmd.c b/drivers/char/tpm/tpm2-cmd.c
index 499f405..a2a0314 100644
--- a/drivers/char/tpm/tpm2-cmd.c
+++ b/drivers/char/tpm/tpm2-cmd.c
@@ -576,7 +576,7 @@ static int tpm2_load(struct tpm_chip *chip,
 		goto out;
 	}
 
-	rc = tpm_transmit_cmd(chip, buf.data, PAGE_SIZE, "loading blob");
+	rc = __tpm_transmit_cmd(chip, buf.data, PAGE_SIZE, "loading blob", 0);
 	if (!rc)
 		*blob_handle = be32_to_cpup(
 			(__be32 *) &buf.data[TPM_HEADER_SIZE]);
@@ -604,7 +604,8 @@ static void tpm2_flush_context(struct tpm_chip *chip, u32 handle)
 
 	tpm_buf_append_u32(&buf, handle);
 
-	rc = tpm_transmit_cmd(chip, buf.data, PAGE_SIZE, "flushing context");
+	rc = __tpm_transmit_cmd(chip, buf.data, PAGE_SIZE, "flushing context",
+				0);
 	if (rc)
 		dev_warn(&chip->dev, "0x%08x was not flushed, rc=%d\n", handle,
 			 rc);
@@ -635,7 +636,7 @@ static int tpm2_unseal(struct tpm_chip *chip,
 			     options->blobauth /* hmac */,
 			     TPM_DIGEST_SIZE);
 
-	rc = tpm_transmit_cmd(chip, buf.data, PAGE_SIZE, "unsealing");
+	rc = __tpm_transmit_cmd(chip, buf.data, PAGE_SIZE, "unsealing", 0);
 	if (rc > 0)
 		rc = -EPERM;
 
@@ -668,14 +669,17 @@ int tpm2_unseal_trusted(struct tpm_chip *chip,
 	u32 blob_handle;
 	int rc;
 
+	mutex_lock(&chip->tpm_mutex);
 	rc = tpm2_load(chip, payload, options, &blob_handle);
 	if (rc)
-		return rc;
+		goto out;
 
 	rc = tpm2_unseal(chip, payload, options, blob_handle);
 
 	tpm2_flush_context(chip, blob_handle);
 
+out:
+	mutex_unlock(&chip->tpm_mutex);
 	return rc;
 }
 
-- 
2.7.4

[PATCH] tpm: fix a race condition tpm2_unseal_trusted()

[Jarkko Sakkinen]

Unseal and load operations should be done as an atomic operation. This
commit introduces unlocked tpm_transmit() so that tpm2_unseal_trusted()
can do the locking by itself.

v2: Introduced an unlocked unseal operation instead of changing locking
    strategy in order to make less intrusive bug fix and thus more
    backportable.

v3: Have also separate __tpm_transmit() that takes 'flags' in order to
    better localize the bug fix and make it easier to backport.

v4: Cleaned up the control flow in tpm2_unseal_trusted. Added the
    missing 'Fixes' line.

Fixes: 0fe5480303a1 ("keys, trusted: seal/unseal with TPM 2.0 chips")
CC: stable-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>
---
 drivers/char/tpm/tpm-interface.c | 16 +++++++++-------
 drivers/char/tpm/tpm.h           | 25 +++++++++++++++++++++----
 drivers/char/tpm/tpm2-cmd.c      | 12 ++++++++----
 3 files changed, 38 insertions(+), 15 deletions(-)

diff --git a/drivers/char/tpm/tpm-interface.c b/drivers/char/tpm/tpm-interface.c
index 43ef0ef..80e702a 100644
--- a/drivers/char/tpm/tpm-interface.c
+++ b/drivers/char/tpm/tpm-interface.c
@@ -330,8 +330,8 @@ EXPORT_SYMBOL_GPL(tpm_calc_ordinal_duration);
 /*
  * Internal kernel interface to transmit TPM commands
  */
-ssize_t tpm_transmit(struct tpm_chip *chip, const char *buf,
-		     size_t bufsiz)
+ssize_t __tpm_transmit(struct tpm_chip *chip, const char *buf,
+		       size_t bufsiz, unsigned int flags)
 {
 	ssize_t rc;
 	u32 count, ordinal;
@@ -350,7 +350,8 @@ ssize_t tpm_transmit(struct tpm_chip *chip, const char *buf,
 		return -E2BIG;
 	}
 
-	mutex_lock(&chip->tpm_mutex);
+	if (flags & TPM_TRANSMIT_LOCK)
+		mutex_lock(&chip->tpm_mutex);
 
 	rc = chip->ops->send(chip, (u8 *) buf, count);
 	if (rc < 0) {
@@ -393,20 +394,21 @@ out_recv:
 		dev_err(&chip->dev,
 			"tpm_transmit: tpm_recv: error %zd\n", rc);
 out:
-	mutex_unlock(&chip->tpm_mutex);
+	if (flags & TPM_TRANSMIT_LOCK)
+		mutex_unlock(&chip->tpm_mutex);
 	return rc;
 }
 
 #define TPM_DIGEST_SIZE 20
 #define TPM_RET_CODE_IDX 6
 
-ssize_t tpm_transmit_cmd(struct tpm_chip *chip, void *cmd,
-			 int len, const char *desc)
+ssize_t __tpm_transmit_cmd(struct tpm_chip *chip, void *cmd,
+			   int len, const char *desc, unsigned int flags)
 {
 	struct tpm_output_header *header;
 	int err;
 
-	len = tpm_transmit(chip, (u8 *) cmd, len);
+	len = __tpm_transmit(chip, cmd, len, flags);
 	if (len <  0)
 		return len;
 	else if (len < TPM_HEADER_SIZE)
diff --git a/drivers/char/tpm/tpm.h b/drivers/char/tpm/tpm.h
index 6e002c4..0a4abf0 100644
--- a/drivers/char/tpm/tpm.h
+++ b/drivers/char/tpm/tpm.h
@@ -476,12 +476,29 @@ extern dev_t tpm_devt;
 extern const struct file_operations tpm_fops;
 extern struct idr dev_nums_idr;
 
+enum tpm_transmit_flags {
+	TPM_TRANSMIT_LOCK,
+};
+
+ssize_t __tpm_transmit(struct tpm_chip *chip, const char *buf,
+		       size_t bufsiz, unsigned int flags);
+ssize_t __tpm_transmit_cmd(struct tpm_chip *chip, void *cmd, int len,
+			   const char *desc, unsigned int flags);
+
+static inline ssize_t tpm_transmit(struct tpm_chip *chip, const char *buf,
+				   size_t bufsiz)
+{
+	return __tpm_transmit(chip, buf, bufsiz, TPM_TRANSMIT_LOCK);
+}
+
+static inline ssize_t tpm_transmit_cmd(struct tpm_chip *chip, void *cmd,
+				       int len, const char *desc)
+{
+	return __tpm_transmit_cmd(chip, cmd, len, desc, TPM_TRANSMIT_LOCK);
+}
+
 ssize_t tpm_getcap(struct tpm_chip *chip, __be32 subcap_id, cap_t *cap,
 		   const char *desc);
-ssize_t tpm_transmit(struct tpm_chip *chip, const char *buf,
-		     size_t bufsiz);
-ssize_t tpm_transmit_cmd(struct tpm_chip *chip, void *cmd, int len,
-			 const char *desc);
 int tpm_get_timeouts(struct tpm_chip *chip);
 int tpm1_auto_startup(struct tpm_chip *chip);
 int tpm_do_selftest(struct tpm_chip *chip);
diff --git a/drivers/char/tpm/tpm2-cmd.c b/drivers/char/tpm/tpm2-cmd.c
index 499f405..a2a0314 100644
--- a/drivers/char/tpm/tpm2-cmd.c
+++ b/drivers/char/tpm/tpm2-cmd.c
@@ -576,7 +576,7 @@ static int tpm2_load(struct tpm_chip *chip,
 		goto out;
 	}
 
-	rc = tpm_transmit_cmd(chip, buf.data, PAGE_SIZE, "loading blob");
+	rc = __tpm_transmit_cmd(chip, buf.data, PAGE_SIZE, "loading blob", 0);
 	if (!rc)
 		*blob_handle = be32_to_cpup(
 			(__be32 *) &buf.data[TPM_HEADER_SIZE]);
@@ -604,7 +604,8 @@ static void tpm2_flush_context(struct tpm_chip *chip, u32 handle)
 
 	tpm_buf_append_u32(&buf, handle);
 
-	rc = tpm_transmit_cmd(chip, buf.data, PAGE_SIZE, "flushing context");
+	rc = __tpm_transmit_cmd(chip, buf.data, PAGE_SIZE, "flushing context",
+				0);
 	if (rc)
 		dev_warn(&chip->dev, "0x%08x was not flushed, rc=%d\n", handle,
 			 rc);
@@ -635,7 +636,7 @@ static int tpm2_unseal(struct tpm_chip *chip,
 			     options->blobauth /* hmac */,
 			     TPM_DIGEST_SIZE);
 
-	rc = tpm_transmit_cmd(chip, buf.data, PAGE_SIZE, "unsealing");
+	rc = __tpm_transmit_cmd(chip, buf.data, PAGE_SIZE, "unsealing", 0);
 	if (rc > 0)
 		rc = -EPERM;
 
@@ -668,14 +669,17 @@ int tpm2_unseal_trusted(struct tpm_chip *chip,
 	u32 blob_handle;
 	int rc;
 
+	mutex_lock(&chip->tpm_mutex);
 	rc = tpm2_load(chip, payload, options, &blob_handle);
 	if (rc)
-		return rc;
+		goto out;
 
 	rc = tpm2_unseal(chip, payload, options, blob_handle);
 
 	tpm2_flush_context(chip, blob_handle);
 
+out:
+	mutex_unlock(&chip->tpm_mutex);
 	return rc;
 }
 
-- 
2.7.4


------------------------------------------------------------------------------

Re: [PATCH] tpm: fix a race condition tpm2_unseal_trusted()

[Jarkko Sakkinen]

Jason, I guess this should be now less intrusive than the original one?

The main goal was to make it as backportable as possible.

/Jarkko

On Tue, Aug 23, 2016 at 08:57:22PM -0400, Jarkko Sakkinen wrote:
> Unseal and load operations should be done as an atomic operation. This
> commit introduces unlocked tpm_transmit() so that tpm2_unseal_trusted()
> can do the locking by itself.
> 
> v2: Introduced an unlocked unseal operation instead of changing locking
>     strategy in order to make less intrusive bug fix and thus more
>     backportable.
> 
> v3: Have also separate __tpm_transmit() that takes 'flags' in order to
>     better localize the bug fix and make it easier to backport.
> 
> v4: Cleaned up the control flow in tpm2_unseal_trusted. Added the
>     missing 'Fixes' line.
> 
> Fixes: 0fe5480303a1 ("keys, trusted: seal/unseal with TPM 2.0 chips")
> CC: stable@vger.kernel.org
> Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
> ---
>  drivers/char/tpm/tpm-interface.c | 16 +++++++++-------
>  drivers/char/tpm/tpm.h           | 25 +++++++++++++++++++++----
>  drivers/char/tpm/tpm2-cmd.c      | 12 ++++++++----
>  3 files changed, 38 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/char/tpm/tpm-interface.c b/drivers/char/tpm/tpm-interface.c
> index 43ef0ef..80e702a 100644
> --- a/drivers/char/tpm/tpm-interface.c
> +++ b/drivers/char/tpm/tpm-interface.c
> @@ -330,8 +330,8 @@ EXPORT_SYMBOL_GPL(tpm_calc_ordinal_duration);
>  /*
>   * Internal kernel interface to transmit TPM commands
>   */
> -ssize_t tpm_transmit(struct tpm_chip *chip, const char *buf,
> -		     size_t bufsiz)
> +ssize_t __tpm_transmit(struct tpm_chip *chip, const char *buf,
> +		       size_t bufsiz, unsigned int flags)
>  {
>  	ssize_t rc;
>  	u32 count, ordinal;
> @@ -350,7 +350,8 @@ ssize_t tpm_transmit(struct tpm_chip *chip, const char *buf,
>  		return -E2BIG;
>  	}
>  
> -	mutex_lock(&chip->tpm_mutex);
> +	if (flags & TPM_TRANSMIT_LOCK)
> +		mutex_lock(&chip->tpm_mutex);
>  
>  	rc = chip->ops->send(chip, (u8 *) buf, count);
>  	if (rc < 0) {
> @@ -393,20 +394,21 @@ out_recv:
>  		dev_err(&chip->dev,
>  			"tpm_transmit: tpm_recv: error %zd\n", rc);
>  out:
> -	mutex_unlock(&chip->tpm_mutex);
> +	if (flags & TPM_TRANSMIT_LOCK)
> +		mutex_unlock(&chip->tpm_mutex);
>  	return rc;
>  }
>  
>  #define TPM_DIGEST_SIZE 20
>  #define TPM_RET_CODE_IDX 6
>  
> -ssize_t tpm_transmit_cmd(struct tpm_chip *chip, void *cmd,
> -			 int len, const char *desc)
> +ssize_t __tpm_transmit_cmd(struct tpm_chip *chip, void *cmd,
> +			   int len, const char *desc, unsigned int flags)
>  {
>  	struct tpm_output_header *header;
>  	int err;
>  
> -	len = tpm_transmit(chip, (u8 *) cmd, len);
> +	len = __tpm_transmit(chip, cmd, len, flags);
>  	if (len <  0)
>  		return len;
>  	else if (len < TPM_HEADER_SIZE)
> diff --git a/drivers/char/tpm/tpm.h b/drivers/char/tpm/tpm.h
> index 6e002c4..0a4abf0 100644
> --- a/drivers/char/tpm/tpm.h
> +++ b/drivers/char/tpm/tpm.h
> @@ -476,12 +476,29 @@ extern dev_t tpm_devt;
>  extern const struct file_operations tpm_fops;
>  extern struct idr dev_nums_idr;
>  
> +enum tpm_transmit_flags {
> +	TPM_TRANSMIT_LOCK,
> +};
> +
> +ssize_t __tpm_transmit(struct tpm_chip *chip, const char *buf,
> +		       size_t bufsiz, unsigned int flags);
> +ssize_t __tpm_transmit_cmd(struct tpm_chip *chip, void *cmd, int len,
> +			   const char *desc, unsigned int flags);
> +
> +static inline ssize_t tpm_transmit(struct tpm_chip *chip, const char *buf,
> +				   size_t bufsiz)
> +{
> +	return __tpm_transmit(chip, buf, bufsiz, TPM_TRANSMIT_LOCK);
> +}
> +
> +static inline ssize_t tpm_transmit_cmd(struct tpm_chip *chip, void *cmd,
> +				       int len, const char *desc)
> +{
> +	return __tpm_transmit_cmd(chip, cmd, len, desc, TPM_TRANSMIT_LOCK);
> +}
> +
>  ssize_t tpm_getcap(struct tpm_chip *chip, __be32 subcap_id, cap_t *cap,
>  		   const char *desc);
> -ssize_t tpm_transmit(struct tpm_chip *chip, const char *buf,
> -		     size_t bufsiz);
> -ssize_t tpm_transmit_cmd(struct tpm_chip *chip, void *cmd, int len,
> -			 const char *desc);
>  int tpm_get_timeouts(struct tpm_chip *chip);
>  int tpm1_auto_startup(struct tpm_chip *chip);
>  int tpm_do_selftest(struct tpm_chip *chip);
> diff --git a/drivers/char/tpm/tpm2-cmd.c b/drivers/char/tpm/tpm2-cmd.c
> index 499f405..a2a0314 100644
> --- a/drivers/char/tpm/tpm2-cmd.c
> +++ b/drivers/char/tpm/tpm2-cmd.c
> @@ -576,7 +576,7 @@ static int tpm2_load(struct tpm_chip *chip,
>  		goto out;
>  	}
>  
> -	rc = tpm_transmit_cmd(chip, buf.data, PAGE_SIZE, "loading blob");
> +	rc = __tpm_transmit_cmd(chip, buf.data, PAGE_SIZE, "loading blob", 0);
>  	if (!rc)
>  		*blob_handle = be32_to_cpup(
>  			(__be32 *) &buf.data[TPM_HEADER_SIZE]);
> @@ -604,7 +604,8 @@ static void tpm2_flush_context(struct tpm_chip *chip, u32 handle)
>  
>  	tpm_buf_append_u32(&buf, handle);
>  
> -	rc = tpm_transmit_cmd(chip, buf.data, PAGE_SIZE, "flushing context");
> +	rc = __tpm_transmit_cmd(chip, buf.data, PAGE_SIZE, "flushing context",
> +				0);
>  	if (rc)
>  		dev_warn(&chip->dev, "0x%08x was not flushed, rc=%d\n", handle,
>  			 rc);
> @@ -635,7 +636,7 @@ static int tpm2_unseal(struct tpm_chip *chip,
>  			     options->blobauth /* hmac */,
>  			     TPM_DIGEST_SIZE);
>  
> -	rc = tpm_transmit_cmd(chip, buf.data, PAGE_SIZE, "unsealing");
> +	rc = __tpm_transmit_cmd(chip, buf.data, PAGE_SIZE, "unsealing", 0);
>  	if (rc > 0)
>  		rc = -EPERM;
>  
> @@ -668,14 +669,17 @@ int tpm2_unseal_trusted(struct tpm_chip *chip,
>  	u32 blob_handle;
>  	int rc;
>  
> +	mutex_lock(&chip->tpm_mutex);
>  	rc = tpm2_load(chip, payload, options, &blob_handle);
>  	if (rc)
> -		return rc;
> +		goto out;
>  
>  	rc = tpm2_unseal(chip, payload, options, blob_handle);
>  
>  	tpm2_flush_context(chip, blob_handle);
>  
> +out:
> +	mutex_unlock(&chip->tpm_mutex);
>  	return rc;
>  }
>  
> -- 
> 2.7.4
> 

Re: [PATCH] tpm: fix a race condition tpm2_unseal_trusted()

[Jarkko Sakkinen]

Jason, I guess this should be now less intrusive than the original one?

The main goal was to make it as backportable as possible.

/Jarkko

On Tue, Aug 23, 2016 at 08:57:22PM -0400, Jarkko Sakkinen wrote:
> Unseal and load operations should be done as an atomic operation. This
> commit introduces unlocked tpm_transmit() so that tpm2_unseal_trusted()
> can do the locking by itself.
> 
> v2: Introduced an unlocked unseal operation instead of changing locking
>     strategy in order to make less intrusive bug fix and thus more
>     backportable.
> 
> v3: Have also separate __tpm_transmit() that takes 'flags' in order to
>     better localize the bug fix and make it easier to backport.
> 
> v4: Cleaned up the control flow in tpm2_unseal_trusted. Added the
>     missing 'Fixes' line.
> 
> Fixes: 0fe5480303a1 ("keys, trusted: seal/unseal with TPM 2.0 chips")
> CC: stable@vger.kernel.org
> Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
> ---
>  drivers/char/tpm/tpm-interface.c | 16 +++++++++-------
>  drivers/char/tpm/tpm.h           | 25 +++++++++++++++++++++----
>  drivers/char/tpm/tpm2-cmd.c      | 12 ++++++++----
>  3 files changed, 38 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/char/tpm/tpm-interface.c b/drivers/char/tpm/tpm-interface.c
> index 43ef0ef..80e702a 100644
> --- a/drivers/char/tpm/tpm-interface.c
> +++ b/drivers/char/tpm/tpm-interface.c
> @@ -330,8 +330,8 @@ EXPORT_SYMBOL_GPL(tpm_calc_ordinal_duration);
>  /*
>   * Internal kernel interface to transmit TPM commands
>   */
> -ssize_t tpm_transmit(struct tpm_chip *chip, const char *buf,
> -		     size_t bufsiz)
> +ssize_t __tpm_transmit(struct tpm_chip *chip, const char *buf,
> +		       size_t bufsiz, unsigned int flags)
>  {
>  	ssize_t rc;
>  	u32 count, ordinal;
> @@ -350,7 +350,8 @@ ssize_t tpm_transmit(struct tpm_chip *chip, const char *buf,
>  		return -E2BIG;
>  	}
>  
> -	mutex_lock(&chip->tpm_mutex);
> +	if (flags & TPM_TRANSMIT_LOCK)
> +		mutex_lock(&chip->tpm_mutex);
>  
>  	rc = chip->ops->send(chip, (u8 *) buf, count);
>  	if (rc < 0) {
> @@ -393,20 +394,21 @@ out_recv:
>  		dev_err(&chip->dev,
>  			"tpm_transmit: tpm_recv: error %zd\n", rc);
>  out:
> -	mutex_unlock(&chip->tpm_mutex);
> +	if (flags & TPM_TRANSMIT_LOCK)
> +		mutex_unlock(&chip->tpm_mutex);
>  	return rc;
>  }
>  
>  #define TPM_DIGEST_SIZE 20
>  #define TPM_RET_CODE_IDX 6
>  
> -ssize_t tpm_transmit_cmd(struct tpm_chip *chip, void *cmd,
> -			 int len, const char *desc)
> +ssize_t __tpm_transmit_cmd(struct tpm_chip *chip, void *cmd,
> +			   int len, const char *desc, unsigned int flags)
>  {
>  	struct tpm_output_header *header;
>  	int err;
>  
> -	len = tpm_transmit(chip, (u8 *) cmd, len);
> +	len = __tpm_transmit(chip, cmd, len, flags);
>  	if (len <  0)
>  		return len;
>  	else if (len < TPM_HEADER_SIZE)
> diff --git a/drivers/char/tpm/tpm.h b/drivers/char/tpm/tpm.h
> index 6e002c4..0a4abf0 100644
> --- a/drivers/char/tpm/tpm.h
> +++ b/drivers/char/tpm/tpm.h
> @@ -476,12 +476,29 @@ extern dev_t tpm_devt;
>  extern const struct file_operations tpm_fops;
>  extern struct idr dev_nums_idr;
>  
> +enum tpm_transmit_flags {
> +	TPM_TRANSMIT_LOCK,
> +};
> +
> +ssize_t __tpm_transmit(struct tpm_chip *chip, const char *buf,
> +		       size_t bufsiz, unsigned int flags);
> +ssize_t __tpm_transmit_cmd(struct tpm_chip *chip, void *cmd, int len,
> +			   const char *desc, unsigned int flags);
> +
> +static inline ssize_t tpm_transmit(struct tpm_chip *chip, const char *buf,
> +				   size_t bufsiz)
> +{
> +	return __tpm_transmit(chip, buf, bufsiz, TPM_TRANSMIT_LOCK);
> +}
> +
> +static inline ssize_t tpm_transmit_cmd(struct tpm_chip *chip, void *cmd,
> +				       int len, const char *desc)
> +{
> +	return __tpm_transmit_cmd(chip, cmd, len, desc, TPM_TRANSMIT_LOCK);
> +}
> +
>  ssize_t tpm_getcap(struct tpm_chip *chip, __be32 subcap_id, cap_t *cap,
>  		   const char *desc);
> -ssize_t tpm_transmit(struct tpm_chip *chip, const char *buf,
> -		     size_t bufsiz);
> -ssize_t tpm_transmit_cmd(struct tpm_chip *chip, void *cmd, int len,
> -			 const char *desc);
>  int tpm_get_timeouts(struct tpm_chip *chip);
>  int tpm1_auto_startup(struct tpm_chip *chip);
>  int tpm_do_selftest(struct tpm_chip *chip);
> diff --git a/drivers/char/tpm/tpm2-cmd.c b/drivers/char/tpm/tpm2-cmd.c
> index 499f405..a2a0314 100644
> --- a/drivers/char/tpm/tpm2-cmd.c
> +++ b/drivers/char/tpm/tpm2-cmd.c
> @@ -576,7 +576,7 @@ static int tpm2_load(struct tpm_chip *chip,
>  		goto out;
>  	}
>  
> -	rc = tpm_transmit_cmd(chip, buf.data, PAGE_SIZE, "loading blob");
> +	rc = __tpm_transmit_cmd(chip, buf.data, PAGE_SIZE, "loading blob", 0);
>  	if (!rc)
>  		*blob_handle = be32_to_cpup(
>  			(__be32 *) &buf.data[TPM_HEADER_SIZE]);
> @@ -604,7 +604,8 @@ static void tpm2_flush_context(struct tpm_chip *chip, u32 handle)
>  
>  	tpm_buf_append_u32(&buf, handle);
>  
> -	rc = tpm_transmit_cmd(chip, buf.data, PAGE_SIZE, "flushing context");
> +	rc = __tpm_transmit_cmd(chip, buf.data, PAGE_SIZE, "flushing context",
> +				0);
>  	if (rc)
>  		dev_warn(&chip->dev, "0x%08x was not flushed, rc=%d\n", handle,
>  			 rc);
> @@ -635,7 +636,7 @@ static int tpm2_unseal(struct tpm_chip *chip,
>  			     options->blobauth /* hmac */,
>  			     TPM_DIGEST_SIZE);
>  
> -	rc = tpm_transmit_cmd(chip, buf.data, PAGE_SIZE, "unsealing");
> +	rc = __tpm_transmit_cmd(chip, buf.data, PAGE_SIZE, "unsealing", 0);
>  	if (rc > 0)
>  		rc = -EPERM;
>  
> @@ -668,14 +669,17 @@ int tpm2_unseal_trusted(struct tpm_chip *chip,
>  	u32 blob_handle;
>  	int rc;
>  
> +	mutex_lock(&chip->tpm_mutex);
>  	rc = tpm2_load(chip, payload, options, &blob_handle);
>  	if (rc)
> -		return rc;
> +		goto out;
>  
>  	rc = tpm2_unseal(chip, payload, options, blob_handle);
>  
>  	tpm2_flush_context(chip, blob_handle);
>  
> +out:
> +	mutex_unlock(&chip->tpm_mutex);
>  	return rc;
>  }
>  
> -- 
> 2.7.4
> 

Re: [PATCH] tpm: fix a race condition tpm2_unseal_trusted()

[Jarkko Sakkinen]

Jason, I guess this should be now less intrusive than the original one?

The main goal was to make it as backportable as possible.

/Jarkko

On Tue, Aug 23, 2016 at 08:57:22PM -0400, Jarkko Sakkinen wrote:
> Unseal and load operations should be done as an atomic operation. This
> commit introduces unlocked tpm_transmit() so that tpm2_unseal_trusted()
> can do the locking by itself.
> 
> v2: Introduced an unlocked unseal operation instead of changing locking
>     strategy in order to make less intrusive bug fix and thus more
>     backportable.
> 
> v3: Have also separate __tpm_transmit() that takes 'flags' in order to
>     better localize the bug fix and make it easier to backport.
> 
> v4: Cleaned up the control flow in tpm2_unseal_trusted. Added the
>     missing 'Fixes' line.
> 
> Fixes: 0fe5480303a1 ("keys, trusted: seal/unseal with TPM 2.0 chips")
> CC: stable-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>
> ---
>  drivers/char/tpm/tpm-interface.c | 16 +++++++++-------
>  drivers/char/tpm/tpm.h           | 25 +++++++++++++++++++++----
>  drivers/char/tpm/tpm2-cmd.c      | 12 ++++++++----
>  3 files changed, 38 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/char/tpm/tpm-interface.c b/drivers/char/tpm/tpm-interface.c
> index 43ef0ef..80e702a 100644
> --- a/drivers/char/tpm/tpm-interface.c
> +++ b/drivers/char/tpm/tpm-interface.c
> @@ -330,8 +330,8 @@ EXPORT_SYMBOL_GPL(tpm_calc_ordinal_duration);
>  /*
>   * Internal kernel interface to transmit TPM commands
>   */
> -ssize_t tpm_transmit(struct tpm_chip *chip, const char *buf,
> -		     size_t bufsiz)
> +ssize_t __tpm_transmit(struct tpm_chip *chip, const char *buf,
> +		       size_t bufsiz, unsigned int flags)
>  {
>  	ssize_t rc;
>  	u32 count, ordinal;
> @@ -350,7 +350,8 @@ ssize_t tpm_transmit(struct tpm_chip *chip, const char *buf,
>  		return -E2BIG;
>  	}
>  
> -	mutex_lock(&chip->tpm_mutex);
> +	if (flags & TPM_TRANSMIT_LOCK)
> +		mutex_lock(&chip->tpm_mutex);
>  
>  	rc = chip->ops->send(chip, (u8 *) buf, count);
>  	if (rc < 0) {
> @@ -393,20 +394,21 @@ out_recv:
>  		dev_err(&chip->dev,
>  			"tpm_transmit: tpm_recv: error %zd\n", rc);
>  out:
> -	mutex_unlock(&chip->tpm_mutex);
> +	if (flags & TPM_TRANSMIT_LOCK)
> +		mutex_unlock(&chip->tpm_mutex);
>  	return rc;
>  }
>  
>  #define TPM_DIGEST_SIZE 20
>  #define TPM_RET_CODE_IDX 6
>  
> -ssize_t tpm_transmit_cmd(struct tpm_chip *chip, void *cmd,
> -			 int len, const char *desc)
> +ssize_t __tpm_transmit_cmd(struct tpm_chip *chip, void *cmd,
> +			   int len, const char *desc, unsigned int flags)
>  {
>  	struct tpm_output_header *header;
>  	int err;
>  
> -	len = tpm_transmit(chip, (u8 *) cmd, len);
> +	len = __tpm_transmit(chip, cmd, len, flags);
>  	if (len <  0)
>  		return len;
>  	else if (len < TPM_HEADER_SIZE)
> diff --git a/drivers/char/tpm/tpm.h b/drivers/char/tpm/tpm.h
> index 6e002c4..0a4abf0 100644
> --- a/drivers/char/tpm/tpm.h
> +++ b/drivers/char/tpm/tpm.h
> @@ -476,12 +476,29 @@ extern dev_t tpm_devt;
>  extern const struct file_operations tpm_fops;
>  extern struct idr dev_nums_idr;
>  
> +enum tpm_transmit_flags {
> +	TPM_TRANSMIT_LOCK,
> +};
> +
> +ssize_t __tpm_transmit(struct tpm_chip *chip, const char *buf,
> +		       size_t bufsiz, unsigned int flags);
> +ssize_t __tpm_transmit_cmd(struct tpm_chip *chip, void *cmd, int len,
> +			   const char *desc, unsigned int flags);
> +
> +static inline ssize_t tpm_transmit(struct tpm_chip *chip, const char *buf,
> +				   size_t bufsiz)
> +{
> +	return __tpm_transmit(chip, buf, bufsiz, TPM_TRANSMIT_LOCK);
> +}
> +
> +static inline ssize_t tpm_transmit_cmd(struct tpm_chip *chip, void *cmd,
> +				       int len, const char *desc)
> +{
> +	return __tpm_transmit_cmd(chip, cmd, len, desc, TPM_TRANSMIT_LOCK);
> +}
> +
>  ssize_t tpm_getcap(struct tpm_chip *chip, __be32 subcap_id, cap_t *cap,
>  		   const char *desc);
> -ssize_t tpm_transmit(struct tpm_chip *chip, const char *buf,
> -		     size_t bufsiz);
> -ssize_t tpm_transmit_cmd(struct tpm_chip *chip, void *cmd, int len,
> -			 const char *desc);
>  int tpm_get_timeouts(struct tpm_chip *chip);
>  int tpm1_auto_startup(struct tpm_chip *chip);
>  int tpm_do_selftest(struct tpm_chip *chip);
> diff --git a/drivers/char/tpm/tpm2-cmd.c b/drivers/char/tpm/tpm2-cmd.c
> index 499f405..a2a0314 100644
> --- a/drivers/char/tpm/tpm2-cmd.c
> +++ b/drivers/char/tpm/tpm2-cmd.c
> @@ -576,7 +576,7 @@ static int tpm2_load(struct tpm_chip *chip,
>  		goto out;
>  	}
>  
> -	rc = tpm_transmit_cmd(chip, buf.data, PAGE_SIZE, "loading blob");
> +	rc = __tpm_transmit_cmd(chip, buf.data, PAGE_SIZE, "loading blob", 0);
>  	if (!rc)
>  		*blob_handle = be32_to_cpup(
>  			(__be32 *) &buf.data[TPM_HEADER_SIZE]);
> @@ -604,7 +604,8 @@ static void tpm2_flush_context(struct tpm_chip *chip, u32 handle)
>  
>  	tpm_buf_append_u32(&buf, handle);
>  
> -	rc = tpm_transmit_cmd(chip, buf.data, PAGE_SIZE, "flushing context");
> +	rc = __tpm_transmit_cmd(chip, buf.data, PAGE_SIZE, "flushing context",
> +				0);
>  	if (rc)
>  		dev_warn(&chip->dev, "0x%08x was not flushed, rc=%d\n", handle,
>  			 rc);
> @@ -635,7 +636,7 @@ static int tpm2_unseal(struct tpm_chip *chip,
>  			     options->blobauth /* hmac */,
>  			     TPM_DIGEST_SIZE);
>  
> -	rc = tpm_transmit_cmd(chip, buf.data, PAGE_SIZE, "unsealing");
> +	rc = __tpm_transmit_cmd(chip, buf.data, PAGE_SIZE, "unsealing", 0);
>  	if (rc > 0)
>  		rc = -EPERM;
>  
> @@ -668,14 +669,17 @@ int tpm2_unseal_trusted(struct tpm_chip *chip,
>  	u32 blob_handle;
>  	int rc;
>  
> +	mutex_lock(&chip->tpm_mutex);
>  	rc = tpm2_load(chip, payload, options, &blob_handle);
>  	if (rc)
> -		return rc;
> +		goto out;
>  
>  	rc = tpm2_unseal(chip, payload, options, blob_handle);
>  
>  	tpm2_flush_context(chip, blob_handle);
>  
> +out:
> +	mutex_unlock(&chip->tpm_mutex);
>  	return rc;
>  }
>  
> -- 
> 2.7.4
> 

------------------------------------------------------------------------------

Re: [PATCH] tpm: fix a race condition tpm2_unseal_trusted()

[Jason Gunthorpe]

On Tue, Aug 23, 2016 at 08:57:22PM -0400, Jarkko Sakkinen wrote:

> +     if (flags & TPM_TRANSMIT_LOCK)
> +             mutex_lock(&chip->tpm_mutex);

I think I would invert this. UNLOCKED is the exceptional case, so I'd
make the 0 flags lock. If we see UNLOCKED in the caller then we know
to audit for locking, 0 is much less obvious.

> @@ -576,7 +576,7 @@ static int tpm2_load(struct tpm_chip *chip,
>  		goto out;
>  	}
>  
> -	rc = tpm_transmit_cmd(chip, buf.data, PAGE_SIZE, "loading blob");
> +	rc = __tpm_transmit_cmd(chip, buf.data, PAGE_SIZE, "loading blob", 0);

All these points should accept a flags too and the caller should pass
in the TPM_TRASNMIT_UNLOCKED if it needs it..

> +	mutex_lock(&chip->tpm_mutex);
>  	rc = tpm2_load(chip, payload, options, &blob_handle);
>  	if (rc)

So when we read here we see the pattern:

> +	mutex_lock(&chip->tpm_mutex);
>  	rc = tpm2_load(chip, payload, options, &blob_handle, TPM_TRASNMIT_UNLOCKED);

Which is much easier to audit..

Jason

Re: [PATCH] tpm: fix a race condition tpm2_unseal_trusted()

[Jason Gunthorpe]

On Tue, Aug 23, 2016 at 08:57:22PM -0400, Jarkko Sakkinen wrote:

> +     if (flags & TPM_TRANSMIT_LOCK)
> +             mutex_lock(&chip->tpm_mutex);

I think I would invert this. UNLOCKED is the exceptional case, so I'd
make the 0 flags lock. If we see UNLOCKED in the caller then we know
to audit for locking, 0 is much less obvious.

> @@ -576,7 +576,7 @@ static int tpm2_load(struct tpm_chip *chip,
>  		goto out;
>  	}
>  
> -	rc = tpm_transmit_cmd(chip, buf.data, PAGE_SIZE, "loading blob");
> +	rc = __tpm_transmit_cmd(chip, buf.data, PAGE_SIZE, "loading blob", 0);

All these points should accept a flags too and the caller should pass
in the TPM_TRASNMIT_UNLOCKED if it needs it..

> +	mutex_lock(&chip->tpm_mutex);
>  	rc = tpm2_load(chip, payload, options, &blob_handle);
>  	if (rc)

So when we read here we see the pattern:

> +	mutex_lock(&chip->tpm_mutex);
>  	rc = tpm2_load(chip, payload, options, &blob_handle, TPM_TRASNMIT_UNLOCKED);

Which is much easier to audit..

Jason

Re: [PATCH] tpm: fix a race condition tpm2_unseal_trusted()

[Jason Gunthorpe]

On Tue, Aug 23, 2016 at 08:57:22PM -0400, Jarkko Sakkinen wrote:

> +     if (flags & TPM_TRANSMIT_LOCK)
> +             mutex_lock(&chip->tpm_mutex);

I think I would invert this. UNLOCKED is the exceptional case, so I'd
make the 0 flags lock. If we see UNLOCKED in the caller then we know
to audit for locking, 0 is much less obvious.

> @@ -576,7 +576,7 @@ static int tpm2_load(struct tpm_chip *chip,
>  		goto out;
>  	}
>  
> -	rc = tpm_transmit_cmd(chip, buf.data, PAGE_SIZE, "loading blob");
> +	rc = __tpm_transmit_cmd(chip, buf.data, PAGE_SIZE, "loading blob", 0);

All these points should accept a flags too and the caller should pass
in the TPM_TRASNMIT_UNLOCKED if it needs it..

> +	mutex_lock(&chip->tpm_mutex);
>  	rc = tpm2_load(chip, payload, options, &blob_handle);
>  	if (rc)

So when we read here we see the pattern:

> +	mutex_lock(&chip->tpm_mutex);
>  	rc = tpm2_load(chip, payload, options, &blob_handle, TPM_TRASNMIT_UNLOCKED);

Which is much easier to audit..

Jason

------------------------------------------------------------------------------

Re: [PATCH] tpm: fix a race condition tpm2_unseal_trusted()

[Jarkko Sakkinen]

On Thu, Aug 25, 2016 at 12:30:59PM -0600, Jason Gunthorpe wrote:
> On Tue, Aug 23, 2016 at 08:57:22PM -0400, Jarkko Sakkinen wrote:
> 
> > +     if (flags & TPM_TRANSMIT_LOCK)
> > +             mutex_lock(&chip->tpm_mutex);
> 
> I think I would invert this. UNLOCKED is the exceptional case, so I'd
> make the 0 flags lock. If we see UNLOCKED in the caller then we know
> to audit for locking, 0 is much less obvious.

I'm fine with either way.

> > @@ -576,7 +576,7 @@ static int tpm2_load(struct tpm_chip *chip,
> >  		goto out;
> >  	}
> >  
> > -	rc = tpm_transmit_cmd(chip, buf.data, PAGE_SIZE, "loading blob");
> > +	rc = __tpm_transmit_cmd(chip, buf.data, PAGE_SIZE, "loading blob", 0);
> 
> All these points should accept a flags too and the caller should pass
> in the TPM_TRASNMIT_UNLOCKED if it needs it..

For this bug fix it makes sense to implement it the way I did because it
needs to be applied to multiple releases (I think I've underlined this
in my changelog).

If you think this is high priority, I can make the next revision into
patch set of two patches. The second patch would implement the change
you suggested.

/Jarkko

Re: [PATCH] tpm: fix a race condition tpm2_unseal_trusted()

[Jarkko Sakkinen]

On Thu, Aug 25, 2016 at 12:30:59PM -0600, Jason Gunthorpe wrote:
> On Tue, Aug 23, 2016 at 08:57:22PM -0400, Jarkko Sakkinen wrote:
> 
> > +     if (flags & TPM_TRANSMIT_LOCK)
> > +             mutex_lock(&chip->tpm_mutex);
> 
> I think I would invert this. UNLOCKED is the exceptional case, so I'd
> make the 0 flags lock. If we see UNLOCKED in the caller then we know
> to audit for locking, 0 is much less obvious.

I'm fine with either way.

> > @@ -576,7 +576,7 @@ static int tpm2_load(struct tpm_chip *chip,
> >  		goto out;
> >  	}
> >  
> > -	rc = tpm_transmit_cmd(chip, buf.data, PAGE_SIZE, "loading blob");
> > +	rc = __tpm_transmit_cmd(chip, buf.data, PAGE_SIZE, "loading blob", 0);
> 
> All these points should accept a flags too and the caller should pass
> in the TPM_TRASNMIT_UNLOCKED if it needs it..

For this bug fix it makes sense to implement it the way I did because it
needs to be applied to multiple releases (I think I've underlined this
in my changelog).

If you think this is high priority, I can make the next revision into
patch set of two patches. The second patch would implement the change
you suggested.

/Jarkko

Re: [PATCH] tpm: fix a race condition tpm2_unseal_trusted()

[Jarkko Sakkinen]

On Thu, Aug 25, 2016 at 12:30:59PM -0600, Jason Gunthorpe wrote:
> On Tue, Aug 23, 2016 at 08:57:22PM -0400, Jarkko Sakkinen wrote:
> 
> > +     if (flags & TPM_TRANSMIT_LOCK)
> > +             mutex_lock(&chip->tpm_mutex);
> 
> I think I would invert this. UNLOCKED is the exceptional case, so I'd
> make the 0 flags lock. If we see UNLOCKED in the caller then we know
> to audit for locking, 0 is much less obvious.

I'm fine with either way.

> > @@ -576,7 +576,7 @@ static int tpm2_load(struct tpm_chip *chip,
> >  		goto out;
> >  	}
> >  
> > -	rc = tpm_transmit_cmd(chip, buf.data, PAGE_SIZE, "loading blob");
> > +	rc = __tpm_transmit_cmd(chip, buf.data, PAGE_SIZE, "loading blob", 0);
> 
> All these points should accept a flags too and the caller should pass
> in the TPM_TRASNMIT_UNLOCKED if it needs it..

For this bug fix it makes sense to implement it the way I did because it
needs to be applied to multiple releases (I think I've underlined this
in my changelog).

If you think this is high priority, I can make the next revision into
patch set of two patches. The second patch would implement the change
you suggested.

/Jarkko

------------------------------------------------------------------------------

Re: [PATCH] tpm: fix a race condition tpm2_unseal_trusted()

[Jason Gunthorpe]

On Thu, Aug 25, 2016 at 05:06:10PM -0400, Jarkko Sakkinen wrote:
> On Thu, Aug 25, 2016 at 12:30:59PM -0600, Jason Gunthorpe wrote:
> > On Tue, Aug 23, 2016 at 08:57:22PM -0400, Jarkko Sakkinen wrote:
> > 
> > > +     if (flags & TPM_TRANSMIT_LOCK)
> > > +             mutex_lock(&chip->tpm_mutex);
> > 
> > I think I would invert this. UNLOCKED is the exceptional case, so I'd
> > make the 0 flags lock. If we see UNLOCKED in the caller then we know
> > to audit for locking, 0 is much less obvious.
> 
> I'm fine with either way.
> 
> > > @@ -576,7 +576,7 @@ static int tpm2_load(struct tpm_chip *chip,
> > >  		goto out;
> > >  	}
> > >  
> > > -	rc = tpm_transmit_cmd(chip, buf.data, PAGE_SIZE, "loading blob");
> > > +	rc = __tpm_transmit_cmd(chip, buf.data, PAGE_SIZE, "loading blob", 0);
> > 
> > All these points should accept a flags too and the caller should pass
> > in the TPM_TRASNMIT_UNLOCKED if it needs it..
> 
> For this bug fix it makes sense to implement it the way I did because it
> needs to be applied to multiple releases (I think I've underlined this
> in my changelog).

You shouldn't compromise the mainline kernel to ease backporting, I'm
not sure why adding a flags to tpm2_load would be a problem for the
-stable kernels?

It is generally better to make the backports move the older kernels
closer to mainline than to have them be something else, it makes it
easier to apply future backport fixes.

> If you think this is high priority, I can make the next revision into
> patch set of two patches. The second patch would implement the change
> you suggested.

Yes, I think it is important the locking requirement be very clear
from the code.

Jason

Re: [PATCH] tpm: fix a race condition tpm2_unseal_trusted()

[Jason Gunthorpe]

On Thu, Aug 25, 2016 at 05:06:10PM -0400, Jarkko Sakkinen wrote:
> On Thu, Aug 25, 2016 at 12:30:59PM -0600, Jason Gunthorpe wrote:
> > On Tue, Aug 23, 2016 at 08:57:22PM -0400, Jarkko Sakkinen wrote:
> > 
> > > +     if (flags & TPM_TRANSMIT_LOCK)
> > > +             mutex_lock(&chip->tpm_mutex);
> > 
> > I think I would invert this. UNLOCKED is the exceptional case, so I'd
> > make the 0 flags lock. If we see UNLOCKED in the caller then we know
> > to audit for locking, 0 is much less obvious.
> 
> I'm fine with either way.
> 
> > > @@ -576,7 +576,7 @@ static int tpm2_load(struct tpm_chip *chip,
> > >  		goto out;
> > >  	}
> > >  
> > > -	rc = tpm_transmit_cmd(chip, buf.data, PAGE_SIZE, "loading blob");
> > > +	rc = __tpm_transmit_cmd(chip, buf.data, PAGE_SIZE, "loading blob", 0);
> > 
> > All these points should accept a flags too and the caller should pass
> > in the TPM_TRASNMIT_UNLOCKED if it needs it..
> 
> For this bug fix it makes sense to implement it the way I did because it
> needs to be applied to multiple releases (I think I've underlined this
> in my changelog).

You shouldn't compromise the mainline kernel to ease backporting, I'm
not sure why adding a flags to tpm2_load would be a problem for the
-stable kernels?

It is generally better to make the backports move the older kernels
closer to mainline than to have them be something else, it makes it
easier to apply future backport fixes.

> If you think this is high priority, I can make the next revision into
> patch set of two patches. The second patch would implement the change
> you suggested.

Yes, I think it is important the locking requirement be very clear
from the code.

Jason

Re: [PATCH] tpm: fix a race condition tpm2_unseal_trusted()

[Jason Gunthorpe]

On Thu, Aug 25, 2016 at 05:06:10PM -0400, Jarkko Sakkinen wrote:
> On Thu, Aug 25, 2016 at 12:30:59PM -0600, Jason Gunthorpe wrote:
> > On Tue, Aug 23, 2016 at 08:57:22PM -0400, Jarkko Sakkinen wrote:
> > 
> > > +     if (flags & TPM_TRANSMIT_LOCK)
> > > +             mutex_lock(&chip->tpm_mutex);
> > 
> > I think I would invert this. UNLOCKED is the exceptional case, so I'd
> > make the 0 flags lock. If we see UNLOCKED in the caller then we know
> > to audit for locking, 0 is much less obvious.
> 
> I'm fine with either way.
> 
> > > @@ -576,7 +576,7 @@ static int tpm2_load(struct tpm_chip *chip,
> > >  		goto out;
> > >  	}
> > >  
> > > -	rc = tpm_transmit_cmd(chip, buf.data, PAGE_SIZE, "loading blob");
> > > +	rc = __tpm_transmit_cmd(chip, buf.data, PAGE_SIZE, "loading blob", 0);
> > 
> > All these points should accept a flags too and the caller should pass
> > in the TPM_TRASNMIT_UNLOCKED if it needs it..
> 
> For this bug fix it makes sense to implement it the way I did because it
> needs to be applied to multiple releases (I think I've underlined this
> in my changelog).

You shouldn't compromise the mainline kernel to ease backporting, I'm
not sure why adding a flags to tpm2_load would be a problem for the
-stable kernels?

It is generally better to make the backports move the older kernels
closer to mainline than to have them be something else, it makes it
easier to apply future backport fixes.

> If you think this is high priority, I can make the next revision into
> patch set of two patches. The second patch would implement the change
> you suggested.

Yes, I think it is important the locking requirement be very clear
from the code.

Jason

------------------------------------------------------------------------------

Re: [PATCH] tpm: fix a race condition tpm2_unseal_trusted()

[Jarkko Sakkinen]

On Tue, Aug 16, 2016 at 10:38:22PM +0300, Jarkko Sakkinen wrote:
> Unseal and load operations should be done as an atomic operation. This
> commit introduces unlocked tpm_transmit() so that tpm2_unseal_trusted()
> can do the locking by itself.
> 
> v2: Introduced an unlocked unseal operation instead of changing locking
>     strategy in order to make less intrusive bug fix and thus more
>     backportable.
> 
> CC: stable@vger.kernel.org
> Fixes: 954650efb79f ("tpm: seal/unseal for TPM 2.0")
> Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
> ---
>  drivers/char/tpm/tpm-dev.c       |  2 +-
>  drivers/char/tpm/tpm-interface.c | 14 ++++++++------
>  drivers/char/tpm/tpm.h           | 18 ++++++++++++++----
>  drivers/char/tpm/tpm2-cmd.c      | 13 +++++++++----
>  4 files changed, 32 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/char/tpm/tpm-dev.c b/drivers/char/tpm/tpm-dev.c
> index f5d4521..8df8846 100644
> --- a/drivers/char/tpm/tpm-dev.c
> +++ b/drivers/char/tpm/tpm-dev.c
> @@ -145,7 +145,7 @@ static ssize_t tpm_write(struct file *file, const char __user *buf,
>  		return -EPIPE;
>  	}
>  	out_size = tpm_transmit(priv->chip, priv->data_buffer,
> -				sizeof(priv->data_buffer));
> +				sizeof(priv->data_buffer), TPM_TRANSMIT_LOCK);
>  
>  	tpm_put_ops(priv->chip);
>  	if (out_size < 0) {
> diff --git a/drivers/char/tpm/tpm-interface.c b/drivers/char/tpm/tpm-interface.c
> index 43ef0ef..627daa7 100644
> --- a/drivers/char/tpm/tpm-interface.c
> +++ b/drivers/char/tpm/tpm-interface.c
> @@ -331,7 +331,7 @@ EXPORT_SYMBOL_GPL(tpm_calc_ordinal_duration);
>   * Internal kernel interface to transmit TPM commands
>   */
>  ssize_t tpm_transmit(struct tpm_chip *chip, const char *buf,
> -		     size_t bufsiz)
> +		     size_t bufsiz, unsigned int flags)
>  {
>  	ssize_t rc;
>  	u32 count, ordinal;
> @@ -350,7 +350,8 @@ ssize_t tpm_transmit(struct tpm_chip *chip, const char *buf,
>  		return -E2BIG;
>  	}
>  
> -	mutex_lock(&chip->tpm_mutex);
> +	if (flags & TPM_TRANSMIT_LOCK)
> +		mutex_lock(&chip->tpm_mutex);
>  
>  	rc = chip->ops->send(chip, (u8 *) buf, count);
>  	if (rc < 0) {
> @@ -393,20 +394,21 @@ out_recv:
>  		dev_err(&chip->dev,
>  			"tpm_transmit: tpm_recv: error %zd\n", rc);
>  out:
> -	mutex_unlock(&chip->tpm_mutex);
> +	if (flags & TPM_TRANSMIT_LOCK)
> +		mutex_unlock(&chip->tpm_mutex);
>  	return rc;
>  }
>  
>  #define TPM_DIGEST_SIZE 20
>  #define TPM_RET_CODE_IDX 6
>  
> -ssize_t tpm_transmit_cmd(struct tpm_chip *chip, void *cmd,
> -			 int len, const char *desc)
> +ssize_t __tpm_transmit_cmd(struct tpm_chip *chip, void *cmd,
> +			   int len, const char *desc, unsigned int flags)
>  {
>  	struct tpm_output_header *header;
>  	int err;
>  
> -	len = tpm_transmit(chip, (u8 *) cmd, len);
> +	len = tpm_transmit(chip, cmd, len, flags);
>  	if (len <  0)
>  		return len;
>  	else if (len < TPM_HEADER_SIZE)
> diff --git a/drivers/char/tpm/tpm.h b/drivers/char/tpm/tpm.h
> index 6e002c4..b9383fd 100644
> --- a/drivers/char/tpm/tpm.h
> +++ b/drivers/char/tpm/tpm.h
> @@ -476,12 +476,22 @@ extern dev_t tpm_devt;
>  extern const struct file_operations tpm_fops;
>  extern struct idr dev_nums_idr;
>  
> +enum tpm_transmit_flags {
> +	TPM_TRANSMIT_LOCK,
> +};
> +
> +ssize_t tpm_transmit(struct tpm_chip *chip, const char *buf,
> +		     size_t bufsiz, unsigned int flags);
> +ssize_t __tpm_transmit_cmd(struct tpm_chip *chip, void *cmd, int len,
> +			   const char *desc, unsigned int flags);
> +static inline ssize_t tpm_transmit_cmd(struct tpm_chip *chip, void *cmd,
> +				       int len, const char *desc)
> +{
> +	return __tpm_transmit_cmd(chip, cmd, len, desc, TPM_TRANSMIT_LOCK);
> +}
> +
>  ssize_t tpm_getcap(struct tpm_chip *chip, __be32 subcap_id, cap_t *cap,
>  		   const char *desc);
> -ssize_t tpm_transmit(struct tpm_chip *chip, const char *buf,
> -		     size_t bufsiz);

By having also __tpm_transmit() the patch would be more localized, which
would make it easier to backport. I think I'll add that.

/Jarkko

> -ssize_t tpm_transmit_cmd(struct tpm_chip *chip, void *cmd, int len,
> -			 const char *desc);
>  int tpm_get_timeouts(struct tpm_chip *chip);
>  int tpm1_auto_startup(struct tpm_chip *chip);
>  int tpm_do_selftest(struct tpm_chip *chip);
> diff --git a/drivers/char/tpm/tpm2-cmd.c b/drivers/char/tpm/tpm2-cmd.c
> index 499f405..99007d8 100644
> --- a/drivers/char/tpm/tpm2-cmd.c
> +++ b/drivers/char/tpm/tpm2-cmd.c
> @@ -576,7 +576,7 @@ static int tpm2_load(struct tpm_chip *chip,
>  		goto out;
>  	}
>  
> -	rc = tpm_transmit_cmd(chip, buf.data, PAGE_SIZE, "loading blob");
> +	rc = __tpm_transmit_cmd(chip, buf.data, PAGE_SIZE, "loading blob", 0);
>  	if (!rc)
>  		*blob_handle = be32_to_cpup(
>  			(__be32 *) &buf.data[TPM_HEADER_SIZE]);
> @@ -604,7 +604,8 @@ static void tpm2_flush_context(struct tpm_chip *chip, u32 handle)
>  
>  	tpm_buf_append_u32(&buf, handle);
>  
> -	rc = tpm_transmit_cmd(chip, buf.data, PAGE_SIZE, "flushing context");
> +	rc = __tpm_transmit_cmd(chip, buf.data, PAGE_SIZE, "flushing context",
> +				0);
>  	if (rc)
>  		dev_warn(&chip->dev, "0x%08x was not flushed, rc=%d\n", handle,
>  			 rc);
> @@ -635,7 +636,7 @@ static int tpm2_unseal(struct tpm_chip *chip,
>  			     options->blobauth /* hmac */,
>  			     TPM_DIGEST_SIZE);
>  
> -	rc = tpm_transmit_cmd(chip, buf.data, PAGE_SIZE, "unsealing");
> +	rc = __tpm_transmit_cmd(chip, buf.data, PAGE_SIZE, "unsealing", 0);
>  	if (rc > 0)
>  		rc = -EPERM;
>  
> @@ -668,13 +669,17 @@ int tpm2_unseal_trusted(struct tpm_chip *chip,
>  	u32 blob_handle;
>  	int rc;
>  
> +	mutex_lock(&chip->tpm_mutex);
>  	rc = tpm2_load(chip, payload, options, &blob_handle);
> -	if (rc)
> +	if (rc) {
> +		mutex_unlock(&chip->tpm_mutex);
>  		return rc;
> +	}
>  
>  	rc = tpm2_unseal(chip, payload, options, blob_handle);
>  
>  	tpm2_flush_context(chip, blob_handle);
> +	mutex_unlock(&chip->tpm_mutex);
>  
>  	return rc;
>  }
> -- 
> 2.7.4
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] tpm: fix a race condition tpm2_unseal_trusted()

[Jarkko Sakkinen]

On Tue, Aug 16, 2016 at 10:38:22PM +0300, Jarkko Sakkinen wrote:
> Unseal and load operations should be done as an atomic operation. This
> commit introduces unlocked tpm_transmit() so that tpm2_unseal_trusted()
> can do the locking by itself.
> 
> v2: Introduced an unlocked unseal operation instead of changing locking
>     strategy in order to make less intrusive bug fix and thus more
>     backportable.
> 
> CC: stable@vger.kernel.org
> Fixes: 954650efb79f ("tpm: seal/unseal for TPM 2.0")
> Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
> ---
>  drivers/char/tpm/tpm-dev.c       |  2 +-
>  drivers/char/tpm/tpm-interface.c | 14 ++++++++------
>  drivers/char/tpm/tpm.h           | 18 ++++++++++++++----
>  drivers/char/tpm/tpm2-cmd.c      | 13 +++++++++----
>  4 files changed, 32 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/char/tpm/tpm-dev.c b/drivers/char/tpm/tpm-dev.c
> index f5d4521..8df8846 100644
> --- a/drivers/char/tpm/tpm-dev.c
> +++ b/drivers/char/tpm/tpm-dev.c
> @@ -145,7 +145,7 @@ static ssize_t tpm_write(struct file *file, const char __user *buf,
>  		return -EPIPE;
>  	}
>  	out_size = tpm_transmit(priv->chip, priv->data_buffer,
> -				sizeof(priv->data_buffer));
> +				sizeof(priv->data_buffer), TPM_TRANSMIT_LOCK);
>  
>  	tpm_put_ops(priv->chip);
>  	if (out_size < 0) {
> diff --git a/drivers/char/tpm/tpm-interface.c b/drivers/char/tpm/tpm-interface.c
> index 43ef0ef..627daa7 100644
> --- a/drivers/char/tpm/tpm-interface.c
> +++ b/drivers/char/tpm/tpm-interface.c
> @@ -331,7 +331,7 @@ EXPORT_SYMBOL_GPL(tpm_calc_ordinal_duration);
>   * Internal kernel interface to transmit TPM commands
>   */
>  ssize_t tpm_transmit(struct tpm_chip *chip, const char *buf,
> -		     size_t bufsiz)
> +		     size_t bufsiz, unsigned int flags)
>  {
>  	ssize_t rc;
>  	u32 count, ordinal;
> @@ -350,7 +350,8 @@ ssize_t tpm_transmit(struct tpm_chip *chip, const char *buf,
>  		return -E2BIG;
>  	}
>  
> -	mutex_lock(&chip->tpm_mutex);
> +	if (flags & TPM_TRANSMIT_LOCK)
> +		mutex_lock(&chip->tpm_mutex);
>  
>  	rc = chip->ops->send(chip, (u8 *) buf, count);
>  	if (rc < 0) {
> @@ -393,20 +394,21 @@ out_recv:
>  		dev_err(&chip->dev,
>  			"tpm_transmit: tpm_recv: error %zd\n", rc);
>  out:
> -	mutex_unlock(&chip->tpm_mutex);
> +	if (flags & TPM_TRANSMIT_LOCK)
> +		mutex_unlock(&chip->tpm_mutex);
>  	return rc;
>  }
>  
>  #define TPM_DIGEST_SIZE 20
>  #define TPM_RET_CODE_IDX 6
>  
> -ssize_t tpm_transmit_cmd(struct tpm_chip *chip, void *cmd,
> -			 int len, const char *desc)
> +ssize_t __tpm_transmit_cmd(struct tpm_chip *chip, void *cmd,
> +			   int len, const char *desc, unsigned int flags)
>  {
>  	struct tpm_output_header *header;
>  	int err;
>  
> -	len = tpm_transmit(chip, (u8 *) cmd, len);
> +	len = tpm_transmit(chip, cmd, len, flags);
>  	if (len <  0)
>  		return len;
>  	else if (len < TPM_HEADER_SIZE)
> diff --git a/drivers/char/tpm/tpm.h b/drivers/char/tpm/tpm.h
> index 6e002c4..b9383fd 100644
> --- a/drivers/char/tpm/tpm.h
> +++ b/drivers/char/tpm/tpm.h
> @@ -476,12 +476,22 @@ extern dev_t tpm_devt;
>  extern const struct file_operations tpm_fops;
>  extern struct idr dev_nums_idr;
>  
> +enum tpm_transmit_flags {
> +	TPM_TRANSMIT_LOCK,
> +};
> +
> +ssize_t tpm_transmit(struct tpm_chip *chip, const char *buf,
> +		     size_t bufsiz, unsigned int flags);
> +ssize_t __tpm_transmit_cmd(struct tpm_chip *chip, void *cmd, int len,
> +			   const char *desc, unsigned int flags);
> +static inline ssize_t tpm_transmit_cmd(struct tpm_chip *chip, void *cmd,
> +				       int len, const char *desc)
> +{
> +	return __tpm_transmit_cmd(chip, cmd, len, desc, TPM_TRANSMIT_LOCK);
> +}
> +
>  ssize_t tpm_getcap(struct tpm_chip *chip, __be32 subcap_id, cap_t *cap,
>  		   const char *desc);
> -ssize_t tpm_transmit(struct tpm_chip *chip, const char *buf,
> -		     size_t bufsiz);

By having also __tpm_transmit() the patch would be more localized, which
would make it easier to backport. I think I'll add that.

/Jarkko

> -ssize_t tpm_transmit_cmd(struct tpm_chip *chip, void *cmd, int len,
> -			 const char *desc);
>  int tpm_get_timeouts(struct tpm_chip *chip);
>  int tpm1_auto_startup(struct tpm_chip *chip);
>  int tpm_do_selftest(struct tpm_chip *chip);
> diff --git a/drivers/char/tpm/tpm2-cmd.c b/drivers/char/tpm/tpm2-cmd.c
> index 499f405..99007d8 100644
> --- a/drivers/char/tpm/tpm2-cmd.c
> +++ b/drivers/char/tpm/tpm2-cmd.c
> @@ -576,7 +576,7 @@ static int tpm2_load(struct tpm_chip *chip,
>  		goto out;
>  	}
>  
> -	rc = tpm_transmit_cmd(chip, buf.data, PAGE_SIZE, "loading blob");
> +	rc = __tpm_transmit_cmd(chip, buf.data, PAGE_SIZE, "loading blob", 0);
>  	if (!rc)
>  		*blob_handle = be32_to_cpup(
>  			(__be32 *) &buf.data[TPM_HEADER_SIZE]);
> @@ -604,7 +604,8 @@ static void tpm2_flush_context(struct tpm_chip *chip, u32 handle)
>  
>  	tpm_buf_append_u32(&buf, handle);
>  
> -	rc = tpm_transmit_cmd(chip, buf.data, PAGE_SIZE, "flushing context");
> +	rc = __tpm_transmit_cmd(chip, buf.data, PAGE_SIZE, "flushing context",
> +				0);
>  	if (rc)
>  		dev_warn(&chip->dev, "0x%08x was not flushed, rc=%d\n", handle,
>  			 rc);
> @@ -635,7 +636,7 @@ static int tpm2_unseal(struct tpm_chip *chip,
>  			     options->blobauth /* hmac */,
>  			     TPM_DIGEST_SIZE);
>  
> -	rc = tpm_transmit_cmd(chip, buf.data, PAGE_SIZE, "unsealing");
> +	rc = __tpm_transmit_cmd(chip, buf.data, PAGE_SIZE, "unsealing", 0);
>  	if (rc > 0)
>  		rc = -EPERM;
>  
> @@ -668,13 +669,17 @@ int tpm2_unseal_trusted(struct tpm_chip *chip,
>  	u32 blob_handle;
>  	int rc;
>  
> +	mutex_lock(&chip->tpm_mutex);
>  	rc = tpm2_load(chip, payload, options, &blob_handle);
> -	if (rc)
> +	if (rc) {
> +		mutex_unlock(&chip->tpm_mutex);
>  		return rc;
> +	}
>  
>  	rc = tpm2_unseal(chip, payload, options, blob_handle);
>  
>  	tpm2_flush_context(chip, blob_handle);
> +	mutex_unlock(&chip->tpm_mutex);
>  
>  	return rc;
>  }
> -- 
> 2.7.4
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH] tpm: fix a race condition tpm2_unseal_trusted()

[Jarkko Sakkinen]

Unseal and load operations should be done as an atomic operation. This
commit introduces unlocked tpm_transmit() so that tpm2_unseal_trusted()
can do the locking by itself.

v2: Introduced an unlocked unseal operation instead of changing locking
    strategy in order to make less intrusive bug fix and thus more
    backportable.

CC: stable@vger.kernel.org
Fixes: 954650efb79f ("tpm: seal/unseal for TPM 2.0")
Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
---
 drivers/char/tpm/tpm-dev.c       |  2 +-
 drivers/char/tpm/tpm-interface.c | 14 ++++++++------
 drivers/char/tpm/tpm.h           | 18 ++++++++++++++----
 drivers/char/tpm/tpm2-cmd.c      | 13 +++++++++----
 4 files changed, 32 insertions(+), 15 deletions(-)

diff --git a/drivers/char/tpm/tpm-dev.c b/drivers/char/tpm/tpm-dev.c
index f5d4521..8df8846 100644
--- a/drivers/char/tpm/tpm-dev.c
+++ b/drivers/char/tpm/tpm-dev.c
@@ -145,7 +145,7 @@ static ssize_t tpm_write(struct file *file, const char __user *buf,
 		return -EPIPE;
 	}
 	out_size = tpm_transmit(priv->chip, priv->data_buffer,
-				sizeof(priv->data_buffer));
+				sizeof(priv->data_buffer), TPM_TRANSMIT_LOCK);
 
 	tpm_put_ops(priv->chip);
 	if (out_size < 0) {
diff --git a/drivers/char/tpm/tpm-interface.c b/drivers/char/tpm/tpm-interface.c
index 43ef0ef..627daa7 100644
--- a/drivers/char/tpm/tpm-interface.c
+++ b/drivers/char/tpm/tpm-interface.c
@@ -331,7 +331,7 @@ EXPORT_SYMBOL_GPL(tpm_calc_ordinal_duration);
  * Internal kernel interface to transmit TPM commands
  */
 ssize_t tpm_transmit(struct tpm_chip *chip, const char *buf,
-		     size_t bufsiz)
+		     size_t bufsiz, unsigned int flags)
 {
 	ssize_t rc;
 	u32 count, ordinal;
@@ -350,7 +350,8 @@ ssize_t tpm_transmit(struct tpm_chip *chip, const char *buf,
 		return -E2BIG;
 	}
 
-	mutex_lock(&chip->tpm_mutex);
+	if (flags & TPM_TRANSMIT_LOCK)
+		mutex_lock(&chip->tpm_mutex);
 
 	rc = chip->ops->send(chip, (u8 *) buf, count);
 	if (rc < 0) {
@@ -393,20 +394,21 @@ out_recv:
 		dev_err(&chip->dev,
 			"tpm_transmit: tpm_recv: error %zd\n", rc);
 out:
-	mutex_unlock(&chip->tpm_mutex);
+	if (flags & TPM_TRANSMIT_LOCK)
+		mutex_unlock(&chip->tpm_mutex);
 	return rc;
 }
 
 #define TPM_DIGEST_SIZE 20
 #define TPM_RET_CODE_IDX 6
 
-ssize_t tpm_transmit_cmd(struct tpm_chip *chip, void *cmd,
-			 int len, const char *desc)
+ssize_t __tpm_transmit_cmd(struct tpm_chip *chip, void *cmd,
+			   int len, const char *desc, unsigned int flags)
 {
 	struct tpm_output_header *header;
 	int err;
 
-	len = tpm_transmit(chip, (u8 *) cmd, len);
+	len = tpm_transmit(chip, cmd, len, flags);
 	if (len <  0)
 		return len;
 	else if (len < TPM_HEADER_SIZE)
diff --git a/drivers/char/tpm/tpm.h b/drivers/char/tpm/tpm.h
index 6e002c4..b9383fd 100644
--- a/drivers/char/tpm/tpm.h
+++ b/drivers/char/tpm/tpm.h
@@ -476,12 +476,22 @@ extern dev_t tpm_devt;
 extern const struct file_operations tpm_fops;
 extern struct idr dev_nums_idr;
 
+enum tpm_transmit_flags {
+	TPM_TRANSMIT_LOCK,
+};
+
+ssize_t tpm_transmit(struct tpm_chip *chip, const char *buf,
+		     size_t bufsiz, unsigned int flags);
+ssize_t __tpm_transmit_cmd(struct tpm_chip *chip, void *cmd, int len,
+			   const char *desc, unsigned int flags);
+static inline ssize_t tpm_transmit_cmd(struct tpm_chip *chip, void *cmd,
+				       int len, const char *desc)
+{
+	return __tpm_transmit_cmd(chip, cmd, len, desc, TPM_TRANSMIT_LOCK);
+}
+
 ssize_t tpm_getcap(struct tpm_chip *chip, __be32 subcap_id, cap_t *cap,
 		   const char *desc);
-ssize_t tpm_transmit(struct tpm_chip *chip, const char *buf,
-		     size_t bufsiz);
-ssize_t tpm_transmit_cmd(struct tpm_chip *chip, void *cmd, int len,
-			 const char *desc);
 int tpm_get_timeouts(struct tpm_chip *chip);
 int tpm1_auto_startup(struct tpm_chip *chip);
 int tpm_do_selftest(struct tpm_chip *chip);
diff --git a/drivers/char/tpm/tpm2-cmd.c b/drivers/char/tpm/tpm2-cmd.c
index 499f405..99007d8 100644
--- a/drivers/char/tpm/tpm2-cmd.c
+++ b/drivers/char/tpm/tpm2-cmd.c
@@ -576,7 +576,7 @@ static int tpm2_load(struct tpm_chip *chip,
 		goto out;
 	}
 
-	rc = tpm_transmit_cmd(chip, buf.data, PAGE_SIZE, "loading blob");
+	rc = __tpm_transmit_cmd(chip, buf.data, PAGE_SIZE, "loading blob", 0);
 	if (!rc)
 		*blob_handle = be32_to_cpup(
 			(__be32 *) &buf.data[TPM_HEADER_SIZE]);
@@ -604,7 +604,8 @@ static void tpm2_flush_context(struct tpm_chip *chip, u32 handle)
 
 	tpm_buf_append_u32(&buf, handle);
 
-	rc = tpm_transmit_cmd(chip, buf.data, PAGE_SIZE, "flushing context");
+	rc = __tpm_transmit_cmd(chip, buf.data, PAGE_SIZE, "flushing context",
+				0);
 	if (rc)
 		dev_warn(&chip->dev, "0x%08x was not flushed, rc=%d\n", handle,
 			 rc);
@@ -635,7 +636,7 @@ static int tpm2_unseal(struct tpm_chip *chip,
 			     options->blobauth /* hmac */,
 			     TPM_DIGEST_SIZE);
 
-	rc = tpm_transmit_cmd(chip, buf.data, PAGE_SIZE, "unsealing");
+	rc = __tpm_transmit_cmd(chip, buf.data, PAGE_SIZE, "unsealing", 0);
 	if (rc > 0)
 		rc = -EPERM;
 
@@ -668,13 +669,17 @@ int tpm2_unseal_trusted(struct tpm_chip *chip,
 	u32 blob_handle;
 	int rc;
 
+	mutex_lock(&chip->tpm_mutex);
 	rc = tpm2_load(chip, payload, options, &blob_handle);
-	if (rc)
+	if (rc) {
+		mutex_unlock(&chip->tpm_mutex);
 		return rc;
+	}
 
 	rc = tpm2_unseal(chip, payload, options, blob_handle);
 
 	tpm2_flush_context(chip, blob_handle);
+	mutex_unlock(&chip->tpm_mutex);
 
 	return rc;
 }
-- 
2.7.4

[PATCH] tpm: fix a race condition tpm2_unseal_trusted()

[Jarkko Sakkinen]

Unseal and load operations should be done as an atomic operation. This
commit introduces unlocked tpm_transmit() so that tpm2_unseal_trusted()
can do the locking by itself.

v2: Introduced an unlocked unseal operation instead of changing locking
    strategy in order to make less intrusive bug fix and thus more
    backportable.

CC: stable-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
Fixes: 954650efb79f ("tpm: seal/unseal for TPM 2.0")
Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>
---
 drivers/char/tpm/tpm-dev.c       |  2 +-
 drivers/char/tpm/tpm-interface.c | 14 ++++++++------
 drivers/char/tpm/tpm.h           | 18 ++++++++++++++----
 drivers/char/tpm/tpm2-cmd.c      | 13 +++++++++----
 4 files changed, 32 insertions(+), 15 deletions(-)

diff --git a/drivers/char/tpm/tpm-dev.c b/drivers/char/tpm/tpm-dev.c
index f5d4521..8df8846 100644
--- a/drivers/char/tpm/tpm-dev.c
+++ b/drivers/char/tpm/tpm-dev.c
@@ -145,7 +145,7 @@ static ssize_t tpm_write(struct file *file, const char __user *buf,
 		return -EPIPE;
 	}
 	out_size = tpm_transmit(priv->chip, priv->data_buffer,
-				sizeof(priv->data_buffer));
+				sizeof(priv->data_buffer), TPM_TRANSMIT_LOCK);
 
 	tpm_put_ops(priv->chip);
 	if (out_size < 0) {
diff --git a/drivers/char/tpm/tpm-interface.c b/drivers/char/tpm/tpm-interface.c
index 43ef0ef..627daa7 100644
--- a/drivers/char/tpm/tpm-interface.c
+++ b/drivers/char/tpm/tpm-interface.c
@@ -331,7 +331,7 @@ EXPORT_SYMBOL_GPL(tpm_calc_ordinal_duration);
  * Internal kernel interface to transmit TPM commands
  */
 ssize_t tpm_transmit(struct tpm_chip *chip, const char *buf,
-		     size_t bufsiz)
+		     size_t bufsiz, unsigned int flags)
 {
 	ssize_t rc;
 	u32 count, ordinal;
@@ -350,7 +350,8 @@ ssize_t tpm_transmit(struct tpm_chip *chip, const char *buf,
 		return -E2BIG;
 	}
 
-	mutex_lock(&chip->tpm_mutex);
+	if (flags & TPM_TRANSMIT_LOCK)
+		mutex_lock(&chip->tpm_mutex);
 
 	rc = chip->ops->send(chip, (u8 *) buf, count);
 	if (rc < 0) {
@@ -393,20 +394,21 @@ out_recv:
 		dev_err(&chip->dev,
 			"tpm_transmit: tpm_recv: error %zd\n", rc);
 out:
-	mutex_unlock(&chip->tpm_mutex);
+	if (flags & TPM_TRANSMIT_LOCK)
+		mutex_unlock(&chip->tpm_mutex);
 	return rc;
 }
 
 #define TPM_DIGEST_SIZE 20
 #define TPM_RET_CODE_IDX 6
 
-ssize_t tpm_transmit_cmd(struct tpm_chip *chip, void *cmd,
-			 int len, const char *desc)
+ssize_t __tpm_transmit_cmd(struct tpm_chip *chip, void *cmd,
+			   int len, const char *desc, unsigned int flags)
 {
 	struct tpm_output_header *header;
 	int err;
 
-	len = tpm_transmit(chip, (u8 *) cmd, len);
+	len = tpm_transmit(chip, cmd, len, flags);
 	if (len <  0)
 		return len;
 	else if (len < TPM_HEADER_SIZE)
diff --git a/drivers/char/tpm/tpm.h b/drivers/char/tpm/tpm.h
index 6e002c4..b9383fd 100644
--- a/drivers/char/tpm/tpm.h
+++ b/drivers/char/tpm/tpm.h
@@ -476,12 +476,22 @@ extern dev_t tpm_devt;
 extern const struct file_operations tpm_fops;
 extern struct idr dev_nums_idr;
 
+enum tpm_transmit_flags {
+	TPM_TRANSMIT_LOCK,
+};
+
+ssize_t tpm_transmit(struct tpm_chip *chip, const char *buf,
+		     size_t bufsiz, unsigned int flags);
+ssize_t __tpm_transmit_cmd(struct tpm_chip *chip, void *cmd, int len,
+			   const char *desc, unsigned int flags);
+static inline ssize_t tpm_transmit_cmd(struct tpm_chip *chip, void *cmd,
+				       int len, const char *desc)
+{
+	return __tpm_transmit_cmd(chip, cmd, len, desc, TPM_TRANSMIT_LOCK);
+}
+
 ssize_t tpm_getcap(struct tpm_chip *chip, __be32 subcap_id, cap_t *cap,
 		   const char *desc);
-ssize_t tpm_transmit(struct tpm_chip *chip, const char *buf,
-		     size_t bufsiz);
-ssize_t tpm_transmit_cmd(struct tpm_chip *chip, void *cmd, int len,
-			 const char *desc);
 int tpm_get_timeouts(struct tpm_chip *chip);
 int tpm1_auto_startup(struct tpm_chip *chip);
 int tpm_do_selftest(struct tpm_chip *chip);
diff --git a/drivers/char/tpm/tpm2-cmd.c b/drivers/char/tpm/tpm2-cmd.c
index 499f405..99007d8 100644
--- a/drivers/char/tpm/tpm2-cmd.c
+++ b/drivers/char/tpm/tpm2-cmd.c
@@ -576,7 +576,7 @@ static int tpm2_load(struct tpm_chip *chip,
 		goto out;
 	}
 
-	rc = tpm_transmit_cmd(chip, buf.data, PAGE_SIZE, "loading blob");
+	rc = __tpm_transmit_cmd(chip, buf.data, PAGE_SIZE, "loading blob", 0);
 	if (!rc)
 		*blob_handle = be32_to_cpup(
 			(__be32 *) &buf.data[TPM_HEADER_SIZE]);
@@ -604,7 +604,8 @@ static void tpm2_flush_context(struct tpm_chip *chip, u32 handle)
 
 	tpm_buf_append_u32(&buf, handle);
 
-	rc = tpm_transmit_cmd(chip, buf.data, PAGE_SIZE, "flushing context");
+	rc = __tpm_transmit_cmd(chip, buf.data, PAGE_SIZE, "flushing context",
+				0);
 	if (rc)
 		dev_warn(&chip->dev, "0x%08x was not flushed, rc=%d\n", handle,
 			 rc);
@@ -635,7 +636,7 @@ static int tpm2_unseal(struct tpm_chip *chip,
 			     options->blobauth /* hmac */,
 			     TPM_DIGEST_SIZE);
 
-	rc = tpm_transmit_cmd(chip, buf.data, PAGE_SIZE, "unsealing");
+	rc = __tpm_transmit_cmd(chip, buf.data, PAGE_SIZE, "unsealing", 0);
 	if (rc > 0)
 		rc = -EPERM;
 
@@ -668,13 +669,17 @@ int tpm2_unseal_trusted(struct tpm_chip *chip,
 	u32 blob_handle;
 	int rc;
 
+	mutex_lock(&chip->tpm_mutex);
 	rc = tpm2_load(chip, payload, options, &blob_handle);
-	if (rc)
+	if (rc) {
+		mutex_unlock(&chip->tpm_mutex);
 		return rc;
+	}
 
 	rc = tpm2_unseal(chip, payload, options, blob_handle);
 
 	tpm2_flush_context(chip, blob_handle);
+	mutex_unlock(&chip->tpm_mutex);
 
 	return rc;
 }
-- 
2.7.4


------------------------------------------------------------------------------

Re: [PATCH] tpm: fix a race condition tpm2_unseal_trusted()

[Jason Gunthorpe]

On Tue, Aug 09, 2016 at 01:36:29PM +0300, Jarkko Sakkinen wrote:

> Functionally my patch should not break anything. I understand the need
> for clean up of locking but why doing this now to make the driver
> non-racy would make clean up later on any harder?

Then rename the functions so they don't confusingly look like the
standard kernel pattern that doesn't lock.

Jason

Re: [PATCH] tpm: fix a race condition tpm2_unseal_trusted()

[Jason Gunthorpe]

On Tue, Aug 09, 2016 at 01:36:29PM +0300, Jarkko Sakkinen wrote:

> Functionally my patch should not break anything. I understand the need
> for clean up of locking but why doing this now to make the driver
> non-racy would make clean up later on any harder?

Then rename the functions so they don't confusingly look like the
standard kernel pattern that doesn't lock.

Jason

------------------------------------------------------------------------------
What NetFlow Analyzer can do for you? Monitors network bandwidth and traffic
patterns at an interface-level. Reveals which users, apps, and protocols are 
consuming the most bandwidth. Provides multi-vendor support for NetFlow, 
J-Flow, sFlow and other flows. Make informed decisions using capacity 
planning reports. http://sdm.link/zohodev2dev

Re: [PATCH] tpm: fix a race condition tpm2_unseal_trusted()

[Jarkko Sakkinen]

On Thu, Jul 21, 2016 at 10:25:36AM -0600, Jason Gunthorpe wrote:
> On Thu, Jul 21, 2016 at 12:02:45PM +0300, Jarkko Sakkinen wrote:
> > On Wed, Jul 20, 2016 at 03:13:32PM -0600, Jason Gunthorpe wrote:
> > > On Wed, Jul 20, 2016 at 11:53:14PM +0300, Jarkko Sakkinen wrote:
> > > 
> > > > The only use cases I see at the moment for it work this way:
> > > > 
> > > > 1. Call tpm_try_get_ops.
> > > > 2. Send a TPM command.
> > > > 3. Call tpm_put_ops.
> > > 
> > > Right, but that is just a reflection of what the in kernel users are
> > > doing today, not necessarily what they should be doing.
> > > 
> > > We should not break the put/get semantics..
> > > 
> > > > I did not find any other form of use. The only use is to make sure that
> > > > there are no transactions running before the ops are cleared. Or did I
> > > > overlook something perhaps?
> > > 
> > > The put/get is intended to allow a kapi user to hold a ref to tpm
> > > without it geting destroyed. It is not intended to be an exclusive lock.
> > 
> > These operations *are not* exposed to kapi. They are interal to the
> > driver. That's why it does not make sense speak about kapi user.
> 
> Right now yes, but look at other subsystems and you will see
> operations like that, because that is typical design pattern. When I
> wrote them I made sure they could be used in that typical way.
> 
> We have issues in our kapi users with regards to hot plug and multiple
> tpms. Fortunately that basically never happens, but it does indicate
> the API is not sufficient..

Functionally my patch should not break anything. I understand the need
for clean up of locking but why doing this now to make the driver
non-racy would make clean up later on any harder?

I would rather think of clean up after the code is non-racy than doing a
huge clean up for racy code. Correct functionality is more important
than clean code because it has direct effect to users.

/Jarkko

Re: [PATCH] tpm: fix a race condition tpm2_unseal_trusted()

[Jarkko Sakkinen]

On Thu, Jul 21, 2016 at 10:25:36AM -0600, Jason Gunthorpe wrote:
> On Thu, Jul 21, 2016 at 12:02:45PM +0300, Jarkko Sakkinen wrote:
> > On Wed, Jul 20, 2016 at 03:13:32PM -0600, Jason Gunthorpe wrote:
> > > On Wed, Jul 20, 2016 at 11:53:14PM +0300, Jarkko Sakkinen wrote:
> > > 
> > > > The only use cases I see at the moment for it work this way:
> > > > 
> > > > 1. Call tpm_try_get_ops.
> > > > 2. Send a TPM command.
> > > > 3. Call tpm_put_ops.
> > > 
> > > Right, but that is just a reflection of what the in kernel users are
> > > doing today, not necessarily what they should be doing.
> > > 
> > > We should not break the put/get semantics..
> > > 
> > > > I did not find any other form of use. The only use is to make sure that
> > > > there are no transactions running before the ops are cleared. Or did I
> > > > overlook something perhaps?
> > > 
> > > The put/get is intended to allow a kapi user to hold a ref to tpm
> > > without it geting destroyed. It is not intended to be an exclusive lock.
> > 
> > These operations *are not* exposed to kapi. They are interal to the
> > driver. That's why it does not make sense speak about kapi user.
> 
> Right now yes, but look at other subsystems and you will see
> operations like that, because that is typical design pattern. When I
> wrote them I made sure they could be used in that typical way.
> 
> We have issues in our kapi users with regards to hot plug and multiple
> tpms. Fortunately that basically never happens, but it does indicate
> the API is not sufficient..

Functionally my patch should not break anything. I understand the need
for clean up of locking but why doing this now to make the driver
non-racy would make clean up later on any harder?

I would rather think of clean up after the code is non-racy than doing a
huge clean up for racy code. Correct functionality is more important
than clean code because it has direct effect to users.

/Jarkko

Re: [PATCH] tpm: fix a race condition tpm2_unseal_trusted()

[Jason Gunthorpe]

On Thu, Jul 21, 2016 at 12:02:45PM +0300, Jarkko Sakkinen wrote:
> On Wed, Jul 20, 2016 at 03:13:32PM -0600, Jason Gunthorpe wrote:
> > On Wed, Jul 20, 2016 at 11:53:14PM +0300, Jarkko Sakkinen wrote:
> > 
> > > The only use cases I see at the moment for it work this way:
> > > 
> > > 1. Call tpm_try_get_ops.
> > > 2. Send a TPM command.
> > > 3. Call tpm_put_ops.
> > 
> > Right, but that is just a reflection of what the in kernel users are
> > doing today, not necessarily what they should be doing.
> > 
> > We should not break the put/get semantics..
> > 
> > > I did not find any other form of use. The only use is to make sure that
> > > there are no transactions running before the ops are cleared. Or did I
> > > overlook something perhaps?
> > 
> > The put/get is intended to allow a kapi user to hold a ref to tpm
> > without it geting destroyed. It is not intended to be an exclusive lock.
> 
> These operations *are not* exposed to kapi. They are interal to the
> driver. That's why it does not make sense speak about kapi user.

Right now yes, but look at other subsystems and you will see
operations like that, because that is typical design pattern. When I
wrote them I made sure they could be used in that typical way.

We have issues in our kapi users with regards to hot plug and multiple
tpms. Fortunately that basically never happens, but it does indicate
the API is not sufficient..

> You are speaking how it could be used. I'm looking at how it's actually
> used. Shouldn't implementation reflect that rather than future
> prospects?

Well, there are common patterns in the kernel for how these things
work and the get/put stuff does not imply an exclusive lock. That is
why it is called get/put :)

> > Those sorts of compound ops should hold the tpm_mutex manually, not
> > through the get_ops scheme.
> 
> The next best option would be to have unlocked transmit_cmd function.

The locking needs a good scrub, it is one of the last legacy things
left now that doesn't make alot of sense, especially if we need to
lock these compound operations.

The get/put locking cleanup was just a small step toward something
more typical..

Jason

Re: [PATCH] tpm: fix a race condition tpm2_unseal_trusted()

[Jason Gunthorpe]

On Thu, Jul 21, 2016 at 12:02:45PM +0300, Jarkko Sakkinen wrote:
> On Wed, Jul 20, 2016 at 03:13:32PM -0600, Jason Gunthorpe wrote:
> > On Wed, Jul 20, 2016 at 11:53:14PM +0300, Jarkko Sakkinen wrote:
> > 
> > > The only use cases I see at the moment for it work this way:
> > > 
> > > 1. Call tpm_try_get_ops.
> > > 2. Send a TPM command.
> > > 3. Call tpm_put_ops.
> > 
> > Right, but that is just a reflection of what the in kernel users are
> > doing today, not necessarily what they should be doing.
> > 
> > We should not break the put/get semantics..
> > 
> > > I did not find any other form of use. The only use is to make sure that
> > > there are no transactions running before the ops are cleared. Or did I
> > > overlook something perhaps?
> > 
> > The put/get is intended to allow a kapi user to hold a ref to tpm
> > without it geting destroyed. It is not intended to be an exclusive lock.
> 
> These operations *are not* exposed to kapi. They are interal to the
> driver. That's why it does not make sense speak about kapi user.

Right now yes, but look at other subsystems and you will see
operations like that, because that is typical design pattern. When I
wrote them I made sure they could be used in that typical way.

We have issues in our kapi users with regards to hot plug and multiple
tpms. Fortunately that basically never happens, but it does indicate
the API is not sufficient..

> You are speaking how it could be used. I'm looking at how it's actually
> used. Shouldn't implementation reflect that rather than future
> prospects?

Well, there are common patterns in the kernel for how these things
work and the get/put stuff does not imply an exclusive lock. That is
why it is called get/put :)

> > Those sorts of compound ops should hold the tpm_mutex manually, not
> > through the get_ops scheme.
> 
> The next best option would be to have unlocked transmit_cmd function.

The locking needs a good scrub, it is one of the last legacy things
left now that doesn't make alot of sense, especially if we need to
lock these compound operations.

The get/put locking cleanup was just a small step toward something
more typical..

Jason

------------------------------------------------------------------------------
What NetFlow Analyzer can do for you? Monitors network bandwidth and traffic
patterns at an interface-level. Reveals which users, apps, and protocols are 
consuming the most bandwidth. Provides multi-vendor support for NetFlow, 
J-Flow, sFlow and other flows. Make informed decisions using capacity planning
reports.http://sdm.link/zohodev2dev

Re: [PATCH] tpm: fix a race condition tpm2_unseal_trusted()

[Jarkko Sakkinen]

On Wed, Jul 20, 2016 at 03:13:32PM -0600, Jason Gunthorpe wrote:
> On Wed, Jul 20, 2016 at 11:53:14PM +0300, Jarkko Sakkinen wrote:
> 
> > The only use cases I see at the moment for it work this way:
> > 
> > 1. Call tpm_try_get_ops.
> > 2. Send a TPM command.
> > 3. Call tpm_put_ops.
> 
> Right, but that is just a reflection of what the in kernel users are
> doing today, not necessarily what they should be doing.
> 
> We should not break the put/get semantics..
> 
> > I did not find any other form of use. The only use is to make sure that
> > there are no transactions running before the ops are cleared. Or did I
> > overlook something perhaps?
> 
> The put/get is intended to allow a kapi user to hold a ref to tpm
> without it geting destroyed. It is not intended to be an exclusive lock.

These operations *are not* exposed to kapi. They are interal to the
driver. That's why it does not make sense speak about kapi user.

All the places where it's used it's always used in the way that you also
take the exclusive lock.

You are speaking how it could be used. I'm looking at how it's actually
used. Shouldn't implementation reflect that rather than future
prospects?

> > Trusted key unseal operation with TPM2 is broken into two operations:
> > 
> > 1. Load the given key blob.
> > 2. Unseal the data.
> > 
> > Without locking and unlocking mutex only once there is a race condition.
> 
> Well, the race condition is fundamentally because we don't have key
> virtualization in the kernel :|

I do agree and I'll share my thought about TPM2 context swapping in
LSS 2016.

> Those sorts of compound ops should hold the tpm_mutex manually, not
> through the get_ops scheme.

The next best option would be to have unlocked transmit_cmd function.

/Jarkko

Re: [PATCH] tpm: fix a race condition tpm2_unseal_trusted()

[Jarkko Sakkinen]

On Wed, Jul 20, 2016 at 03:13:32PM -0600, Jason Gunthorpe wrote:
> On Wed, Jul 20, 2016 at 11:53:14PM +0300, Jarkko Sakkinen wrote:
> 
> > The only use cases I see at the moment for it work this way:
> > 
> > 1. Call tpm_try_get_ops.
> > 2. Send a TPM command.
> > 3. Call tpm_put_ops.
> 
> Right, but that is just a reflection of what the in kernel users are
> doing today, not necessarily what they should be doing.
> 
> We should not break the put/get semantics..
> 
> > I did not find any other form of use. The only use is to make sure that
> > there are no transactions running before the ops are cleared. Or did I
> > overlook something perhaps?
> 
> The put/get is intended to allow a kapi user to hold a ref to tpm
> without it geting destroyed. It is not intended to be an exclusive lock.

These operations *are not* exposed to kapi. They are interal to the
driver. That's why it does not make sense speak about kapi user.

All the places where it's used it's always used in the way that you also
take the exclusive lock.

You are speaking how it could be used. I'm looking at how it's actually
used. Shouldn't implementation reflect that rather than future
prospects?

> > Trusted key unseal operation with TPM2 is broken into two operations:
> > 
> > 1. Load the given key blob.
> > 2. Unseal the data.
> > 
> > Without locking and unlocking mutex only once there is a race condition.
> 
> Well, the race condition is fundamentally because we don't have key
> virtualization in the kernel :|

I do agree and I'll share my thought about TPM2 context swapping in
LSS 2016.

> Those sorts of compound ops should hold the tpm_mutex manually, not
> through the get_ops scheme.

The next best option would be to have unlocked transmit_cmd function.

/Jarkko

------------------------------------------------------------------------------
What NetFlow Analyzer can do for you? Monitors network bandwidth and traffic
patterns at an interface-level. Reveals which users, apps, and protocols are 
consuming the most bandwidth. Provides multi-vendor support for NetFlow, 
J-Flow, sFlow and other flows. Make informed decisions using capacity planning
reports.http://sdm.link/zohodev2dev

Re: [PATCH] tpm: fix a race condition tpm2_unseal_trusted()

[Jason Gunthorpe]

On Wed, Jul 20, 2016 at 11:53:14PM +0300, Jarkko Sakkinen wrote:

> The only use cases I see at the moment for it work this way:
> 
> 1. Call tpm_try_get_ops.
> 2. Send a TPM command.
> 3. Call tpm_put_ops.

Right, but that is just a reflection of what the in kernel users are
doing today, not necessarily what they should be doing.

We should not break the put/get semantics..

> I did not find any other form of use. The only use is to make sure that
> there are no transactions running before the ops are cleared. Or did I
> overlook something perhaps?

The put/get is intended to allow a kapi user to hold a ref to tpm
without it geting destroyed. It is not intended to be an exclusive lock.

> Trusted key unseal operation with TPM2 is broken into two operations:
> 
> 1. Load the given key blob.
> 2. Unseal the data.
> 
> Without locking and unlocking mutex only once there is a race condition.

Well, the race condition is fundamentally because we don't have key
virtualization in the kernel :|

Those sorts of compound ops should hold the tpm_mutex manually, not
through the get_ops scheme.

Jason

Re: [PATCH] tpm: fix a race condition tpm2_unseal_trusted()

[Jason Gunthorpe]

On Wed, Jul 20, 2016 at 11:53:14PM +0300, Jarkko Sakkinen wrote:

> The only use cases I see at the moment for it work this way:
> 
> 1. Call tpm_try_get_ops.
> 2. Send a TPM command.
> 3. Call tpm_put_ops.

Right, but that is just a reflection of what the in kernel users are
doing today, not necessarily what they should be doing.

We should not break the put/get semantics..

> I did not find any other form of use. The only use is to make sure that
> there are no transactions running before the ops are cleared. Or did I
> overlook something perhaps?

The put/get is intended to allow a kapi user to hold a ref to tpm
without it geting destroyed. It is not intended to be an exclusive lock.

> Trusted key unseal operation with TPM2 is broken into two operations:
> 
> 1. Load the given key blob.
> 2. Unseal the data.
> 
> Without locking and unlocking mutex only once there is a race condition.

Well, the race condition is fundamentally because we don't have key
virtualization in the kernel :|

Those sorts of compound ops should hold the tpm_mutex manually, not
through the get_ops scheme.

Jason

------------------------------------------------------------------------------
What NetFlow Analyzer can do for you? Monitors network bandwidth and traffic
patterns at an interface-level. Reveals which users, apps, and protocols are 
consuming the most bandwidth. Provides multi-vendor support for NetFlow, 
J-Flow, sFlow and other flows. Make informed decisions using capacity planning
reports.http://sdm.link/zohodev2dev

Re: [PATCH] tpm: fix a race condition tpm2_unseal_trusted()

[Jarkko Sakkinen]

On Wed, Jul 20, 2016 at 10:48:18AM -0600, Jason Gunthorpe wrote:
> On Wed, Jul 20, 2016 at 03:16:32AM +0300, Jarkko Sakkinen wrote:
> > Unseal and load operations should be done as an atomic unit. This
> > commit fixes the issue by moving TPM mutex handling to tpm_try_get_ops()
> > and tpm_put_ops(), which is probably more logical place for it anyway.
> 
> No..
> 
> 'get_ops' is to be used to hold a persisent kref to a single tpm. It
> cannot block other tpm access.
> 
> Eg a upper protocol might get_ops to for a long period to ensure it
> consistently talks to the same TPM in a multi-tpm system.
> 
> We need something else to solve whatever you are concerned with
> here..

The only use cases I see at the moment for it work this way:

1. Call tpm_try_get_ops.
2. Send a TPM command.
3. Call tpm_put_ops.

I did not find any other form of use. The only use is to make sure that
there are no transactions running before the ops are cleared. Or did I
overlook something perhaps?

Trusted key unseal operation with TPM2 is broken into two operations:

1. Load the given key blob.
2. Unseal the data.

Without locking and unlocking mutex only once there is a race condition.

/Jarkko

Re: [PATCH] tpm: fix a race condition tpm2_unseal_trusted()

[Jarkko Sakkinen]

On Wed, Jul 20, 2016 at 10:48:18AM -0600, Jason Gunthorpe wrote:
> On Wed, Jul 20, 2016 at 03:16:32AM +0300, Jarkko Sakkinen wrote:
> > Unseal and load operations should be done as an atomic unit. This
> > commit fixes the issue by moving TPM mutex handling to tpm_try_get_ops()
> > and tpm_put_ops(), which is probably more logical place for it anyway.
> 
> No..
> 
> 'get_ops' is to be used to hold a persisent kref to a single tpm. It
> cannot block other tpm access.
> 
> Eg a upper protocol might get_ops to for a long period to ensure it
> consistently talks to the same TPM in a multi-tpm system.
> 
> We need something else to solve whatever you are concerned with
> here..

The only use cases I see at the moment for it work this way:

1. Call tpm_try_get_ops.
2. Send a TPM command.
3. Call tpm_put_ops.

I did not find any other form of use. The only use is to make sure that
there are no transactions running before the ops are cleared. Or did I
overlook something perhaps?

Trusted key unseal operation with TPM2 is broken into two operations:

1. Load the given key blob.
2. Unseal the data.

Without locking and unlocking mutex only once there is a race condition.

/Jarkko

------------------------------------------------------------------------------
What NetFlow Analyzer can do for you? Monitors network bandwidth and traffic
patterns at an interface-level. Reveals which users, apps, and protocols are 
consuming the most bandwidth. Provides multi-vendor support for NetFlow, 
J-Flow, sFlow and other flows. Make informed decisions using capacity planning
reports.http://sdm.link/zohodev2dev

Re: [PATCH] tpm: fix a race condition tpm2_unseal_trusted()

[Jason Gunthorpe]

On Wed, Jul 20, 2016 at 03:16:32AM +0300, Jarkko Sakkinen wrote:
> Unseal and load operations should be done as an atomic unit. This
> commit fixes the issue by moving TPM mutex handling to tpm_try_get_ops()
> and tpm_put_ops(), which is probably more logical place for it anyway.

No..

'get_ops' is to be used to hold a persisent kref to a single tpm. It
cannot block other tpm access.

Eg a upper protocol might get_ops to for a long period to ensure it
consistently talks to the same TPM in a multi-tpm system.

We need something else to solve whatever you are concerned with
here..

Jason

Re: [PATCH] tpm: fix a race condition tpm2_unseal_trusted()

[Jason Gunthorpe]

On Wed, Jul 20, 2016 at 03:16:32AM +0300, Jarkko Sakkinen wrote:
> Unseal and load operations should be done as an atomic unit. This
> commit fixes the issue by moving TPM mutex handling to tpm_try_get_ops()
> and tpm_put_ops(), which is probably more logical place for it anyway.

No..

'get_ops' is to be used to hold a persisent kref to a single tpm. It
cannot block other tpm access.

Eg a upper protocol might get_ops to for a long period to ensure it
consistently talks to the same TPM in a multi-tpm system.

We need something else to solve whatever you are concerned with
here..

Jason

------------------------------------------------------------------------------
What NetFlow Analyzer can do for you? Monitors network bandwidth and traffic
patterns at an interface-level. Reveals which users, apps, and protocols are 
consuming the most bandwidth. Provides multi-vendor support for NetFlow, 
J-Flow, sFlow and other flows. Make informed decisions using capacity planning
reports.http://sdm.link/zohodev2dev

[PATCH] tpm: fix a race condition tpm2_unseal_trusted()

[Jarkko Sakkinen]

Unseal and load operations should be done as an atomic unit. This
commit fixes the issue by moving TPM mutex handling to tpm_try_get_ops()
and tpm_put_ops(), which is probably more logical place for it anyway.

Fixes: 954650efb79f ("tpm: seal/unseal for TPM 2.0")
Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
---
 drivers/char/tpm/tpm-chip.c      | 2 ++
 drivers/char/tpm/tpm-interface.c | 3 ---
 2 files changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/char/tpm/tpm-chip.c b/drivers/char/tpm/tpm-chip.c
index e595013..9749f59 100644
--- a/drivers/char/tpm/tpm-chip.c
+++ b/drivers/char/tpm/tpm-chip.c
@@ -56,6 +56,7 @@ int tpm_try_get_ops(struct tpm_chip *chip)
 	if (!chip->ops)
 		goto out_lock;
 
+	mutex_lock(&chip->tpm_mutex);
 	return 0;
 out_lock:
 	up_read(&chip->ops_sem);
@@ -73,6 +74,7 @@ EXPORT_SYMBOL_GPL(tpm_try_get_ops);
  */
 void tpm_put_ops(struct tpm_chip *chip)
 {
+	mutex_unlock(&chip->tpm_mutex);
 	up_read(&chip->ops_sem);
 	put_device(&chip->dev);
 }
diff --git a/drivers/char/tpm/tpm-interface.c b/drivers/char/tpm/tpm-interface.c
index 1abe2d7..a2a9c36 100644
--- a/drivers/char/tpm/tpm-interface.c
+++ b/drivers/char/tpm/tpm-interface.c
@@ -350,8 +350,6 @@ ssize_t tpm_transmit(struct tpm_chip *chip, const char *buf,
 		return -E2BIG;
 	}
 
-	mutex_lock(&chip->tpm_mutex);
-
 	rc = chip->ops->send(chip, (u8 *) buf, count);
 	if (rc < 0) {
 		dev_err(&chip->dev,
@@ -393,7 +391,6 @@ out_recv:
 		dev_err(&chip->dev,
 			"tpm_transmit: tpm_recv: error %zd\n", rc);
 out:
-	mutex_unlock(&chip->tpm_mutex);
 	return rc;
 }
 
-- 
2.7.4

[PATCH] tpm: fix a race condition tpm2_unseal_trusted()

[Jarkko Sakkinen]

Unseal and load operations should be done as an atomic unit. This
commit fixes the issue by moving TPM mutex handling to tpm_try_get_ops()
and tpm_put_ops(), which is probably more logical place for it anyway.

Fixes: 954650efb79f ("tpm: seal/unseal for TPM 2.0")
Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
---
 drivers/char/tpm/tpm-chip.c      | 2 ++
 drivers/char/tpm/tpm-interface.c | 3 ---
 2 files changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/char/tpm/tpm-chip.c b/drivers/char/tpm/tpm-chip.c
index e595013..9749f59 100644
--- a/drivers/char/tpm/tpm-chip.c
+++ b/drivers/char/tpm/tpm-chip.c
@@ -56,6 +56,7 @@ int tpm_try_get_ops(struct tpm_chip *chip)
 	if (!chip->ops)
 		goto out_lock;
 
+	mutex_lock(&chip->tpm_mutex);
 	return 0;
 out_lock:
 	up_read(&chip->ops_sem);
@@ -73,6 +74,7 @@ EXPORT_SYMBOL_GPL(tpm_try_get_ops);
  */
 void tpm_put_ops(struct tpm_chip *chip)
 {
+	mutex_unlock(&chip->tpm_mutex);
 	up_read(&chip->ops_sem);
 	put_device(&chip->dev);
 }
diff --git a/drivers/char/tpm/tpm-interface.c b/drivers/char/tpm/tpm-interface.c
index 1abe2d7..a2a9c36 100644
--- a/drivers/char/tpm/tpm-interface.c
+++ b/drivers/char/tpm/tpm-interface.c
@@ -350,8 +350,6 @@ ssize_t tpm_transmit(struct tpm_chip *chip, const char *buf,
 		return -E2BIG;
 	}
 
-	mutex_lock(&chip->tpm_mutex);
-
 	rc = chip->ops->send(chip, (u8 *) buf, count);
 	if (rc < 0) {
 		dev_err(&chip->dev,
@@ -393,7 +391,6 @@ out_recv:
 		dev_err(&chip->dev,
 			"tpm_transmit: tpm_recv: error %zd\n", rc);
 out:
-	mutex_unlock(&chip->tpm_mutex);
 	return rc;
 }
 
-- 
2.7.4