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

Cc: stable@vger.kernel.org # v6.10+
Fixes: 699e3efd6c64 ("tpm: Add HMAC session start and end functions")
Signed-off-by: Jarkko Sakkinen <jarkko@kernel.org>
---
v5:
- No changes.
v4:
- Change to bug.
v3:
- No changes.
v2:
- A new patch.
---
 drivers/char/tpm/tpm2-sessions.c | 43 +++++++++++++++++++-------------
 1 file changed, 25 insertions(+), 18 deletions(-)

diff --git a/drivers/char/tpm/tpm2-sessions.c b/drivers/char/tpm/tpm2-sessions.c
index 78c650ce4c9f..6e52785de9fd 100644
--- a/drivers/char/tpm/tpm2-sessions.c
+++ b/drivers/char/tpm/tpm2-sessions.c
@@ -484,7 +484,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 +544,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 +555,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);
@@ -854,6 +854,8 @@ int tpm_buf_check_hmac_response(struct tpm_chip *chip, struct tpm_buf *buf,
 			/* manually close the session if it wasn't consumed */
 			tpm2_flush_context(chip, auth->handle);
 		memzero_explicit(auth, sizeof(*auth));
+		kfree(auth);
+		chip->auth = NULL;
 	} else {
 		/* reset for next use  */
 		auth->session = TPM_HEADER_SIZE;
@@ -882,6 +884,8 @@ void tpm2_end_auth_session(struct tpm_chip *chip)
 
 	tpm2_flush_context(chip, auth->handle);
 	memzero_explicit(auth, sizeof(*auth));
+	kfree(auth);
+	chip->auth = NULL;
 }
 EXPORT_SYMBOL(tpm2_end_auth_session);
 
@@ -970,25 +974,29 @@ 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;
+		goto err;
 
 	auth->session = TPM_HEADER_SIZE;
 
 	rc = tpm_buf_init(&buf, TPM2_ST_NO_SESSIONS, TPM2_CC_START_AUTH_SESS);
 	if (rc)
-		goto out;
+		goto err;
 
 	/* salt key handle */
 	tpm_buf_append_u32(&buf, null_key);
@@ -1000,7 +1008,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);
 
@@ -1021,10 +1029,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:
+err:
+	kfree(auth);
 	return rc;
 }
 EXPORT_SYMBOL(tpm2_start_auth_session);
@@ -1377,10 +1388,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/21/24 1:39 AM, Jarkko Sakkinen wrote:
> Move allocation of chip->auth to tpm2_start_auth_session() so that the
> field can be used as flag to tell whether auth session is active or not.
> 
> Cc: stable@vger.kernel.org # v6.10+
> Fixes: 699e3efd6c64 ("tpm: Add HMAC session start and end functions")
> Signed-off-by: Jarkko Sakkinen <jarkko@kernel.org>
> ---
> v5:
> - No changes.
> v4:
> - Change to bug.
> v3:
> - No changes.
> v2:
> - A new patch.
> ---
>   drivers/char/tpm/tpm2-sessions.c | 43 +++++++++++++++++++-------------
>   1 file changed, 25 insertions(+), 18 deletions(-)
> 
> diff --git a/drivers/char/tpm/tpm2-sessions.c b/drivers/char/tpm/tpm2-sessions.c
> index 78c650ce4c9f..6e52785de9fd 100644
> --- a/drivers/char/tpm/tpm2-sessions.c
> +++ b/drivers/char/tpm/tpm2-sessions.c
> @@ -484,7 +484,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 +544,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 +555,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);
> @@ -854,6 +854,8 @@ int tpm_buf_check_hmac_response(struct tpm_chip *chip, struct tpm_buf *buf,
>   			/* manually close the session if it wasn't consumed */
>   			tpm2_flush_context(chip, auth->handle);
>   		memzero_explicit(auth, sizeof(*auth));
> +		kfree(auth);
> +		chip->auth = NULL;
>   	} else {
>   		/* reset for next use  */
>   		auth->session = TPM_HEADER_SIZE;
> @@ -882,6 +884,8 @@ void tpm2_end_auth_session(struct tpm_chip *chip)
>   
>   	tpm2_flush_context(chip, auth->handle);
>   	memzero_explicit(auth, sizeof(*auth));
> +	kfree(auth);
> +	chip->auth = NULL;
>   }
>   EXPORT_SYMBOL(tpm2_end_auth_session);
>   
> @@ -970,25 +974,29 @@ 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;
> +		goto err;
>   
>   	auth->session = TPM_HEADER_SIZE;
>   
>   	rc = tpm_buf_init(&buf, TPM2_ST_NO_SESSIONS, TPM2_CC_START_AUTH_SESS);
>   	if (rc)
> -		goto out;
> +		goto err;
>   
>   	/* salt key handle */
>   	tpm_buf_append_u32(&buf, null_key);
> @@ -1000,7 +1008,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);
>   
> @@ -1021,10 +1029,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:
> +err:

