lists.openwall.net   lists  /  announce  owl-users  owl-dev  john-users  john-dev  passwdqc-users  yescrypt  popa3d-users  /  oss-security  kernel-hardening  musl  sabotage  tlsify  passwords  /  crypt-dev  xvendor  /  Bugtraq  Full-Disclosure  linux-kernel  linux-netdev  linux-ext4  linux-hardening  linux-cve-announce  PHC 
Open Source and information security mailing list archives
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <fa6b6c7d-1b90-40ad-b7f4-73e1a0eef1d5@linux.ibm.com>
Date: Mon, 28 Oct 2024 13:52:09 -0400
From: Stefan Berger <stefanb@...ux.ibm.com>
To: Jarkko Sakkinen <jarkko@...nel.org>, linux-integrity@...r.kernel.org,
        Peter Huewe <peterhuewe@....de>, Jason Gunthorpe <jgg@...pe.ca>
Cc: linux-kernel@...r.kernel.org, David Howells <dhowells@...hat.com>,
        James Bottomley <James.Bottomley@...senPartnership.com>,
        Mimi Zohar <zohar@...ux.ibm.com>,
        Roberto Sassu <roberto.sassu@...wei.com>,
        Paul Moore <paul@...l-moore.com>, James Morris <jmorris@...ei.org>,
        "Serge E. Hallyn" <serge@...lyn.com>,
        Dmitry Kasatkin <dmitry.kasatkin@...il.com>,
        Eric Snowberg <eric.snowberg@...cle.com>,
        "open list:KEYS-TRUSTED" <keyrings@...r.kernel.org>,
        "open list:SECURITY SUBSYSTEM" <linux-security-module@...r.kernel.org>,
        Pengyu Ma <mapengyu@...il.com>, stable@...r.kernel.org
Subject: Re: [PATCH v8 3/3] tpm: Lazily flush the auth session


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@...il.com>
> Closes: https://bugzilla.kernel.org/show_bug.cgi?id=219229
> Cc: stable@...r.kernel.org # v6.10+
> Fixes: 7ca110f2679b ("tpm: Address !chip->auth in tpm_buf_append_hmac_session*()")
> Tested-by: Pengyu Ma <mapengyu@...il.com>
> Signed-off-by: Jarkko Sakkinen <jarkko@...nel.org>

Reviewed-by: Stefan Berger <stefanb@...ux.ibm.com>
Tested-by: Stefan Berger <stefanb@...ux.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 */


Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