Move the allocation of chip->auth to tpm2_start_auth_session() so that this
field can be used as flag to tell whether auth session is active or not.

Instead of flushing and reloading the auth session for every transaction
separately, keep the session open unless /dev/tpm0 is used.

Reported-by: Pengyu Ma <mapengyu@gmail.com>
Closes: https://bugzilla.kernel.org/show_bug.cgi?id=219229
Cc: stable@vger.kernel.org # v6.10+
Fixes: 7ca110f2679b ("tpm: Address !chip->auth in tpm_buf_append_hmac_session*()")
Tested-by: Pengyu Ma <mapengyu@gmail.com>
Signed-off-by: Jarkko Sakkinen <jarkko@kernel.org>
---
v8:
- Since auth session and null key are flushed at a same time, only
  either needs to be checked. Addresses and a remark from James
  Bottomley few revisions ago.
- kfree_sensitive()
- Effectively squash top three patches given the simplifications.
v7:
- No changes.
v6:
- No changes.
v5:
- No changes.
v4:
- Changed as bug.
v3:
- Refined the commit message.
- Removed the conditional for applying TPM2_SA_CONTINUE_SESSION only when
  /dev/tpm0 is open. It is not required as the auth session is flushed,
  not saved.
v2:
- A new patch.
---
 drivers/char/tpm/tpm-chip.c       | 10 +++++++
 drivers/char/tpm/tpm-dev-common.c |  3 +++
 drivers/char/tpm/tpm-interface.c  |  6 +++--
 drivers/char/tpm/tpm2-sessions.c  | 45 ++++++++++++++++++-------------
 4 files changed, 44 insertions(+), 20 deletions(-)