like in many other cases before kfree(auth):
memzero_explicit(auth, sizeof(*auth));

With this:

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

> +	kfree(auth);
>   	return rc;
>   }
>   EXPORT_SYMBOL(tpm2_start_auth_session);
> @@ -1377,10 +1388,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 Wed Oct 23, 2024 at 10:15 PM EEST, Stefan Berger wrote:
>
>
> On 10/21/24 1:39 AM, Jarkko Sakkinen wrote:
> > Move allocation of chip->auth to tpm2_start_auth_session() so that the
> > field can be used as flag to tell whether auth session is active or not.
> > 
> > Cc: stable@vger.kernel.org # v6.10+
> > Fixes: 699e3efd6c64 ("tpm: Add HMAC session start and end functions")
> > Signed-off-by: Jarkko Sakkinen <jarkko@kernel.org>
> > ---
> > v5:
> > - No changes.
> > v4:
> > - Change to bug.
> > v3:
> > - No changes.
> > v2:
> > - A new patch.
> > ---
> >   drivers/char/tpm/tpm2-sessions.c | 43 +++++++++++++++++++-------------
> >   1 file changed, 25 insertions(+), 18 deletions(-)
> > 
> > diff --git a/drivers/char/tpm/tpm2-sessions.c b/drivers/char/tpm/tpm2-sessions.c
> > index 78c650ce4c9f..6e52785de9fd 100644
> > --- a/drivers/char/tpm/tpm2-sessions.c
> > +++ b/drivers/char/tpm/tpm2-sessions.c
> > @@ -484,7 +484,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 +544,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 +555,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);
> > @@ -854,6 +854,8 @@ int tpm_buf_check_hmac_response(struct tpm_chip *chip, struct tpm_buf *buf,
> >   			/* manually close the session if it wasn't consumed */
> >   			tpm2_flush_context(chip, auth->handle);
> >   		memzero_explicit(auth, sizeof(*auth));
> > +		kfree(auth);
> > +		chip->auth = NULL;
> >   	} else {
> >   		/* reset for next use  */
> >   		auth->session = TPM_HEADER_SIZE;
> > @@ -882,6 +884,8 @@ void tpm2_end_auth_session(struct tpm_chip *chip)
> >   
> >   	tpm2_flush_context(chip, auth->handle);
> >   	memzero_explicit(auth, sizeof(*auth));
> > +	kfree(auth);
> > +	chip->auth = NULL;
> >   }
> >   EXPORT_SYMBOL(tpm2_end_auth_session);
> >   
> > @@ -970,25 +974,29 @@ 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;
> > +		goto err;
> >   
> >   	auth->session = TPM_HEADER_SIZE;
> >   
> >   	rc = tpm_buf_init(&buf, TPM2_ST_NO_SESSIONS, TPM2_CC_START_AUTH_SESS);
> >   	if (rc)
> > -		goto out;
> > +		goto err;
> >   
> >   	/* salt key handle */
> >   	tpm_buf_append_u32(&buf, null_key);
> > @@ -1000,7 +1008,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);
> >   
> > @@ -1021,10 +1029,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:
> > +err:
>
> like in many other cases before kfree(auth):
> memzero_explicit(auth, sizeof(*auth));
>
> With this:
>
> Reviewed-by: Stefan Berger <stefanb@linux.ibm.com>

Thanks, or should we use kfree_sensitive()?

It has some additional functionality, which is missed now:

https://elixir.bootlin.com/linux/v6.11.5/source/mm/slab_common.c#L1339

I.e. kasan_unpoison().

BR, Jarkko

On 10/24/24 7:28 AM, Jarkko Sakkinen wrote:
> On Wed Oct 23, 2024 at 10:15 PM EEST, Stefan Berger wrote:
>>
>>
>> On 10/21/24 1:39 AM, Jarkko Sakkinen wrote:
>>> Move allocation of chip->auth to tpm2_start_auth_session() so that the
>>> field can be used as flag to tell whether auth session is active or not.
>>>
>>> Cc: stable@vger.kernel.org # v6.10+
>>> Fixes: 699e3efd6c64 ("tpm: Add HMAC session start and end functions")
>>> Signed-off-by: Jarkko Sakkinen <jarkko@kernel.org>
>>> ---
>>> v5:
>>> - No changes.
>>> v4:
>>> - Change to bug.
>>> v3:
>>> - No changes.
>>> v2:
>>> - A new patch.
>>> ---
>>>    drivers/char/tpm/tpm2-sessions.c | 43 +++++++++++++++++++-------------
>>>    1 file changed, 25 insertions(+), 18 deletions(-)
>>>
>>> diff --git a/drivers/char/tpm/tpm2-sessions.c b/drivers/char/tpm/tpm2-sessions.c
>>> index 78c650ce4c9f..6e52785de9fd 100644
>>> --- a/drivers/char/tpm/tpm2-sessions.c
>>> +++ b/drivers/char/tpm/tpm2-sessions.c
>>> @@ -484,7 +484,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 +544,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 +555,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);
>>> @@ -854,6 +854,8 @@ int tpm_buf_check_hmac_response(struct tpm_chip *chip, struct tpm_buf *buf,
>>>    			/* manually close the session if it wasn't consumed */
>>>    			tpm2_flush_context(chip, auth->handle);
>>>    		memzero_explicit(auth, sizeof(*auth));
>>> +		kfree(auth);
>>> +		chip->auth = NULL;
>>>    	} else {
>>>    		/* reset for next use  */
>>>    		auth->session = TPM_HEADER_SIZE;
>>> @@ -882,6 +884,8 @@ void tpm2_end_auth_session(struct tpm_chip *chip)
>>>    
>>>    	tpm2_flush_context(chip, auth->handle);
>>>    	memzero_explicit(auth, sizeof(*auth));
>>> +	kfree(auth);
>>> +	chip->auth = NULL;
>>>    }
>>>    EXPORT_SYMBOL(tpm2_end_auth_session);
>>>    
>>> @@ -970,25 +974,29 @@ 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;
>>> +		goto err;
>>>    
>>>    	auth->session = TPM_HEADER_SIZE;
>>>    
>>>    	rc = tpm_buf_init(&buf, TPM2_ST_NO_SESSIONS, TPM2_CC_START_AUTH_SESS);
>>>    	if (rc)
>>> -		goto out;
>>> +		goto err;
>>>    
>>>    	/* salt key handle */
>>>    	tpm_buf_append_u32(&buf, null_key);
>>> @@ -1000,7 +1008,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);
>>>    
>>> @@ -1021,10 +1029,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:
>>> +err:
>>
>> like in many other cases before kfree(auth):
>> memzero_explicit(auth, sizeof(*auth));
>>
>> With this:
>>
>> Reviewed-by: Stefan Berger <stefanb@linux.ibm.com>
> 
> Thanks, or should we use kfree_sensitive()?
> 
> It has some additional functionality, which is missed now:
> 
> https://elixir.bootlin.com/linux/v6.11.5/source/mm/slab_common.c#L1339
> 
> I.e. kasan_unpoison().

And change the other ones that use memzero_explicit()?

> 
> BR, Jarkko
>