diff --git a/drivers/char/tpm/tpm-chip.c b/drivers/char/tpm/tpm-chip.c
index 854546000c92..1ff99a7091bb 100644
--- a/drivers/char/tpm/tpm-chip.c
+++ b/drivers/char/tpm/tpm-chip.c
@@ -674,6 +674,16 @@ EXPORT_SYMBOL_GPL(tpm_chip_register);
  */
 void tpm_chip_unregister(struct tpm_chip *chip)
 {
+#ifdef CONFIG_TCG_TPM2_HMAC
+	int rc;
+
+	rc = tpm_try_get_ops(chip);
+	if (!rc) {
+		tpm2_end_auth_session(chip);
+		tpm_put_ops(chip);
+	}
+#endif
+
 	tpm_del_legacy_sysfs(chip);
 	if (tpm_is_hwrng_enabled(chip))
 		hwrng_unregister(&chip->hwrng);
diff --git a/drivers/char/tpm/tpm-dev-common.c b/drivers/char/tpm/tpm-dev-common.c
index 30b4c288c1bb..c7a88fa7b0fc 100644
--- a/drivers/char/tpm/tpm-dev-common.c
+++ b/drivers/char/tpm/tpm-dev-common.c
@@ -27,6 +27,9 @@ static ssize_t tpm_dev_transmit(struct tpm_chip *chip, struct tpm_space *space,
 	struct tpm_header *header = (void *)buf;
 	ssize_t ret, len;
 
+	if (chip->flags & TPM_CHIP_FLAG_TPM2)
+		tpm2_end_auth_session(chip);
+
 	ret = tpm2_prepare_space(chip, space, buf, bufsiz);
 	/* If the command is not implemented by the TPM, synthesize a
 	 * response with a TPM2_RC_COMMAND_CODE return for user-space.
diff --git a/drivers/char/tpm/tpm-interface.c b/drivers/char/tpm/tpm-interface.c
index 5da134f12c9a..8134f002b121 100644
--- a/drivers/char/tpm/tpm-interface.c
+++ b/drivers/char/tpm/tpm-interface.c
@@ -379,10 +379,12 @@ int tpm_pm_suspend(struct device *dev)
 
 	rc = tpm_try_get_ops(chip);
 	if (!rc) {
-		if (chip->flags & TPM_CHIP_FLAG_TPM2)
+		if (chip->flags & TPM_CHIP_FLAG_TPM2) {
+			tpm2_end_auth_session(chip);
 			tpm2_shutdown(chip, TPM2_SU_STATE);
-		else
+		} else {
 			rc = tpm1_pm_suspend(chip, tpm_suspend_pcr);
+		}
 
 		tpm_put_ops(chip);
 	}
diff --git a/drivers/char/tpm/tpm2-sessions.c b/drivers/char/tpm/tpm2-sessions.c
index 950a3e48293b..03145a465b5d 100644
--- a/drivers/char/tpm/tpm2-sessions.c
+++ b/drivers/char/tpm/tpm2-sessions.c
@@ -333,6 +333,9 @@ void tpm_buf_append_hmac_session(struct tpm_chip *chip, struct tpm_buf *buf,
 	}
 
 #ifdef CONFIG_TCG_TPM2_HMAC
+	/* The first write to /dev/tpm{rm0} will flush the session. */
+	attributes |= TPM2_SA_CONTINUE_SESSION;
+
 	/*
 	 * The Architecture Guide requires us to strip trailing zeros
 	 * before computing the HMAC
@@ -484,7 +487,8 @@ static void tpm2_KDFe(u8 z[EC_PT_SZ], const char *str, u8 *pt_u, u8 *pt_v,
 	sha256_final(&sctx, out);
 }
 
-static void tpm_buf_append_salt(struct tpm_buf *buf, struct tpm_chip *chip)
+static void tpm_buf_append_salt(struct tpm_buf *buf, struct tpm_chip *chip,
+				struct tpm2_auth *auth)
 {
 	struct crypto_kpp *kpp;
 	struct kpp_request *req;
@@ -543,7 +547,7 @@ static void tpm_buf_append_salt(struct tpm_buf *buf, struct tpm_chip *chip)
 	sg_set_buf(&s[0], chip->null_ec_key_x, EC_PT_SZ);
 	sg_set_buf(&s[1], chip->null_ec_key_y, EC_PT_SZ);
 	kpp_request_set_input(req, s, EC_PT_SZ*2);
-	sg_init_one(d, chip->auth->salt, EC_PT_SZ);
+	sg_init_one(d, auth->salt, EC_PT_SZ);
 	kpp_request_set_output(req, d, EC_PT_SZ);
 	crypto_kpp_compute_shared_secret(req);
 	kpp_request_free(req);
@@ -554,8 +558,7 @@ static void tpm_buf_append_salt(struct tpm_buf *buf, struct tpm_chip *chip)
 	 * This works because KDFe fully consumes the secret before it
 	 * writes the salt
 	 */
-	tpm2_KDFe(chip->auth->salt, "SECRET", x, chip->null_ec_key_x,
-		  chip->auth->salt);
+	tpm2_KDFe(auth->salt, "SECRET", x, chip->null_ec_key_x, auth->salt);
 
  out:
 	crypto_free_kpp(kpp);
@@ -853,7 +856,9 @@ int tpm_buf_check_hmac_response(struct tpm_chip *chip, struct tpm_buf *buf,
 		if (rc)
 			/* manually close the session if it wasn't consumed */
 			tpm2_flush_context(chip, auth->handle);
-		memzero_explicit(auth, sizeof(*auth));
+
+		kfree_sensitive(auth);
+		chip->auth = NULL;
 	} else {
 		/* reset for next use  */
 		auth->session = TPM_HEADER_SIZE;
@@ -881,7 +886,8 @@ void tpm2_end_auth_session(struct tpm_chip *chip)
 		return;
 
 	tpm2_flush_context(chip, auth->handle);
-	memzero_explicit(auth, sizeof(*auth));
+	kfree_sensitive(auth);
+	chip->auth = NULL;
 }
 EXPORT_SYMBOL(tpm2_end_auth_session);
 
@@ -962,16 +968,20 @@ static int tpm2_load_null(struct tpm_chip *chip, u32 *null_key)
  */
 int tpm2_start_auth_session(struct tpm_chip *chip)
 {
+	struct tpm2_auth *auth;
 	struct tpm_buf buf;
-	struct tpm2_auth *auth = chip->auth;
-	int rc;
 	u32 null_key;
+	int rc;
 
-	if (!auth) {
-		dev_warn_once(&chip->dev, "auth session is not active\n");
+	if (chip->auth) {
+		dev_warn_once(&chip->dev, "auth session is active\n");
 		return 0;
 	}
 
+	auth = kzalloc(sizeof(*auth), GFP_KERNEL);
+	if (!auth)
+		return -ENOMEM;
+
 	rc = tpm2_load_null(chip, &null_key);
 	if (rc)
 		goto out;
@@ -992,7 +1002,7 @@ int tpm2_start_auth_session(struct tpm_chip *chip)
 	tpm_buf_append(&buf, auth->our_nonce, sizeof(auth->our_nonce));
 
 	/* append encrypted salt and squirrel away unencrypted in auth */
-	tpm_buf_append_salt(&buf, chip);
+	tpm_buf_append_salt(&buf, chip, auth);
 	/* session type (HMAC, audit or policy) */
 	tpm_buf_append_u8(&buf, TPM2_SE_HMAC);
 
@@ -1014,10 +1024,13 @@ int tpm2_start_auth_session(struct tpm_chip *chip)
 
 	tpm_buf_destroy(&buf);
 
-	if (rc)
-		goto out;
+	if (rc == TPM2_RC_SUCCESS) {
+		chip->auth = auth;
+		return 0;
+	}
 
- out:
+out:
+	kfree_sensitive(auth);
 	return rc;
 }
 EXPORT_SYMBOL(tpm2_start_auth_session);
@@ -1367,10 +1380,6 @@ int tpm2_sessions_init(struct tpm_chip *chip)
 		return rc;
 	}
 
-	chip->auth = kmalloc(sizeof(*chip->auth), GFP_KERNEL);
-	if (!chip->auth)
-		return -ENOMEM;
-
 	return rc;
 }
 #endif /* CONFIG_TCG_TPM2_HMAC */
-- 
2.47.0

On 10/28/24 1:50 AM, Jarkko Sakkinen wrote:
> Move the allocation of chip->auth to tpm2_start_auth_session() so that this
> field can be used as flag to tell whether auth session is active or not.
> 
> Instead of flushing and reloading the auth session for every transaction
> separately, keep the session open unless /dev/tpm0 is used.
> 
> Reported-by: Pengyu Ma <mapengyu@gmail.com>
> Closes: https://bugzilla.kernel.org/show_bug.cgi?id=219229
> Cc: stable@vger.kernel.org # v6.10+
> Fixes: 7ca110f2679b ("tpm: Address !chip->auth in tpm_buf_append_hmac_session*()")
> Tested-by: Pengyu Ma <mapengyu@gmail.com>
> Signed-off-by: Jarkko Sakkinen <jarkko@kernel.org>

Reviewed-by: Stefan Berger <stefanb@linux.ibm.com>
Tested-by: Stefan Berger <stefanb@linux.ibm.com>

> ---
> v8:
> - Since auth session and null key are flushed at a same time, only
>    either needs to be checked. Addresses and a remark from James
>    Bottomley few revisions ago.
> - kfree_sensitive()
> - Effectively squash top three patches given the simplifications.
> v7:
> - No changes.
> v6:
> - No changes.
> v5:
> - No changes.
> v4:
> - Changed as bug.
> v3:
> - Refined the commit message.
> - Removed the conditional for applying TPM2_SA_CONTINUE_SESSION only when
>    /dev/tpm0 is open. It is not required as the auth session is flushed,
>    not saved.
> v2:
> - A new patch.
> ---
>   drivers/char/tpm/tpm-chip.c       | 10 +++++++
>   drivers/char/tpm/tpm-dev-common.c |  3 +++
>   drivers/char/tpm/tpm-interface.c  |  6 +++--
>   drivers/char/tpm/tpm2-sessions.c  | 45 ++++++++++++++++++-------------
>   4 files changed, 44 insertions(+), 20 deletions(-)
> 
> diff --git a/drivers/char/tpm/tpm-chip.c b/drivers/char/tpm/tpm-chip.c
> index 854546000c92..1ff99a7091bb 100644
> --- a/drivers/char/tpm/tpm-chip.c
> +++ b/drivers/char/tpm/tpm-chip.c
> @@ -674,6 +674,16 @@ EXPORT_SYMBOL_GPL(tpm_chip_register);
>    */
>   void tpm_chip_unregister(struct tpm_chip *chip)
>   {
> +#ifdef CONFIG_TCG_TPM2_HMAC
> +	int rc;
> +
> +	rc = tpm_try_get_ops(chip);
> +	if (!rc) {
> +		tpm2_end_auth_session(chip);
> +		tpm_put_ops(chip);
> +	}
> +#endif
> +
>   	tpm_del_legacy_sysfs(chip);
>   	if (tpm_is_hwrng_enabled(chip))
>   		hwrng_unregister(&chip->hwrng);
> diff --git a/drivers/char/tpm/tpm-dev-common.c b/drivers/char/tpm/tpm-dev-common.c
> index 30b4c288c1bb..c7a88fa7b0fc 100644
> --- a/drivers/char/tpm/tpm-dev-common.c
> +++ b/drivers/char/tpm/tpm-dev-common.c
> @@ -27,6 +27,9 @@ static ssize_t tpm_dev_transmit(struct tpm_chip *chip, struct tpm_space *space,
>   	struct tpm_header *header = (void *)buf;
>   	ssize_t ret, len;
>   
> +	if (chip->flags & TPM_CHIP_FLAG_TPM2)
> +		tpm2_end_auth_session(chip);
> +
>   	ret = tpm2_prepare_space(chip, space, buf, bufsiz);
>   	/* If the command is not implemented by the TPM, synthesize a
>   	 * response with a TPM2_RC_COMMAND_CODE return for user-space.
> diff --git a/drivers/char/tpm/tpm-interface.c b/drivers/char/tpm/tpm-interface.c
> index 5da134f12c9a..8134f002b121 100644
> --- a/drivers/char/tpm/tpm-interface.c
> +++ b/drivers/char/tpm/tpm-interface.c
> @@ -379,10 +379,12 @@ int tpm_pm_suspend(struct device *dev)
>   
>   	rc = tpm_try_get_ops(chip);
>   	if (!rc) {
> -		if (chip->flags & TPM_CHIP_FLAG_TPM2)
> +		if (chip->flags & TPM_CHIP_FLAG_TPM2) {
> +			tpm2_end_auth_session(chip);
>   			tpm2_shutdown(chip, TPM2_SU_STATE);
> -		else
> +		} else {
>   			rc = tpm1_pm_suspend(chip, tpm_suspend_pcr);
> +		}
>   
>   		tpm_put_ops(chip);
>   	}
> diff --git a/drivers/char/tpm/tpm2-sessions.c b/drivers/char/tpm/tpm2-sessions.c
> index 950a3e48293b..03145a465b5d 100644
> --- a/drivers/char/tpm/tpm2-sessions.c
> +++ b/drivers/char/tpm/tpm2-sessions.c
> @@ -333,6 +333,9 @@ void tpm_buf_append_hmac_session(struct tpm_chip *chip, struct tpm_buf *buf,
>   	}
>   
>   #ifdef CONFIG_TCG_TPM2_HMAC
> +	/* The first write to /dev/tpm{rm0} will flush the session. */
> +	attributes |= TPM2_SA_CONTINUE_SESSION;
> +
>   	/*
>   	 * The Architecture Guide requires us to strip trailing zeros
>   	 * before computing the HMAC
> @@ -484,7 +487,8 @@ static void tpm2_KDFe(u8 z[EC_PT_SZ], const char *str, u8 *pt_u, u8 *pt_v,
>   	sha256_final(&sctx, out);
>   }
>   
> -static void tpm_buf_append_salt(struct tpm_buf *buf, struct tpm_chip *chip)
> +static void tpm_buf_append_salt(struct tpm_buf *buf, struct tpm_chip *chip,
> +				struct tpm2_auth *auth)
>   {
>   	struct crypto_kpp *kpp;
>   	struct kpp_request *req;
> @@ -543,7 +547,7 @@ static void tpm_buf_append_salt(struct tpm_buf *buf, struct tpm_chip *chip)
>   	sg_set_buf(&s[0], chip->null_ec_key_x, EC_PT_SZ);
>   	sg_set_buf(&s[1], chip->null_ec_key_y, EC_PT_SZ);
>   	kpp_request_set_input(req, s, EC_PT_SZ*2);
> -	sg_init_one(d, chip->auth->salt, EC_PT_SZ);
> +	sg_init_one(d, auth->salt, EC_PT_SZ);
>   	kpp_request_set_output(req, d, EC_PT_SZ);
>   	crypto_kpp_compute_shared_secret(req);
>   	kpp_request_free(req);
> @@ -554,8 +558,7 @@ static void tpm_buf_append_salt(struct tpm_buf *buf, struct tpm_chip *chip)
>   	 * This works because KDFe fully consumes the secret before it
>   	 * writes the salt
>   	 */
> -	tpm2_KDFe(chip->auth->salt, "SECRET", x, chip->null_ec_key_x,
> -		  chip->auth->salt);
> +	tpm2_KDFe(auth->salt, "SECRET", x, chip->null_ec_key_x, auth->salt);
>   
>    out:
>   	crypto_free_kpp(kpp);
> @@ -853,7 +856,9 @@ int tpm_buf_check_hmac_response(struct tpm_chip *chip, struct tpm_buf *buf,
>   		if (rc)
>   			/* manually close the session if it wasn't consumed */
>   			tpm2_flush_context(chip, auth->handle);
> -		memzero_explicit(auth, sizeof(*auth));
> +
> +		kfree_sensitive(auth);
> +		chip->auth = NULL;
>   	} else {
>   		/* reset for next use  */
>   		auth->session = TPM_HEADER_SIZE;
> @@ -881,7 +886,8 @@ void tpm2_end_auth_session(struct tpm_chip *chip)
>   		return;
>   
>   	tpm2_flush_context(chip, auth->handle);
> -	memzero_explicit(auth, sizeof(*auth));
> +	kfree_sensitive(auth);
> +	chip->auth = NULL;
>   }
>   EXPORT_SYMBOL(tpm2_end_auth_session);
>   
> @@ -962,16 +968,20 @@ static int tpm2_load_null(struct tpm_chip *chip, u32 *null_key)
>    */
>   int tpm2_start_auth_session(struct tpm_chip *chip)
>   {
> +	struct tpm2_auth *auth;
>   	struct tpm_buf buf;
> -	struct tpm2_auth *auth = chip->auth;
> -	int rc;
>   	u32 null_key;
> +	int rc;
>   
> -	if (!auth) {
> -		dev_warn_once(&chip->dev, "auth session is not active\n");
> +	if (chip->auth) {
> +		dev_warn_once(&chip->dev, "auth session is active\n");
>   		return 0;
>   	}
>   
> +	auth = kzalloc(sizeof(*auth), GFP_KERNEL);
> +	if (!auth)
> +		return -ENOMEM;
> +
>   	rc = tpm2_load_null(chip, &null_key);
>   	if (rc)
>   		goto out;
> @@ -992,7 +1002,7 @@ int tpm2_start_auth_session(struct tpm_chip *chip)
>   	tpm_buf_append(&buf, auth->our_nonce, sizeof(auth->our_nonce));
>   
>   	/* append encrypted salt and squirrel away unencrypted in auth */
> -	tpm_buf_append_salt(&buf, chip);
> +	tpm_buf_append_salt(&buf, chip, auth);
>   	/* session type (HMAC, audit or policy) */
>   	tpm_buf_append_u8(&buf, TPM2_SE_HMAC);
>   
> @@ -1014,10 +1024,13 @@ int tpm2_start_auth_session(struct tpm_chip *chip)
>   
>   	tpm_buf_destroy(&buf);
>   
> -	if (rc)
> -		goto out;
> +	if (rc == TPM2_RC_SUCCESS) {
> +		chip->auth = auth;
> +		return 0;
> +	}
>   
> - out:
> +out:
> +	kfree_sensitive(auth);
>   	return rc;
>   }
>   EXPORT_SYMBOL(tpm2_start_auth_session);
> @@ -1367,10 +1380,6 @@ int tpm2_sessions_init(struct tpm_chip *chip)
>   		return rc;
>   	}
>   
> -	chip->auth = kmalloc(sizeof(*chip->auth), GFP_KERNEL);
> -	if (!chip->auth)
> -		return -ENOMEM;
> -
>   	return rc;
>   }
>   #endif /* CONFIG_TCG_TPM2_HMAC */

On Mon Oct 28, 2024 at 7:52 PM EET, Stefan Berger wrote:
>
> On 10/28/24 1:50 AM, Jarkko Sakkinen wrote:
> > Move the allocation of chip->auth to tpm2_start_auth_session() so that this
> > field can be used as flag to tell whether auth session is active or not.
> > 
> > Instead of flushing and reloading the auth session for every transaction
> > separately, keep the session open unless /dev/tpm0 is used.
> > 
> > Reported-by: Pengyu Ma <mapengyu@gmail.com>
> > Closes: https://bugzilla.kernel.org/show_bug.cgi?id=219229
> > Cc: stable@vger.kernel.org # v6.10+
> > Fixes: 7ca110f2679b ("tpm: Address !chip->auth in tpm_buf_append_hmac_session*()")
> > Tested-by: Pengyu Ma <mapengyu@gmail.com>
> > Signed-off-by: Jarkko Sakkinen <jarkko@kernel.org>
>
> Reviewed-by: Stefan Berger <stefanb@linux.ibm.com>
> Tested-by: Stefan Berger <stefanb@linux.ibm.com>

Thanks!

Next after this: tpm2_get_random() issues reported.

I think biggest problem with that in general, and independent of bugs,
is that it does not pool random but instead pulls random small chunks.
This is more like performance issue exposed by bus encryption than
introducing a new issue (not formally but with better implementation
would not be necessarily a problem).

BR, Jarkko