On Thu Oct 24, 2024 at 3:59 PM EEST, Stefan Berger wrote:
>
>
> On 10/24/24 7:28 AM, Jarkko Sakkinen wrote:
> > On Wed Oct 23, 2024 at 10:15 PM EEST, Stefan Berger wrote:
> >>
> >>
> >> On 10/21/24 1:39 AM, Jarkko Sakkinen wrote:
> >>> Move allocation of chip->auth to tpm2_start_auth_session() so that the
> >>> field can be used as flag to tell whether auth session is active or not.
> >>>
> >>> Cc: stable@vger.kernel.org # v6.10+
> >>> Fixes: 699e3efd6c64 ("tpm: Add HMAC session start and end functions")
> >>> Signed-off-by: Jarkko Sakkinen <jarkko@kernel.org>
> >>> ---
> >>> v5:
> >>> - No changes.
> >>> v4:
> >>> - Change to bug.
> >>> v3:
> >>> - No changes.
> >>> v2:
> >>> - A new patch.
> >>> ---
> >>>    drivers/char/tpm/tpm2-sessions.c | 43 +++++++++++++++++++-------------
> >>>    1 file changed, 25 insertions(+), 18 deletions(-)
> >>>
> >>> diff --git a/drivers/char/tpm/tpm2-sessions.c b/drivers/char/tpm/tpm2-sessions.c
> >>> index 78c650ce4c9f..6e52785de9fd 100644
> >>> --- a/drivers/char/tpm/tpm2-sessions.c
> >>> +++ b/drivers/char/tpm/tpm2-sessions.c
> >>> @@ -484,7 +484,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 +544,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 +555,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);
> >>> @@ -854,6 +854,8 @@ int tpm_buf_check_hmac_response(struct tpm_chip *chip, struct tpm_buf *buf,
> >>>    			/* manually close the session if it wasn't consumed */
> >>>    			tpm2_flush_context(chip, auth->handle);
> >>>    		memzero_explicit(auth, sizeof(*auth));
> >>> +		kfree(auth);
> >>> +		chip->auth = NULL;
> >>>    	} else {
> >>>    		/* reset for next use  */
> >>>    		auth->session = TPM_HEADER_SIZE;
> >>> @@ -882,6 +884,8 @@ void tpm2_end_auth_session(struct tpm_chip *chip)
> >>>    
> >>>    	tpm2_flush_context(chip, auth->handle);
> >>>    	memzero_explicit(auth, sizeof(*auth));
> >>> +	kfree(auth);
> >>> +	chip->auth = NULL;
> >>>    }
> >>>    EXPORT_SYMBOL(tpm2_end_auth_session);
> >>>    
> >>> @@ -970,25 +974,29 @@ 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;
> >>> +		goto err;
> >>>    
> >>>    	auth->session = TPM_HEADER_SIZE;
> >>>    
> >>>    	rc = tpm_buf_init(&buf, TPM2_ST_NO_SESSIONS, TPM2_CC_START_AUTH_SESS);
> >>>    	if (rc)
> >>> -		goto out;
> >>> +		goto err;
> >>>    
> >>>    	/* salt key handle */
> >>>    	tpm_buf_append_u32(&buf, null_key);
> >>> @@ -1000,7 +1008,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);
> >>>    
> >>> @@ -1021,10 +1029,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:
> >>> +err:
> >>
> >> like in many other cases before kfree(auth):
> >> memzero_explicit(auth, sizeof(*auth));
> >>
> >> With this:
> >>
> >> Reviewed-by: Stefan Berger <stefanb@linux.ibm.com>
> > 
> > Thanks, or should we use kfree_sensitive()?
> > 
> > It has some additional functionality, which is missed now:
> > 
> > https://elixir.bootlin.com/linux/v6.11.5/source/mm/slab_common.c#L1339
> > 
> > I.e. kasan_unpoison().
>
> And change the other ones that use memzero_explicit()?

Yeah, might be a good idea too. Don't invent your own "safe primitives"
sounds like a good idea to me at least...

>
> > 
> > BR, Jarkko
> > 

BR, Jarkko